[PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Todd Zullinger]

In 29ff1f8f74 (t: lib-gpg: flush gpg agent on startup, 2017-07-20), a
call to gpgconf was added to kill the gpg-agent.  The intention was to
ignore all output from the call, but the order of the redirection needs
to be switched to ensure that both stdout and stderr are redirected to
/dev/null.  Without this, gpgconf from gnupg-2.0 releases would output
'gpgconf: invalid option "--kill"' each time it was called.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---

I noticed that gpgconf produced error output for a number of tests on
CentOS/RHEL.  As an example:

    *** t5534-push-signed.sh ***
    gpgconf: invalid option "--kill"

Looking at the code in lib-gpg.sh, it appeared the intention was to ignore this
output.  Reading through the review of the patch confirmed that feeling[1].  The
current code gets caught by the subtleties of output redirection.  (Who hasn't
been burned at some point by the difference between '2>&1 >/dev/null' and
'>/dev/null 2>&1' ? ;)

[1] https://public-inbox.org/git/xmqq379qlvzi.fsf@gitster.mtv.corp.google.com/

Of course, beyond getting stderr to /dev/null, there is the fact that on
versions of gnupg < 2.1, gpgconf --kill is not available.  I noticed this with
gnupg-2.0.14 on CentOS 6.  It also occurs on CentOS 7, which provides
gnupg-2.0.22.

I don't know if there's much value in trying to better handle older gnupg-2.0
systems.  Using gpgconf --reload might be sufficient to work on gnupg-2.0 and
newer systems.  That might solve the issues with gpg-agent caching stale file
handles that motivated the initial patch.  If not, finding what works well with
both gnupg-2.0 and newer seems mildly painful.  This method works for me on
2.0, 2.1, and 2.2:

    pid=$(echo GETINFO pid | gpg-connect-agent | awk '/^D / {print $2}')

And people say crypto tools aren't intuitive.  Pfff. :/

A fairly gross way to use that in lib-gpg.sh might be:

    diff --git c/t/lib-gpg.sh w/t/lib-gpg.sh
    index 43679a4c64..c91d9b334f 100755
    --- c/t/lib-gpg.sh
    +++ w/t/lib-gpg.sh
    @@ -1,5 +1,10 @@
     #!/bin/sh

    +gpg_killagent() {
    +	pid=$(echo GETINFO pid | gpg-connect-agent | awk '/^D / {print $2}')
    +	test -n "$pid" && kill "$pid"
    +}
    +
     gpg_version=$(gpg --version 2>&1)
     if test $? != 127
     then
    @@ -31,7 +36,7 @@ then
     		chmod 0700 ./gpghome &&
     		GNUPGHOME="$(pwd)/gpghome" &&
     		export GNUPGHOME &&
    -		(gpgconf --kill gpg-agent 2>&1 >/dev/null || : ) &&
    +		(gpg_killagent >/dev/null 2>&1 || : ) &&
     		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
     			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
     		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \

I have my doubts that doing something like the above is worthwhile.  It's
probably good enough to simply fix up the gpgconf stderr redirection and call
it a day.

I haven't seen any gpg failures in the test runs I've done, so I haven't had a
need to re-run those tests.  (I also have not run the test suite with the ugly
gpg_killagent() diff above.  I have run it with the change to fix stderr
redirection and confirmed it succeeds without the gpgconf error messages.)

Lastly, I also noticed that git-rebase.sh uses the same 2>&1 >/dev/null.  I
suspect it's similarly not intentional:

    $ git grep -h -C4 '2>&1 >/dev/null' -- git-rebase.sh
    apply_autostash () {
    	if test -f "$state_dir/autostash"
    	then
    		stash_sha1=$(cat "$state_dir/autostash")
    		if git stash apply $stash_sha1 2>&1 >/dev/null
    		then
    			echo "$(gettext 'Applied autostash.')" >&2
    		else
    			git stash store -m "autostash" -q $stash_sha1 ||

I'll send a separate patch to adjust that code as well.

 t/lib-gpg.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 43679a4c64..a5d3b2cbaa 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -31,7 +31,7 @@ then
 		chmod 0700 ./gpghome &&
 		GNUPGHOME="$(pwd)/gpghome" &&
 		export GNUPGHOME &&
-		(gpgconf --kill gpg-agent 2>&1 >/dev/null || : ) &&
+		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
 		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
 			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
 		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
-- 
2.15.0

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

 
> Of course, beyond getting stderr to /dev/null, there is the fact that on
> versions of gnupg < 2.1, gpgconf --kill is not available.  I noticed this with
> gnupg-2.0.14 on CentOS 6.  It also occurs on CentOS 7, which provides
> gnupg-2.0.22.
> 
> I don't know if there's much value in trying to better handle older gnupg-2.0
> systems. 

Hi Todd.

Thanks for catching the redirection issue! I agree that the other fixes feel
like overkill. Are you certain that switching to gpgconf --reload will have the
same effect as --kill? (I know that this is the case for scdaemon only).

Thanks again!
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Todd Zullinger]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

Hi Santiago,

Santiago Torres wrote:
> Thanks for catching the redirection issue! I agree that the other 
> fixes feel like overkill. Are you certain that switching to gpgconf 
> --reload will have the same effect as --kill? (I know that this is 
> the case for scdaemon only).

I am not at all certain whether reload would work to fix the issues 
you fixed by killing the agent between runs. :)

Were the ENOENT errors you encountered in running the tests multiple 
times easy to reproduce?  If so, I can certainly try to reproduce them 
and then run the tests with --reload in place of --kill to gpgconf.  
If that worked across the various gnupg 2.x releases, it would be a 
simple enough change to make as a follow-up.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Whatever it is that the government does, sensible Americans would
prefer that the government does it to somebody else. This is the idea
behind foreign policy.
    -- P.J. O'Rourke


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 543 bytes --]

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

 
> Were the ENOENT errors you encountered in running the tests multiple times
> easy to reproduce? 

If you had the right gpg2, it should be easy to repro with just re-running.

> If so, I can certainly try to reproduce them and then
> run the tests with --reload in place of --kill to gpgconf.  If that worked
> across the various gnupg 2.x releases, it would be a simple enough change to
> make as a follow-up.

Let me dig up the exact versions. IIRC it was somewhere between 2.1.0 and 2.2.x
or so. I think somewhere within the patch re-rolls I had the exact versions.

Cheers!
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

Quick followup.

The version that triggers this is at least 2.1.21[1]. I recall there was some
wiggle room on minor versions before it.

Thanks!
-Santiago.

[1] https://dev.gnupg.org/T3218

On Mon, Nov 13, 2017 at 06:02:02PM -0500, Santiago Torres wrote:
>  
> > Were the ENOENT errors you encountered in running the tests multiple times
> > easy to reproduce? 
> 
> If you had the right gpg2, it should be easy to repro with just re-running.
> 
> > If so, I can certainly try to reproduce them and then
> > run the tests with --reload in place of --kill to gpgconf.  If that worked
> > across the various gnupg 2.x releases, it would be a simple enough change to
> > make as a follow-up.
> 
> Let me dig up the exact versions. IIRC it was somewhere between 2.1.0 and 2.2.x
> or so. I think somewhere within the patch re-rolls I had the exact versions.
> 
> Cheers!
> -Santiago.



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> In 29ff1f8f74 (t: lib-gpg: flush gpg agent on startup, 2017-07-20), a
> call to gpgconf was added to kill the gpg-agent.  The intention was to
> ignore all output from the call, but the order of the redirection needs
> to be switched to ensure that both stdout and stderr are redirected to
> /dev/null.  Without this, gpgconf from gnupg-2.0 releases would output
> 'gpgconf: invalid option "--kill"' each time it was called.
>
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
>
> I noticed that gpgconf produced error output for a number of tests on
> CentOS/RHEL.  As an example:
>
>     *** t5534-push-signed.sh ***
>     gpgconf: invalid option "--kill"
>
> Looking at the code in lib-gpg.sh, it appeared the intention was to ignore this
> output.  Reading through the review of the patch confirmed that feeling[1].  The
> current code gets caught by the subtleties of output redirection.  (Who hasn't
> been burned at some point by the difference between '2>&1 >/dev/null' and
> '>/dev/null 2>&1' ? ;)

**Blush**.  I should have caught this during the review.  Thanks.

> Lastly, I also noticed that git-rebase.sh uses the same 2>&1 >/dev/null.  I
> suspect it's similarly not intentional:
>
>     $ git grep -h -C4 '2>&1 >/dev/null' -- git-rebase.sh
>     apply_autostash () {
>     	if test -f "$state_dir/autostash"
>     	then
>     		stash_sha1=$(cat "$state_dir/autostash")
>     		if git stash apply $stash_sha1 2>&1 >/dev/null
>     		then
>     			echo "$(gettext 'Applied autostash.')" >&2
>     		else
>     			git stash store -m "autostash" -q $stash_sha1 ||
>
> I'll send a separate patch to adjust that code as well.

If it were intentional, the caller of apply_autostash() must be
expecting to see an error message from its standard output and
prepared to do something interesting with it, which I do not see, so
I agree that it is a typo.  Thanks.

I wonder if this line in 3320 is doing what it meant to do:

    test_must_fail git notes merge z 2>&1 >out &&
    test_i18ngrep "Automatic notes merge failed" out &&
    grep -v "A notes merge into refs/notes/x is already in-progress in" out

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Todd Zullinger]

Junio C Hamano wrote:
> **Blush**.  I should have caught this during the review.  Thanks.

I've written that code myself in the past and I am sure I will do it 
again. :)

> I wonder if this line in 3320 is doing what it meant to do:
>
>    test_must_fail git notes merge z 2>&1 >out && 
>    test_i18ngrep "Automatic notes merge failed" out && 
>    grep -v "A notes merge into refs/notes/x is already in-progress in" out

That's a fine question.  I only grepped for 2>&1 >/dev/null.  Dropping 
/dev/null, as you did only turns up that test as an additional hit.

I think, based on a very cursory reading of the test, that it's 
intending to direct stderr and stdout to the file out.  The test gets 
lucky that the code in builtin/notes.c directs the error message to 
stdout:

        printf(_("Automatic notes merge failed. Fix conflicts in %s and "
                 "commit the result with 'git notes merge --commit', or "
                 "abort the merge with 'git notes merge --abort'.\n"),
               git_path(NOTES_MERGE_WORKTREE));

Perhaps that should be using fprintf(stderr, ...) instead?  (And the 
test redirection corrected as well, of course.)  If that seems 
correct, I can submit the trivial patch for that as well, while I'm on 
the subject.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Chaos, panic, and disorder - my job is done here.

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Todd Zullinger]

Santiago Torres wrote:
> Quick followup.
>
> The version that triggers this is at least 2.1.21[1]. I recall there 
> was some wiggle room on minor versions before it.

Thanks for digging that up!  I had 2.1.13 at hand in a fedora-25 
chroot which I used to build git recently, but I've not been able to 
coax the test failures from it, yet.  I'll try a little more and with 
some different gnupg versions before I punt.

If it's a small range of gnupg versions which fail badly when the 
GNUPGHOME dir is removed, then there's far less reason for git to do 
much more than make an effort to kill the agent.

It seems like all the gnupg versions which may suffer from the bug 
also support gpgconf --kill, which would make any further change in 
the test to handle versions which lack the --kill option moot.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Our task must be to free ourselves from this prison by widening our
circle of compassion to embrace all living creatures and the whole of
nature in its beauty.
    -- Albert Einstein

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

>> I wonder if this line in 3320 is doing what it meant to do:
>>
>>    test_must_fail git notes merge z 2>&1 >out &&    test_i18ngrep
>> "Automatic notes merge failed" out &&    grep -v "A notes merge into
>> refs/notes/x is already in-progress in" out
>
> That's a fine question.  I only grepped for 2>&1 >/dev/null.  Dropping
> /dev/null, as you did only turns up that test as an additional hit.
>
> I think, based on a very cursory reading of the test, that it's
> intending to direct stderr and stdout to the file out.  The test gets
> lucky that the code in builtin/notes.c directs the error message to
> stdout:
>
>        printf(_("Automatic notes merge failed. Fix conflicts in %s and "
>                 "commit the result with 'git notes merge --commit', or "
>                 "abort the merge with 'git notes merge --abort'.\n"),
>               git_path(NOTES_MERGE_WORKTREE));
>
> Perhaps that should be using fprintf(stderr, ...) instead?  (And the
> test redirection corrected as well, of course.)  If that seems
> correct, I can submit the trivial patch for that as well, while I'm on
> the subject.

The message goes to the standard output stream since it was
introduced in 809f38c8 ("git notes merge: Manual conflict
resolution, part 1/2", 2010-11-09) and 6abb3655 ("git notes merge:
Manual conflict resolution, part 2/2", 2010-11-09).  I do think it
makes more sense to send it to the standard error stream, but just
in case if the original author thinks of a reason why it shouldn't,
let's summon Johan and ask his input.

Thanks.

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Todd Zullinger]

Junio C Hamano wrote:
> The message goes to the standard output stream since it was 
> introduced in 809f38c8 ("git notes merge: Manual conflict 
> resolution, part 1/2", 2010-11-09) and 6abb3655 ("git notes merge:
> Manual conflict resolution, part 2/2", 2010-11-09).  I do think it 
> makes more sense to send it to the standard error stream, but just 
> in case if the original author thinks of a reason why it shouldn't, 
> let's summon Johan and ask his input.

Sounds like a good plan.  If the message does move to stderr, there 
are also a few tests in 3310 that need adjusted.  They presume an 
error message from `git notes merge`, but they only redirect stdout to 
the output file.

While I was bored, I prepared a commit with these changes and 
confirmed the test suite passes, in case we get an ACK from Johan.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It is impossible to enjoy idling thoroughly unless one has plenty of
work to do.
    -- Jerome K. Jerome

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Johan Herland]

On Tue, Nov 14, 2017 at 6:15 AM, Todd Zullinger <tmz@pobox.com> wrote:
> Junio C Hamano wrote:
>>
>> The message goes to the standard output stream since it was introduced in
>> 809f38c8 ("git notes merge: Manual conflict resolution, part 1/2",
>> 2010-11-09) and 6abb3655 ("git notes merge:
>> Manual conflict resolution, part 2/2", 2010-11-09).  I do think it makes
>> more sense to send it to the standard error stream, but just in case if the
>> original author thinks of a reason why it shouldn't, let's summon Johan and
>> ask his input.
>
>
> Sounds like a good plan.  If the message does move to stderr, there are also
> a few tests in 3310 that need adjusted.  They presume an error message from
> `git notes merge`, but they only redirect stdout to the output file.
>
> While I was bored, I prepared a commit with these changes and confirmed the
> test suite passes, in case we get an ACK from Johan.

ACK :-)

Error messages should go to stderr, and redirection in the tests
should be fixed.

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> Sounds like a good plan.  If the message does move to stderr, there are also
>> a few tests in 3310 that need adjusted.  They presume an error message from
>> `git notes merge`, but they only redirect stdout to the output file.
>>
>> While I was bored, I prepared a commit with these changes and confirmed the
>> test suite passes, in case we get an ACK from Johan.
>
> ACK :-)
>
> Error messages should go to stderr, and redirection in the tests
> should be fixed.

Thanks, both.

Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

> If it's a small range of gnupg versions which fail badly when the GNUPGHOME
> dir is removed, then there's far less reason for git to do much more than
> make an effort to kill the agent.
> 

Yeah. FWIW, it may be reasonable to consider dropping the patch once we are
certain distros don't ship this range anymore.

> It seems like all the gnupg versions which may suffer from the bug also
> support gpgconf --kill, which would make any further change in the test to
> handle versions which lack the --kill option moot.
> 

This is also true. We should definitely not litter the stdout with ENOENT-like
error messages though...

Thanks again for catching this!
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] notes: send "Automatic notes merge failed" messages to stderr

[Todd Zullinger]

All other error messages from notes use stderr.  Do the same when
alerting users of an unresolved notes merge.

Fix the output redirection in t3310 and t3320 as well.  Previously, the
tests directed output to a file, but stderr was either not captured or
not sent to the file due to the order of the redirection operators.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---

Johan Herland wrote:
> ACK :-)
> 
> Error messages should go to stderr, and redirection in the tests
> should be fixed.

Excellent, thanks Johan!

Here's what I came up with.  Hopefully I caught all the tests that need
adjustment.  The test suite passes for me, but it's always possible that I've
missed something.

Style-wise, I'm not sure about the re-wrapping of the error message text.  If
that should be avoided to make the patch's change from printf to fprintf
clearer or should be wrapped differently, let me know.

 builtin/notes.c                       | 8 ++++----
 t/t3310-notes-merge-manual-resolve.sh | 8 ++++----
 t/t3320-notes-merge-worktrees.sh      | 2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 12afdf1907..4468adaf29 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -865,10 +865,10 @@ static int merge(int argc, const char **argv, const char *prefix)
 		if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
 			die(_("failed to store link to current notes ref (%s)"),
 			    default_notes_ref());
-		printf(_("Automatic notes merge failed. Fix conflicts in %s and "
-			 "commit the result with 'git notes merge --commit', or "
-			 "abort the merge with 'git notes merge --abort'.\n"),
-		       git_path(NOTES_MERGE_WORKTREE));
+		fprintf(stderr, _("Automatic notes merge failed. Fix conflicts in %s "
+				  "and commit the result with 'git notes merge --commit', "
+				  "or abort the merge with 'git notes merge --abort'.\n"),
+			git_path(NOTES_MERGE_WORKTREE));
 	}
 
 	free_notes(t);
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index baef2d6924..9c1bf6eb3d 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -176,7 +176,7 @@ git rev-parse refs/notes/z > pre_merge_z
 test_expect_success 'merge z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
 	git update-ref refs/notes/m refs/notes/y &&
 	git config core.notesRef refs/notes/m &&
-	test_must_fail git notes merge z >output &&
+	test_must_fail git notes merge z >output 2>&1 &&
 	# Output should point to where to resolve conflicts
 	test_i18ngrep "\\.git/NOTES_MERGE_WORKTREE" output &&
 	# Inspect merge conflicts
@@ -379,7 +379,7 @@ git rev-parse refs/notes/z > pre_merge_z
 test_expect_success 'redo merge of z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
 	git update-ref refs/notes/m refs/notes/y &&
 	git config core.notesRef refs/notes/m &&
-	test_must_fail git notes merge z >output &&
+	test_must_fail git notes merge z >output 2>&1 &&
 	# Output should point to where to resolve conflicts
 	test_i18ngrep "\\.git/NOTES_MERGE_WORKTREE" output &&
 	# Inspect merge conflicts
@@ -413,7 +413,7 @@ git rev-parse refs/notes/y > pre_merge_y
 git rev-parse refs/notes/z > pre_merge_z
 
 test_expect_success 'redo merge of z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
-	test_must_fail git notes merge z >output &&
+	test_must_fail git notes merge z >output 2>&1 &&
 	# Output should point to where to resolve conflicts
 	test_i18ngrep "\\.git/NOTES_MERGE_WORKTREE" output &&
 	# Inspect merge conflicts
@@ -494,7 +494,7 @@ cp expect_log_y expect_log_m
 
 test_expect_success 'redo merge of z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
 	git update-ref refs/notes/m refs/notes/y &&
-	test_must_fail git notes merge z >output &&
+	test_must_fail git notes merge z >output 2>&1 &&
 	# Output should point to where to resolve conflicts
 	test_i18ngrep "\\.git/NOTES_MERGE_WORKTREE" output &&
 	# Inspect merge conflicts
diff --git a/t/t3320-notes-merge-worktrees.sh b/t/t3320-notes-merge-worktrees.sh
index b9c3bc2487..10bfc8b947 100755
--- a/t/t3320-notes-merge-worktrees.sh
+++ b/t/t3320-notes-merge-worktrees.sh
@@ -61,7 +61,7 @@ test_expect_success 'merge z into x while mid-merge on y succeeds' '
 	(
 		cd worktree2 &&
 		git config core.notesRef refs/notes/x &&
-		test_must_fail git notes merge z 2>&1 >out &&
+		test_must_fail git notes merge z >out 2>&1 &&
 		test_i18ngrep "Automatic notes merge failed" out &&
 		grep -v "A notes merge into refs/notes/x is already in-progress in" out
 	) &&
-- 
2.15.0

Re: [PATCH] notes: send "Automatic notes merge failed" messages to stderr

[Johan Herland]

On Tue, Nov 14, 2017 at 5:17 PM, Todd Zullinger <tmz@pobox.com> wrote:
> All other error messages from notes use stderr.  Do the same when
> alerting users of an unresolved notes merge.
>
> Fix the output redirection in t3310 and t3320 as well.  Previously, the
> tests directed output to a file, but stderr was either not captured or
> not sent to the file due to the order of the redirection operators.
>
> Signed-off-by: Todd Zullinger <tmz@pobox.com>

Looks good to me.

...Johan

Re: [PATCH] notes: send "Automatic notes merge failed" messages to stderr

[Todd Zullinger]

Johan Herland wrote:
> On Tue, Nov 14, 2017 at 5:17 PM, Todd Zullinger <tmz@pobox.com> wrote:
>> All other error messages from notes use stderr.  Do the same when 
>> alerting users of an unresolved notes merge.
>>
>> Fix the output redirection in t3310 and t3320 as well.  Previously, the 
>> tests directed output to a file, but stderr was either not captured or 
>> not sent to the file due to the order of the redirection operators.
>>
>> Signed-off-by: Todd Zullinger <tmz@pobox.com>
>
> Looks good to me.

Thanks Johan.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Deliberation, n. The act of examining one's bread to determine which
side it is buttered on.
    -- Ambrose Bierce, "The Devil's Dictionary"