threads / patch / 62677

patchshow-index: the short help should say the command reads from its input

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

## tl;dr

3 messages between Dec 20, 2024 and Dec 27, 2024. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Junio C Hamano· Dec 20, 2024, 18:02 UTC · lore

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

The command takes a pack .idx file from its standard input. The user has to _know_ this, as there is no indication from this output.

Give a hint that the data to work on is fed from its standard input.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * 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.

 builtin/show-index.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to diff +1 −1

diff --git c/builtin/show-index.c w/builtin/show-index.c
index f164c01bbe..8678b741a4 100644
--- c/builtin/show-index.c
+++ w/builtin/show-index.c
@@ -7,7 +7,7 @@
 #include "parse-options.h"
 
 static const char *const show_index_usage[] = {
-	"git show-index [--object-format=<hash-algorithm>]",
+	"git show-index [--object-format=<hash-algorithm>] < <pack-idx-file>",
 	NULL
 };

Patrick Steinhardt· Dec 27, 2024, 14:06 UTC · re: Junio C Hamano · lore

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

On Fri, Dec 20, 2024 at 10:02:15AM -0800, Junio C Hamano wrote:

Show 15 quoted lines

> 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
> 
> 
> The command takes a pack .idx file from its standard input.  The
> user has to _know_ this, as there is no indication from this output.
> 
> Give a hint that the data to work on is fed from its standard input.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Makes sense.

Show 17 quoted lines

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

Show 15 quoted lines

>  builtin/show-index.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git c/builtin/show-index.c w/builtin/show-index.c
> index f164c01bbe..8678b741a4 100644
> --- c/builtin/show-index.c
> +++ w/builtin/show-index.c
> @@ -7,7 +7,7 @@
>  #include "parse-options.h"
>  
>  static const char *const show_index_usage[] = {
> -	"git show-index [--object-format=<hash-algorithm>]",
> +	"git show-index [--object-format=<hash-algorithm>] < <pack-idx-file>",
>  	NULL
>  };

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.

Patrick

Junio C Hamano· Dec 27, 2024, 15:07 UTC · re: Patrick Steinhardt · lore

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

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.