Avoid checking for -h in commands that already use the parse-options API

[dscho]

From https://lore.kernel.org/git/CABPp-BG1UXDLVh4_F_TQJmiM4=fFMOLFo5k27=MghbBPWPkL7A@mail.gmail.com/:

+     /* Check for a request for basic help */
+     if (argc == 2 && !strcmp(argv[1], "-h"))
+             usage_with_options(merge_tree_usage, mt_options);

[...] I don't think this is needed, the parse_options() below intercepts "-h" by default. [...]

+     /* Parse arguments */
+     argc = parse_options(argc, argv, prefix, mt_options,
+                          merge_tree_usage, 0);

It would appear that we have quite a few places where we use the parse-options API yet check (unnecessarily) for -h:

 $ comm -12 <(git grep -l parse-options builtin/ | sort) <(git grep -l strcmp.*-h\\b builtin/ | sort)
builtin/am.c
builtin/branch.c
builtin/checkout-index.c
builtin/checkout--worker.c
builtin/commit.c
builtin/commit-tree.c
builtin/gc.c
builtin/ls-files.c
builtin/merge.c
builtin/merge-tree.c
builtin/rebase.c
builtin/rev-parse.c
builtin/sparse-checkout.c
builtin/submodule--helper.c
builtin/update-index.c

And indeed, https://github.com/git/git/blob/v2.35.0/builtin/am.c#L2427-L2428 seems to do exactly that.

Let's fix those, and while at it, enhance the static-analysis job of Git's CI to find new code that introduced unnecessary -h checking.

[Abhra303]

Hey @dscho I would like to try this :)

[dscho]

Sure, go ahead!

[Abhra303]

Hi @dscho , I have a question though.

Say for commit.c file -

        if (argc == 2 && !strcmp(argv[1], "-h"))
		usage_with_options(builtin_status_usage, builtin_status_options);

	prepare_repo_settings(the_repository);
	the_repository->settings.command_requires_full_index = 0;

	status_init_config(&s, git_status_config);
	argc = parse_options(argc, argv, prefix,
			     builtin_status_options,
			     builtin_status_usage, 0);

Here if -h flag is passed it will show the usage of the commit sub-command and no further works (such as repo_settings, status init config etc.) would be done ( due to the if block). And it makes sense.

But if I remove those two lines ( i.e. the if check for -h flag), it will run prepare_repo_settings(), status_init_config() first and then parse the options using parse_options(). In this case prepare_repo_settings() and status_init_config are unnecessary ( in the context of git commit -h).

So, would it be good to remove the "unnecessary" if check for -h which would cause to run those unnecessary functions?

[dscho]

So, would it be good to remove the "unnecessary" if check for -h which would cause to run those unnecessary functions?

@Abhra303 I opened this ticket because it has been suggested on the Git mailing list that this -h check should be removed.

But now that I think of it, in particular in the context of prepare_repo_settings() (see #1139), I start to think that this falls squarely into the "Don't fix it if it ain't broken" category (because it may well be in the "if you 'fix' it, you may actually break it instead" category).

So if you ask me, actually: because you asked me, I would say no, it's not worth the effort to work on this. There are other, more rewarding micro projects, e.g. #636, or even vetting return values so that cmd_*() functions always return non-negative numbers.

Now, I opened this ticket, and since I now finally made up my mind that I'd rather not see time spent on this, I better close it, eh?

[Abhra303]

Now, I opened this ticket, and since I now finally made up my mind that I'd rather not see time spent on this, I better close it, eh?

Yeah, certainly!

I am also in favour of closing it :)