[PATCHv2] git-remote-testgit: fix race when spawning fast-import

[Pete Wyckoff <pw@padd.com>]

Test "pushing to local repo" in t5800-remote-helpers can hang
due to a race condition in git-remote-testgit.  Fix it by
setting stdin to unbuffered.

On the writer side, "git push" invokes push_refs_with_export(),
which sends to stdout the command "export\n" and immediately
starts up "git fast-export".  The latter writes its output stream
to the same stdout.

On the reader side, remote helper "git-remote-testgit" reads from
stdin to get its next command.  It uses getc() to read characters
from libc up until \n.  Libc has buffered a potentially much
larger chunk of stdin.  When it sees the "export\n" command, it
forks "git fast-import" to read the stream.

If fast-export finishes before git fast-import starts, the
fast-export output can end up in libc's buffer in
git-remote-testgit, rather than in git fast-import.  The latter
hangs indefinitely on a now-empty stdin.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
gitster@pobox.com wrote on Sat, 21 Apr 2012 21:50 -0700:
> If I understand your explanation correctly, the primary purpose of the
> remote-testgit is to test the parts of the system that talk to remote
> helpers that are used in production in the t/t5800 script, and this
> "sleep" is to make it easier to trigger the particular bug you fixed 
> in *this* script.  The bug is _not_ in the parts of the system being
> tested, but is in this test scaffolding.

Indeed.  I tried to make that more obvious in the commit message.

> If that is the case, then it should not be enabled unconditionally.
> When somebody wants to see if remote-testgit was broken again (perhaps
> after observing occassional hangs), the environment should be set when
> running the test, but not in t5800.

Clemens suggested disabling the test by default, as I've done
here.  I think it would be okay to remove it entirely, too.

		-- Pete

 git-remote-testgit.py     |    7 +++++++
 t/t5800-remote-helpers.sh |   14 ++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 3dc4851..5f3ebd2 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -22,6 +22,7 @@ except ImportError:
     _digest = sha.new
 import sys
 import os
+import time
 sys.path.insert(0, os.getenv("GITPYTHONLIB","."))
 
 from git_remote_helpers.util import die, debug, warn
@@ -204,6 +205,11 @@ def read_one_line(repo):
     """Reads and processes one command.
     """
 
+    sleepy = os.environ.get("GIT_REMOTE_TESTGIT_SLEEPY")
+    if sleepy:
+        debug("Sleeping %d sec before readline" % int(sleepy))
+        time.sleep(int(sleepy))
+
     line = sys.stdin.readline()
 
     cmdline = line
@@ -258,6 +264,7 @@ def main(args):
 
     more = True
 
+    sys.stdin = os.fdopen(sys.stdin.fileno(), 'r', 0)
     while (more):
         more = read_one_line(repo)
 
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 1c62001..85a8042 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -72,6 +72,20 @@ test_expect_success 'pushing to local repo' '
 	compare_refs localclone HEAD server HEAD
 '
 
+# Generally, skip this test.  It demonstrates a now-fixed
+# race in git-remote-testgit, but is too slow to leave in
+# for general use.
+test_expect_success DEBUG_TESTGIT_RACE 'racily pushing to local repo' '
+	cp -a server server2 &&
+	git clone "testgit::${PWD}/server2" localclone2 &&
+	test_when_finished "rm -rf server2 localclone2" &&
+	(cd localclone2 &&
+	echo content >>file &&
+	git commit -a -m three &&
+	GIT_REMOTE_TESTGIT_SLEEPY=2 git push) &&
+	compare_refs localclone2 HEAD server2 HEAD
+'
+
 test_expect_success 'synch with changes from localclone' '
 	(cd clone &&
 	 git pull)
-- 
1.7.10.57.g437cb.dirty