[PATCH 0/7] fix transfer.hiderefs with smart http

[PATCH 0/7] fix transfer.hiderefs with smart http

[Jeff King]

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

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

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5551-http-fetch-smart.sh | 11 +++++++++++
 upload-pack.c               | 14 ++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 6cbc12d..b970773 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -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" &&
diff --git a/upload-pack.c b/upload-pack.c
index b531a32..c8e8713 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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.

> 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.

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

Thanks, will queue.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t5551-http-fetch-smart.sh | 11 +++++++++++
>  upload-pack.c               | 14 ++++++++++----
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 6cbc12d..b970773 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -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" &&
> diff --git a/upload-pack.c b/upload-pack.c
> index b531a32..c8e8713 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -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)

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

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 upload-pack.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index c8e8713..aa84576 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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

[PATCH 3/7] t: translate SIGINT to an exit

[Jeff King]

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).

Signed-off-by: Jeff King <peff@peff.net>
---
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(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..f4ba3ff 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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

[PATCH 4/7] t: redirect stderr GIT_TRACE to descriptor 4

[Jeff King]

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).

Signed-off-by: Jeff King <peff@peff.net>
---
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(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f4ba3ff..7dd4b4d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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

[PATCH 5/7] t: pass GIT_TRACE through Apache

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-httpd.sh          | 1 +
 t/lib-httpd/apache.conf | 1 +
 2 files changed, 2 insertions(+)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index d154d1e..e6adf2f 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -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
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 03a4c2e..0b81a00 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -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

[PATCH 6/7] t5541: move run_with_cmdline_limit to test-lib.sh

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
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(-)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index d2c681e..1ecb588 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -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 |
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7dd4b4d..9914d3e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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

Re: [PATCH 6/7] t5541: move run_with_cmdline_limit to test-lib.sh

[Eric Sunshine]

On Fri, Mar 13, 2015 at 12:53 AM, Jeff King <peff@peff.net> wrote:
> 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.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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.

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.

>  t/t5541-http-push-smart.sh | 6 ------
>  t/test-lib.sh              | 6 ++++++
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
> index d2c681e..1ecb588 100755
> --- a/t/t5541-http-push-smart.sh
> +++ b/t/t5541-http-push-smart.sh
> @@ -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 |
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 7dd4b4d..9914d3e 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -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

[PATCH 7/7] t5551: make EXPENSIVE test cheaper

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
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(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index b970773..df47851 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -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

Re: [PATCH 0/7] fix transfer.hiderefs with smart http

[Jeff King]

On Fri, Mar 13, 2015 at 12:41:01AM -0400, Jeff King wrote:

> 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.

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

Re: [PATCH 0/7] fix transfer.hiderefs with smart http

[Duy Nguyen]

On Fri, Mar 13, 2015 at 11:59 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Mar 13, 2015 at 12:41:01AM -0400, Jeff King wrote:
>
>> 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.
>
> 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.

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