t5700-clone-reference: send trace to fd 2, not 3, to please Windows git

PATCH In review Johannes Sixt 4 participants 11 messages Jun 15, 2016

lore

Share & tools

lore

Johannes Sixt

author

2016-06-15 22:56 UTC #0 lore raw

PATCH 1/1 t5700-clone-reference: send trace to fd 2, not 3, to please Windows git

From: Johannes Sixt <[email protected]>

Two tests use GIT_TRACE=3 to dump debugging information of git. On Windows, however, bash is unable to set up file descriptor 3 correctly for its child process, so that git reports "Bad file descriptor" on every trace attempt. The 'git clone' test succeeds nevertheless because an empty trace file remains, and there is only a check for the absence of a particular line. But the 'git fetch' process ultimately hangs (the dynamics that lead to this surprising result have not been investigated).

Since we do not otherwise use stderr in the test cases, divert the trace dump to stderr.

Signed-off-by Johannes Sixt <[email protected]>

---
This fixes a regression introduced in

 t/t5700-clone-reference.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t5700-clone-reference.sh +2 -2

index 9cd3b4d..8b5c58e 100755
@@ -55,7 +55,7 @@ cd "$base_dir"
 rm -f "$U.D"
 
 test_expect_success 'cloning with reference (no -l -s)' \
-'GIT_TRACE_PACKET=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"'
+'GIT_TRACE_PACKET=2 git clone --reference B "file://$(pwd)/A" D 2>"$U.D"'
 
 test_expect_success 'fetched no objects' \
 '! grep " want" "$U.D"'

@@ -173,7 +173,7 @@ test_expect_success 'fetch with incomplete alternates' '
 	(
 		cd K &&
 		git remote add J "file://$base_dir/J" &&
-		GIT_TRACE_PACKET=3 git fetch J 3>"$U.K"
+		GIT_TRACE_PACKET=2 git fetch J 2>"$U.K"
 	) &&
 	master_object=$(cd A && git for-each-ref --format="%(objectname)" refs/heads/master) &&
 	! grep " want $master_object" "$U.K" &&

Signature · 1 line

1.8.2.1298.g2825a8e

Jeff King

2016-06-15 22:56 UTC #1 lore raw

Quoted 11 lines · Johannes Sixt

From: Johannes Sixt <[email protected]>
Two tests use GIT_TRACE=3 to dump debugging information of git. On Windows, however, bash is unable to set up file descriptor 3 correctly for its child process, so that git reports "Bad file descriptor" on every trace attempt. The 'git clone' test succeeds nevertheless because an empty trace file remains, and there is only a check for the absence of a particular line. But the 'git fetch' process ultimately hangs (the dynamics that lead to this surprising result have not been investigated).

Quoted 2 lines

Since we do not otherwise use stderr in the test cases, divert the trace dump to stderr.

I think that is OK, but I'm curious why this is a problem _now_, and not with the code prior to 97a83fa8. The old GIT_DEBUG_SEND_PACK was also just calling write() to descriptor 3.

Quoted 3 lines

Signed-off-by Johannes Sixt <[email protected]>
---
This fixes a regression introduced in

...in 97a83fa8, I assume.

-Peff

Johannes Sixt

author

2016-06-15 22:56 UTC #2 lore raw

Am 3/20/2013 10:33, schrieb Jeff King:

Quoted 13 lines · Johannes Sixt

Quoted 12 lines
From: Johannes Sixt <[email protected]>
Two tests use GIT_TRACE=3 to dump debugging information of git. On Windows, however, bash is unable to set up file descriptor 3 correctly for its child process, so that git reports "Bad file descriptor" on every trace attempt. The 'git clone' test succeeds nevertheless because an empty trace file remains, and there is only a check for the absence of a particular line. But the 'git fetch' process ultimately hangs (the dynamics that lead to this surprising result have not been investigated).

Quoted 6 lines

Quoted 2 lines
Since we do not otherwise use stderr in the test cases, divert the trace dump to stderr.
I think that is OK, but I'm curious why this is a problem _now_, and not with the code prior to 97a83fa8. The old GIT_DEBUG_SEND_PACK was also just calling write() to descriptor 3.

Before this change, both affected commands completed and the trace file was empty. Notice that in both test cases we only check for the absence of certain lines, which is naturally true for an empty file, so that the tests pass.

With the updated code, 'git fetch' hung, which is how I noticed the problem. As I said, I didn't investigate where and why this happens.

Quoted 3 lines

Quoted 1 line
This fixes a regression introduced in
...in 97a83fa8, I assume.

Yes, correct. (Sorry, I got distracted while I looked up the commit.)

-- Hannes

Jeff King

2016-06-15 22:56 UTC #3 lore raw

Quoted 8 lines · Johannes Sixt

Quoted 3 lines
I think that is OK, but I'm curious why this is a problem _now_, and not with the code prior to 97a83fa8. The old GIT_DEBUG_SEND_PACK was also just calling write() to descriptor 3.
Before this change, both affected commands completed and the trace file was empty. Notice that in both test cases we only check for the absence of certain lines, which is naturally true for an empty file, so that the tests pass.

Hmm. The code in t5503 is similar, but before my patch used to actually use "test -s" to make sure that some trace output was written. Did it fail before 97a83fa8 (and does it pass now)?

We should probably be adjusting t5503, too. And possibly adding back in the "test -s" checks (which I removed as redundant, but I guess the double-checking might have caught a platform error in this case).

Also, though I think your use of stderr is probably OK, might it be a little more robust against future output changes to use:

GIT_TRACE_PACKET=$PWD/$U.D git clone ...

to write directly to the file. The original GIT_DEBUG_SEND_PACK did not support such niceties, but GIT_TRACE gives them to us for free.

Quoted 2 lines

With the updated code, 'git fetch' hung, which is how I noticed the problem. As I said, I didn't investigate where and why this happens.

Thinking on it more, almost certainly what is happening is that fd 3 is being used for the actual protocol, and we are jamming random bytes over it. Since we have changed the bytes, the code is reacting in a different way (but the actual reaction doesn't matter; we should stop the cause, not worry about the symptom).

-Peff

Jeff King

2016-06-15 22:56 UTC #4 lore raw

Quoted 6 lines · Jeff King

Quoted 4 lines · Johannes Sixt
Quoted 3 lines
I think that is OK, but I'm curious why this is a problem _now_, and not with the code prior to 97a83fa8. The old GIT_DEBUG_SEND_PACK was also just calling write() to descriptor 3.

Quoted 8 lines

Quoted 4 lines
Before this change, both affected commands completed and the trace file was empty. Notice that in both test cases we only check for the absence of certain lines, which is naturally true for an empty file, so that the tests pass.
Hmm. The code in t5503 is similar, but before my patch used to actually use "test -s" to make sure that some trace output was written. Did it fail before 97a83fa8 (and does it pass now)?

Ah, I see. It did fail, and it was marked with NOT_MINGW. I think we can fix that now. Patch coming in a moment.

-Peff

Jeff King

2016-06-15 22:56 UTC #5 lore raw

PATCH 1/1 do not use GIT_TRACE_PACKET=3 in tests

Some test scripts use the GIT_TRACE mechanism to dump debugging information to descriptor 3 (and point it to a file using the shell). On Windows, however, bash is unable to set up descriptor 3. We do not write our trace to the file, and worse, we may interfere with other operations happening on descriptor 3, causing tests to fail or other even behave inconsistently.

Prior to commit 97a83fa (upload-pack: remove packet debugging harness), these tests used GIT_DEBUG_SEND_PACK, which only supported output to a descriptor. The tests in t5503 were always broken on Windows, and were marked to be skipped via the NOT_MINGW prerequisite. In t5700, the tests used to pass prior to 97a83fa, but only because they were not careful enough; because we only grepped the trace file, an empty file looked successful to us. But post-97a83fa, the writing to descriptor 3 causes "git fetch" to hang (presumably because we are throwing random bytes into the middle of the protocol).

Now that we are using the GIT_TRACE mechanism, we can improve both scripts by asking git to write directly to a file rather than a descriptor. That fixes the hang in t5700, and should allow t5503 to successfully run on Windows.

In both cases we now also use "test -s" to double-check that our trace file actually contains output, which should reduce the possibility of an erroneously passing test.

Signed-off-by Jeff King <[email protected]>

---
On top of jk/pkt-line-cleanup.

This works for me on Linux, but I do not have a mingw system to test t5503 on. If my reasoning is right, it should, but one never knows. :)

Johannes, can you report whether it fixes the problem?

 t/t5503-tagfollow.sh       | 38 ++++++++++++++++++--------------------
 t/t5700-clone-reference.sh | 14 +++++++++-----
 2 files changed, 27 insertions(+), 25 deletions(-)

t/t5503-tagfollow.sh +18 ~~-20~~

index d181c96..f30c038 100755
@@ -4,10 +4,6 @@ test_description='test automatic tag following'
 
 . ./test-lib.sh
 
-if ! test_have_prereq NOT_MINGW; then
-	say "GIT_TRACE_PACKET not supported - skipping tests"
-fi
-
 # End state of the repository:
 #
 #         T - tag1          S - tag2

@@ -17,7 +13,7 @@ fi
 #     \   C - origin/cat    \
 #      origin/master         master
 
-test_expect_success NOT_MINGW setup '
+test_expect_success setup '
 	test_tick &&
 	echo ichi >file &&
 	git add file &&

@@ -39,33 +35,35 @@ test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '
 '
 
 U=UPLOAD_LOG
+UPATH="$(pwd)/$U"
 
-test_expect_success NOT_MINGW 'setup expect' '
+test_expect_success 'setup expect' '
 cat - <<EOF >expect
 want $A
 EOF
 '
 
 get_needs () {
+	test -s "$1" &&
 	perl -alne '
 		next unless $F[1] eq "upload-pack<";
 		last if $F[2] eq "0000";
 		print $F[2], " ", $F[3];
-	' "$@"
+	' "$1"
 }
 
-test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '
+test_expect_success 'fetch A (new commit : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_TRACE_PACKET=3 git fetch 3>../$U &&
+		GIT_TRACE_PACKET=$UPATH git fetch &&
 		test $A = $(git rev-parse --verify origin/master)
 	) &&
 	get_needs $U >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success NOT_MINGW "create tag T on A, create C on branch cat" '
+test_expect_success "create tag T on A, create C on branch cat" '
 	git tag -a -m tag1 tag1 $A &&
 	T=$(git rev-parse --verify tag1) &&

@@ -77,18 +75,18 @@ test_expect_success NOT_MINGW 'fetch C, T (new branch, tag : 1 connection)' '
 	git checkout master
 '
 
-test_expect_success NOT_MINGW 'setup expect' '
+test_expect_success 'setup expect' '
 cat - <<EOF >expect
 want $C
 want $T
 EOF
 '
 
-test_expect_success NOT_MINGW 'fetch C, T (new branch, tag : 1 connection)' '
+test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_TRACE_PACKET=3 git fetch 3>../$U &&
+		GIT_TRACE_PACKET=$UPATH git fetch &&
 		test $C = $(git rev-parse --verify origin/cat) &&
 		test $T = $(git rev-parse --verify tag1) &&
 		test $A = $(git rev-parse --verify tag1^0)

@@ -97,7 +95,7 @@ test_expect_success NOT_MINGW 'fetch C, T (new branch, tag : 1 connection)' '
 	test_cmp expect actual
 '
 
-test_expect_success NOT_MINGW "create commits O, B, tag S on B" '
+test_expect_success "create commits O, B, tag S on B" '
 	test_tick &&
 	echo O >file &&
 	git add file &&

@@ -113,18 +111,18 @@ test_expect_success NOT_MINGW 'fetch B, S (commit and tag : 1 connection)' '
 	S=$(git rev-parse --verify tag2)
 '
 
-test_expect_success NOT_MINGW 'setup expect' '
+test_expect_success 'setup expect' '
 cat - <<EOF >expect
 want $B
 want $S
 EOF
 '
 
-test_expect_success NOT_MINGW 'fetch B, S (commit and tag : 1 connection)' '
+test_expect_success 'fetch B, S (commit and tag : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_TRACE_PACKET=3 git fetch 3>../$U &&
+		GIT_TRACE_PACKET=$UPATH git fetch &&
 		test $B = $(git rev-parse --verify origin/master) &&
 		test $B = $(git rev-parse --verify tag2^0) &&
 		test $S = $(git rev-parse --verify tag2)

@@ -133,14 +131,14 @@ EOF
 	test_cmp expect actual
 '
 
-test_expect_success NOT_MINGW 'setup expect' '
+test_expect_success 'setup expect' '
 cat - <<EOF >expect
 want $B
 want $S
 EOF
 '
 
-test_expect_success NOT_MINGW 'new clone fetch master and tags' '
+test_expect_success 'new clone fetch master and tags' '
 	git branch -D cat
 	rm -f $U
 	(

@@ -148,7 +146,7 @@ test_expect_success NOT_MINGW 'new clone fetch master and tags' '
 		cd clone2 &&
 		git init &&
 		git remote add origin .. &&
-		GIT_TRACE_PACKET=3 git fetch 3>../$U &&
+		GIT_TRACE_PACKET=$UPATH git fetch &&
 		test $B = $(git rev-parse --verify origin/master) &&
 		test $S = $(git rev-parse --verify tag2) &&
 		test $B = $(git rev-parse --verify tag2^0) &&

t/t5700-clone-reference.sh +9 -5

index 9cd3b4d..60f1552 100755
@@ -54,11 +54,14 @@ test_expect_success 'cloning with reference (no -l -s)' \
 
 rm -f "$U.D"
 
-test_expect_success 'cloning with reference (no -l -s)' \
-'GIT_TRACE_PACKET=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"'
+test_expect_success 'cloning with reference (no -l -s)' '
+	GIT_TRACE_PACKET=$U.D git clone --reference B "file://$(pwd)/A" D
+'
 
-test_expect_success 'fetched no objects' \
-'! grep " want" "$U.D"'
+test_expect_success 'fetched no objects' '
+	test -s "$U.D" &&
+	! grep " want" "$U.D"
+'
 
 cd "$base_dir"

@@ -173,9 +176,10 @@ test_expect_success 'fetch with incomplete alternates' '
 	(
 		cd K &&
 		git remote add J "file://$base_dir/J" &&
-		GIT_TRACE_PACKET=3 git fetch J 3>"$U.K"
+		GIT_TRACE_PACKET=$U.K git fetch J
 	) &&
 	master_object=$(cd A && git for-each-ref --format="%(objectname)" refs/heads/master) &&
+	test -s "$U.K" &&
 	! grep " want $master_object" "$U.K" &&
 	tag_object=$(cd A && git for-each-ref --format="%(objectname)" refs/tags/HEAD) &&
 	! grep " want $tag_object" "$U.K"

Signature · 1 line

1.8.2.22.g1efe1a3

Johannes Sixt

author Tested-by on v1 1/1

2016-06-15 22:56 UTC #6 lore raw

Re: [PATCH] do not use GIT_TRACE_PACKET=3 in tests

Am 3/20/2013 18:43, schrieb Jeff King:

Quoted 4 lines

Now that we are using the GIT_TRACE mechanism, we can improve both scripts by asking git to write directly to a file rather than a descriptor. That fixes the hang in t5700, and should allow t5503 to successfully run on Windows.

Well spotted, and, right, both tests pass with this patch.

Tested-by Johannes Sixt <[email protected]>

Thanks,
-- Hannes

Junio C Hamano

2016-06-15 22:56 UTC #7 lore raw

Re: [PATCH] do not use GIT_TRACE_PACKET=3 in tests

Quoted 9 lines · Johannes Sixt

Am 3/20/2013 18:43, schrieb Jeff King:
Quoted 4 lines
Now that we are using the GIT_TRACE mechanism, we can improve both scripts by asking git to write directly to a file rather than a descriptor. That fixes the hang in t5700, and should allow t5503 to successfully run on Windows.
Well spotted, and, right, both tests pass with this patch.
Tested-by Johannes Sixt <[email protected]>

Thanks.

Eric Sunshine

2016-06-15 22:56 UTC #8 lore raw

Re: [PATCH] do not use GIT_TRACE_PACKET=3 in tests

In reply to Jeff King

Quoted 7 lines · Jeff King

Some test scripts use the GIT_TRACE mechanism to dump debugging information to descriptor 3 (and point it to a file using the shell). On Windows, however, bash is unable to set up descriptor 3. We do not write our trace to the file, and worse, we may interfere with other operations happening on descriptor 3, causing tests to fail or other even behave inconsistently.

s/other even/even/

Jeff King

2016-06-15 22:56 UTC #9 lore raw

Re: [PATCH] do not use GIT_TRACE_PACKET=3 in tests

Quoted 10 lines · Eric Sunshine

Quoted 7 lines · Jeff King
Some test scripts use the GIT_TRACE mechanism to dump debugging information to descriptor 3 (and point it to a file using the shell). On Windows, however, bash is unable to set up descriptor 3. We do not write our trace to the file, and worse, we may interfere with other operations happening on descriptor 3, causing tests to fail or other even behave inconsistently.
s/other even/even/

Urgh, I should probably add another proofreading pass to my commit messages. Thanks.

-Peff

Junio C Hamano

2016-06-15 22:56 UTC #10 lore raw

In reply to Jeff King

Quoted 7 lines · Jeff King

Also, though I think your use of stderr is probably OK, might it be a little more robust against future output changes to use:
GIT_TRACE_PACKET=$PWD/$U.D git clone ...
to write directly to the file. The original GIT_DEBUG_SEND_PACK did not support such niceties, but GIT_TRACE gives them to us for free.

Sounds very sensible.