{"thread":{"id":"62813","subject":"Git branch outputs usage message on stderr","startedAt":"2025-01-15T11:22:21Z","lastAt":"2025-01-16T10:24:27Z","messageCount":27,"participants":["Jonas Konrad","Matěj Cepl","Junio C Hamano","Kristoffer Haugsbakk","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"510565","messageId":"04cfaa3b-847f-4850-9dd6-c1cf9f72807f@uni-muenster.de","threadId":"62813","inReplyTo":null,"subject":"Git branch outputs usage message on stderr","fromName":"Jonas Konrad","fromEmail":"jonas.konrad@uni-muenster.de","sentAt":"2025-01-15T11:21:10Z","receivedAt":"2025-01-15T11:22:21Z","isPatch":false,"sender":{"key":"jonas.konrad@uni-muenster.de","avatar":null},"body":"Hey, please find the report below.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\nI opened a terminal on Arch Linux with a bash shell and called `git \nbranch -h` to get a usage overview of git's `branch` command. I then \ntried processing the output with `grep` by `git branch -h | grep list` \nwhich gave the whole (unfiltered) output, i.e., the displayed message \nwas not processed by `grep`.\n\nWhat did you expect to happen? (Expected behavior)\nPipe redirects stdout to grep (here: usage info of git branch) and grep \ncan filter it.\n\nWhat happened instead? (Actual behavior)\n`git branch -h` output was not redirected to `grep`. Instead, it went to \nstderr, bypassing the pipe. Using `|&` as pipe gives the expected result \nand confirms that the usage output is indeed redirected to stderr.\n\nWhat's different between what you expected and what actually happened?\nTo me, `git branch -h` (invoked without any non-valid option) shall \noutput its help/usage message to stdout, and exit without error status \n(exit status 0).\n\nAnything else you want to add:\nIn fact, `git branch -h` exits with a non-zero status. This is coherent \nwith outputting the usage message on stderr and made me think the \nbehavior could be intended. I can hardly imagine, as outputting usage \ninfo to stderr to me only made sense if `git branch` is invoked with, \ne.g., a non-valid option (if at all). I would prefer the usage output on \nstderr even in this case, but allow for a non-zero exit status of git \nbranch. Also, `git -h` gives its output on stdout.\n\n\n[System Info]\ngit version:\ngit version 2.48.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nlibcurl: 8.11.1\nOpenSSL: OpenSSL 3.4.0 22 Oct 2024\nzlib: 1.3.1\nuname: Linux 6.12.8-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 02 Jan 2025 \n22:52:26 +0000 x86_64\ncompiler info: gnuc: 14.2\nlibc info: glibc: 2.40\n$SHELL (typically, interactive shell): /bin/bash\n\nBest,\n\n-- \nJonas Konrad, PhD Student\nInstitute for Geoinformatics, University Münster\nHeisenbergstr. 2, 48149 Münster\n\n"},{"id":"510566","messageId":"D72M6S9O1E9F.WVEBV7ZJ1JTC@cepl.eu","threadId":"62813","inReplyTo":"04cfaa3b-847f-4850-9dd6-c1cf9f72807f@uni-muenster.de","subject":"Re: Git branch outputs usage message on stderr","fromName":"Matěj Cepl","fromEmail":"mcepl@cepl.eu","sentAt":"2025-01-15T11:36:15Z","receivedAt":"2025-01-15T11:36:21Z","isPatch":false,"sender":{"key":"mcepl@cepl.eu","avatar":"https://avatars.githubusercontent.com/u/198999?v=4"},"body":"On Wed Jan 15, 2025 at 12:22 PM CET, Jonas Konrad wrote:\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> I opened a terminal on Arch Linux with a bash shell and called `git \n> branch -h` to get a usage overview of git's `branch` command. I then \n> tried processing the output with `grep` by `git branch -h | grep list` \n> which gave the whole (unfiltered) output, i.e., the displayed message \n> was not processed by `grep`.\n\nAnd that is exactly the correct behaviour. In the world of UNIX,\nwhere pipes are normal, utilities should send to the stdout\nonly substantial material, which could be processed down the\npipeline. Error messages, help, and similar diagnostics, should\ngo to stderr. Also, you know about `|&`, right?\n\nBest,\n\nMatěj\n\n-- \nhttp://matej.ceplovi.cz/blog/, @mcepl@en.osm.town\nGPG Finger: 3C76 A027 CA45 AD70 98B5  BC1D 7920 5802 880B C9D8\n \nThou shalt not lie. Thou shalt not deceive one another.\n  -- Leviticus 19:11\n\n"},{"id":"510569","messageId":"17c78893-d200-4646-a81b-28f32bcf958e@uni-muenster.de","threadId":"62813","inReplyTo":"D72M6S9O1E9F.WVEBV7ZJ1JTC@cepl.eu","subject":"Re: Git branch outputs usage message on stderr","fromName":"Jonas Konrad","fromEmail":"jonas.konrad@uni-muenster.de","sentAt":"2025-01-15T14:47:56Z","receivedAt":"2025-01-15T14:48:01Z","isPatch":false,"sender":{"key":"jonas.konrad@uni-muenster.de","avatar":null},"body":"Well, let me add that one detail: I could not find any other git sub \ncommand which behaves the same (admittedly haven't tried all). Instead, \nthe ones I have tried give their help output usually on stdout. Try \nyourself with `git -h`. It perfectly acts according to what I'd expect \n(exit status 0). Where as `git reflog -h`outputs to stdout but still \nexit with non-zero status (which seems okay). `git branch -h` adds a \nthird way, as described. I argue: That diverging behavior is confusing \nto the user.\n\nIf your claim was right, this bug report got even bigger as a lot of \nbehavior from other subcommands would have to be changed. In fact, what \n\"substantial material\" is, has to be defined for every piece of software \nregarding each output. However, I cannot see how one subcommand's usage \nmessage is considered \"substantial\" whereas another subcommand's usage \nmessage is not (besides, I guess, `git branch` is more frequently used \nthan `git reflog`).\n\n\nBest,\nJonas\n\nOn 15.01.25 12:36, Matěj Cepl wrote:\n> On Wed Jan 15, 2025 at 12:22 PM CET, Jonas Konrad wrote:\n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>> I opened a terminal on Arch Linux with a bash shell and called `git\n>> branch -h` to get a usage overview of git's `branch` command. I then\n>> tried processing the output with `grep` by `git branch -h | grep list`\n>> which gave the whole (unfiltered) output, i.e., the displayed message\n>> was not processed by `grep`.\n> And that is exactly the correct behaviour. In the world of UNIX,\n> where pipes are normal, utilities should send to the stdout\n> only substantial material, which could be processed down the\n> pipeline. Error messages, help, and similar diagnostics, should\n> go to stderr. Also, you know about `|&`, right?\n>\n> Best,\n>\n> Matěj\n>\n"},{"id":"510573","messageId":"xmqqed1414gt.fsf@gitster.g","threadId":"62813","inReplyTo":"D72M6S9O1E9F.WVEBV7ZJ1JTC@cepl.eu","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T15:28:34Z","receivedAt":"2025-01-15T15:28:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matěj Cepl <mcepl@cepl.eu> writes:\n\n> On Wed Jan 15, 2025 at 12:22 PM CET, Jonas Konrad wrote:\n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>> I opened a terminal on Arch Linux with a bash shell and called `git \n>> branch -h` to get a usage overview of git's `branch` command. I then \n>> tried processing the output with `grep` by `git branch -h | grep list` \n>> which gave the whole (unfiltered) output, i.e., the displayed message \n>> was not processed by `grep`.\n>\n> And that is exactly the correct behaviour. In the world of UNIX,\n> where pipes are normal, utilities should send to the stdout\n> only substantial material, ...\n\nIf I understand the case Jonas reports correctly, he is talking\nabout \"git branch -h<RETURN>\", and the \"substantial material\" (I'd\nrather phrase it as the primary output in response to the end user\nrequest) in that case is the help text.\n\nSomebody may want to go over \"git help --all\" and for each of them\ntry \"git $cmd -h >/dev/null\" to find those that give the help output\nto their standard error stream.\n\nThanks, both.\n"},{"id":"510583","messageId":"c92e7b16-b70d-46f3-9858-2be805c5285f@app.fastmail.com","threadId":"62813","inReplyTo":"xmqqed1414gt.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-01-15T16:55:19Z","receivedAt":"2025-01-15T16:58:36Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Jan 15, 2025, at 16:28, Junio C Hamano wrote:\n> Somebody may want to go over \"git help --all\" and for each of them\n> try \"git $cmd -h >/dev/null\" to find those that give the help output\n> to their standard error stream.\n\n    #!/bin/sh\n\n    for cmd in $(git --list-cmds=builtins); do\n        git $cmd -h >/dev/null\n    done 2>&1 | grep '^usage: ' \\\n        | perl -pe 's/^usage:\\s*(\\(EXPERIMENTAL!\\)\\s*)?//; s/^(git\\s+[a-zA-Z0-9-]+).*/\\1/'\n\nGives\n\n    git am\n    git branch\n    git check-ref-format\n    git checkout--worker\n    git checkout-index\n    git commit\n    git commit-tree\n    git credential\n    git diff\n    git diff-files\n    git diff-index\n    git diff-tree\n    git fast-import\n    git fetch-pack\n    git fsmonitor--daemon\n    git gc\n    git get-tar-commit-id\n    git index-pack\n    git ls-files\n    git mailsplit\n    git merge\n    git merge-index\n    git merge-ours\n    git merge-recursive\n    git merge-recursive-ours\n    git merge-recursive-theirs\n    git merge-subtree\n    git pack-redundant\n    git rebase\n    git remote-ext\n    git remote-fd\n    git rev-list\n    git rev-parse\n    git status\n    git unpack-file\n    git unpack-objects\n    git update-index\n    git upload-archive\n    git upload-archive\n    git var\n\nFor\n\n    $ git version\n    git version 2.48.0\n"},{"id":"510585","messageId":"20250115171423.GB57018@coredump.intra.peff.net","threadId":"62813","inReplyTo":"c92e7b16-b70d-46f3-9858-2be805c5285f@app.fastmail.com","subject":"Re: Git branch outputs usage message on stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-15T17:14:23Z","receivedAt":"2025-01-15T17:14:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2025 at 05:55:19PM +0100, Kristoffer Haugsbakk wrote:\n\n> On Wed, Jan 15, 2025, at 16:28, Junio C Hamano wrote:\n> > Somebody may want to go over \"git help --all\" and for each of them\n> > try \"git $cmd -h >/dev/null\" to find those that give the help output\n> > to their standard error stream.\n> \n>     #!/bin/sh\n> \n>     for cmd in $(git --list-cmds=builtins); do\n>         git $cmd -h >/dev/null\n>     done 2>&1 | grep '^usage: ' \\\n>         | perl -pe 's/^usage:\\s*(\\(EXPERIMENTAL!\\)\\s*)?//; s/^(git\\s+[a-zA-Z0-9-]+).*/\\1/'\n> [...]\n\nWe may want:\n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 1d273d91c2..469cb12eb2 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -255,7 +255,8 @@ do\n \t\t(\n \t\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n \t\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n+\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err &&\n+\t\t\ttest_must_be_empty err\n \t\t) &&\n \t\ttest_grep usage output\n \t'\n\nwhich produces a similar list. In the case of git-branch, it is due to\n1dacfbcf13 (branch -h: show usage even in an invalid repository,\n2010-10-22). Instead of letting parse-options handle it (which then goes\nto stdout), we call usage_with_options(), which is usually for\ncomplaining about a broken option, not showing \"-h\".\n\nThe reason there is that some of the pre-parse_options() setup accesses\nthe ref store (causing a BUG() if you run \"branch -h\" outside of a\nrepository). In this case, I think it can simply be reordered like:\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6e7b0cfddb..4617e32fff 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -784,31 +784,28 @@ int cmd_branch(int argc,\n \tfilter.kind = FILTER_REFS_BRANCHES;\n \tfilter.abbrev = -1;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_branch_usage, options);\n-\n \t/*\n \t * Try to set sort keys from config. If config does not set any,\n \t * fall back on default (refname) sorting.\n \t */\n \tgit_config(git_branch_config, &sorting_options);\n \tif (!sorting_options.nr)\n \t\tstring_list_append(&sorting_options, \"refname\");\n \n \ttrack = git_branch_track;\n \n+\targc = parse_options(argc, argv, prefix, options, builtin_branch_usage,\n+\t\t\t     0);\n+\n \thead = refs_resolve_refdup(get_main_ref_store(the_repository), \"HEAD\",\n \t\t\t\t   0, &head_oid, NULL);\n \tif (!head)\n \t\tdie(_(\"failed to resolve HEAD as a valid ref\"));\n \tif (!strcmp(head, \"HEAD\"))\n \t\tfilter.detached = 1;\n \telse if (!skip_prefix(head, \"refs/heads/\", &head))\n \t\tdie(_(\"HEAD not found below refs/heads!\"));\n \n-\targc = parse_options(argc, argv, prefix, options, builtin_branch_usage,\n-\t\t\t     0);\n-\n \tif (!delete && !rename && !copy && !edit_description && !new_upstream &&\n \t    !show_current && !unset_upstream && argc == 0)\n \t\tlist = 1;\n\nKnowing that is safe means confirming manually that setup code is not\nneeded by parse_options(). E.g., if it were setting defaults the user\ncould overwrite with an option. In this case neither \"head\" nor\n\"filter.detached\" is touched by any of the options.\n\nBut there are a ton of commands, as you saw, and handling each one would\nbe a pain. So it would probably be easier to just introduce a variant of\nusage_with_options() that writes to stdout (the underlying _internal\nfunction already does so, we'd just need a one-liner wrapper). And then\nuse that everywhere. Possibly it could even do the argc/argv check, too,\nsince every call site is going to be doing that itself.\n\n-Peff\n"},{"id":"510586","messageId":"c8365a5f-bcda-40c5-bcf5-ebfd9a04ae64@uni-muenster.de","threadId":"62813","inReplyTo":"c92e7b16-b70d-46f3-9858-2be805c5285f@app.fastmail.com","subject":"Re: Git branch outputs usage message on stderr","fromName":"Jonas Konrad","fromEmail":"jonas.konrad@uni-muenster.de","sentAt":"2025-01-15T17:14:36Z","receivedAt":"2025-01-15T17:14:39Z","isPatch":false,"sender":{"key":"jonas.konrad@uni-muenster.de","avatar":null},"body":"On 15.01.25 17:55, Kristoffer Haugsbakk wrote:\n> On Wed, Jan 15, 2025, at 16:28, Junio C Hamano wrote:\n>> Somebody may want to go over \"git help --all\" and for each of them\n>> try \"git $cmd -h >/dev/null\" to find those that give the help output\n>> to their standard error stream.\n>      #!/bin/sh\n>\n>      for cmd in $(git --list-cmds=builtins); do\n>          git $cmd -h >/dev/null\n>      done 2>&1 | grep '^usage: ' \\\n>          | perl -pe 's/^usage:\\s*(\\(EXPERIMENTAL!\\)\\s*)?//; s/^(git\\s+[a-zA-Z0-9-]+).*/\\1/'\n>\n> Gives\n>\n>      git am\n>      [...]\n> For\n>\n>      $ git version\n>      git version 2.48.0\n\nWas just about to share the very same results, leading to 40 commands \nout of 142 built-ins outputting their usage info to stderr. Some \nadditions on non-builtins: git-scalar also outputs its usage info to \nstderr. git-lfs does not have \"usage text for -h\". I have not tested \ngit-svn and git-cvsserver properly (do not have installed the respective \nmodules). On another note, git-p4 does not know \"-h\", but then gives \nusage info - to stdout(!)). Lastly, if you still read, test \ngit-http-backend and git-filter-branch, as they show special behavior.\n\n"},{"id":"510587","messageId":"xmqq34hkyoys.fsf@gitster.g","threadId":"62813","inReplyTo":"c92e7b16-b70d-46f3-9858-2be805c5285f@app.fastmail.com","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T17:19:23Z","receivedAt":"2025-01-15T17:19:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <kristofferhaugsbakk@fastmail.com> writes:\n\n> On Wed, Jan 15, 2025, at 16:28, Junio C Hamano wrote:\n>> Somebody may want to go over \"git help --all\" and for each of them\n>> try \"git $cmd -h >/dev/null\" to find those that give the help output\n>> to their standard error stream.\n>\n>     #!/bin/sh\n>\n>     for cmd in $(git --list-cmds=builtins); do\n>         git $cmd -h >/dev/null\n>     done 2>&1 | grep '^usage: ' \\\n>         | perl -pe 's/^usage:\\s*(\\(EXPERIMENTAL!\\)\\s*)?//; s/^(git\\s+[a-zA-Z0-9-]+).*/\\1/'\n>\n> Gives ...\n\nBeing consistent is a good idea, and I wanted to first gauge which\nway we should unify.  It seems that those who spit their help text\ninto their standard error stream are indeed in minority?\n\nThanks.\n\n\n"},{"id":"510588","messageId":"0cf0b268-c691-4fed-a58b-ea9f77eab295@app.fastmail.com","threadId":"62813","inReplyTo":"xmqq34hkyoys.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-01-15T17:39:45Z","receivedAt":"2025-01-15T17:40:25Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Jan 15, 2025, at 18:19, Junio C Hamano wrote:\n> \"Kristoffer Haugsbakk\" <kristofferhaugsbakk@fastmail.com> writes:\n>> [snip]\n>\n> Being consistent is a good idea, and I wanted to first gauge which\n> way we should unify.  It seems that those who spit their help text\n> into their standard error stream are indeed in minority?\n\nYes: 40 of those stderr `-h` outputs.\n\nVersus 102 that use stdout.[1]\n\nTrying a random command with usage-on-error:\n\n    $ git-upload-pack >/dev/null\n    usage: [snip]\n\nDoes give usage on stderr.\n\n† 1:\n    git add\n    git annotate\n    git apply\n    git archive\n    git bisect\n    git blame\n    git bugreport\n    git bundle\n    git cat-file\n    git check-attr\n    git check-ignore\n    git check-mailmap\n    git checkout\n    git cherry\n    git cherry-pick\n    git clean\n    git clone\n    git column\n    git commit-graph\n    git config\n    git count-objects\n    git credential-cache\n    git credential-cache--daemon\n    git credential-store\n    git describe\n    git diagnose\n    git difftool\n    git fast-export\n    git fetch\n    git fmt-merge-msg\n    git for-each-ref\n    git for-each-repo\n    git format-patch\n    git fsck\n    git fsck\n    git grep\n    git hash-object\n    git help\n    git hook\n    git init\n    git init\n    git interpret-trailers\n    git log\n    git ls-remote\n    git ls-tree\n    git mailinfo\n    git maintenance\n    git merge-base\n    git merge-file\n    git merge-tree\n    git mktag\n    git mktree\n    git multi-pack-index\n    git mv\n    git name-rev\n    git notes\n    git pack-objects\n    git pack-refs\n    git patch-id\n    git blame\n    git prune\n    git prune-packed\n    git pull\n    git push\n    git range-diff\n    git read-tree\n    git receive-pack\n    git reflog\n    git refs\n    git remote\n    git repack\n    git replace\n    git replay\n    git rerere\n    git reset\n    git restore\n    git revert\n    git rm\n    git send-pack\n    git shortlog\n    git log\n    git show-branch\n    git show-index\n    git show-ref\n    git sparse-checkout\n    git add\n    git stash\n    git stripspace\n    git submodule--helper\n    git switch\n    git symbolic-ref\n    git tag\n    git update-ref\n    git update-server-info\n    git-upload-pack\n    git verify-commit\n    git verify-pack\n    git verify-tag\n    git version\n    git log\n    git worktree\n    git write-tree\n"},{"id":"510590","messageId":"xmqqy0zcx92y.fsf@gitster.g","threadId":"62813","inReplyTo":"0cf0b268-c691-4fed-a58b-ea9f77eab295@app.fastmail.com","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T17:47:49Z","receivedAt":"2025-01-15T17:47:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <kristofferhaugsbakk@fastmail.com> writes:\n\n> Trying a random command with usage-on-error:\n>\n>     $ git-upload-pack >/dev/null\n>     usage: [snip]\n>\n> Does give usage on stderr.\n\nGood; that is what we want to see.  Thanks.\n"},{"id":"510591","messageId":"xmqqtta0x90s.fsf@gitster.g","threadId":"62813","inReplyTo":"20250115171423.GB57018@coredump.intra.peff.net","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T17:49:07Z","receivedAt":"2025-01-15T17:49:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But there are a ton of commands, as you saw, and handling each one would\n> be a pain. So it would probably be easier to just introduce a variant of\n> usage_with_options() that writes to stdout (the underlying _internal\n> function already does so, we'd just need a one-liner wrapper).\n\nYup, I was looking at the code involved and came to the same conclusion.\n"},{"id":"510592","messageId":"6b6e5488-8a61-437f-acc8-e5868c4cc9cc@app.fastmail.com","threadId":"62813","inReplyTo":"c8365a5f-bcda-40c5-bcf5-ebfd9a04ae64@uni-muenster.de","subject":"Re: Git branch outputs usage message on stderr","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-01-15T17:53:52Z","receivedAt":"2025-01-15T17:54:33Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Jan 15, 2025, at 18:14, Jonas Konrad wrote:\n> On 15.01.25 17:55, Kristoffer Haugsbakk wrote:\n>> [snip]\n>\n> Was just about to share the very same results, leading to 40 commands\n> out of 142 built-ins outputting their usage info to stderr. Some\n> additions on non-builtins: git-scalar also outputs its usage info to\n> stderr. git-lfs does not have \"usage text for -h\". I have not tested\n> git-svn and git-cvsserver properly (do not have installed the respective\n> modules). On another note, git-p4 does not know \"-h\", but then gives\n> usage info - to stdout(!)). Lastly, if you still read, test\n> git-http-backend and git-filter-branch, as they show special behavior.\n\nOkay, `git-http-backend` is listed in `git --list-cmds=main`.\n\nThat one doesn’t support `-h`.\n\n    $ git http-backend -h >/dev/null\n    fatal: No REQUEST_METHOD from server\n\nand\n\n    $ git http-backend -h 2>/dev/null\n    Status: 500 Internal Server Error\n    Expires: Fri, 01 Jan 1980 00:00:00 GMT\n    Pragma: no-cache\n    Cache-Control: no-cache, max-age=0, must-revalidate\n\ngit-filter-branch(1) prints a warning about “glut of gotchas”, waits 10\nseconds, then prints usage to stdout.\n\n    $ time git filter-branch -h >/dev/null\n\n    real\t0m10,058s\n    user\t0m0,023s\n    sys\t0m0,041s\n\n-- \nKristoffer Haugsbakk\n"},{"id":"510593","messageId":"xmqqmsfsx8oo.fsf@gitster.g","threadId":"62813","inReplyTo":"20250115171423.GB57018@coredump.intra.peff.net","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T17:56:23Z","receivedAt":"2025-01-15T17:56:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> use that everywhere. Possibly it could even do the argc/argv check, too,\n> since every call site is going to be doing that itself.\n\nIt would look something like this; I am not sure if I like the \"this\nmay show help and exit if the user requested, but otherwise it is a\nno-op\" semantics, though.\n\n builtin/am.c    |  3 +--\n parse-options.c | 10 ++++++++++\n parse-options.h |  4 ++++\n 3 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git c/builtin/am.c w/builtin/am.c\nindex 370f5593f2..c9571f605a 100644\n--- c/builtin/am.c\n+++ w/builtin/am.c\n@@ -2417,8 +2417,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(usage, options);\n+\tshow_usage_help(argc, argv, usage, options);\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git c/parse-options.c w/parse-options.c\nindex 30b9e68f8a..9419b174de 100644\n--- c/parse-options.c\n+++ w/parse-options.c\n@@ -1276,6 +1276,16 @@ void NORETURN usage_with_options(const char * const *usagestr,\n \texit(129);\n }\n \n+void show_usage_help(int ac, const char **av,\n+\t\t     const char * const *usagestr,\n+\t\t     const struct option *opts)\n+{\n+\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n+\t\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n+\t\texit(0);\n+\t}\n+}\n+\n void NORETURN usage_msg_opt(const char *msg,\n \t\t   const char * const *usagestr,\n \t\t   const struct option *options)\ndiff --git c/parse-options.h w/parse-options.h\nindex ae15342390..75a7493350 100644\n--- c/parse-options.h\n+++ w/parse-options.h\n@@ -388,6 +388,10 @@ int parse_options(int argc, const char **argv, const char *prefix,\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n \n+void show_usage_help(int, const char **,\n+\t\t     const char * const *usagestr,\n+\t\t     const struct option *options);\n+\n NORETURN void usage_msg_opt(const char *msg,\n \t\t\t    const char * const *usagestr,\n \t\t\t    const struct option *options);\n"},{"id":"510594","messageId":"20250115182419.GA86610@coredump.intra.peff.net","threadId":"62813","inReplyTo":"xmqqmsfsx8oo.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-15T18:24:19Z","receivedAt":"2025-01-15T18:24:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2025 at 09:56:23AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > use that everywhere. Possibly it could even do the argc/argv check, too,\n> > since every call site is going to be doing that itself.\n> \n> It would look something like this; I am not sure if I like the \"this\n> may show help and exit if the user requested, but otherwise it is a\n> no-op\" semantics, though.\n\nYeah, I agree it is funny to have a \"maybe noop, maybe exit\" function.\nPerhaps a different name would help? I'd expect show_usage_help() to\nalways do what the name says. Maybe check_help_option() or something?\n\n> +void show_usage_help(int ac, const char **av,\n> +\t\t     const char * const *usagestr,\n> +\t\t     const struct option *opts)\n> +{\n> +\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n> +\t\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n> +\t\texit(0);\n> +\t}\n> +}\n\nI think parse-options will exit(129) in this case, and that's what t0012\ninsists upon.\n\n-Peff\n"},{"id":"510595","messageId":"xmqqikqgx74o.fsf@gitster.g","threadId":"62813","inReplyTo":"xmqqmsfsx8oo.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T18:29:59Z","receivedAt":"2025-01-15T18:30:04Z","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> Jeff King <peff@peff.net> writes:\n>\n>> use that everywhere. Possibly it could even do the argc/argv check, too,\n>> since every call site is going to be doing that itself.\n>\n> It would look something like this; I am not sure if I like the \"this\n> may show help and exit if the user requested, but otherwise it is a\n> no-op\" semantics, though.\n\nAfter thinking about it a bit more, I am starting to like it, not\nbecause it reduces a few more lines from the calling site, but\nbecause it makes it almost impossible to use for a careless and\nincorrect conversion from usage_with_options().  Any existing\ncallsite of usage_with_options() MUST be inspected to make sure it\nis responding to an end-user request to give \"-h\"(elp) text before\nbeing replaced with the call to a new helper, and if I made\nshow_usage_help() without argc/argv, a careless developer can easily\nand blindly do string replacement to send a ton of patch that\nreviewers need to waste time to do the inspection for them.\n\nBut with your \"argc and argv checking included\" approach, it is\nharder to do the blind replacement.\n\n--- >8 ---\nFrom: Junio C Hamano <gitster@pobox.com>\nDate: Wed, 15 Jan 2025 09:56:23 -0800\nSubject: [PATCH] parse-options: add show_usage_help()\n\nMany commands call usage_with_options() when they are asked to give\nthe help message, but it incorrectly sends the help text to the\nstandard error stream.  When the user asked for it with \"git cmd -h\",\nthe help message is the primary output from the command, hence we\nshould send it to the standard output stream.\n\nIntroduce a helper function that captures the common pattern\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage_with_options(usage, options);\n\nand replaces it with\n\n\tshow_usage_help(argc, argv, usage, options);\n\nto help correct code paths (there are 40 or so of them).\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n parse-options.c | 10 ++++++++++\n parse-options.h |  4 ++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 33bfba0ed4..4b00065692 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1282,6 +1282,16 @@ void NORETURN usage_with_options(const char * const *usagestr,\n \texit(129);\n }\n \n+void show_usage_help(int ac, const char **av,\n+\t\t     const char * const *usagestr,\n+\t\t     const struct option *opts)\n+{\n+\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n+\t\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n+\t\texit(0);\n+\t}\n+}\n+\n void NORETURN usage_msg_opt(const char *msg,\n \t\t   const char * const *usagestr,\n \t\t   const struct option *options)\ndiff --git a/parse-options.h b/parse-options.h\nindex d01361ca97..f93f13434c 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -402,6 +402,10 @@ int parse_options(int argc, const char **argv, const char *prefix,\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n \n+void show_usage_help(int, const char **,\n+\t\t     const char * const *usagestr,\n+\t\t     const struct option *options);\n+\n NORETURN void usage_msg_opt(const char *msg,\n \t\t\t    const char * const *usagestr,\n \t\t\t    const struct option *options);\n-- \n2.48.1-187-gd93ffc6ef3\n\n"},{"id":"510596","messageId":"a543c92d-215a-4cc1-a7a3-bcb34c62f33d@app.fastmail.com","threadId":"62813","inReplyTo":"xmqqikqgx74o.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-01-15T18:33:46Z","receivedAt":"2025-01-15T18:36:15Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Jan 15, 2025, at 19:29, Junio C Hamano wrote:\n> From: Junio C Hamano <gitster@pobox.com>\n> Date: Wed, 15 Jan 2025 09:56:23 -0800\n> Subject: [PATCH] parse-options: add show_usage_help()\n>\n> Many commands call usage_with_options() when they are asked to give\n> the help message, but it incorrectly sends the help text to the\n> standard error stream.  When the user asked for it with \"git cmd -h\",\n> the help message is the primary output from the command, hence we\n> should send it to the standard output stream.\n>\n> Introduce a helper function that captures the common pattern\n>\n> \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n> \t\tusage_with_options(usage, options);\n>\n> and replaces it with\n>\n> \tshow_usage_help(argc, argv, usage, options);\n>\n> to help correct code paths (there are 40 or so of them).\n>\n> Suggested-by: Jeff King <peff@peff.net>\n\n+Reported-by: Jonas Konrad <jonas.konrad@uni-muenster.de>\n\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\n-- \nKristoffer Haugsbakk\n"},{"id":"510609","messageId":"xmqqed13ye3t.fsf@gitster.g","threadId":"62813","inReplyTo":"a543c92d-215a-4cc1-a7a3-bcb34c62f33d@app.fastmail.com","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T21:13:58Z","receivedAt":"2025-01-15T21:14:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <kristofferhaugsbakk@fastmail.com> writes:\n\n>> and replaces it with\n>>\n>> \tshow_usage_help(argc, argv, usage, options);\n>>\n>> to help correct code paths (there are 40 or so of them).\n>>\n>> Suggested-by: Jeff King <peff@peff.net>\n>\n> +Reported-by: Jonas Konrad <jonas.konrad@uni-muenster.de>\n\nNot on this patch, but it would be a good addition to a follow-up\npatch that uses this new API function to update builtin/branch.c\n(which I do not plan to do myself).\n\nThanks.\n"},{"id":"510610","messageId":"xmqqa5brydz1.fsf@gitster.g","threadId":"62813","inReplyTo":"20250115182419.GA86610@coredump.intra.peff.net","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T21:16:50Z","receivedAt":"2025-01-15T21:16:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yeah, I agree it is funny to have a \"maybe noop, maybe exit\" function.\n> Perhaps a different name would help? I'd expect show_usage_help() to\n> always do what the name says. Maybe check_help_option() or something?\n\nmaybe_show_usage_help()?\n\n>> +void show_usage_help(int ac, const char **av,\n>> +\t\t     const char * const *usagestr,\n>> +\t\t     const struct option *opts)\n>> +{\n>> +\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n>> +\t\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n>> +\t\texit(0);\n>> +\t}\n>> +}\n>\n> I think parse-options will exit(129) in this case, and that's what t0012\n> insists upon.\n\nYeah, but the test can be adjusted to updated reality if needed.  \n\nIn this case, the command is doing what the end-user asked it to do,\nand if we were writing the system from scratch, 0 would certainly be\nthe right exit status in this case.  If hit usage_with_options()\nbecause the command line option supplied by the user was nonsense,\nwe should exit with non-zero, but I am not sure if exit(129) is a\ngood idea here.\n"},{"id":"510611","messageId":"20250115212952.GA96537@coredump.intra.peff.net","threadId":"62813","inReplyTo":"xmqqa5brydz1.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-15T21:29:52Z","receivedAt":"2025-01-15T21:29:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2025 at 01:16:50PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Yeah, I agree it is funny to have a \"maybe noop, maybe exit\" function.\n> > Perhaps a different name would help? I'd expect show_usage_help() to\n> > always do what the name says. Maybe check_help_option() or something?\n> \n> maybe_show_usage_help()?\n\nHeh, I almost suggested that one, too, but worried it was too clunky.\nBut maybe since we both thought of it...\n\n> > I think parse-options will exit(129) in this case, and that's what t0012\n> > insists upon.\n> \n> Yeah, but the test can be adjusted to updated reality if needed.  \n> \n> In this case, the command is doing what the end-user asked it to do,\n> and if we were writing the system from scratch, 0 would certainly be\n> the right exit status in this case.  If hit usage_with_options()\n> because the command line option supplied by the user was nonsense,\n> we should exit with non-zero, but I am not sure if exit(129) is a\n> good idea here.\n\nI certainly see an argument for exit(0), but whatever we do should be\nconsistent with how parse-options handles it (since whether we use this\nor leave it to parse-options is purely an implementation detail that the\nuser should not need to be aware of).\n\nAnd it uses code 129, even for \"-h\". I don't see any explicit rationale\nfor that in the history; I think it goes back to the beginning of\nparse-options. It happens via the PARSE_OPT_HELP flag, but curiously we\nalso trigger that for ambiguous options (which should exit with error).\nThat might be a bug-in-waiting if we start handling PARSE_OPT_HELP\ndifferently.\n\n-Peff\n"},{"id":"510612","messageId":"xmqq5xmfyc4w.fsf@gitster.g","threadId":"62813","inReplyTo":"20250115212952.GA96537@coredump.intra.peff.net","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T21:56:31Z","receivedAt":"2025-01-15T21:56:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I certainly see an argument for exit(0), but whatever we do should be\n> consistent with how parse-options handles it (since whether we use this\n> or leave it to parse-options is purely an implementation detail that the\n> user should not need to be aware of).\n>\n> And it uses code 129, even for \"-h\". I don't see any explicit rationale\n> for that in the history; I think it goes back to the beginning of\n> parse-options. It happens via the PARSE_OPT_HELP flag, but curiously we\n> also trigger that for ambiguous options (which should exit with error).\n> That might be a bug-in-waiting if we start handling PARSE_OPT_HELP\n> differently.\n\nHere is what I have as v2; there will be patches that touch\nbuiltin/*.c in between and I expect that the last patch to conclude\nthe series will end with an update to parse-options.c (to exit with\n0 when asked to give a help) and t0012 (to stop expecting 129).\n\n--- >8 ---\nSubject: [PATCH v2] parse-options: add show_usage_help_and_exit_if_asked()\n\nMany commands call usage_with_options() when they are asked to give\nthe help message, but it incorrectly sends the help text to the\nstandard error stream.  When the user asked for it with \"git cmd -h\",\nthe help message is the primary output from the command, hence we\nshould send it to the standard output stream.\n\nIntroduce a helper function that captures the common pattern\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage_with_options(usage, options);\n\nand replaces it with\n\n\tshow_usage_help_and_exit_if_asked(argc, argv, usage, options);\n\nto help correct code paths (there are 40 or so of them).\n\nNote that this helper function still exits with status 129, and\nt0012 insists on it.  After converting all the mistaken callers of\nusage_with_options() to call this new helper, we may want to address\nit---the end user is asking us to give the help text, and we are\ndoing exactly as asked, so there is no reason to exit with non-zero\nstatus.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n parse-options.c | 10 ++++++++++\n parse-options.h |  4 ++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 33bfba0ed4..8a8b934e67 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1282,6 +1282,16 @@ void NORETURN usage_with_options(const char * const *usagestr,\n \texit(129);\n }\n \n+void show_usage_help_and_exit_if_asked(int ac, const char **av,\n+\t\t\t\t       const char * const *usagestr,\n+\t\t\t\t       const struct option *opts)\n+{\n+\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n+\t\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n+\t\texit(129);\n+\t}\n+}\n+\n void NORETURN usage_msg_opt(const char *msg,\n \t\t   const char * const *usagestr,\n \t\t   const struct option *options)\ndiff --git a/parse-options.h b/parse-options.h\nindex d01361ca97..6af4ee148a 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -402,6 +402,10 @@ int parse_options(int argc, const char **argv, const char *prefix,\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n \n+void show_usage_help_and_exit_if_asked(int ac, const char **av,\n+\t\t\t\t      const char * const *usage,\n+\t\t\t\t      const struct option *options);\n+\n NORETURN void usage_msg_opt(const char *msg,\n \t\t\t    const char * const *usagestr,\n \t\t\t    const struct option *options);\n-- \n2.48.1-187-gd93ffc6ef3\n\n"},{"id":"510613","messageId":"xmqq1px3ybf7.fsf@gitster.g","threadId":"62813","inReplyTo":"20250115212952.GA96537@coredump.intra.peff.net","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T22:11:56Z","receivedAt":"2025-01-15T22:11:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> And it uses code 129, even for \"-h\". I don't see any explicit rationale\n> for that in the history; I think it goes back to the beginning of\n> parse-options. It happens via the PARSE_OPT_HELP flag, but curiously we\n> also trigger that for ambiguous options (which should exit with error).\n> That might be a bug-in-waiting if we start handling PARSE_OPT_HELP\n> differently.\n\nThere is another class of callers that are protected by the same\n\"argc == 2 && !strcmp(argv[1], \"-h\")\" condition, and they call\nusage.c:usage(), instead of calling usage_with_options().  These\ncalls (but not all calls to usage()) need to be updated to use a\nsimilar helper, say, show_usage_and_exit_if_asked().  Sigh...\n\n"},{"id":"510614","messageId":"20250115222728.GA132248@coredump.intra.peff.net","threadId":"62813","inReplyTo":"xmqq5xmfyc4w.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-15T22:27:28Z","receivedAt":"2025-01-15T22:27:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2025 at 01:56:31PM -0800, Junio C Hamano wrote:\n\n> Here is what I have as v2; there will be patches that touch\n> builtin/*.c in between and I expect that the last patch to conclude\n> the series will end with an update to parse-options.c (to exit with\n> 0 when asked to give a help) and t0012 (to stop expecting 129).\n> \n> --- >8 ---\n> Subject: [PATCH v2] parse-options: add show_usage_help_and_exit_if_asked()\n\nThanks, this looks fine. The name is clunky but probably OK. ;)\n\nI don't know if we'd want something like this on top. If somebody is\ninterested in just doing all the conversions in the near-term, we could\ndo without the optional flag.\n\n-- >8 --\nSubject: [PATCH] t0012: optionally check that \"-h\" output goes to stdout\n\nFor most commands, \"git foo -h\" will send the help output to stdout, as\nthis is what parse-options.c does. But some commands send it to stderr\ninstead. This is usually because they call usage_with_options(), and\nshould be switched to show_usage_help_and_exit_if_asked().\n\nCurrently t0012 is permissive and allows either behavior. We'd like it\nto eventually enforce that help goes to stdout, and teaching it to do so\nidentifies the commands that need to be changed. But during the\ntransition period, we don't want to enforce that for most test runs.\n\nSo let's introduce a flag that will let most test runs use the\npermissive behavior, and people interested in converting commands can\nrun:\n\n  GIT_TEST_HELP_MUST_BE_STDOUT=1 ./t0012-help.sh\n\nto see the failures. Eventually (when all builtins have been converted)\nwe'll remove this flag entirely and always check the strict behavior.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0012-help.sh | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 1d273d91c2..9c7ae9fd36 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -255,9 +255,16 @@ do\n \t\t(\n \t\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n \t\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n+\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err\n \t\t) &&\n-\t\ttest_grep usage output\n+\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n+\t\tthen\n+\t\t\ttest_must_be_empty err &&\n+\t\t\ttest_grep usage output\n+\t\telse\n+\t\t\ttest_grep usage output ||\n+\t\t\ttest_grep usage err\n+\t\tfi\n \t'\n done <builtins\n \n-- \n2.48.1.434.g4084d8f956\n\n"},{"id":"510615","messageId":"20250115222840.GB132248@coredump.intra.peff.net","threadId":"62813","inReplyTo":"xmqq1px3ybf7.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-15T22:28:40Z","receivedAt":"2025-01-15T22:28:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2025 at 02:11:56PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > And it uses code 129, even for \"-h\". I don't see any explicit rationale\n> > for that in the history; I think it goes back to the beginning of\n> > parse-options. It happens via the PARSE_OPT_HELP flag, but curiously we\n> > also trigger that for ambiguous options (which should exit with error).\n> > That might be a bug-in-waiting if we start handling PARSE_OPT_HELP\n> > differently.\n> \n> There is another class of callers that are protected by the same\n> \"argc == 2 && !strcmp(argv[1], \"-h\")\" condition, and they call\n> usage.c:usage(), instead of calling usage_with_options().  These\n> calls (but not all calls to usage()) need to be updated to use a\n> similar helper, say, show_usage_and_exit_if_asked().  Sigh...\n\nOof. And that uses vreportf(), which always writes to stderr. So more\nrefactoring.\n\n-Peff\n"},{"id":"510619","messageId":"xmqqplknvek2.fsf@gitster.g","threadId":"62813","inReplyTo":"20250115222728.GA132248@coredump.intra.peff.net","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T23:32:29Z","receivedAt":"2025-01-15T23:32:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I don't know if we'd want something like this on top. If somebody is\n> interested in just doing all the conversions in the near-term, we could\n> do without the optional flag.\n\nAh, you are much more practical than I am ;-)  I was wondering if we\nwant a list of \"these commands have already been updated\" and behave\ndifferently.\n\n> Currently t0012 is permissive and allows either behavior. We'd like it\n> to eventually enforce that help goes to stdout, and teaching it to do so\n> identifies the commands that need to be changed. But during the\n> transition period, we don't want to enforce that for most test runs.\n\nYeah, that is why the \"git branch -h\" thing has been left broken for\nsuch a long time.\n\n> So let's introduce a flag that will let most test runs use the\n> permissive behavior, and people interested in converting commands can\n> run:\n>\n>   GIT_TEST_HELP_MUST_BE_STDOUT=1 ./t0012-help.sh\n>\n> to see the failures. Eventually (when all builtins have been converted)\n> we'll remove this flag entirely and always check the strict behavior.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t0012-help.sh | 11 +++++++++--\n>  1 file changed, 9 insertions(+), 2 deletions(-)\n\nNice.\n\nIt is a tangent, but I wonder how many among the 40 really needed to\nuse usage_with_options() to react to \"-h\" in the first place.  In\nother words, these manual checks for \"-h\" are done only because the\ncode _wants_ to react to \"-h\" before it calls parse_options(), but\ndoes everybody who _wants_ to do so really _needs_ to do so?  You\nalready have shown that \"gir branch\" did not have to, and to me, 40\namong 100+ felt way too many.\n\n> diff --git a/t/t0012-help.sh b/t/t0012-help.sh\n> index 1d273d91c2..9c7ae9fd36 100755\n> --- a/t/t0012-help.sh\n> +++ b/t/t0012-help.sh\n> @@ -255,9 +255,16 @@ do\n>  \t\t(\n>  \t\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n>  \t\t\texport GIT_CEILING_DIRECTORIES &&\n> -\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n> +\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err\n>  \t\t) &&\n> -\t\ttest_grep usage output\n> +\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n> +\t\tthen\n> +\t\t\ttest_must_be_empty err &&\n\nThis may be a bit stricter than needed (things other than usage may\nneed to be spitted out), but it is sufficent to declare that we will\ndeal with any potential fallout only after it becomes necessary ;-).\n\n> +\t\t\ttest_grep usage output\n> +\t\telse\n> +\t\t\ttest_grep usage output ||\n> +\t\t\ttest_grep usage err\n> +\t\tfi\n>  \t'\n>  done <builtins\n\nWill queue.  Thanks.\n"},{"id":"510620","messageId":"xmqqldvbvefn.fsf@gitster.g","threadId":"62813","inReplyTo":"20250115222840.GB132248@coredump.intra.peff.net","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-15T23:35:08Z","receivedAt":"2025-01-15T23:35:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jan 15, 2025 at 02:11:56PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > And it uses code 129, even for \"-h\". I don't see any explicit rationale\n>> > for that in the history; I think it goes back to the beginning of\n>> > parse-options. It happens via the PARSE_OPT_HELP flag, but curiously we\n>> > also trigger that for ambiguous options (which should exit with error).\n>> > That might be a bug-in-waiting if we start handling PARSE_OPT_HELP\n>> > differently.\n>> \n>> There is another class of callers that are protected by the same\n>> \"argc == 2 && !strcmp(argv[1], \"-h\")\" condition, and they call\n>> usage.c:usage(), instead of calling usage_with_options().  These\n>> calls (but not all calls to usage()) need to be updated to use a\n>> similar helper, say, show_usage_and_exit_if_asked().  Sigh...\n>\n> Oof. And that uses vreportf(), which always writes to stderr. So more\n> refactoring.\n\nYes, it's ugly ;-)\n\nIn any case, this is a result of my sweek in builtin/ for\nusage_with_options().  The remaining\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\nare all protecting usage() call.\n\n---- >8 ----\nSubject: [PATCH] builtins: send help text to standard output\n\nUsing the show_usage_help_and_exit_if_asked() helper we introduced\nearlier, fix callers of usage_with_options() that want to show the\nhelp text when explicitly asked by the end-user.  The help text now\ngoes to the standard output stream for them.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/am.c                | 3 +--\n builtin/branch.c            | 4 ++--\n builtin/checkout--worker.c  | 6 +++---\n builtin/checkout-index.c    | 6 +++---\n builtin/commit-tree.c       | 4 ++--\n builtin/commit.c            | 8 ++++----\n builtin/fsmonitor--daemon.c | 4 ++--\n builtin/gc.c                | 4 ++--\n builtin/ls-files.c          | 4 ++--\n builtin/merge.c             | 4 ++--\n builtin/rebase.c            | 6 +++---\n builtin/update-index.c      | 4 ++--\n t/t7600-merge.sh            | 2 +-\n 13 files changed, 29 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1338b606fe..0801b556c2 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2427,8 +2427,7 @@ int cmd_am(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv, usage, options);\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6e7b0cfddb..366729a78b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -784,8 +784,8 @@ int cmd_branch(int argc,\n \tfilter.kind = FILTER_REFS_BRANCHES;\n \tfilter.abbrev = -1;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_branch_usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  builtin_branch_usage, options);\n \n \t/*\n \t * Try to set sort keys from config. If config does not set any,\ndiff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c\nindex b81002a1df..7093d1efd5 100644\n--- a/builtin/checkout--worker.c\n+++ b/builtin/checkout--worker.c\n@@ -128,9 +128,9 @@ int cmd_checkout__worker(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(checkout_worker_usage,\n-\t\t\t\t   checkout_worker_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  checkout_worker_usage,\n+\t\t\t\t\t  checkout_worker_options);\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, checkout_worker_options,\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex a81501098d..d928d6b5e3 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -250,9 +250,9 @@ int cmd_checkout_index(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_checkout_index_usage,\n-\t\t\t\t   builtin_checkout_index_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  builtin_checkout_index_usage,\n+\t\t\t\t\t  builtin_checkout_index_options);\n \tgit_config(git_default_config, NULL);\n \tprefix_length = prefix ? strlen(prefix) : 0;\n \ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 2ca1a57ebb..2efc224d32 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -119,8 +119,8 @@ int cmd_commit_tree(int argc,\n \n \tgit_config(git_default_config, NULL);\n \n-\tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(commit_tree_usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  commit_tree_usage, options);\n \n \targc = parse_options(argc, argv, prefix, options, commit_tree_usage, 0);\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex ef5e622c07..4268915120 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1559,8 +1559,8 @@ struct repository *repo UNUSED)\n \t\tOPT_END(),\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_status_usage, builtin_status_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  builtin_status_usage, builtin_status_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\n@@ -1736,8 +1736,8 @@ int cmd_commit(int argc,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  builtin_commit_usage, builtin_commit_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex 029dc64d6c..dabf190bbe 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1598,8 +1598,8 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix UNUSED\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_fsmonitor__daemon_usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  builtin_fsmonitor__daemon_usage, options);\n \n \tdie(_(\"fsmonitor--daemon not supported on this platform\"));\n }\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex a9b1c36de2..5f831e1f94 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -710,8 +710,8 @@ struct repository *repo UNUSED)\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  builtin_gc_usage, builtin_gc_options);\n \n \tstrvec_pushl(&reflog, \"reflog\", \"expire\", \"--all\", NULL);\n \tstrvec_pushl(&repack, \"repack\", \"-d\", \"-l\", NULL);\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 15499cd12b..9efe92b7c0 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -644,8 +644,8 @@ int cmd_ls_files(int argc,\n \t};\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(ls_files_usage, builtin_ls_files_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  ls_files_usage, builtin_ls_files_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5f67007bba..95d798fc89 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1300,8 +1300,8 @@ int cmd_merge(int argc,\n \tvoid *branch_to_free;\n \tint orig_argc = argc;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  builtin_merge_usage, builtin_merge_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0498fff3c9..cb49323c44 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1223,9 +1223,9 @@ int cmd_rebase(int argc,\n \t};\n \tint i;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_rebase_usage,\n-\t\t\t\t   builtin_rebase_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  builtin_rebase_usage,\n+\t\t\t\t\t  builtin_rebase_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 74bbad9f87..b0e2ad4970 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1045,8 +1045,8 @@ int cmd_update_index(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(update_index_usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t  update_index_usage, options);\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex ef54cff4fa..2a8df29219 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -173,7 +173,7 @@ test_expect_success 'merge -h with invalid index' '\n \t\tcd broken &&\n \t\tgit init &&\n \t\t>.git/index &&\n-\t\ttest_expect_code 129 git merge -h 2>usage\n+\t\ttest_expect_code 129 git merge -h >usage\n \t) &&\n \ttest_grep \"[Uu]sage: git merge\" broken/usage\n '\n-- \n2.48.1-187-gd93ffc6ef3\n\n"},{"id":"510623","messageId":"xmqq7c6vv9hp.fsf@gitster.g","threadId":"62813","inReplyTo":"xmqqplknvek2.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T01:21:54Z","receivedAt":"2025-01-16T01:21:57Z","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> It is a tangent, but I wonder how many among the 40 really needed to\n> use usage_with_options() to react to \"-h\" in the first place.  In\n> other words, these manual checks for \"-h\" are done only because the\n> code _wants_ to react to \"-h\" before it calls parse_options(), but\n> does everybody who _wants_ to do so really _needs_ to do so?  You\n> already have shown that \"gir branch\" did not have to, and to me, 40\n> among 100+ felt way too many.\n\nIt turns out that some of the actually cannot be helped, as they do\nnot even use parse-options and calling usage() themselves is the\nonly way to show the usage text for them.\n\nI'll send a 6-patch series out shortly.\n"},{"id":"510674","messageId":"20250116102426.GA773990@coredump.intra.peff.net","threadId":"62813","inReplyTo":"xmqqplknvek2.fsf@gitster.g","subject":"Re: Git branch outputs usage message on stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-16T10:24:26Z","receivedAt":"2025-01-16T10:24:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2025 at 03:32:29PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I don't know if we'd want something like this on top. If somebody is\n> > interested in just doing all the conversions in the near-term, we could\n> > do without the optional flag.\n> \n> Ah, you are much more practical than I am ;-)  I was wondering if we\n> want a list of \"these commands have already been updated\" and behave\n> differently.\n\nHeh, I started writing it that way but then got turned off by how\nannoying it is to look up in a list of strings in bash. ;)\n\n> It is a tangent, but I wonder how many among the 40 really needed to\n> use usage_with_options() to react to \"-h\" in the first place.  In\n> other words, these manual checks for \"-h\" are done only because the\n> code _wants_ to react to \"-h\" before it calls parse_options(), but\n> does everybody who _wants_ to do so really _needs_ to do so?  You\n> already have shown that \"gir branch\" did not have to, and to me, 40\n> among 100+ felt way too many.\n\nYes, I had the same thought. Unfortunately it is a lot of brain-power to\nexamine each one, with relatively little gain.\n\n> > -\t\ttest_grep usage output\n> > +\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n> > +\t\tthen\n> > +\t\t\ttest_must_be_empty err &&\n> \n> This may be a bit stricter than needed (things other than usage may\n> need to be spitted out), but it is sufficent to declare that we will\n> deal with any potential fallout only after it becomes necessary ;-).\n\nYep. I'd hope for the most part that \"-h\" would spew help and nothing\nelse, but I guess we could get hit with a deprecation notice or\nsomething. I agree on crossing that bridge when we come to it.\n\n-Peff\n"}]}