Re: [PATCH] show-index: the short help should say the command reads from its input
From: Patrick Steinhardt <hidden>
Date: 2024-12-27 14:06:34
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 <redacted>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?
quoted hunk ↗ jump to hunk
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