Re: [PATCH v2] cli: add -v and -h shorthands

[Junio C Hamano <gitster@pobox.com>]

Garrit Franke <garrit@slashdev.space> writes:

> On 31.03.22 02:07, Ævar Arnfjörð Bjarmason wrote:
>
>> I think this is a good trade-off in this case. I.e. -v and -h are
>> commonly understood.
>
> An interesting observation I just made is that curl [0] uses both
> "--verbose" and "--version" on the top level [1][2] including
> shorthands. "-v" corresponds to "verbose", "-V" corresponds to
> "version.
>
> Not that I'm a fan of this clutter, but it's a possible path to go
> down if we actually needed a second shorthand using this letter.

Do you mean you want to use "-V" for version, instead of the "-v"
used in the patch, so that "-v" can be left for "--verbose"?

I am not sure consistency with whom we are aiming for anymore with
that mixed to the proposal X-<.

>  const char git_usage_string[] =
> -	N_("git [--version] [--help] [-C <path>] [-c <name>=<value>]\n"
> +	N_("git [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]\n"
>  	   "           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
>  	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n"
>  	   "           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"

This is an improvement in that the first line that used to be
unusually shorter now becomes comparable length to other lines ;-)

> @@ -146,7 +146,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  		 * commands can be written with "--" prepended
>  		 * to make them look like flags.
>  		 */
> -		if (!strcmp(cmd, "--help") || !strcmp(cmd, "--version"))
> +		if (!strcmp(cmd, "--help") || !strcmp(cmd, "--version") ||
> +		    !strcmp(cmd, "-h") || !strcmp(cmd, "-v"))
>  			break;

OK.

> @@ -893,7 +894,12 @@ int cmd_main(int argc, const char **argv)
>  	handle_options(&argv, &argc, NULL);
>  	if (argc > 0) {
>  		/* translate --help and --version into commands */
> -		skip_prefix(argv[0], "--", &argv[0]);
> +		if (!strcmp("-v", argv[0]))
> +			argv[0] = "version";
> +		else if (!strcmp("-h", argv[0]))
> +			argv[0] = "help";
> +		else
> +			skip_prefix(argv[0], "--", &argv[0]);

I find this unnecessarily hard to read.

    (Side note) A tip for reviewers.  Be suspicious of any change
    that adds new things _in front_ of existing sequence.  Question
    if there is a good justification for it.  If there isn't, see if
    it makes it better if you instead append the new stuff to
    existing sequence.  If neither results in satisfying code,
    perhaps it is good time to totally rewrite it to make both
    existing and new stuff fit in the flow.

The original was in a sense a bit sloppy to strip "--" from anything
that begins with "--", when we only care about "--verbose" and
"--help" and nothing else (granted, handle_options() would barf when
argv[0] begins with "--" and is not one of these two, so the
sloppiness does not make a difference in practice).

If we accept the same kind of sloppiness, perhaps

		if (skip_prefix(argv[0], "--", &argv[0]))
                	; /* removed "--" from "--help" and "--version" */
		else if (argv[0][0] == '-')
                	argv[0] = argv[0][1] == 'v' ? "version" : "help";
                	
would make the new side match the existing one better, but I would
not necessarily recommend it.

We may want to be a bit more explicit and readable, by spelling out
the expectation, i.e.

		if (!strcmp("--version", argv[0]) || !strcmp("-v", argv[0]))
			argv[0] = "version";
		else if (!strcmp("--help", argv[0]) || !strcmp("-h", argv[0]))
			argv[0] = "help"; 

This makes it clear that these two pseudo-commands, spelled with a
dash in front and stand for other commands, are the only thing we
care about and what their accepted spelling are.

Hmm?