{"thread":{"id":"47195","subject":"[PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","startedAt":"2017-11-13T21:07:52Z","lastAt":"2017-11-15T21:09:30Z","messageCount":16,"participants":["Todd Zullinger","Santiago Torres","Junio C Hamano","Johan Herland"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"332447","messageId":"20171113210745.24638-1-tmz@pobox.com","threadId":"47195","inReplyTo":null,"subject":"[PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-11-13T21:07:45Z","receivedAt":"2017-11-13T21:07:52Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"In 29ff1f8f74 (t: lib-gpg: flush gpg agent on startup, 2017-07-20), a\ncall to gpgconf was added to kill the gpg-agent.  The intention was to\nignore all output from the call, but the order of the redirection needs\nto be switched to ensure that both stdout and stderr are redirected to\n/dev/null.  Without this, gpgconf from gnupg-2.0 releases would output\n'gpgconf: invalid option \"--kill\"' each time it was called.\n\nSigned-off-by: Todd Zullinger <tmz@pobox.com>\n---\n\nI noticed that gpgconf produced error output for a number of tests on\nCentOS/RHEL.  As an example:\n\n    *** t5534-push-signed.sh ***\n    gpgconf: invalid option \"--kill\"\n\nLooking at the code in lib-gpg.sh, it appeared the intention was to ignore this\noutput.  Reading through the review of the patch confirmed that feeling[1].  The\ncurrent code gets caught by the subtleties of output redirection.  (Who hasn't\nbeen burned at some point by the difference between '2>&1 >/dev/null' and\n'>/dev/null 2>&1' ? ;)\n\n[1] https://public-inbox.org/git/xmqq379qlvzi.fsf@gitster.mtv.corp.google.com/\n\nOf course, beyond getting stderr to /dev/null, there is the fact that on\nversions of gnupg < 2.1, gpgconf --kill is not available.  I noticed this with\ngnupg-2.0.14 on CentOS 6.  It also occurs on CentOS 7, which provides\ngnupg-2.0.22.\n\nI don't know if there's much value in trying to better handle older gnupg-2.0\nsystems.  Using gpgconf --reload might be sufficient to work on gnupg-2.0 and\nnewer systems.  That might solve the issues with gpg-agent caching stale file\nhandles that motivated the initial patch.  If not, finding what works well with\nboth gnupg-2.0 and newer seems mildly painful.  This method works for me on\n2.0, 2.1, and 2.2:\n\n    pid=$(echo GETINFO pid | gpg-connect-agent | awk '/^D / {print $2}')\n\nAnd people say crypto tools aren't intuitive.  Pfff. :/\n\nA fairly gross way to use that in lib-gpg.sh might be:\n\n    diff --git c/t/lib-gpg.sh w/t/lib-gpg.sh\n    index 43679a4c64..c91d9b334f 100755\n    --- c/t/lib-gpg.sh\n    +++ w/t/lib-gpg.sh\n    @@ -1,5 +1,10 @@\n     #!/bin/sh\n\n    +gpg_killagent() {\n    +\tpid=$(echo GETINFO pid | gpg-connect-agent | awk '/^D / {print $2}')\n    +\ttest -n \"$pid\" && kill \"$pid\"\n    +}\n    +\n     gpg_version=$(gpg --version 2>&1)\n     if test $? != 127\n     then\n    @@ -31,7 +36,7 @@ then\n     \t\tchmod 0700 ./gpghome &&\n     \t\tGNUPGHOME=\"$(pwd)/gpghome\" &&\n     \t\texport GNUPGHOME &&\n    -\t\t(gpgconf --kill gpg-agent 2>&1 >/dev/null || : ) &&\n    +\t\t(gpg_killagent >/dev/null 2>&1 || : ) &&\n     \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n     \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n     \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n\nI have my doubts that doing something like the above is worthwhile.  It's\nprobably good enough to simply fix up the gpgconf stderr redirection and call\nit a day.\n\nI haven't seen any gpg failures in the test runs I've done, so I haven't had a\nneed to re-run those tests.  (I also have not run the test suite with the ugly\ngpg_killagent() diff above.  I have run it with the change to fix stderr\nredirection and confirmed it succeeds without the gpgconf error messages.)\n\nLastly, I also noticed that git-rebase.sh uses the same 2>&1 >/dev/null.  I\nsuspect it's similarly not intentional:\n\n    $ git grep -h -C4 '2>&1 >/dev/null' -- git-rebase.sh\n    apply_autostash () {\n    \tif test -f \"$state_dir/autostash\"\n    \tthen\n    \t\tstash_sha1=$(cat \"$state_dir/autostash\")\n    \t\tif git stash apply $stash_sha1 2>&1 >/dev/null\n    \t\tthen\n    \t\t\techo \"$(gettext 'Applied autostash.')\" >&2\n    \t\telse\n    \t\t\tgit stash store -m \"autostash\" -q $stash_sha1 ||\n\nI'll send a separate patch to adjust that code as well.\n\n t/lib-gpg.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 43679a4c64..a5d3b2cbaa 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -31,7 +31,7 @@ then\n \t\tchmod 0700 ./gpghome &&\n \t\tGNUPGHOME=\"$(pwd)/gpghome\" &&\n \t\texport GNUPGHOME &&\n-\t\t(gpgconf --kill gpg-agent 2>&1 >/dev/null || : ) &&\n+\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n-- \n2.15.0\n\n"},{"id":"332459","messageId":"20171113221823.jzt7jfhxeuyivbcn@LykOS.localdomain","threadId":"47195","inReplyTo":"20171113210745.24638-1-tmz@pobox.com","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-11-13T22:18:24Z","receivedAt":"2017-11-13T22:17:18Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":" \n> Of course, beyond getting stderr to /dev/null, there is the fact that on\n> versions of gnupg < 2.1, gpgconf --kill is not available.  I noticed this with\n> gnupg-2.0.14 on CentOS 6.  It also occurs on CentOS 7, which provides\n> gnupg-2.0.22.\n> \n> I don't know if there's much value in trying to better handle older gnupg-2.0\n> systems. \n\nHi Todd.\n\nThanks for catching the redirection issue! I agree that the other fixes feel\nlike overkill. Are you certain that switching to gpgconf --reload will have the\nsame effect as --kill? (I know that this is the case for scdaemon only).\n\nThanks again!\n-Santiago.\n"},{"id":"332467","messageId":"20171113224323.GR5144@zaya.teonanacatl.net","threadId":"47195","inReplyTo":"20171113221823.jzt7jfhxeuyivbcn@LykOS.localdomain","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-11-13T22:43:23Z","receivedAt":"2017-11-13T22:43:32Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi Santiago,\n\nSantiago Torres wrote:\n> Thanks for catching the redirection issue! I agree that the other \n> fixes feel like overkill. Are you certain that switching to gpgconf \n> --reload will have the same effect as --kill? (I know that this is \n> the case for scdaemon only).\n\nI am not at all certain whether reload would work to fix the issues \nyou fixed by killing the agent between runs. :)\n\nWere the ENOENT errors you encountered in running the tests multiple \ntimes easy to reproduce?  If so, I can certainly try to reproduce them \nand then run the tests with --reload in place of --kill to gpgconf.  \nIf that worked across the various gnupg 2.x releases, it would be a \nsimple enough change to make as a follow-up.\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nWhatever it is that the government does, sensible Americans would\nprefer that the government does it to somebody else. This is the idea\nbehind foreign policy.\n    -- P.J. O'Rourke\n\n"},{"id":"332469","messageId":"20171113230201.3gyqh2oknic2o6mg@LykOS.localdomain","threadId":"47195","inReplyTo":"20171113224323.GR5144@zaya.teonanacatl.net","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-11-13T23:02:02Z","receivedAt":"2017-11-13T23:00:54Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":" \n> Were the ENOENT errors you encountered in running the tests multiple times\n> easy to reproduce? \n\nIf you had the right gpg2, it should be easy to repro with just re-running.\n\n> If so, I can certainly try to reproduce them and then\n> run the tests with --reload in place of --kill to gpgconf.  If that worked\n> across the various gnupg 2.x releases, it would be a simple enough change to\n> make as a follow-up.\n\nLet me dig up the exact versions. IIRC it was somewhere between 2.1.0 and 2.2.x\nor so. I think somewhere within the patch re-rolls I had the exact versions.\n\nCheers!\n-Santiago.\n"},{"id":"332470","messageId":"20171113230612.nyygui2ahuqzrjsr@LykOS.localdomain","threadId":"47195","inReplyTo":"20171113230201.3gyqh2oknic2o6mg@LykOS.localdomain","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-11-13T23:06:13Z","receivedAt":"2017-11-13T23:05:05Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"Quick followup.\n\nThe version that triggers this is at least 2.1.21[1]. I recall there was some\nwiggle room on minor versions before it.\n\nThanks!\n-Santiago.\n\n[1] https://dev.gnupg.org/T3218\n\nOn Mon, Nov 13, 2017 at 06:02:02PM -0500, Santiago Torres wrote:\n>  \n> > Were the ENOENT errors you encountered in running the tests multiple times\n> > easy to reproduce? \n> \n> If you had the right gpg2, it should be easy to repro with just re-running.\n> \n> > If so, I can certainly try to reproduce them and then\n> > run the tests with --reload in place of --kill to gpgconf.  If that worked\n> > across the various gnupg 2.x releases, it would be a simple enough change to\n> > make as a follow-up.\n> \n> Let me dig up the exact versions. IIRC it was somewhere between 2.1.0 and 2.2.x\n> or so. I think somewhere within the patch re-rolls I had the exact versions.\n> \n> Cheers!\n> -Santiago.\n\n\n"},{"id":"332494","messageId":"xmqq60ad7ewx.fsf@gitster.mtv.corp.google.com","threadId":"47195","inReplyTo":"20171113210745.24638-1-tmz@pobox.com","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T02:49:34Z","receivedAt":"2017-11-14T02:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Todd Zullinger <tmz@pobox.com> writes:\n\n> In 29ff1f8f74 (t: lib-gpg: flush gpg agent on startup, 2017-07-20), a\n> call to gpgconf was added to kill the gpg-agent.  The intention was to\n> ignore all output from the call, but the order of the redirection needs\n> to be switched to ensure that both stdout and stderr are redirected to\n> /dev/null.  Without this, gpgconf from gnupg-2.0 releases would output\n> 'gpgconf: invalid option \"--kill\"' each time it was called.\n>\n> Signed-off-by: Todd Zullinger <tmz@pobox.com>\n> ---\n>\n> I noticed that gpgconf produced error output for a number of tests on\n> CentOS/RHEL.  As an example:\n>\n>     *** t5534-push-signed.sh ***\n>     gpgconf: invalid option \"--kill\"\n>\n> Looking at the code in lib-gpg.sh, it appeared the intention was to ignore this\n> output.  Reading through the review of the patch confirmed that feeling[1].  The\n> current code gets caught by the subtleties of output redirection.  (Who hasn't\n> been burned at some point by the difference between '2>&1 >/dev/null' and\n> '>/dev/null 2>&1' ? ;)\n\n**Blush**.  I should have caught this during the review.  Thanks.\n\n> Lastly, I also noticed that git-rebase.sh uses the same 2>&1 >/dev/null.  I\n> suspect it's similarly not intentional:\n>\n>     $ git grep -h -C4 '2>&1 >/dev/null' -- git-rebase.sh\n>     apply_autostash () {\n>     \tif test -f \"$state_dir/autostash\"\n>     \tthen\n>     \t\tstash_sha1=$(cat \"$state_dir/autostash\")\n>     \t\tif git stash apply $stash_sha1 2>&1 >/dev/null\n>     \t\tthen\n>     \t\t\techo \"$(gettext 'Applied autostash.')\" >&2\n>     \t\telse\n>     \t\t\tgit stash store -m \"autostash\" -q $stash_sha1 ||\n>\n> I'll send a separate patch to adjust that code as well.\n\nIf it were intentional, the caller of apply_autostash() must be\nexpecting to see an error message from its standard output and\nprepared to do something interesting with it, which I do not see, so\nI agree that it is a typo.  Thanks.\n\nI wonder if this line in 3320 is doing what it meant to do:\n\n    test_must_fail git notes merge z 2>&1 >out &&\n    test_i18ngrep \"Automatic notes merge failed\" out &&\n    grep -v \"A notes merge into refs/notes/x is already in-progress in\" out\n\n"},{"id":"332495","messageId":"20171114030351.GS5144@zaya.teonanacatl.net","threadId":"47195","inReplyTo":"xmqq60ad7ewx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-11-14T03:03:51Z","receivedAt":"2017-11-14T03:03:59Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Junio C Hamano wrote:\n> **Blush**.  I should have caught this during the review.  Thanks.\n\nI've written that code myself in the past and I am sure I will do it \nagain. :)\n\n> I wonder if this line in 3320 is doing what it meant to do:\n>\n>    test_must_fail git notes merge z 2>&1 >out && \n>    test_i18ngrep \"Automatic notes merge failed\" out && \n>    grep -v \"A notes merge into refs/notes/x is already in-progress in\" out\n\nThat's a fine question.  I only grepped for 2>&1 >/dev/null.  Dropping \n/dev/null, as you did only turns up that test as an additional hit.\n\nI think, based on a very cursory reading of the test, that it's \nintending to direct stderr and stdout to the file out.  The test gets \nlucky that the code in builtin/notes.c directs the error message to \nstdout:\n\n        printf(_(\"Automatic notes merge failed. Fix conflicts in %s and \"\n                 \"commit the result with 'git notes merge --commit', or \"\n                 \"abort the merge with 'git notes merge --abort'.\\n\"),\n               git_path(NOTES_MERGE_WORKTREE));\n\nPerhaps that should be using fprintf(stderr, ...) instead?  (And the \ntest redirection corrected as well, of course.)  If that seems \ncorrect, I can submit the trivial patch for that as well, while I'm on \nthe subject.\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nChaos, panic, and disorder - my job is done here.\n\n"},{"id":"332497","messageId":"20171114031026.GT5144@zaya.teonanacatl.net","threadId":"47195","inReplyTo":"20171113230612.nyygui2ahuqzrjsr@LykOS.localdomain","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-11-14T03:10:27Z","receivedAt":"2017-11-14T03:10:35Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Santiago Torres wrote:\n> Quick followup.\n>\n> The version that triggers this is at least 2.1.21[1]. I recall there \n> was some wiggle room on minor versions before it.\n\nThanks for digging that up!  I had 2.1.13 at hand in a fedora-25 \nchroot which I used to build git recently, but I've not been able to \ncoax the test failures from it, yet.  I'll try a little more and with \nsome different gnupg versions before I punt.\n\nIf it's a small range of gnupg versions which fail badly when the \nGNUPGHOME dir is removed, then there's far less reason for git to do \nmuch more than make an effort to kill the agent.\n\nIt seems like all the gnupg versions which may suffer from the bug \nalso support gpgconf --kill, which would make any further change in \nthe test to handle versions which lack the --kill option moot.\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nOur task must be to free ourselves from this prison by widening our\ncircle of compassion to embrace all living creatures and the whole of\nnature in its beauty.\n    -- Albert Einstein\n\n"},{"id":"332500","messageId":"xmqqshdh5ygy.fsf@gitster.mtv.corp.google.com","threadId":"47195","inReplyTo":"20171114030351.GS5144@zaya.teonanacatl.net","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T03:30:05Z","receivedAt":"2017-11-14T03:30:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Todd Zullinger <tmz@pobox.com> writes:\n\n>> I wonder if this line in 3320 is doing what it meant to do:\n>>\n>>    test_must_fail git notes merge z 2>&1 >out &&    test_i18ngrep\n>> \"Automatic notes merge failed\" out &&    grep -v \"A notes merge into\n>> refs/notes/x is already in-progress in\" out\n>\n> That's a fine question.  I only grepped for 2>&1 >/dev/null.  Dropping\n> /dev/null, as you did only turns up that test as an additional hit.\n>\n> I think, based on a very cursory reading of the test, that it's\n> intending to direct stderr and stdout to the file out.  The test gets\n> lucky that the code in builtin/notes.c directs the error message to\n> stdout:\n>\n>        printf(_(\"Automatic notes merge failed. Fix conflicts in %s and \"\n>                 \"commit the result with 'git notes merge --commit', or \"\n>                 \"abort the merge with 'git notes merge --abort'.\\n\"),\n>               git_path(NOTES_MERGE_WORKTREE));\n>\n> Perhaps that should be using fprintf(stderr, ...) instead?  (And the\n> test redirection corrected as well, of course.)  If that seems\n> correct, I can submit the trivial patch for that as well, while I'm on\n> the subject.\n\nThe message goes to the standard output stream since it was\nintroduced in 809f38c8 (\"git notes merge: Manual conflict\nresolution, part 1/2\", 2010-11-09) and 6abb3655 (\"git notes merge:\nManual conflict resolution, part 2/2\", 2010-11-09).  I do think it\nmakes more sense to send it to the standard error stream, but just\nin case if the original author thinks of a reason why it shouldn't,\nlet's summon Johan and ask his input.\n\nThanks.\n\n\n"},{"id":"332506","messageId":"20171114051520.GU5144@zaya.teonanacatl.net","threadId":"47195","inReplyTo":"xmqqshdh5ygy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-11-14T05:15:20Z","receivedAt":"2017-11-14T05:15:30Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Junio C Hamano wrote:\n> The message goes to the standard output stream since it was \n> introduced in 809f38c8 (\"git notes merge: Manual conflict \n> resolution, part 1/2\", 2010-11-09) and 6abb3655 (\"git notes merge:\n> Manual conflict resolution, part 2/2\", 2010-11-09).  I do think it \n> makes more sense to send it to the standard error stream, but just \n> in case if the original author thinks of a reason why it shouldn't, \n> let's summon Johan and ask his input.\n\nSounds like a good plan.  If the message does move to stderr, there \nare also a few tests in 3310 that need adjusted.  They presume an \nerror message from `git notes merge`, but they only redirect stdout to \nthe output file.\n\nWhile I was bored, I prepared a commit with these changes and \nconfirmed the test suite passes, in case we get an ACK from Johan.\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nIt is impossible to enjoy idling thoroughly unless one has plenty of\nwork to do.\n    -- Jerome K. Jerome\n\n"},{"id":"332528","messageId":"CALKQrgc427=JNkkH+k+EohgKYuJSBPbDNR3uUcmGuf_ZyQ0X4Q@mail.gmail.com","threadId":"47195","inReplyTo":"20171114051520.GU5144@zaya.teonanacatl.net","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2017-11-14T09:31:49Z","receivedAt":"2017-11-14T09:32:14Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tue, Nov 14, 2017 at 6:15 AM, Todd Zullinger <tmz@pobox.com> wrote:\n> Junio C Hamano wrote:\n>>\n>> The message goes to the standard output stream since it was introduced in\n>> 809f38c8 (\"git notes merge: Manual conflict resolution, part 1/2\",\n>> 2010-11-09) and 6abb3655 (\"git notes merge:\n>> Manual conflict resolution, part 2/2\", 2010-11-09).  I do think it makes\n>> more sense to send it to the standard error stream, but just in case if the\n>> original author thinks of a reason why it shouldn't, let's summon Johan and\n>> ask his input.\n>\n>\n> Sounds like a good plan.  If the message does move to stderr, there are also\n> a few tests in 3310 that need adjusted.  They presume an error message from\n> `git notes merge`, but they only redirect stdout to the output file.\n>\n> While I was bored, I prepared a commit with these changes and confirmed the\n> test suite passes, in case we get an ACK from Johan.\n\nACK :-)\n\nError messages should go to stderr, and redirection in the tests\nshould be fixed.\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"332540","messageId":"xmqqd14k3oar.fsf@gitster.mtv.corp.google.com","threadId":"47195","inReplyTo":"CALKQrgc427=JNkkH+k+EohgKYuJSBPbDNR3uUcmGuf_ZyQ0X4Q@mail.gmail.com","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T14:52:44Z","receivedAt":"2017-11-14T14:52:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n>> Sounds like a good plan.  If the message does move to stderr, there are also\n>> a few tests in 3310 that need adjusted.  They presume an error message from\n>> `git notes merge`, but they only redirect stdout to the output file.\n>>\n>> While I was bored, I prepared a commit with these changes and confirmed the\n>> test suite passes, in case we get an ACK from Johan.\n>\n> ACK :-)\n>\n> Error messages should go to stderr, and redirection in the tests\n> should be fixed.\n\nThanks, both.\n"},{"id":"332545","messageId":"20171114153517.3z3ndsvqhekijuad@LykOS.localdomain","threadId":"47195","inReplyTo":"20171114031026.GT5144@zaya.teonanacatl.net","subject":"Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-11-14T15:35:19Z","receivedAt":"2017-11-14T15:34:11Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> If it's a small range of gnupg versions which fail badly when the GNUPGHOME\n> dir is removed, then there's far less reason for git to do much more than\n> make an effort to kill the agent.\n> \n\nYeah. FWIW, it may be reasonable to consider dropping the patch once we are\ncertain distros don't ship this range anymore.\n\n> It seems like all the gnupg versions which may suffer from the bug also\n> support gpgconf --kill, which would make any further change in the test to\n> handle versions which lack the --kill option moot.\n> \n\nThis is also true. We should definitely not litter the stdout with ENOENT-like\nerror messages though...\n\nThanks again for catching this!\n-Santiago.\n"},{"id":"332551","messageId":"20171114161752.13204-1-tmz@pobox.com","threadId":"47195","inReplyTo":"CALKQrgc427=JNkkH+k+EohgKYuJSBPbDNR3uUcmGuf_ZyQ0X4Q@mail.gmail.com","subject":"[PATCH] notes: send \"Automatic notes merge failed\" messages to stderr","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-11-14T16:17:52Z","receivedAt":"2017-11-14T16:17:59Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"All other error messages from notes use stderr.  Do the same when\nalerting users of an unresolved notes merge.\n\nFix the output redirection in t3310 and t3320 as well.  Previously, the\ntests directed output to a file, but stderr was either not captured or\nnot sent to the file due to the order of the redirection operators.\n\nSigned-off-by: Todd Zullinger <tmz@pobox.com>\n---\n\nJohan Herland wrote:\n> ACK :-)\n> \n> Error messages should go to stderr, and redirection in the tests\n> should be fixed.\n\nExcellent, thanks Johan!\n\nHere's what I came up with.  Hopefully I caught all the tests that need\nadjustment.  The test suite passes for me, but it's always possible that I've\nmissed something.\n\nStyle-wise, I'm not sure about the re-wrapping of the error message text.  If\nthat should be avoided to make the patch's change from printf to fprintf\nclearer or should be wrapped differently, let me know.\n\n builtin/notes.c                       | 8 ++++----\n t/t3310-notes-merge-manual-resolve.sh | 8 ++++----\n t/t3320-notes-merge-worktrees.sh      | 2 +-\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 12afdf1907..4468adaf29 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -865,10 +865,10 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\tif (create_symref(\"NOTES_MERGE_REF\", default_notes_ref(), NULL))\n \t\t\tdie(_(\"failed to store link to current notes ref (%s)\"),\n \t\t\t    default_notes_ref());\n-\t\tprintf(_(\"Automatic notes merge failed. Fix conflicts in %s and \"\n-\t\t\t \"commit the result with 'git notes merge --commit', or \"\n-\t\t\t \"abort the merge with 'git notes merge --abort'.\\n\"),\n-\t\t       git_path(NOTES_MERGE_WORKTREE));\n+\t\tfprintf(stderr, _(\"Automatic notes merge failed. Fix conflicts in %s \"\n+\t\t\t\t  \"and commit the result with 'git notes merge --commit', \"\n+\t\t\t\t  \"or abort the merge with 'git notes merge --abort'.\\n\"),\n+\t\t\tgit_path(NOTES_MERGE_WORKTREE));\n \t}\n \n \tfree_notes(t);\ndiff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\nindex baef2d6924..9c1bf6eb3d 100755\n--- a/t/t3310-notes-merge-manual-resolve.sh\n+++ b/t/t3310-notes-merge-manual-resolve.sh\n@@ -176,7 +176,7 @@ git rev-parse refs/notes/z > pre_merge_z\n test_expect_success 'merge z into m (== y) with default (\"manual\") resolver => Conflicting 3-way merge' '\n \tgit update-ref refs/notes/m refs/notes/y &&\n \tgit config core.notesRef refs/notes/m &&\n-\ttest_must_fail git notes merge z >output &&\n+\ttest_must_fail git notes merge z >output 2>&1 &&\n \t# Output should point to where to resolve conflicts\n \ttest_i18ngrep \"\\\\.git/NOTES_MERGE_WORKTREE\" output &&\n \t# Inspect merge conflicts\n@@ -379,7 +379,7 @@ git rev-parse refs/notes/z > pre_merge_z\n test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resolver => Conflicting 3-way merge' '\n \tgit update-ref refs/notes/m refs/notes/y &&\n \tgit config core.notesRef refs/notes/m &&\n-\ttest_must_fail git notes merge z >output &&\n+\ttest_must_fail git notes merge z >output 2>&1 &&\n \t# Output should point to where to resolve conflicts\n \ttest_i18ngrep \"\\\\.git/NOTES_MERGE_WORKTREE\" output &&\n \t# Inspect merge conflicts\n@@ -413,7 +413,7 @@ git rev-parse refs/notes/y > pre_merge_y\n git rev-parse refs/notes/z > pre_merge_z\n \n test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resolver => Conflicting 3-way merge' '\n-\ttest_must_fail git notes merge z >output &&\n+\ttest_must_fail git notes merge z >output 2>&1 &&\n \t# Output should point to where to resolve conflicts\n \ttest_i18ngrep \"\\\\.git/NOTES_MERGE_WORKTREE\" output &&\n \t# Inspect merge conflicts\n@@ -494,7 +494,7 @@ cp expect_log_y expect_log_m\n \n test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resolver => Conflicting 3-way merge' '\n \tgit update-ref refs/notes/m refs/notes/y &&\n-\ttest_must_fail git notes merge z >output &&\n+\ttest_must_fail git notes merge z >output 2>&1 &&\n \t# Output should point to where to resolve conflicts\n \ttest_i18ngrep \"\\\\.git/NOTES_MERGE_WORKTREE\" output &&\n \t# Inspect merge conflicts\ndiff --git a/t/t3320-notes-merge-worktrees.sh b/t/t3320-notes-merge-worktrees.sh\nindex b9c3bc2487..10bfc8b947 100755\n--- a/t/t3320-notes-merge-worktrees.sh\n+++ b/t/t3320-notes-merge-worktrees.sh\n@@ -61,7 +61,7 @@ test_expect_success 'merge z into x while mid-merge on y succeeds' '\n \t(\n \t\tcd worktree2 &&\n \t\tgit config core.notesRef refs/notes/x &&\n-\t\ttest_must_fail git notes merge z 2>&1 >out &&\n+\t\ttest_must_fail git notes merge z >out 2>&1 &&\n \t\ttest_i18ngrep \"Automatic notes merge failed\" out &&\n \t\tgrep -v \"A notes merge into refs/notes/x is already in-progress in\" out\n \t) &&\n-- \n2.15.0\n\n"},{"id":"332604","messageId":"CALKQrgfs9QNg9AcQ_CGq43XkSpAAgS4L0KokpCj2qnhPa9+-=w@mail.gmail.com","threadId":"47195","inReplyTo":"20171114161752.13204-1-tmz@pobox.com","subject":"Re: [PATCH] notes: send \"Automatic notes merge failed\" messages to stderr","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2017-11-15T13:13:03Z","receivedAt":"2017-11-15T13:13:17Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tue, Nov 14, 2017 at 5:17 PM, Todd Zullinger <tmz@pobox.com> wrote:\n> All other error messages from notes use stderr.  Do the same when\n> alerting users of an unresolved notes merge.\n>\n> Fix the output redirection in t3310 and t3320 as well.  Previously, the\n> tests directed output to a file, but stderr was either not captured or\n> not sent to the file due to the order of the redirection operators.\n>\n> Signed-off-by: Todd Zullinger <tmz@pobox.com>\n\nLooks good to me.\n\n...Johan\n"},{"id":"332619","messageId":"20171115210920.GH5144@zaya.teonanacatl.net","threadId":"47195","inReplyTo":"CALKQrgfs9QNg9AcQ_CGq43XkSpAAgS4L0KokpCj2qnhPa9+-=w@mail.gmail.com","subject":"Re: [PATCH] notes: send \"Automatic notes merge failed\" messages to stderr","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-11-15T21:09:20Z","receivedAt":"2017-11-15T21:09:30Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Johan Herland wrote:\n> On Tue, Nov 14, 2017 at 5:17 PM, Todd Zullinger <tmz@pobox.com> wrote:\n>> All other error messages from notes use stderr.  Do the same when \n>> alerting users of an unresolved notes merge.\n>>\n>> Fix the output redirection in t3310 and t3320 as well.  Previously, the \n>> tests directed output to a file, but stderr was either not captured or \n>> not sent to the file due to the order of the redirection operators.\n>>\n>> Signed-off-by: Todd Zullinger <tmz@pobox.com>\n>\n> Looks good to me.\n\nThanks Johan.\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nDeliberation, n. The act of examining one's bread to determine which\nside it is buttered on.\n    -- Ambrose Bierce, \"The Devil's Dictionary\"\n\n"}]}