{"thread":{"id":"61562","subject":"Commands using -h as an option don't work consistently","startedAt":"2024-05-29T22:03:07Z","lastAt":"2024-06-03T18:33:17Z","messageCount":5,"participants":["Kevin Day","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"495863","messageId":"52819526-4C6F-418C-8B8B-A4D5C7E371EA@dragondata.com","threadId":"61562","inReplyTo":null,"subject":"Commands using -h as an option don't work consistently","fromName":"Kevin Day","fromEmail":"toasty@dragondata.com","sentAt":"2024-05-29T22:03:04Z","receivedAt":"2024-05-29T22:03:07Z","isPatch":false,"sender":{"key":"toasty@dragondata.com","avatar":null},"body":"Because of a bug in parse-options.c, any command that has a '-h' option will sometimes display the usage page instead of executing. For example, ls-remote has two options:\n\n    -t, --[no-]tags       limit to tags\n    -h, --[no-]heads      limit to heads\n\ngit ls-remote --heads  #works\ngit ls-remote --tags  #works\ngit ls-remote -t  #works\ngit ls-remote -t -h  #works\ngit ls-remote -h  #shows the help page\n\nThis is because of these lines in parse-options.c:\n\n/* lone -h asks for help */\nif (internal_help && ctx->total == 1 && !strcmp(arg + 1, \"h\"))\ngoto show_usage;\n\nThis is being executed before it looks to see if there actually is a -h option. So if a program has a -h option, and that's the ONLY parameter you pass, the usage page gets displayed incorrectly. This appears to affect ls-remote, show-ref (it's not documented, but show-ref accepts -h as an alias for --heads) and grep.\n\nI fixed this by moving the lone -h check lower down, which fixed everything. Except now lots and lots of tests are failing because many of them assume you can always pass -h to get the usage page, and now you can't for some commands. I don't think this actually breaks grep because you need to pass at least one more option other than -h to use it, but tests for it are still failing after fixing this bug because it's now showing the man page instead of the expected short usage page because it's erroring out at a different place.\n\nThe specific tests that are failing are t1502-rev-parse-parseopt.sh, t0012-help.sh and t0450-txt-doc-vs-help.sh all of which are trying to use -h on commands that have repurposed it.\n\nThe options I see:\n\n1) Fix -h handling and add ignores and fixes where possible to the failing tests and try to not use -h as an option for anything new.\n\n2) Change -h to -H or something, but this breaks backwards compatibility\n\n3) Fix it so that -h works if a command uses it, and additionally make a new global option -? or --usage or something that always shows the usage page and change tests to use that, while leaving -h sometimes showing usage and sometimes executing the option to preserve as much backward compatibility as possible.\n\n\n\nI'm happy to do the work and submit it, but this is looking more like a policy decision than just a bug now.\n\n-- Kevin\n\n"},{"id":"495864","messageId":"xmqqfru0tg1a.fsf@gitster.g","threadId":"61562","inReplyTo":"52819526-4C6F-418C-8B8B-A4D5C7E371EA@dragondata.com","subject":"Re: Commands using -h as an option don't work consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-29T22:22:09Z","receivedAt":"2024-05-29T22:22:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Day <toasty@dragondata.com> writes:\n\n> Because of a bug in parse-options.c, any command that has a '-h' option will sometimes display the usage page instead of executing. For example, ls-remote has two options:\n>\n>     -t, --[no-]tags       limit to tags\n>     -h, --[no-]heads      limit to heads\n>\n> git ls-remote --heads  #works\n> git ls-remote --tags  #works\n> git ls-remote -t  #works\n> git ls-remote -t -h  #works\n> git ls-remote -h  #shows the help page\n>\n> This is because of these lines in parse-options.c:\n>\n> /* lone -h asks for help */\n> if (internal_help && ctx->total == 1 && !strcmp(arg + 1, \"h\"))\n> goto show_usage;\n>\n> This is being executed before it looks to see if there actually is a -h option.\n\nThis is very much deliberate design.  I think in these cases we make\nan unambiguous longhand (e.g. \"--heads\" we see above) available, or\nyou can explicitly say the remote, i.e. \"git ls-remote -h origin\",\nand that has been the officially accepted \"solution\".\n\nLet's see what improvements you bring to the table.\n\n> The options I see:\n>\n> 1) Fix -h handling and add ignores and fixes where possible to the\n> failing tests and try to not use -h as an option for anything new.\n\nI do not quite understand the former half, but \"try not to use -h\nfor new things\" is a very good idea and it is pretty much what we\nhave been doing.\n\n    Note: There however is a scenario where we cannot avoid it---if\n    we were trying to match/mimick some established external\n    command, we may have to use the same \"-h\" as they use (e.g.,\n    \"git grep -h -e ...\" tries to mimick \"grep -h -e ...\").  Even in\n    that case, because \"git grep -h\" alone would not make any sense\n    without any pattern, the command behaves very sensibly, by the\n    way.\n\n> 2) Change -h to -H or something, but this breaks backwards compatibility\n\nAs you said it yourself, this is a non starter.\n\n> 3) Fix it so that -h works if a command uses it, and additionally\n> make a new global option -? or --usage or something that always\n> shows the usage page and change tests to use that, while leaving\n> -h sometimes showing usage and sometimes executing the option to\n> preserve as much backward compatibility as possible.\n\nReading this and then going back to the ls-remote examples you gave\nearlier, I do not think the current behaviour is all that bad,\nrelative to what is being proposed, except for an explicit support\nfor \"-?\".\n\nWe can certainly add support for \"-?\", but the behaviour when \"-?\"\nis not used (and when \"-h\" is used) would not have to change from\nthe current behaviour, and that would still be serviceable, I\npresume?\n\nWe actually do not have to *add* support for \"-?\", as it comes with\nparse-options for free ;-)\n\n    $ git ls-remote -\\?\n    error: unknown switch '?'\n    usage: git ls-remote ...\n\t... the same \"ls-remote -h\" output given here ...\n\nSo, I dunno.\n"},{"id":"495869","messageId":"0F971F6E-6462-4E0D-880B-8FD67F42A782@dragondata.com","threadId":"61562","inReplyTo":"xmqqfru0tg1a.fsf@gitster.g","subject":"Re: Commands using -h as an option don't work consistently","fromName":"Kevin Day","fromEmail":"toasty@dragondata.com","sentAt":"2024-05-29T22:40:31Z","receivedAt":"2024-05-29T22:40:33Z","isPatch":false,"sender":{"key":"toasty@dragondata.com","avatar":null},"body":"\n\n> On May 29, 2024, at 5:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> This is very much deliberate design.  I think in these cases we make\n> an unambiguous longhand (e.g. \"--heads\" we see above) available, or\n> you can explicitly say the remote, i.e. \"git ls-remote -h origin\",\n> and that has been the officially accepted \"solution\".\n\nYeah, and that totally makes sense. This bit me because I was programmatically generating git commands that would sometimes call ls-remote with no options, sometimes asking only for heads, sometimes only for tags.... and was baffled for longer than I want to admit why it wasn't working consistently. I thought the usage was being shown because I was doing something wrong. My first thought was it was showing me the help message because if you ask for heads you must also use some other option along with it. It didn't occur to me that -h and --heads would behave differently.\n\n> Let's see what improvements you bring to the table.\n> \n>> The options I see:\n>> \n>> 1) Fix -h handling and add ignores and fixes where possible to the\n>> failing tests and try to not use -h as an option for anything new.\n> \n> I do not quite understand the former half, but \"try not to use -h\n> for new things\" is a very good idea and it is pretty much what we\n> have been doing.\n\nSorry, let me rephrase that.  1) Apply the fix I was proposing to make it so that the command's -h usage has precedence over showing the usage. Any tests that break because of that which don't have a plausible fix mark as \"# TODO known breakage\" like several other commands have because they aren't generating usage output correctly for one reason or another.\n\n\n> We can certainly add support for \"-?\", but the behaviour when \"-?\"\n> is not used (and when \"-h\" is used) would not have to change from\n> the current behaviour, and that would still be serviceable, I\n> presume?\n> \n\nThat was proposing making -? unconditionally show usage, if a command uses -h give that priority over showing usage even if it's the only option, but leaving -h to still show usage everywhere else, and changing all the tests to use -? Instead of -h so the tests can unambiguously request the usage.\n\nI think that covers every case then. If a command advertises -h it always works, it falls back to showing usage if there is no -h, and tests can use -? which will always show usage and we can even make parse-options reject anyone's attempt to use -? as an option in the future so this doesn't come up again in 10 years.\n\n> We actually do not have to *add* support for \"-?\", as it comes with\n> parse-options for free ;-)\n> \n>    $ git ls-remote -\\?\n>    error: unknown switch '?'\n>    usage: git ls-remote ...\n> ... the same \"ls-remote -h\" output given here ...\n\nThat *almost* works, except some tests would have to be modified because they don't like that error line in there, but that is super clever! It would be pretty trivial to make parse-options deliberately spit the usage out without an error though in the -? case.\n\n\n\n"},{"id":"495882","messageId":"xmqq34q0rnio.fsf@gitster.g","threadId":"61562","inReplyTo":"0F971F6E-6462-4E0D-880B-8FD67F42A782@dragondata.com","subject":"Re: Commands using -h as an option don't work consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-30T03:23:27Z","receivedAt":"2024-05-30T03:23:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Day <toasty@dragondata.com> writes:\n\n>>> 1) Fix -h handling and add ignores and fixes where possible to the\n>>> failing tests and try to not use -h as an option for anything new.\n>> \n>> I do not quite understand the former half, but \"try not to use -h\n>> for new things\" is a very good idea and it is pretty much what we\n>> have been doing.\n>\n> Sorry, let me rephrase that.  1) Apply the fix I was proposing to make\n> it so that the command's -h usage has precedence over showing the\n> usage. Any tests that break because of that which don't have a\n> plausible fix mark as \"# TODO known breakage\" like several other\n> commands have because they aren't generating usage output correctly\n> for one reason or another.\n\nAh, then no.\n\nI am afraid that it is a non starter.  \"git ls-remote -h\" would then\nstop breaking existing users' long established expectation, i.e.,\n\"for any git subcommand 'cmd', you can say 'git cmd -h' to get the\nshort help\".  We need to design a good transition plan to wean\nexisting users off of that expectation.\n\nFor example, we _could_ introduce \"-?\" to parse-options _now_, and\nmark the use of \"-h\" as deprecated.  When the special casing of \"-h\"\nparse-options has triggers, the user will be reminded that the \"-h\"\nsupport will have a limited shelf life, and they are urged to\nretrain their fingers and update their scripts to use \"-?\", possibly\nwith \"advice.shorthelpdeprecationnotice\" configuration knob that\nhelps ease the transition.  We would update the t/ scripts to use\n\"-?\" at the same time, but would probably need to keep tests that\nuse \"-h\" to ensure that the warning triggers.\n\nWe will keep things that way for a while, and once we get the users'\nexpectation updated so that they think 'git cmd -?' (not 'git cmd\n-h') is _the_ way to get the short help for any git subcommand.\n\nAt a major version boundary, e.g., Git 3.0, we could remove the\nspecial casing of '-h' from parse-options, and \"git ls-remote -h\"\nwill start working the same way as \"git ls-remote -h origin\" all of\na sudden, but nobody remembers \"git ls-remote -h\" used to be the way\nto get short help for the command any more by that time, so nobody\nwill get hurt by such a flipping of the behaviour.\n\n"},{"id":"496169","messageId":"xmqq8qzl3mhg.fsf@gitster.g","threadId":"61562","inReplyTo":"xmqq34q0rnio.fsf@gitster.g","subject":"Re: Commands using -h as an option don't work consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-03T18:33:15Z","receivedAt":"2024-06-03T18:33:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Ah, then no.\n>\n> I am afraid that it is a non starter.  \"git ls-remote -h\" would then\n> start breaking existing users' long established expectation, i.e.,\n> \"for any git subcommand 'cmd', you can say 'git cmd -h' to get the\n> short help\".  We need to design a good transition plan to wean\n> existing users off of that expectation.\n\nAnother thing I forgot to mention.  For \"ls-remote\" and \"show-ref\",\nthere is a much nicer transition path we can take.  Back in Linus's\ndays, we called branches \"heads\" (and we used $GIT_DIR/refs/heads/\nto store them as individual files), but if we were starting Git from\nscratch today, there is no reason to.  Call branches branches, not\nheads, which means we do not need to use \"-h\" to mean \"I want to see\nonly the branches and no other refs\" at all.  You say \"--branches\"\ninstead.\n\nWhich leads to an obvious transition path, which is\n\n * Introduce \"--branches\" that sits next to \"--tags\" to \"ls-remote\"\n   and \"show-ref\".  If the command has \"-t\" as a short-hand for\n   \"--tags\", then add \"-b\" as a short-hand for \"--branches\".\n   Announce that \"--heads\" (and \"-h\" if supported) is deprecated for\n   these commands, and when \"--heads\" (and \"-h\" if supported) is\n   used, give a warning to train finger/muscle memory early.\n\n * We will keep things that way for a while, and then remove the\n   \"--heads\" (and \"-h\" if supported) from these two commands.\n\nThe approach cannot be used to transition \"git grep -h\" for obvious\nreasons, but then it makes no sense to give \"-h\" alone to tell the\ncommand \"I do not want to see filenames\", so that is not a problem.\n\nI checked output from \n\n    $ git grep -A2 '^-h' Documentation/\n\nand it seems that ls-remote is the only one that matters, so we do\nnot even have to do the \"-?\" transition at all.\n\nThanks.\n\n"}]}