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

From: Jeff King <peff@peff.net>
Date: Aug 2, 2018, 19:20 UTC
Message-ID: <20180802192032.GA28962@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kY78rgb8VpRn-A2moucy16z1TsDGdz2cjJzVOxsiKiiYw@mail.gmail.com>

On Thu, Aug 02, 2018 at 11:47:30AM -0700, Stefan Beller wrote:

Show 21 quoted lines

> > +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");

-Peff