Re: [PATCH] show-index: the short help should say the command reads from its input

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2024, 15:07 UTC
Message-ID: <xmqqv7v5tb92.fsf@gitster.g>
In-Reply-To: <Z260VVuIH_-0Ylis@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 30 quoted lines

> On Fri, Dec 20, 2024 at 10:02:15AM -0800, Junio C Hamano wrote:
>> The short help text given by "git show-index -h" says
>> 
>>     $ git show-index -h
>>     usage: git show-index [--object-format=<hash-algorithm>]
>> 
>>         --[no-]object-format <hash-algorithm>
>>                               specify the hash algorithm to use
>> ...
>>  * I also found the option description somewhat funny in that
>> 
>>    (1) it makes it look like "--no-object-format sha256" is
>>    accepted, which is not a case, and
>> 
>>    (2) "git show-index --no-object-format" already is a curious
>>    thing to say; the command certainly needs to work in _some_
>>    format.
>> 
>>    But (2) is common to all the usual command line options to allow
>>    defeating another instance of the same option that is given
>>    positively previously on the command line (i.e. "git show-index
>>    --object-format=sha256 --no-object-format" should behave as if no
>>    object-format option was given), and (1) is shared by all the
>>    other options that allow such override.  So I'll let it pass, but
>>    if we really wanted to improve it, the fix should go into how the
>>    parse-options subsystem works.
>
> Can't we already fix this via OPT_NONEG? Or is your point rather that it
> is awkward in general and choices like this should never have a negated
> variant by default?

Because '--object-format sha256 --no-object-format" is accepted by tools in the wild, OPT_NONEG is a regression, I think. IOW, (2) could be a position to take for a new tool without any users, but not for existing ones including show-index.

To solve (1), we would probably want to phrase it like so:

	--object-format <hash-algorithm>
			specify the hash algorithm to use.
	--no-object-format
			override a previous --object-format <hash-algorithm>
			and revert to the default.

I think we should say something about what negating does, but with the wish to make the mechanism generic so that callers of parse-options API do not have to write a new string, I did not think of one better than somewhat overly verbose one you see above. The parse-options API should be able to construct the above without any extra input from the programmers.

Or, just show this, without saying what negating does:

	--no-object-format
	--object-format <hash-algorithm>
			specify the hash algorithm to use.

and defer it to "git help cli" to give the most generic version, e.g., "the effect of an earlier option '--opt <value>' on the command line can be cancelled with '--no-opt' later on the command line, if the command supports '--no-opt'", or something like that, at a central place just once without repeating. I dunno.

Show 6 quoted lines

> I was wondering whether we have any other usage strings that show an
> expected stdin like this, and indeed we do. The usage string in
> "builtin/mailinfo.c" uses different syntax though without the angular
> brackets, but "builtin/pack-objects.c" does use them. I think with the
> angular brackets is more idiomatic in our codebase though, so the
> addition looks good to me.

Yeah, we may want to make things more consistent. These two commands were what came to my mind and had me check before settling on the version of the change you commented.

Thanks.