Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

[Antonio Ospite <ao2@ao2.it>]

On Thu, 2 Aug 2018 15:20:33 -0400
Jeff King <peff@peff.net> wrote:

> On Thu, Aug 02, 2018 at 11:47:30AM -0700, Stefan Beller wrote:
> 
> > > +static int module_config(int argc, const char **argv, const char *prefix)
> > > +{
> > > +       if (argc < 2 || argc > 3)
> > > +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> > > +
> > > +       /* Equivalent to ACTION_GET in builtin/config.c */
> > > +       if (argc == 2)
> > > +               return print_config_from_gitmodules(argv[1]);
> > > +
> > > +       /* Equivalent to ACTION_SET in builtin/config.c */
> > > +       if (argc == 3)
> > > +               return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> > > +
> > > +       return 0;
> > 
> > Technically we cannot reach this point here?
> > Maybe it would be more defensive to
> > 
> >     BUG("How did we get here?");
> > 
> > or at least return something !=0 ?
> 
> When I find myself reaching for a BUG(), it is often a good time to see
> if we can restructure the code so that the logic more naturally falls
> out.  Here the issue is that the first if conditional repeats the "else"
> for the other two. So I think we could just write:
> 
>   if (argc == 2)
> 	return ...
>   if (argc == 3)
> 	return ...
> 
>   die("need 1 or 2 arguments");
> 

Hi Jeff,

I like that, I'll see how this plays out after patch 06 which adds
another option, and decide whether to use this style; validating
arguments beforehand may still look a little cleaner.

Thanks for the comment.

Ciao,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?