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

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

[Junio C Hamano]

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(-)

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
 };
 

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

[Patrick Steinhardt]

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

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

>  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

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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