fix transfer.hiderefs with smart http

I'm experimenting with using transfer.hiderefs on a server, and it's rather easy to cause a git client to hang when fetching from such a repo over smart http. Details are in the first patch.

There are 7 patches here, but the entirety of the fix is contained in the first one. The rest are cleanups and test enhancements I found along the way. I moved the fix to the front of the series as we probably want it to go to "maint", but the others can wait (being mostly test modifications, they should not cause regressions, but they'd possibly want more cooking time in case I broke the test suite for somebody).

The patches are:

[1/7]: upload-pack: fix transfer.hiderefs over smart-http

The fix.

[2/7]: upload-pack: do not check NULL return of lookup_unknown_object

A nearby cleanup.

[3/7]: t: translate SIGINT to an exit [4/7]: t: redirect stderr GIT_TRACE to descriptor 4 [5/7]: t: pass GIT_TRACE through Apache

These all solve irritations I had when trying to debug the test.

[6/7]: t5541: move run_with_cmdline_limit to test-lib.sh [7/7]: t5551: make EXPENSIVE test cheaper

I had thought at first that the problem was related to large http fetches, but it turned out not to be. But I think these cleanups are a good thing, as they increase the default test coverage.

-Peff

When upload-pack advertises the refs (either for a normal, non-stateless request, or for the initial contact in a stateless one), we call for_each_ref with the send_ref function as its callback. send_ref, in turn, calls mark_our_ref, which checks whether the ref is hidden, and sets OUR_REF or HIDDEN_REF on the object as appropriate. If it is hidden, mark_our_ref also returns "1" to signal send_ref that the ref should not be advertised.

If we are not advertising refs, (i.e., the follow-up invocation by an http client to send its "want" lines), we use mark_our_ref directly as a callback to for_each_ref. Its marking does the right thing, but when it then returns "1" to for_each_ref, the latter interprets this as an error and stops iterating. As a result, we skip marking all of the refs that come lexicographically after it. Any "want" lines from the client asking for those objects will fail, as they were not properly marked with OUR_REF.

To solve this, we introduce a wrapper callback around mark_our_ref which always returns 0 (even if the ref is hidden, we want to keep iterating). We also tweak the signature of mark_our_ref to exclude unnecessary parameters that were present only to conform to the callback interface. This should make it less likely for somebody to accidentally use it as a callback in the future.

---
 t/t5551-http-fetch-smart.sh | 11 +++++++++++
 upload-pack.c               | 14 ++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

index 6cbc12d..b970773 100755
@@ -213,6 +213,17 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
 	test_cmp expect_cookies.txt cookies_tail.txt
 '
 
+test_expect_success 'transfer.hiderefs works over smart-http' '
+	test_commit hidden &&
+	test_commit visible &&
+	git push public HEAD^:refs/heads/a HEAD:refs/heads/b &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		config transfer.hiderefs refs/heads/a &&
+	git clone --bare "$HTTPD_URL/smart/repo.git" hidden.git &&
+	test_must_fail git -C hidden.git rev-parse --verify a &&
+	git -C hidden.git rev-parse --verify b
+'
+
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
 	(
 	cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&

index b531a32..c8e8713 100644
@@ -681,7 +681,7 @@ static void receive_needs(void)
 }
 
 /* return non-zero if the ref is hidden, otherwise 0 */
-static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
+static int mark_our_ref(const char *refname, const unsigned char *sha1)
 {
 	struct object *o = lookup_unknown_object(sha1);
 

@@ -695,6 +695,12 @@ static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag
 	return 0;
 }
 
+static int check_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
+{
+	mark_our_ref(refname, sha1);
+	return 0;
+}
+
 static void format_symref_info(struct strbuf *buf, struct string_list *symref)
 {
 	struct string_list_item *item;

@@ -713,7 +719,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	const char *refname_nons = strip_namespace(refname);
 	unsigned char peeled[20];
 
-	if (mark_our_ref(refname, sha1, flag, NULL))
+	if (mark_our_ref(refname, sha1))
 		return 0;
 
 	if (capabilities) {

@@ -767,8 +773,8 @@ static void upload_pack(void)
 		advertise_shallow_grafts(1);
 		packet_flush(1);
 	} else {
-		head_ref_namespaced(mark_our_ref, NULL);
-		for_each_namespaced_ref(mark_our_ref, NULL);
+		head_ref_namespaced(check_ref, NULL);
+		for_each_namespaced_ref(check_ref, NULL);
 	}
 	string_list_clear(&symref, 1);
 	if (advertise_refs)

2.3.2.472.geadab3c

Nicely described in a way that the reason of the breakage and the fix is obvious to those who already know what the codepath is supposed to be doing.

I especially love this kind of future-proofing ;-).

Thanks, will queue.

We check whether the return value of lookup_unknown_object is NULL, but some code paths dereference it before our check. This turns out not to be capable of causing a segfault, though. The lookup_unknown_object function will never return NULL, since the whole point is to allocate an object struct if it does not find an existing one. So the code here is not wrong, it is just confusing. Let's just drop the NULL check.

---
 upload-pack.c | 2 --
 1 file changed, 2 deletions(-)

index c8e8713..aa84576 100644
@@ -689,8 +689,6 @@ static int mark_our_ref(const char *refname, const unsigned char *sha1)
 		o->flags |= HIDDEN_REF;
 		return 1;
 	}
-	if (!o)
-		die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1));
 	o->flags |= OUR_REF;
 	return 0;
 }

2.3.2.472.geadab3c

Right now if a test script receives SIGINT (e.g., because a test was hanging and the user hit ^C), the shell exits immediately. This can be annoying if the test script did any global setup, like starting apache or git-daemon, as it will not have an opportunity to clean up after itself. A subsequent run of the test won't be able to start its own daemon, and will either fail or skip the tests.

Instead, let's trap SIGINT to make sure we do a clean shutdown, and just chain it to a normal exit (which will trigger any cleanup).

---
Possibly we should catch other signals here, too, but SIGINT is the one that has been biting me over the years (I would sometimes have a stray apache process hanging around, and I only recently figured out what caused it).

I think "trap ... INT" is the most portable way to say this. I would have thought numbers were more portable, but they are actually an XSI add-on. Likewise, "SIGINT" is allowed in POSIX but not required. "INT" is part of POSIX.

This is the cleanest way I could come up with to implement this. We could also just include "INT" on the same line as the die, but:

1. Then every script which sets a handler (like lib-httpd.sh) would need to be modified to munge the INT trap, too.

2. It double-calls die, then. We die from the INT handler, which triggers the EXIT handler. I guess we could clear the handler inside die() if we needed to.

So I'd rather go this route, unless there turns out to be a weird portability problem.

 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

index c096778..f4ba3ff 100644
@@ -299,6 +299,7 @@ die () {
 
 GIT_EXIT_OK=
 trap 'die' EXIT
+trap 'exit $?' INT
 
 # The user-facing functions are loaded from a separate file so that
 # test_perf subshells can have them too

2.3.2.472.geadab3c

If you run a test script like:

GIT_TRACE=1 ./t0061-run-command.sh

you may get test failures, because some tests capture and check the stderr output from git commands (and with GIT_TRACE set to 1, the trace output will be included there).

When we see GIT_TRACE set like this, we print a warning to the user. However, we can do even better than that by just pointing it to descriptor 4, which all tests leave connected to the test script's stderr. That's likely what the user intended (and any scripts that do want to see GIT_TRACE output will set GIT_TRACE themselves).

Not only does this avoid false negatives in the tests, but it means the user will actually see trace output for git calls that redirect their stderr (whereas before, it was sometimes confusingly buried in a file).

---
I only today figured out the GIT_TRACE=4 trick. Perhaps everybody else did long ago, and I am just slow. :)

My first inclination was to mention it in the warning message below, but I cannot see a downside to automatically redirecting it. Even outside of test_expect_* we have the "4>&2" redirection, so we should hit even badly written scripts which call git outside of that environment.

 t/test-lib.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index f4ba3ff..7dd4b4d 100644
@@ -152,10 +152,7 @@ unset UNZIP
 
 case $(echo $GIT_TRACE |tr "[A-Z]" "[a-z]") in
 1|2|true)
-	echo "* warning: Some tests will not work if GIT_TRACE" \
-		"is set as to trace on STDERR ! *"
-	echo "* warning: Please set GIT_TRACE to something" \
-		"other than 1, 2 or true ! *"
+	GIT_TRACE=4
 	;;
 esac
 

2.3.2.472.geadab3c

Apache removes GIT_TRACE from the environment before running git-http-backend. This can make it hard to debug the server side of an http session. Let's let it through.

---
 t/lib-httpd.sh          | 1 +
 t/lib-httpd/apache.conf | 1 +
 2 files changed, 2 insertions(+)

index d154d1e..e6adf2f 100644
@@ -79,6 +79,7 @@ HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www
 # hack to suppress apache PassEnv warnings
 GIT_VALGRIND=$GIT_VALGRIND; export GIT_VALGRIND
 GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export GIT_VALGRIND_OPTIONS
+GIT_TRACE=$GIT_TRACE; export GIT_TRACE
 
 if ! test -x "$LIB_HTTPD_PATH"
 then

index 03a4c2e..0b81a00 100644
@@ -70,6 +70,7 @@ PassEnv GIT_VALGRIND
 PassEnv GIT_VALGRIND_OPTIONS
 PassEnv GNUPGHOME
 PassEnv ASAN_OPTIONS
+PassEnv GIT_TRACE
 
 Alias /dumb/ www/
 Alias /auth/dumb/ www/auth/dumb/

2.3.2.472.geadab3c

We use this to test http pushing with a restricted commandline. Other scripts (like t5551, which does http fetching) will want to use it, too.

---
As we discussed a while ago, this is the exact same code that run_with_limited_stack uses in t7004. However, I think they are conceptually two different things, and I could imagine a platform where they have distinct implementations. So I did not refactor t7004 to make use of this.

 t/t5541-http-push-smart.sh | 6 ------
 t/test-lib.sh              | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

index d2c681e..1ecb588 100755
@@ -324,12 +324,6 @@ test_expect_success 'push into half-auth-complete requires password' '
 	test_cmp expect actual
 '
 
-run_with_limited_cmdline () {
-	(ulimit -s 128 && "$@")
-}
-
-test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
-
 test_expect_success CMDLINE_LIMIT 'push 2000 tags over http' '
 	sha1=$(git rev-parse HEAD) &&
 	test_seq 2000 |

index 7dd4b4d..9914d3e 100644
@@ -1062,3 +1062,9 @@ test_lazy_prereq UNZIP '
 	"$GIT_UNZIP" -v
 	test $? -ne 127
 '
+
+run_with_limited_cmdline () {
+	(ulimit -s 128 && "$@")
+}
+
+test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'

2.3.2.472.geadab3c

Perhaps this commentary should be moved to the commit message so that the next person who notices that run_with_limited_stack() is the same will understand why it was left alone.

We create 50,000 tags to check that we don't overflow the command-line of fetch-pack. But by using run_with_cmdline_limit, we can get the same effect with a much smaller number of tags. This makes the test fast enough that we can drop the EXPENSIVE prereq, which means people will actually run it.

It was not documented to do so, but this test was also the only test of a clone-over-http that requires multiple POSTs during the conversation. We can continue to test that by dropping http.postbuffer to its minimum size, and checking that we get two POSTs.

---
This is still a slightly expensive test, but it's on par with what the push test does, so hopefully it's OK. And it bothers me that we didn't exercise the multiple-POST code at all in the test suite, as I assume basically nobody runs the tests with "--long".

I don't think we can shrink it any more. We enforce a hard minimum of LARGE_PACKET_MAX on http.postbuffer, and that assumption runs through the code. Lots of things break if you try to circumvent it, and I didn't bother digging further.

 t/t5551-http-fetch-smart.sh | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

index b970773..df47851 100755
@@ -224,10 +224,10 @@ test_expect_success 'transfer.hiderefs works over smart-http' '
 	git -C hidden.git rev-parse --verify b
 '
 
-test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
+test_expect_success 'create 2,000 tags in the repo' '
 	(
 	cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
-	for i in `test_seq 50000`
+	for i in $(test_seq 2000)
 	do
 		echo "commit refs/heads/too-many-refs"
 		echo "mark :$i"

@@ -248,13 +248,22 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
 	)
 '
 
-test_expect_success EXPENSIVE 'clone the 50,000 tag repo to check OS command line overflow' '
-	git clone $HTTPD_URL/smart/repo.git too-many-refs &&
+test_expect_success CMDLINE_LIMIT \
+	'clone the 2,000 tag repo to check OS command line overflow' '
+	run_with_limited_cmdline git clone $HTTPD_URL/smart/repo.git too-many-refs &&
 	(
 		cd too-many-refs &&
-		test $(git for-each-ref refs/tags | wc -l) = 50000
+		git for-each-ref refs/tags >actual &&
+		test_line_count = 2000 actual
 	)
 '
 
+test_expect_success 'large fetch-pack requests can be split across POSTs' '
+	GIT_CURL_VERBOSE=1 git -c http.postbuffer=65536 \
+		clone --bare "$HTTPD_URL/smart/repo.git" split.git 2>err &&
+	grep "^> POST" err >posts &&
+	test_line_count = 2 posts
+'
+
 stop_httpd
 test_done

2.3.2.472.geadab3c

A note on this hang. What happens is that upload-pack sees a bogus "want" line and calls die(). The client then sits there forever, but I'm not exactly sure what it's waiting for.

This series fixes the bug that caused us to erroneously call die() in upload-pack, so the hang is "fixed". But there are other reasons to call die(); it would probably be a good thing if the client side noticed the problem and aborted.

-Peff

Maybe we could install a custom die handler that also sends "ERR" line to the client before dying. Even with old clients where ERR lines are not recognized, they would see that as a sign of error and abort. The only thing to be careful is not sending ERR while we're in the middle of sending some pkt-line, and that only happens when die() is called inside packet_write() and we can catch that easily. This is for upload-pack only as the client side can also use packet_buf_write(), a bit harder to know if some pkt-line is being sent.

Duy