{"thread":{"id":"62677","subject":"[PATCH] show-index: the short help should say the command reads from its input","startedAt":"2024-12-20T18:02:18Z","lastAt":"2024-12-27T15:07:56Z","messageCount":3,"participants":["Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509437","messageId":"xmqqfrmidyhk.fsf@gitster.g","threadId":"62677","inReplyTo":null,"subject":"[PATCH] show-index: the short help should say the command reads from its input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-20T18:02:15Z","receivedAt":"2024-12-20T18:02:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The short help text given by \"git show-index -h\" says\n\n    $ git show-index -h\n    usage: git show-index [--object-format=<hash-algorithm>]\n\n        --[no-]object-format <hash-algorithm>\n                              specify the hash algorithm to use\n\n\nThe command takes a pack .idx file from its standard input.  The\nuser has to _know_ this, as there is no indication from this output.\n\nGive a hint that the data to work on is fed from its standard input.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * I also found the option description somewhat funny in that\n\n   (1) it makes it look like \"--no-object-format sha256\" is\n   accepted, which is not a case, and\n\n   (2) \"git show-index --no-object-format\" already is a curious\n   thing to say; the command certainly needs to work in _some_\n   format.\n\n   But (2) is common to all the usual command line options to allow\n   defeating another instance of the same option that is given\n   positively previously on the command line (i.e. \"git show-index\n   --object-format=sha256 --no-object-format\" should behave as if no\n   object-format option was given), and (1) is shared by all the\n   other options that allow such override.  So I'll let it pass, but\n   if we really wanted to improve it, the fix should go into how the\n   parse-options subsystem works.\n\n builtin/show-index.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git c/builtin/show-index.c w/builtin/show-index.c\nindex f164c01bbe..8678b741a4 100644\n--- c/builtin/show-index.c\n+++ w/builtin/show-index.c\n@@ -7,7 +7,7 @@\n #include \"parse-options.h\"\n \n static const char *const show_index_usage[] = {\n-\t\"git show-index [--object-format=<hash-algorithm>]\",\n+\t\"git show-index [--object-format=<hash-algorithm>] < <pack-idx-file>\",\n \tNULL\n };\n \n\n\n\n\n"},{"id":"509629","messageId":"Z260VVuIH_-0Ylis@pks.im","threadId":"62677","inReplyTo":"xmqqfrmidyhk.fsf@gitster.g","subject":"Re: [PATCH] show-index: the short help should say the command reads from its input","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T14:06:13Z","receivedAt":"2024-12-27T14:06:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 20, 2024 at 10:02:15AM -0800, Junio C Hamano wrote:\n> The short help text given by \"git show-index -h\" says\n> \n>     $ git show-index -h\n>     usage: git show-index [--object-format=<hash-algorithm>]\n> \n>         --[no-]object-format <hash-algorithm>\n>                               specify the hash algorithm to use\n> \n> \n> The command takes a pack .idx file from its standard input.  The\n> user has to _know_ this, as there is no indication from this output.\n> \n> Give a hint that the data to work on is fed from its standard input.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nMakes sense.\n\n>  * I also found the option description somewhat funny in that\n> \n>    (1) it makes it look like \"--no-object-format sha256\" is\n>    accepted, which is not a case, and\n> \n>    (2) \"git show-index --no-object-format\" already is a curious\n>    thing to say; the command certainly needs to work in _some_\n>    format.\n> \n>    But (2) is common to all the usual command line options to allow\n>    defeating another instance of the same option that is given\n>    positively previously on the command line (i.e. \"git show-index\n>    --object-format=sha256 --no-object-format\" should behave as if no\n>    object-format option was given), and (1) is shared by all the\n>    other options that allow such override.  So I'll let it pass, but\n>    if we really wanted to improve it, the fix should go into how the\n>    parse-options subsystem works.\n\nCan't we already fix this via OPT_NONEG? Or is your point rather that it\nis awkward in general and choices like this should never have a negated\nvariant by default?\n\n>  builtin/show-index.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git c/builtin/show-index.c w/builtin/show-index.c\n> index f164c01bbe..8678b741a4 100644\n> --- c/builtin/show-index.c\n> +++ w/builtin/show-index.c\n> @@ -7,7 +7,7 @@\n>  #include \"parse-options.h\"\n>  \n>  static const char *const show_index_usage[] = {\n> -\t\"git show-index [--object-format=<hash-algorithm>]\",\n> +\t\"git show-index [--object-format=<hash-algorithm>] < <pack-idx-file>\",\n>  \tNULL\n>  };\n\nI was wondering whether we have any other usage strings that show an\nexpected stdin like this, and indeed we do. The usage string in\n\"builtin/mailinfo.c\" uses different syntax though without the angular\nbrackets, but \"builtin/pack-objects.c\" does use them. I think with the\nangular brackets is more idiomatic in our codebase though, so the\naddition looks good to me.\n\nPatrick\n"},{"id":"509639","messageId":"xmqqv7v5tb92.fsf@gitster.g","threadId":"62677","inReplyTo":"Z260VVuIH_-0Ylis@pks.im","subject":"Re: [PATCH] show-index: the short help should say the command reads from its input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-27T15:07:53Z","receivedAt":"2024-12-27T15:07:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\nPatrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Dec 20, 2024 at 10:02:15AM -0800, Junio C Hamano wrote:\n>> The short help text given by \"git show-index -h\" says\n>> \n>>     $ git show-index -h\n>>     usage: git show-index [--object-format=<hash-algorithm>]\n>> \n>>         --[no-]object-format <hash-algorithm>\n>>                               specify the hash algorithm to use\n>> ...\n>>  * I also found the option description somewhat funny in that\n>> \n>>    (1) it makes it look like \"--no-object-format sha256\" is\n>>    accepted, which is not a case, and\n>> \n>>    (2) \"git show-index --no-object-format\" already is a curious\n>>    thing to say; the command certainly needs to work in _some_\n>>    format.\n>> \n>>    But (2) is common to all the usual command line options to allow\n>>    defeating another instance of the same option that is given\n>>    positively previously on the command line (i.e. \"git show-index\n>>    --object-format=sha256 --no-object-format\" should behave as if no\n>>    object-format option was given), and (1) is shared by all the\n>>    other options that allow such override.  So I'll let it pass, but\n>>    if we really wanted to improve it, the fix should go into how the\n>>    parse-options subsystem works.\n>\n> Can't we already fix this via OPT_NONEG? Or is your point rather that it\n> is awkward in general and choices like this should never have a negated\n> variant by default?\n\nBecause '--object-format sha256 --no-object-format\" is accepted by\ntools in the wild, OPT_NONEG is a regression, I think.  IOW, (2)\ncould be a position to take for a new tool without any users, but\nnot for existing ones including show-index.\n\nTo solve (1), we would probably want to phrase it like so:\n\n\t--object-format <hash-algorithm>\n\t\t\tspecify the hash algorithm to use.\n\t--no-object-format\n\t\t\toverride a previous --object-format <hash-algorithm>\n\t\t\tand revert to the default.\n\nI think we should say something about what negating does, but with\nthe wish to make the mechanism generic so that callers of\nparse-options API do not have to write a new string, I did not think\nof one better than somewhat overly verbose one you see above.  The\nparse-options API should be able to construct the above without any\nextra input from the programmers.\n\nOr, just show this, without saying what negating does:\n\n\t--no-object-format\n\t--object-format <hash-algorithm>\n\t\t\tspecify the hash algorithm to use.\n\n\nand defer it to \"git help cli\" to give the most generic version,\ne.g., \"the effect of an earlier option '--opt <value>' on the\ncommand line can be cancelled with '--no-opt' later on the command\nline, if the command supports '--no-opt'\", or something like that,\nat a central place just once without repeating.  I dunno.\n\n> I was wondering whether we have any other usage strings that show an\n> expected stdin like this, and indeed we do. The usage string in\n> \"builtin/mailinfo.c\" uses different syntax though without the angular\n> brackets, but \"builtin/pack-objects.c\" does use them. I think with the\n> angular brackets is more idiomatic in our codebase though, so the\n> addition looks good to me.\n\nYeah, we may want to make things more consistent.  These two\ncommands were what came to my mind and had me check before settling\non the version of the change you commented.\n\nThanks.\n"}]}