git-daemon wrapper to wait until daemon is ready

PATCH In review Clemens Buchacher 5 participants 18 messages Jun 15, 2016 Kbuild

Share & tools

Clemens Buchacher

author

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

PATCH 1/1 git-daemon wrapper to wait until daemon is ready

The shell script which is currently used to parse git daemon output does not seem to work unreliably. In order to work around such issues, re-implement the same procedure in C and write the daemon pid to a file.

This means that we can no longer wait on the daemon process, since it is no longer a direct child of the shell process.

Signed-off-by Clemens Buchacher <[email protected]>

---

Does this patch improve your situation?

Note that t5570 fails on current pu, because of the push.default warnings. I am sending an independent patch for that.

Clemens

 .gitignore          |    1 +
 Makefile            |    1 +
 cache.h             |    1 +
 t/lib-git-daemon.sh |   30 +++-------------------
 test-git-daemon.c   |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++
 wrapper.c           |   21 +++++++++++++++
 6 files changed, 99 insertions(+), 26 deletions(-)
 create mode 100644 test-git-daemon.c

.gitignore +1 -0

index 87fcc5f..18a484c 100644
@@ -177,6 +177,7 @@
 /test-dump-cache-tree
 /test-scrap-cache-tree
 /test-genrandom
+/test-git-daemon
 /test-index-version
 /test-line-buffer
 /test-match-trees

Makefile +1 -0

index be1957a..7317daa 100644
@@ -477,6 +477,7 @@ TEST_PROGRAMS_NEED_X += test-delta
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
 TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
 TEST_PROGRAMS_NEED_X += test-genrandom
+TEST_PROGRAMS_NEED_X += test-git-daemon
 TEST_PROGRAMS_NEED_X += test-index-version
 TEST_PROGRAMS_NEED_X += test-line-buffer
 TEST_PROGRAMS_NEED_X += test-match-trees

cache.h +1 -0

index e5e1aa4..6351c15 100644
@@ -1176,6 +1176,7 @@ extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg
 extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
 extern void fsync_or_die(int fd, const char *);
 
+extern ssize_t read_line(int fd, void *buf, size_t count);
 extern ssize_t read_in_full(int fd, void *buf, size_t count);
 extern ssize_t write_in_full(int fd, const void *buf, size_t count);
 static inline ssize_t write_str_in_full(int fd, const char *str)

t/lib-git-daemon.sh +4 ~~-26~~

index ef2d01f..9fefae1 100644
@@ -23,27 +23,13 @@ start_git_daemon() {
 	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
 
 	say >&3 "Starting git daemon ..."
-	mkfifo git_daemon_output
-	git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
+	test-git-daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
 		--reuseaddr --verbose \
 		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
-		>&3 2>git_daemon_output &
-	GIT_DAEMON_PID=$!
-	{
-		read line
-		echo >&4 "$line"
-		cat >&4 &
-
-		# Check expected output
-		if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
-		then
-			kill "$GIT_DAEMON_PID"
-			wait "$GIT_DAEMON_PID"
-			trap 'die' EXIT
-			error "git daemon failed to start"
-		fi
-	} <git_daemon_output
+		>&3 2>&4 ||
+		error "git daemon failed to start"
+	GIT_DAEMON_PID=$(cat git-daemon.pid)
 }
 
 stop_git_daemon() {

@@ -57,13 +43,5 @@ stop_git_daemon() {
 	# kill git-daemon child of git
 	say >&3 "Stopping git daemon ..."
 	kill "$GIT_DAEMON_PID"
-	wait "$GIT_DAEMON_PID" >&3 2>&4
-	ret=$?
-	# expect exit with status 143 = 128+15 for signal TERM=15
-	if test $ret -ne 143
-	then
-		error "git daemon exited with status: $ret"
-	fi
 	GIT_DAEMON_PID=
-	rm -f git_daemon_output
 }

test-git-daemon.c new file +71 -0

71 lines not on this page · raw message

wrapper.c +21 -0

index 85f09df..7bf6dda 100644
@@ -141,6 +141,27 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 	}
 }
 
+ssize_t read_line(int fd, void *buf, size_t count)
+{
+	char *p = buf;
+	ssize_t total = 0;
+
+	while (count > 0) {
+		ssize_t loaded = xread(fd, p, 1);
+		if (loaded < 0)
+			return -1;
+		if (loaded == 0)
+			return total;
+		count -= loaded;
+		total += loaded;
+		if (*p == '\n')
+			break;
+		p += loaded;
+	}
+
+	return total;
+}
+
 ssize_t read_in_full(int fd, void *buf, size_t count)
 {
 	char *p = buf;

Signature · 1 line

1.7.9.6

Ben Walton

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

Excerpts from Clemens Buchacher's message of Sat Apr 14 14:29:07 -0400 2012:

Hi Clemens,

Quoted 2 lines

The shell script which is currently used to parse git daemon output does not seem to work unreliably. In order to work around such issues,

Presumably you mean "work reliably" here?

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Clemens Buchacher

author

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

Quoted 5 lines · Ben Walton

Quoted 3 lines
The shell script which is currently used to parse git daemon output does not seem to work unreliably. In order to work around such issues,
Presumably you mean "work reliably" here?

Yes, that's what I meant the commit message to say. Thanks for noticing.

But I would like to wait for more info from the OP before we conclude that this is indeed the case.

Clemens Buchacher

author

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

PATCH v2 1/1 git-daemon wrapper to wait until daemon is ready

The shell script which is currently used to parse git daemon output does not seem to work reliably. In order to work around such issues, re-implement the same procedure in C and write the daemon pid to a file.

This means that we can no longer wait on the daemon process, since it is not a direct child of the shell process.

Signed-off-by Clemens Buchacher <[email protected]>

---

Quoted 3 lines · Ben Walton

Quoted 2 lines
Presumably you mean "work reliably" here?

I fixed the commit message. I also noticed that the buffer returned by read_line had no zero terminator. And the parse_error variable was unnecessary.

 .gitignore          |    1 +
 Makefile            |    1 +
 cache.h             |    1 +
 t/lib-git-daemon.sh |   30 +++-------------------
 test-git-daemon.c   |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++
 wrapper.c           |   21 ++++++++++++++++
 6 files changed, 97 insertions(+), 26 deletions(-)
 create mode 100644 test-git-daemon.c

.gitignore +1 -0

index 87fcc5f..18a484c 100644
@@ -177,6 +177,7 @@
 /test-dump-cache-tree
 /test-scrap-cache-tree
 /test-genrandom
+/test-git-daemon
 /test-index-version
 /test-line-buffer
 /test-match-trees

Makefile +1 -0

index be1957a..7317daa 100644
@@ -477,6 +477,7 @@ TEST_PROGRAMS_NEED_X += test-delta
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
 TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
 TEST_PROGRAMS_NEED_X += test-genrandom
+TEST_PROGRAMS_NEED_X += test-git-daemon
 TEST_PROGRAMS_NEED_X += test-index-version
 TEST_PROGRAMS_NEED_X += test-line-buffer
 TEST_PROGRAMS_NEED_X += test-match-trees

cache.h +1 -0

index e5e1aa4..6351c15 100644
@@ -1176,6 +1176,7 @@ extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg
 extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
 extern void fsync_or_die(int fd, const char *);
 
+extern ssize_t read_line(int fd, void *buf, size_t count);
 extern ssize_t read_in_full(int fd, void *buf, size_t count);
 extern ssize_t write_in_full(int fd, const void *buf, size_t count);
 static inline ssize_t write_str_in_full(int fd, const char *str)

t/lib-git-daemon.sh +4 ~~-26~~

index ef2d01f..9fefae1 100644
@@ -23,27 +23,13 @@ start_git_daemon() {
 	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
 
 	say >&3 "Starting git daemon ..."
-	mkfifo git_daemon_output
-	git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
+	test-git-daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
 		--reuseaddr --verbose \
 		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
-		>&3 2>git_daemon_output &
-	GIT_DAEMON_PID=$!
-	{
-		read line
-		echo >&4 "$line"
-		cat >&4 &
-
-		# Check expected output
-		if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
-		then
-			kill "$GIT_DAEMON_PID"
-			wait "$GIT_DAEMON_PID"
-			trap 'die' EXIT
-			error "git daemon failed to start"
-		fi
-	} <git_daemon_output
+		>&3 2>&4 ||
+		error "git daemon failed to start"
+	GIT_DAEMON_PID=$(cat git-daemon.pid)
 }
 
 stop_git_daemon() {

@@ -57,13 +43,5 @@ stop_git_daemon() {
 	# kill git-daemon child of git
 	say >&3 "Stopping git daemon ..."
 	kill "$GIT_DAEMON_PID"
-	wait "$GIT_DAEMON_PID" >&3 2>&4
-	ret=$?
-	# expect exit with status 143 = 128+15 for signal TERM=15
-	if test $ret -ne 143
-	then
-		error "git daemon exited with status: $ret"
-	fi
 	GIT_DAEMON_PID=
-	rm -f git_daemon_output
 }

test-git-daemon.c new file +69 -0

69 lines not on this page · raw message

wrapper.c +21 -0

index 85f09df..7bf6dda 100644
@@ -141,6 +141,27 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 	}
 }
 
+ssize_t read_line(int fd, void *buf, size_t count)
+{
+	char *p = buf;
+	ssize_t total = 0;
+
+	while (count > 0) {
+		ssize_t loaded = xread(fd, p, 1);
+		if (loaded < 0)
+			return -1;
+		if (loaded == 0)
+			return total;
+		count -= loaded;
+		total += loaded;
+		if (*p == '\n')
+			break;
+		p += loaded;
+	}
+
+	return total;
+}
+
 ssize_t read_in_full(int fd, void *buf, size_t count)
 {
 	char *p = buf;

Signature · 1 line

1.7.9.6

Clemens Buchacher

author

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

PATCH 1/1 t5570: use explicit push refspec

In reply to Clemens Buchacher

The default mode for push without arguments will change. Some warnings are about to be enabled for such use, which causes some t5570 tests to fail because they do not expect this output. Fix this by passing an explicit refspec to git push.

Signed-off-by Clemens Buchacher <[email protected]>

---

Quoted 3 lines · Clemens Buchacher

Note that t5570 fails on current pu, because of the push.default warnings. I am sending an independent patch for that.

Here we go.

 t/t5570-git-daemon.sh |   30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

t/t5570-git-daemon.sh +14 ~~-16~~

index 7cbc999..a3a4e47 100755
@@ -103,14 +103,12 @@ test_remote_error()
 		esac
 	done
 
-	if test $# -ne 3
-	then
-		error "invalid number of arguments"
-	fi
-
+	msg=$1
+	shift
 	cmd=$1
-	repo=$2
-	msg=$3
+	shift
+	repo=$1
+	shift || error "invalid number of arguments"
 
 	if test -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo"
 	then

@@ -122,7 +120,7 @@ test_remote_error()
 		fi
 	fi
 
-	test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" 2>output &&
+	test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" "$@" 2>output &&
 	echo "fatal: remote error: $msg: /$repo" >expect &&
 	test_cmp expect output
 	ret=$?

@@ -131,18 +129,18 @@ test_remote_error()
 }
 
 msg="access denied or repository not exported"
-test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git '$msg'"
-test_expect_success 'push disabled'      "test_remote_error    push  repo.git    '$msg'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    '$msg'"
-test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    '$msg'"
+test_expect_success 'clone non-existent' "test_remote_error    '$msg' clone nowhere.git    "
+test_expect_success 'push disabled'      "test_remote_error    '$msg' push  repo.git master"
+test_expect_success 'read access denied' "test_remote_error -x '$msg' fetch repo.git       "
+test_expect_success 'not exported'       "test_remote_error -n '$msg' fetch repo.git       "
 
 stop_git_daemon
 start_git_daemon --informative-errors
 
-test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
-test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
-test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
+test_expect_success 'clone non-existent' "test_remote_error    'no such repository'      clone nowhere.git    "
+test_expect_success 'push disabled'      "test_remote_error    'service not enabled'     push  repo.git master"
+test_expect_success 'read access denied' "test_remote_error -x 'no such repository'      fetch repo.git       "
+test_expect_success 'not exported'       "test_remote_error -n 'repository not exported' fetch repo.git       "
 
 stop_git_daemon
 test_done

Signature · 1 line

1.7.9.6

Junio C Hamano

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

Re: [PATCH] t5570: use explicit push refspec

Quoted 4 lines · Clemens Buchacher

The default mode for push without arguments will change. Some warnings are about to be enabled for such use, which causes some t5570 tests to fail because they do not expect this output. Fix this by passing an explicit refspec to git push.

I wonder if a better fix is to configure "push.default = matching" in the test repository. Otherwise wouldn't the result of the push change once the default changes?

Quoted 73 lines

Signed-off-by Clemens Buchacher <[email protected]>

---

Quoted 3 lines · Clemens Buchacher

Note that t5570 fails on current pu, because of the push.default warnings. I am sending an independent patch for that.

Here we go.

 t/t5570-git-daemon.sh |   30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

t/t5570-git-daemon.sh +14 ~~-16~~

index 7cbc999..a3a4e47 100755
@@ -103,14 +103,12 @@ test_remote_error()
 		esac
 	done
 
-	if test $# -ne 3
-	then
-		error "invalid number of arguments"
-	fi
-
+	msg=$1
+	shift
 	cmd=$1
-	repo=$2
-	msg=$3
+	shift
+	repo=$1
+	shift || error "invalid number of arguments"
 
 	if test -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo"
 	then

@@ -122,7 +120,7 @@ test_remote_error()
 		fi
 	fi
 
-	test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" 2>output &&
+	test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" "$@" 2>output &&
 	echo "fatal: remote error: $msg: /$repo" >expect &&
 	test_cmp expect output
 	ret=$?

@@ -131,18 +129,18 @@ test_remote_error()
 }
 
 msg="access denied or repository not exported"
-test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git '$msg'"
-test_expect_success 'push disabled'      "test_remote_error    push  repo.git    '$msg'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    '$msg'"
-test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    '$msg'"
+test_expect_success 'clone non-existent' "test_remote_error    '$msg' clone nowhere.git    "
+test_expect_success 'push disabled'      "test_remote_error    '$msg' push  repo.git master"
+test_expect_success 'read access denied' "test_remote_error -x '$msg' fetch repo.git       "
+test_expect_success 'not exported'       "test_remote_error -n '$msg' fetch repo.git       "
 
 stop_git_daemon
 start_git_daemon --informative-errors
 
-test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
-test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
-test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
+test_expect_success 'clone non-existent' "test_remote_error    'no such repository'      clone nowhere.git    "
+test_expect_success 'push disabled'      "test_remote_error    'service not enabled'     push  repo.git master"
+test_expect_success 'read access denied' "test_remote_error -x 'no such repository'      fetch repo.git       "
+test_expect_success 'not exported'       "test_remote_error -n 'repository not exported' fetch repo.git       "
 
 stop_git_daemon
 test_done

Clemens Buchacher

author

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

Re: [PATCH] t5570: use explicit push refspec

Quoted 10 lines · Junio C Hamano

Quoted 4 lines · Clemens Buchacher
The default mode for push without arguments will change. Some warnings are about to be enabled for such use, which causes some t5570 tests to fail because they do not expect this output. Fix this by passing an explicit refspec to git push.
I wonder if a better fix is to configure "push.default = matching" in the test repository. Otherwise wouldn't the result of the push change once the default changes?

The push.default option matters only if a refspec is not specified. By adding a refspec, push.default should not matter any more. Unless that is going to change as well?

Junio C Hamano

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

Re: [PATCH] t5570: use explicit push refspec

Quoted 8 lines · Clemens Buchacher

Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Clemens Buchacher
The default mode for push without arguments will change. Some warnings are about to be enabled for such use, which causes some t5570 tests to fail because they do not expect this output. Fix this by passing an explicit refspec to git push.

Quoted 7 lines

Quoted 3 lines
I wonder if a better fix is to configure "push.default = matching" in the test repository. Otherwise wouldn't the result of the push change once the default changes?
The push.default option matters only if a refspec is not specified. By adding a refspec, push.default should not matter any more. Unless that is going to change as well?

No, I was thinking more about testing cases where there is no refspec on the command line, which we used to test, but with your patch we no longer do. In other words, your fix not just squelches the advice message and make them pass, but it changes the way the command behaves, no?

Besides, that way you do not have to swap the parameters to test_remote_error so we do not have scratch our heads wondering why we have changes to test vectors that run clones and fetches.

Clemens Buchacher

author

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

Re: [PATCH] t5570: use explicit push refspec

Quoted 14 lines · Junio C Hamano

Quoted 12 lines · Clemens Buchacher
Quoted 10 lines · Junio C Hamano
Quoted 4 lines · Clemens Buchacher
The default mode for push without arguments will change. Some warnings are about to be enabled for such use, which causes some t5570 tests to fail because they do not expect this output. Fix this by passing an explicit refspec to git push.
I wonder if a better fix is to configure "push.default = matching" in the test repository. Otherwise wouldn't the result of the push change once the default changes?

Quoted 8 lines

Quoted 3 lines
The push.default option matters only if a refspec is not specified. By adding a refspec, push.default should not matter any more. Unless that is going to change as well?
No, I was thinking more about testing cases where there is no refspec on the command line, which we used to test, but with your patch we no longer do. In other words, your fix not just squelches the advice message and make them pass, but it changes the way the command behaves, no?

It does exactly the same thing it did before, since there is only one local branch. The goal of the test is not to check behavior of git push without arguments. Since that is subject to change, and is causing causing the test to fail already for no good reason, I think it better to use the more explicit version of the command.

Quoted 3 lines

Besides, that way you do not have to swap the parameters to test_remote_error so we do not have scratch our heads wondering why we have changes to test vectors that run clones and fetches.

In my opinion, this change is also an improvement in itself, since now we can more easily pass extra arguments to test_remote_error. Maybe the scratching of heads can be alleviated by amending the commit message like so?

-->o--
Subject: [PATCH] t5570: use explicit push refspec

The default mode for push without arguments will change. Some warnings are about to be enabled for such use, which causes some t5570 tests to fail because they do not expect this output.

Fix this by passing an explicit refspec to git push. To that end, change the calling conventions of test_remote_error in order to accomodate extra command arguments.

Signed-off-by Clemens Buchacher <[email protected]>

---
 t/t5570-git-daemon.sh |   30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

t/t5570-git-daemon.sh +14 ~~-16~~

index 7cbc999..a3a4e47 100755
@@ -103,14 +103,12 @@ test_remote_error()
 		esac
 	done
 
-	if test $# -ne 3
-	then
-		error "invalid number of arguments"
-	fi
-
+	msg=$1
+	shift
 	cmd=$1
-	repo=$2
-	msg=$3
+	shift
+	repo=$1
+	shift || error "invalid number of arguments"
 
 	if test -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo"
 	then

@@ -122,7 +120,7 @@ test_remote_error()
 		fi
 	fi
 
-	test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" 2>output &&
+	test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" "$@" 2>output &&
 	echo "fatal: remote error: $msg: /$repo" >expect &&
 	test_cmp expect output
 	ret=$?

@@ -131,18 +129,18 @@ test_remote_error()
 }
 
 msg="access denied or repository not exported"
-test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git '$msg'"
-test_expect_success 'push disabled'      "test_remote_error    push  repo.git    '$msg'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    '$msg'"
-test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    '$msg'"
+test_expect_success 'clone non-existent' "test_remote_error    '$msg' clone nowhere.git    "
+test_expect_success 'push disabled'      "test_remote_error    '$msg' push  repo.git master"
+test_expect_success 'read access denied' "test_remote_error -x '$msg' fetch repo.git       "
+test_expect_success 'not exported'       "test_remote_error -n '$msg' fetch repo.git       "
 
 stop_git_daemon
 start_git_daemon --informative-errors
 
-test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
-test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
-test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
+test_expect_success 'clone non-existent' "test_remote_error    'no such repository'      clone nowhere.git    "
+test_expect_success 'push disabled'      "test_remote_error    'service not enabled'     push  repo.git master"
+test_expect_success 'read access denied' "test_remote_error -x 'no such repository'      fetch repo.git       "
+test_expect_success 'not exported'       "test_remote_error -n 'repository not exported' fetch repo.git       "
 
 stop_git_daemon
 test_done

Signature · 1 line

1.7.9.6

Junio C Hamano

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

Re: [PATCH] t5570: use explicit push refspec

Quoted 12 lines · Clemens Buchacher

In my opinion, this change is also an improvement in itself, since now we can more easily pass extra arguments to test_remote_error. Maybe the scratching of heads can be alleviated by amending the commit message like so?
-->o--
Subject: [PATCH] t5570: use explicit push refspec
The default mode for push without arguments will change. Some warnings are about to be enabled for such use, which causes some t5570 tests to fail because they do not expect this output.

Quoted 5 lines

Fix this by passing an explicit refspec to git push. To that end, change the calling conventions of test_remote_error in order to accomodate extra command arguments.
Signed-off-by Clemens Buchacher <[email protected]>

Sounds good. Thanks.

Johannes Sixt

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

In reply to Clemens Buchacher

Am 14.04.2012 20:29, schrieb Clemens Buchacher:

test-git-daemon.c line 42

Quoted 1 line

test-git-daemon.c new file +1 -0
+	r = read_line(proc.err, buf, sizeof(buf));

We have strbuf_getwholeline_fd().

test-git-daemon.c line 54

Quoted 4 lines

test-git-daemon.c new file +4 -0

+	memset(&cat, 0, sizeof(cat));
+	cat.argv = (const char **)cat_argv;
+	cat.in = proc.err;
+	cat.out = 2;

Useless use of cat?

-- Hannes

Clemens Buchacher

author

2016-06-15 22:53 UTC #11 lore raw

Quoted 4 lines · Johannes Sixt

Am 14.04.2012 20:29, schrieb Clemens Buchacher:
test-git-daemon.c line 42
Quoted 1 line
test-git-daemon.c new file +1 -0
+	r = read_line(proc.err, buf, sizeof(buf));
We have strbuf_getwholeline_fd().

Thanks. Will fix.

Quoted 6 lines

test-git-daemon.c line 54
Quoted 4 lines
test-git-daemon.c new file +4 -0
+	memset(&cat, 0, sizeof(cat));
+	cat.argv = (const char **)cat_argv;
+	cat.in = proc.err;
+	cat.out = 2;
Useless use of cat?

I don't see how I could avoid cat here. I have to create a pipe first so that I can read the first line. And then I have to terminate test-git-daemon in order to start the tests. So I cannot continue reading synchronously.

Clemens Buchacher

author

2016-06-15 22:53 UTC #12 lore raw

PATCH v3 1/1 git-daemon wrapper to wait until daemon is ready

This means that we can no longer wait on the daemon process, since it is no longer a direct child of the shell process.

Signed-off-by Clemens Buchacher <[email protected]>

---

Quoted 7 lines · Clemens Buchacher

Quoted 4 lines · Johannes Sixt
Am 14.04.2012 20:29, schrieb Clemens Buchacher:
Review
Quoted 1 line
(quoted diff) +1 -0
+	r = read_line(proc.err, buf, sizeof(buf));
We have strbuf_getwholeline_fd().
Thanks. Will fix.

Here's the re-roll for completeness. Still waiting on feedback from the OP if this actually solves the problem, though.

 .gitignore          |    1 +
 Makefile            |    1 +
 t/lib-git-daemon.sh |   30 ++++---------------------
 test-git-daemon.c   |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+), 26 deletions(-)
 create mode 100644 test-git-daemon.c

.gitignore +1 -0

index 87fcc5f..18a484c 100644
@@ -177,6 +177,7 @@
 /test-dump-cache-tree
 /test-scrap-cache-tree
 /test-genrandom
+/test-git-daemon
 /test-index-version
 /test-line-buffer
 /test-match-trees

Makefile +1 -0

index be1957a..7317daa 100644
@@ -477,6 +477,7 @@ TEST_PROGRAMS_NEED_X += test-delta
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
 TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
 TEST_PROGRAMS_NEED_X += test-genrandom
+TEST_PROGRAMS_NEED_X += test-git-daemon
 TEST_PROGRAMS_NEED_X += test-index-version
 TEST_PROGRAMS_NEED_X += test-line-buffer
 TEST_PROGRAMS_NEED_X += test-match-trees

t/lib-git-daemon.sh +4 ~~-26~~

index ef2d01f..9fefae1 100644
@@ -23,27 +23,13 @@ start_git_daemon() {
 	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
 
 	say >&3 "Starting git daemon ..."
-	mkfifo git_daemon_output
-	git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
+	test-git-daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
 		--reuseaddr --verbose \
 		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
-		>&3 2>git_daemon_output &
-	GIT_DAEMON_PID=$!
-	{
-		read line
-		echo >&4 "$line"
-		cat >&4 &
-
-		# Check expected output
-		if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
-		then
-			kill "$GIT_DAEMON_PID"
-			wait "$GIT_DAEMON_PID"
-			trap 'die' EXIT
-			error "git daemon failed to start"
-		fi
-	} <git_daemon_output
+		>&3 2>&4 ||
+		error "git daemon failed to start"
+	GIT_DAEMON_PID=$(cat git-daemon.pid)
 }
 
 stop_git_daemon() {

@@ -57,13 +43,5 @@ stop_git_daemon() {
 	# kill git-daemon child of git
 	say >&3 "Stopping git daemon ..."
 	kill "$GIT_DAEMON_PID"
-	wait "$GIT_DAEMON_PID" >&3 2>&4
-	ret=$?
-	# expect exit with status 143 = 128+15 for signal TERM=15
-	if test $ret -ne 143
-	then
-		error "git daemon exited with status: $ret"
-	fi
 	GIT_DAEMON_PID=
-	rm -f git_daemon_output
 }

test-git-daemon.c new file +62 -0

62 lines not on this page · raw message

Signature · 1 line

1.7.9.6

Zbigniew Jędrzejewski-Szmek

2016-06-15 22:53 UTC #13 lore raw

Quoted 4 lines · Clemens Buchacher

The shell script which is currently used to parse git daemon output does not seem to work reliably. In order to work around such issues, re-implement the same procedure in C and write the daemon pid to a file.

Quoted 8 lines

This means that we can no longer wait on the daemon process, since it is no longer a direct child of the shell process.
Signed-off-by Clemens Buchacher<[email protected]>
---
Here's the re-roll for completeness. Still waiting on feedback from the OP if this actually solves the problem, though.

I posted a reply in the other thread, but I'm replying here too for completeness: yes, the problem is solved.

Thanks,
Zbyszek

Junio C Hamano

2016-06-15 22:53 UTC #14 lore raw

In reply to Clemens Buchacher

2012/4/15 Clemens Buchacher <[email protected]>

Quoted 7 lines

The shell script which is currently used to parse git daemon output does not seem to work reliably. In order to work around such issues, re-implement the same procedure in C and write the daemon pid to a file.
...
+ strbuf_getwholeline_fd(&line, proc.err, '\n');
+ fprintf(stderr, line.buf);

Just a note. I'll update this part with "fputs(line.buf, stderr)".

Johannes Sixt

2016-06-15 22:53 UTC #15 lore raw

In reply to Clemens Buchacher

Am 15.04.2012 00:06, schrieb Clemens Buchacher:

Quoted 9 lines · Johannes Sixt

Quoted 7 lines
Am 14.04.2012 20:29, schrieb Clemens Buchacher:
test-git-daemon.c line 54
Quoted 4 lines
test-git-daemon.c new file +4 -0
+	memset(&cat, 0, sizeof(cat));
+	cat.argv = (const char **)cat_argv;
+	cat.in = proc.err;
+	cat.out = 2;
Useless use of cat?

Quoted 4 lines

I don't see how I could avoid cat here. I have to create a pipe first so that I can read the first line. And then I have to terminate test-git-daemon in order to start the tests. So I cannot continue reading synchronously.

OK, I got it.

But reading the first line in this way needs a few assumptions to be true:

- git-daemon does not write an incomplete line and then waits.

- git-daemon does not write more than one line, because xread() happily reads everything it can get. Your implementation differs from the old version because the shell's 'read' is required to read no more than one line, i.e., to read byte-wise from the pipe until it sees the LF.

-- Hannes

Clemens Buchacher

author

2016-06-15 22:53 UTC #16 lore raw

Quoted 4 lines · Johannes Sixt

But reading the first line in this way needs a few assumptions to be true:
- git-daemon does not write an incomplete line and then waits.

Yes. One way to avoid that assumption would be a timeout.

Quoted 4 lines

- git-daemon does not write more than one line, because xread() happily reads everything it can get. Your implementation differs from the old version because the shell's 'read' is required to read no more than one line, i.e., to read byte-wise from the pipe until it sees the LF.

The strbuf_getwholeline_fd implementation calls xread(fd, buf, 1), reading only one byte at a time. It does not try to read beyond the newline.

Johannes Sixt

2016-06-15 22:53 UTC #17 lore raw

Am 15.04.2012 21:32, schrieb Clemens Buchacher:

Quoted 3 lines

The strbuf_getwholeline_fd implementation calls xread(fd, buf, 1), reading only one byte at a time. It does not try to read beyond the newline.

Point taken. I only saw "xread" and didn't look further. Sorry for the noise. I now crawl back under my rock.

-- Hannes