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

On Thu, 2 Aug 2018 15:20:33 -0400
Jeff King [off-list ref] wrote:

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

quoted

quoted

+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?