patch, 6 partsFix the shallow deepen bug with no-done

> Originally I had t5537 use port 5536 and 5538 use port 5537(!). Then
> Jeff found my fault so I changed port in t5537 from 5536 to 5537 in
> 3b32a7c (t5537: fix incorrect expectation in test case 10 -
> 2014-01-08). Which makes it worse because now both t5537 and t5538
> both use the same port. Fix it.

> On Thu, Feb 06, 2014 at 10:10:35PM +0700, Nguyễn Thái Ngọc Duy wrote:
> 
> > Originally I had t5537 use port 5536 and 5538 use port 5537(!). Then
> > Jeff found my fault so I changed port in t5537 from 5536 to 5537 in
> > 3b32a7c (t5537: fix incorrect expectation in test case 10 -
> > 2014-01-08). Which makes it worse because now both t5537 and t5538
> > both use the same port. Fix it.
> [...]

> Thinking on this more, I wonder if we should just do something like
> this:
>
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index bfdff2a..c82b4ee 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -64,7 +64,9 @@ case $(uname) in
>  esac
>
>  LIB_HTTPD_PATH=${LIB_HTTPD_PATH-"$DEFAULT_HTTPD_PATH"}
> -LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}
> +if test -z "$LIB_HTTPD_PORT"; then
> +       LIB_HTTPD_PORT=${this_test#t}
> +fi
>
>  TEST_PATH="$TEST_DIRECTORY"/lib-httpd
>  HTTPD_ROOT_PATH="$PWD"/httpd
>
> and drop the manual LIB_HTTPD_PORT setting in each of the test scripts.
> That would prevent this type of error in the future, and people can
> still override if they want to.
>
> Any test scripts that are not numbered would need to set it, but we
> should not have any of those (and if we did, the failure mode would be
> OK, as Apache would barf on the bogus port number).

>   1. Is there anybody who has apache installed who would _not_ want to
>      bring it up for the test?
>
>   2. Is there anybody for whom the failure mode of bringing up apache
>      would be unpleasant (e.g., if it hangs the tests or something)?
>
> For (1), we could perhaps have a GIT_NO_TEST_HTTPD to avoid it.
>
> For (2), I suspect we may need to make our error handling more robust,
> but getting people to run it is the first step to figuring out what the
> problems are.
>
> If we go this route, we should probably do the same for
> GIT_TEST_GIT_DAEMON in t5570 (and for that matter, we should probably do
> the same for the port numbers).

> Jeff King <peff@peff.net> writes:
> 
> > Here it is as a Real Patch™. I just based it on master, so it can
> > replace your 5537/5538 fix in your series.
> 
> Thanks, looks good.  Will put this at the bottom and rebuild the
> nd/http-fetch-shallow-fix series on top.

> I dug in the history to see if there was any reasoning given for the
> current "off by default" setting. It looks like Junio asked for it when
> the original http-push tests were added, and everything else just
> followed that. The reasoning there was basically "they're heavyweight
> and we might not be able to run them", but hopefully having the option
> variable will make that OK.

>  t/lib-git-daemon.sh     |  8 +++++---
>  t/lib-httpd.sh          | 22 +++++++++++-----------
>  t/test-lib-functions.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 60 insertions(+), 14 deletions(-)
>
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index 1f22de2..e623bef 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -16,9 +16,10 @@
>  #	stop_git_daemon
>  #	test_done
>  
> -if test -z "$GIT_TEST_GIT_DAEMON"
> +GIT_TEST_GIT_DAEMON=$(test_tristate "$GIT_TEST_GIT_DAEMON")
> +if test "$GIT_TEST_GIT_DAEMON" = false
>  then
> -	skip_all="git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)"
> +	skip_all="git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)"
>  	test_done
>  fi
>  
> @@ -58,7 +59,8 @@ start_git_daemon() {
>  		kill "$GIT_DAEMON_PID"
>  		wait "$GIT_DAEMON_PID"
>  		trap 'die' EXIT
> -		error "git daemon failed to start"
> +		test_skip_or_die $GIT_TEST_GIT_DAEMON \
> +			"git daemon failed to start"
>  	fi
>  }
>  
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index b43162e..bb900ca 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -30,9 +30,10 @@
>  # Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
>  #
>  
> -if test -z "$GIT_TEST_HTTPD"
> +GIT_TEST_HTTPD=$(test_tristate "$GIT_TEST_HTTPD")
> +if test "$GIT_TEST_HTTPD" = false
>  then
> -	skip_all="Network testing disabled (define GIT_TEST_HTTPD to enable)"
> +	skip_all="Network testing disabled (unset GIT_TEST_HTTPD to enable)"
>  	test_done
>  fi
>  
> @@ -76,8 +77,7 @@ GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export GIT_VALGRIND_OPTIONS
>  
>  if ! test -x "$LIB_HTTPD_PATH"
>  then
> -	skip_all="skipping test, no web server found at '$LIB_HTTPD_PATH'"
> -	test_done
> +	test_skip_or_die $GIT_TEST_HTTPD "no web server found at '$LIB_HTTPD_PATH'"
>  fi
>  
>  HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \
> @@ -89,19 +89,20 @@ then
>  	then
>  		if ! test $HTTPD_VERSION -ge 2
>  		then
> -			skip_all="skipping test, at least Apache version 2 is required"
> -			test_done
> +			test_skip_or_die $GIT_TEST_HTTPD \
> +				"at least Apache version 2 is required"
>  		fi
>  		if ! test -d "$DEFAULT_HTTPD_MODULE_PATH"
>  		then
> -			skip_all="Apache module directory not found.  Skipping tests."
> -			test_done
> +			test_skip_or_die $GIT_TEST_HTTPD \
> +				"Apache module directory not found"
>  		fi
>  
>  		LIB_HTTPD_MODULE_PATH="$DEFAULT_HTTPD_MODULE_PATH"
>  	fi
>  else
> -	error "Could not identify web server at '$LIB_HTTPD_PATH'"
> +	test_skip_or_die $GIT_TEST_HTTPD \
> +		"Could not identify web server at '$LIB_HTTPD_PATH'"
>  fi
>  
>  prepare_httpd() {
> @@ -155,9 +156,8 @@ start_httpd() {
>  		>&3 2>&4
>  	if test $? -ne 0
>  	then
> -		skip_all="skipping test, web server setup failed"
>  		trap 'die' EXIT
> -		test_done
> +		test_skip_or_die $GIT_TEST_HTTPD "web server setup failed"
>  	fi
>  }
>  
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index aeae3ca..3cc09c0 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -716,6 +716,50 @@ perl () {
>  	command "$PERL_PATH" "$@"
>  }
>  
> +# Normalize the value given in $1 to one of "true", "false", or "auto". The
> +# result is written to stdout. E.g.:
> +#
> +#     GIT_TEST_HTTPD=$(test_tristate "$GIT_TEST_HTTPD")
> +#
> +test_tristate () {
> +	case "$1" in
> +	""|auto)
> +		echo auto
> +		;;
> +	*)
> +		# Rely on git-config to handle all the variants of
> +		# true/1/on/etc that we allow.
> +		if ! git -c magic.hack="$1" config --bool magic.hack 2>/dev/null
> +		then
> +			# If git-config failed, it was some non-bool value like
> +			# YesPlease. Default this to "true" for historical
> +			# compatibility.
> +			echo true
> +		fi
> +	esac
> +}
> +
> +# Exit the test suite, either by skipping all remaining tests or by
> +# exiting with an error. If "$1" is "auto", we then we assume we were
> +# opportunistically trying to set up some tests and we skip. If it is
> +# "true", then we report a failure.
> +#
> +# The error/skip message should be given by $2.
> +#
> +test_skip_or_die () {
> +	case "$1" in
> +	auto)
> +		skip_all=$2
> +		test_done
> +		;;
> +	true)
> +		error "$2"
> +		;;
> +	*)
> +		error "BUG: test tristate is '$1' (real error: $2)"
> +	esac
> +}
> +
>  # The following mingw_* functions obey POSIX shell syntax, but are actually
>  # bash scripts, and are meant to be used only with bash on Windows.

> On Tue, Feb 11, 2014 at 11:51:18AM -0800, Junio C Hamano wrote:
>
>> Those who run buildfarms may want to disable the networking test if
>> the buildfarms are not isolated well, for example.  They have to be
>> told somewhere that now they need to explicitly disable these tests
>> and how.
>
> I think they should be OK. The daemons run on the loopback interface, so
> there is hopefully not a security implication. If multiple buildfarms
> are sharing the same loopback space (e.g., running in separate
> directories on the same machine), the "auto" setting should degrade
> gracefully. One daemon will "win" the setup, and the tests will run, and
> on the other, they will be skipped.
>
>> I am in favor of this change but just pointing out possible fallouts
>> might be larger than we think.
>
> Agreed, but I think the only way to know the size of those fallouts is
> to try it and see who complains.  I would not normally be so cavalier
> with git itself, but I think for the test infrastructure, we have a
> small, tech-savvy audience that can help us iterate on it without too
> much pain.

> Sure. One immediate complaint is that I would probably need to do
> something like this in the build automation:
> 
> 	if testing a branch without this patch
>         then
> 		: do nothing
> 	else
> 		GIT_TEST_GIT_DAEMON=false
> 	fi
> 
> Arguably, it is the fault of the current/original code that treated
> *any* non-empty value that is set in the environment variable as
> "true"---if it paid attention to GIT_TEST_GIT_DAEMON=NoThanks, we
> wouldn't have to have a workaround like this.

> I wonder if GIT_TEST_X=$(test_tristate "$GIT_TEST_X") pattern can be
> made a bit more friendly, though.  For example, can we behave
> differently depending on the reason why $GIT_TEST_X is empty?
> 
>  - People who have *not* been opting in to the expensive tests have
>    not done anything special; GIT_TEST_X environment variable did
>    not exist for them (i.e. unset), and we used to skip when
>    "$GIT_TEST_X" is an empty string.
> 
>  - We want to encourage people who do not care to run these tests.
>    If people do not do anything, their $GIT_TEST_X will continue to
>    be an empty string without GIT_TEST_X variable in the
>    environment.
> 
> If we let people who *do* want to opt out of the expensive tests by
> explicitly setting $GIT_TEST_X to an empty string in the new scheme,
> wouldn't the transition go a lot smoother?

> and then the callee may become
> 
> 	test_tristate () {
> 		variable=$1
>                 if eval "test x\"\${$variable+isset}\" = xisset"

> Jeff King <peff@peff.net> writes:
>
>>   test_normalize_tristate GIT_TEST_DAEMON
>
> Heh, great minds think alike.  This is what I am playing with,
> without committing (because I do like your "ask config if this is a
> kind of various boolean 'false' representations, which I haven't
> managed to add to it).

> >>   test_normalize_tristate GIT_TEST_DAEMON
> >
> > Heh, great minds think alike.  This is what I am playing with,
> > without committing (because I do like your "ask config if this is a
> > kind of various boolean 'false' representations, which I haven't
> > managed to add to it).
> 
> And this is with the "ask config" helper.

> Two tangents.
> 
>  - We may want to do something similar in cvsserver and git-gui to
>    make them more robust.
> 
>    $ git grep -e true --and -e 1 --and -e yes

> -- >8 --
> From: Jeff King <peff@peff.net>
> Date: Mon, 10 Feb 2014 16:29:37 -0500
> Subject: [PATCH] tests: turn on network daemon tests by default
> [...]
> In addition, we are forgiving of common setup failures (e.g., you do
> not have apache installed, or have an old version) when the
> tri-state is "auto" (or empty), but report an error when it is

> ---
>  t/lib-git-daemon.sh     |  8 ++++---
>  t/lib-httpd.sh          | 22 +++++++++----------
>  t/test-lib-functions.sh | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 74 insertions(+), 14 deletions(-)

>>  - We may want to do something similar in cvsserver and git-gui to
>>    make them more robust.
>> 
>>    $ git grep -e true --and -e 1 --and -e yes
>
> I assume the "something" here is to respect bool options more
> consistently?

>>  - Do we want to do something similar to GIT_TEST_CREDENTIAL_HELPER?
>
> No, it is not a boolean. It is a bit of a hack, but it is meant to be
> used like:
>
>   GIT_TEST_CREDENTIAL_HELPER=foo ./t0303-*
>
> to test some random git-credential-foo you have in your PATH. There is
> nothing to run "by default" there.

> Agreed, but I think the only way to know the size of those fallouts is
> to try it and see who complains.  I would not normally be so cavalier
> with git itself, but I think for the test infrastructure, we have a
> small, tech-savvy audience that can help us iterate on it without too
> much pain.

> Jeff King <peff@peff.net> writes:
> 
> > Agreed, but I think the only way to know the size of those fallouts is
> > to try it and see who complains.  I would not normally be so cavalier
> > with git itself, but I think for the test infrastructure, we have a
> > small, tech-savvy audience that can help us iterate on it without too
> > much pain.
> 
> There is another.

> $ GIT_TEST_HTTPD=false sh t5537-fetch-shallow.sh 
> ok 1 - setup
> ok 2 - setup shallow clone
> ok 3 - clone from shallow clone
> ok 4 - fetch from shallow clone
> ok 5 - fetch --depth from shallow clone
> ok 6 - fetch --unshallow from shallow clone
> ok 7 - fetch something upstream has but hidden by clients shallow boundaries
> ok 8 - fetch that requires changes in .git/shallow is filtered
> ok 9 - fetch --update-shallow
> error: Can't use skip_all after running some tests

> lib-httpd was never designed to be included from anywhere except the
> beginning of the file. But that wouldn't be right for t5537, because it
> wants to run some of the tests, even if apache setup fails. The right
> way to do it is probably to have lib-httpd do all of its work in a lazy
> prereq. I don't know how clunky that will end up, though; it might be
> simpler to just move the shallow http test into one of the http-fetch
> scripts.

> start_httpd is supposed to be at the beginning of the test file, not
> the middle of it. The "test_seq" line in "no shallow lines.." test is
> updated to compensate missing refs that are there in t5537, but not in
> the new t5539.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  On Thu, Feb 13, 2014 at 8:22 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>  > On Thu, Feb 13, 2014 at 5:12 AM, Jeff King <peff@peff.net> wrote:
>  >> lib-httpd was never designed to be included from anywhere except the
>  >> beginning of the file. But that wouldn't be right for t5537, because it
>  >> wants to run some of the tests, even if apache setup fails. The right
>  >> way to do it is probably to have lib-httpd do all of its work in a lazy
>  >> prereq. I don't know how clunky that will end up, though; it might be
>  >> simpler to just move the shallow http test into one of the http-fetch
>  >> scripts.
>  >
>  > I'll move it out later.
>  
>  Here it is, on top of nd/http-fetch-shallow-fix because the new test
>  in t5537 is picky and a simple merge resolution wouldn't do it.

>
>  t/t5537-fetch-shallow.sh               | 57 -----------------------
>  t/t5539-fetch-http-shallow.sh (new +x) | 82 ++++++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+), 57 deletions(-)
>
> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index 098f220..3ae9092 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -173,61 +173,4 @@ EOF
>  	)
>  '
>  
> -if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
> -	say 'skipping remaining tests, git built without http support'
> -	test_done
> -fi
> -
> -. "$TEST_DIRECTORY"/lib-httpd.sh
> -start_httpd
> -
> -test_expect_success 'clone http repository' '
> -	git clone --bare --no-local shallow "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> -	git clone $HTTPD_URL/smart/repo.git clone &&
> -	(
> -	cd clone &&
> -	git fsck &&
> -	git log --format=%s origin/master >actual &&
> -	cat <<EOF >expect &&
> -7
> -6
> -5
> -4
> -3
> -EOF
> -	test_cmp expect actual
> -	)
> -'
> -
> -# This test is tricky. We need large enough "have"s that fetch-pack
> -# will put pkt-flush in between. Then we need a "have" the server
> -# does not have, it'll send "ACK %s ready"
> -test_expect_success 'no shallow lines after receiving ACK ready' '
> -	(
> -		cd shallow &&
> -		for i in $(test_seq 10)
> -		do
> -			git checkout --orphan unrelated$i &&
> -			test_commit unrelated$i &&
> -			git push -q "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
> -				refs/heads/unrelated$i:refs/heads/unrelated$i &&
> -			git push -q ../clone/.git \
> -				refs/heads/unrelated$i:refs/heads/unrelated$i ||
> -			exit 1
> -		done &&
> -		git checkout master &&
> -		test_commit new &&
> -		git push  "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" master
> -	) &&
> -	(
> -		cd clone &&
> -		git checkout --orphan newnew &&
> -		test_commit new-too &&
> -		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch --depth=2 &&
> -		grep "fetch-pack< ACK .* ready" ../trace &&
> -		! grep "fetch-pack> done" ../trace
> -	)
> -'
> -
> -stop_httpd
>  test_done
> diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
> new file mode 100755
> index 0000000..94553e1
> --- /dev/null
> +++ b/t/t5539-fetch-http-shallow.sh
> @@ -0,0 +1,82 @@
> +#!/bin/sh
> +
> +test_description='fetch/clone from a shallow clone over http'
> +
> +. ./test-lib.sh
> +
> +if test -n "$NO_CURL"; then
> +	skip_all='skipping test, git built without http support'
> +	test_done
> +fi
> +
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +start_httpd
> +
> +commit() {
> +	echo "$1" >tracked &&
> +	git add tracked &&
> +	git commit -m "$1"
> +}
> +
> +test_expect_success 'setup shallow clone' '
> +	commit 1 &&
> +	commit 2 &&
> +	commit 3 &&
> +	commit 4 &&
> +	commit 5 &&
> +	commit 6 &&
> +	commit 7 &&
> +	git clone --no-local --depth=5 .git shallow &&
> +	git config --global transfer.fsckObjects true
> +'
> +
> +test_expect_success 'clone http repository' '
> +	git clone --bare --no-local shallow "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +	git clone $HTTPD_URL/smart/repo.git clone &&
> +	(
> +	cd clone &&
> +	git fsck &&
> +	git log --format=%s origin/master >actual &&
> +	cat <<EOF >expect &&
> +7
> +6
> +5
> +4
> +3
> +EOF
> +	test_cmp expect actual
> +	)
> +'
> +
> +# This test is tricky. We need large enough "have"s that fetch-pack
> +# will put pkt-flush in between. Then we need a "have" the server
> +# does not have, it'll send "ACK %s ready"
> +test_expect_success 'no shallow lines after receiving ACK ready' '
> +	(
> +		cd shallow &&
> +		for i in $(test_seq 15)
> +		do
> +			git checkout --orphan unrelated$i &&
> +			test_commit unrelated$i &&
> +			git push -q "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
> +				refs/heads/unrelated$i:refs/heads/unrelated$i &&
> +			git push -q ../clone/.git \
> +				refs/heads/unrelated$i:refs/heads/unrelated$i ||
> +			exit 1
> +		done &&
> +		git checkout master &&
> +		test_commit new &&
> +		git push  "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" master
> +	) &&
> +	(
> +		cd clone &&
> +		git checkout --orphan newnew &&
> +		test_commit new-too &&
> +		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch --depth=2 &&
> +		grep "fetch-pack< ACK .* ready" ../trace &&
> +		! grep "fetch-pack> done" ../trace
> +	)
> +'
> +
> +stop_httpd
> +test_done

> It's introduced in 1bd8c8f (git-upload-pack: Support the multi_ack
> protocol - 2005-10-28) but probably better documented in the commit
> message of 78affc4 (Add multi_ack_detailed capability to
> fetch-pack/upload-pack - 2009-10-30)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/technical/pack-protocol.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
> index c73b62f..eb0b8a1 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -338,7 +338,8 @@ during a prior round.  This helps to ensure that at least one common
>  ancestor is found before we give up entirely.
>  
>  Once the 'done' line is read from the client, the server will either
> -send a final 'ACK obj-id' or it will send a 'NAK'. The server only sends
> +send a final 'ACK obj-id' or it will send a 'NAK'. 'obj-id' is the
> +last SHA-1 determined to be common. The server only sends

> See 3e63b21 (upload-pack: Implement no-done capability - 2011-03-14)
> and 761ecf0 (fetch-pack: Implement no-done capability - 2011-03-14)
> for more information.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/technical/protocol-capabilities.txt | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
> index cb40ebb..cb2f5eb 100644
> --- a/Documentation/technical/protocol-capabilities.txt
> +++ b/Documentation/technical/protocol-capabilities.txt
> @@ -75,6 +75,18 @@ This is an extension of multi_ack that permits client to better
>  understand the server's in-memory state. See pack-protocol.txt,
>  section "Packfile Negotiation" for more information.
>  
> +no-done
> +-------
> +This capability should be only used with smart HTTP protocol. If
> +multi_ack_detailed and no-done are both present, then the sender is
> +free to immediately send a pack following its first "ACK obj-id ready"
> +message.
> +
> +Without no-done in smart HTTP protocol, the server session would end
> +and the client has to make another trip to send "done" and the server
> +can send the pack. no-done removes the last round and thus slightly
> +reduces latency.
> +
>  thin-pack
>  ---------

> In smart http, upload-pack adds new shallow lines at the beginning of
> each rpc response. Only shallow lines from the first rpc call are
> useful. After that they are thrown away. It's designed this way
> because upload-pack is stateless and has no idea when its shallow
> lines are helpful or not.
>
> So after refs are negotiated with multi_ack_detailed and both sides
> happy. The server sends "ACK obj-id ready", terminates the rpc call

> and waits for the final rpc round. The client sends "done". The server
> sends another response, which also has shallow lines at the beginning,
> and the last "ACK obj-id" line.
>
> When no-done is active, the last round is cut out, the server sends
> "ACK obj-id ready" and "ACK obj-id" in the same rpc
> response. fetch-pack is updated to recognize this and not send
> "done". However it still tries to consume shallow lines, which are
> never sent.
>
> Update the code, make sure to skip consuming shallow lines when
> no-done is enabled.
>
> Reported-by: Jeff King <peff@peff.net>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

>  fetch-pack.c             |  3 ++-
>  t/t5537-fetch-shallow.sh | 24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 90fdd49..f061f1f 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -439,7 +439,8 @@ done:
>  	}
>  	strbuf_release(&req_buf);
>  
> -	consume_shallow_list(args, fd[0]);
> +	if (!got_ready || !no_done)
> +		consume_shallow_list(args, fd[0]);
>  	while (flushes || multi_ack) {
>  		int ack = get_ack(fd[0], result_sha1);
>  		if (ack) {
> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index b0fa738..fb11073 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -200,5 +200,29 @@ EOF
>  	)
>  '
>  
> +# This test is tricky. We need large enough "have"s that fetch-pack
> +# will put pkt-flush in between. Then we need a "have" the the server
> +# does not have, it'll send "ACK %s ready"
> +test_expect_success 'add more commits' '
> +	(
> +	cd shallow &&
> +	for i in $(seq 10); do
> +	git checkout --orphan unrelated$i &&
> +	test_commit unrelated$i >/dev/null &&
> +	git push -q "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" refs/heads/unrelated$i:refs/heads/unrelated$i
> +	git push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i
> +	done &&
> +	git checkout master &&
> +	test_commit new &&
> +	git push  "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" master
> +	) &&
> +	(
> +	cd clone &&
> +	git checkout --orphan newnew &&
> +	test_commit new-too &&
> +	git fetch --depth=2
> +	)
> +'
> +
>  stop_httpd
>  test_done

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> In smart http, upload-pack adds new shallow lines at the beginning of
>> each rpc response. Only shallow lines from the first rpc call are
>> useful. After that they are thrown away. It's designed this way
>> because upload-pack is stateless and has no idea when its shallow
>> lines are helpful or not.
>>
>> So after refs are negotiated with multi_ack_detailed and both sides
>> happy. The server sends "ACK obj-id ready", terminates the rpc call
>
> Is the above "... and both sides are happy, the server sends ..."?

> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index b0fa738..fb11073 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -200,5 +200,29 @@ EOF
>         )
>  '
>
> +# This test is tricky. We need large enough "have"s that fetch-pack
> +# will put pkt-flush in between. Then we need a "have" the the server

> +# does not have, it'll send "ACK %s ready"
> +test_expect_success 'add more commits' '
> +       (
> +       cd shallow &&
> +       for i in $(seq 10); do

> +       git checkout --orphan unrelated$i &&
> +       test_commit unrelated$i >/dev/null &&
> +       git push -q "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" refs/heads/unrelated$i:refs/heads/unrelated$i
> +       git push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i
> +       done &&
> +       git checkout master &&
> +       test_commit new &&
> +       git push  "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" master
> +       ) &&
> +       (
> +       cd clone &&
> +       git checkout --orphan newnew &&
> +       test_commit new-too &&
> +       git fetch --depth=2
> +       )
> +'
> +
>  stop_httpd
>  test_done
> --
> 1.8.5.2.240.g8478abd

> In smart http, upload-pack adds new shallow lines at the beginning of
> each rpc response. Only shallow lines from the first rpc call are
> useful. After that they are thrown away. It's designed this way
> because upload-pack is stateless and has no idea when its shallow
> lines are helpful or not.
> 
> So after refs are negotiated with multi_ack_detailed and both sides
> happy. The server sends "ACK obj-id ready", terminates the rpc call
> and waits for the final rpc round. The client sends "done". The server
> sends another response, which also has shallow lines at the beginning,
> and the last "ACK obj-id" line.
> 
> When no-done is active, the last round is cut out, the server sends
> "ACK obj-id ready" and "ACK obj-id" in the same rpc
> response. fetch-pack is updated to recognize this and not send
> "done". However it still tries to consume shallow lines, which are
> never sent.
> 
> Update the code, make sure to skip consuming shallow lines when
> no-done is enabled.

> +# does not have, it'll send "ACK %s ready"
> +test_expect_success 'add more commits' '
> +	(
> +	cd shallow &&
> +	for i in $(seq 10); do

> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index b0fa738..fb11073 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -200,5 +200,29 @@ EOF
>  	)
>  '
>  
> +# This test is tricky. We need large enough "have"s that fetch-pack
> +# will put pkt-flush in between. Then we need a "have" the the server
> +# does not have, it'll send "ACK %s ready"
> +test_expect_success 'add more commits' '
> +	(
> +	cd shallow &&
> +	for i in $(seq 10); do
> +	git checkout --orphan unrelated$i &&
> +	test_commit unrelated$i >/dev/null &&
> +	git push -q "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" refs/heads/unrelated$i:refs/heads/unrelated$i
> +	git push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i

> On Fri, Feb 07, 2014 at 10:01:08AM -0800, Junio C Hamano wrote:
>> Here is the difference between the posted series and what I queued
>> after applying the changes suggested during the review.
>> 
>> Thanks.
>
> I was going to send a reroll after the received comments. Could you
> put this on top of 6/6, just to make sure future changes in t5537
> (maybe more or less commits created..) does not change the test
> behavior?
>
> It fixes the test name too. I originally thought, ok let's create
> commits in one test and do fetch in another. But it ended up in the
> same test and I forgot to update test name.

> -- 8< --
> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index 1413caf..b300383 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -203,7 +203,7 @@ EOF
>  # This test is tricky. We need large enough "have"s that fetch-pack
>  # will put pkt-flush in between. Then we need a "have" the server
>  # does not have, it'll send "ACK %s ready"
> -test_expect_success 'add more commits' '
> +test_expect_success 'no shallow lines after receiving ACK ready' '
>  	(
>  		cd shallow &&
>  		for i in $(test_seq 10)
> @@ -224,7 +224,9 @@ test_expect_success 'add more commits' '
>  		cd clone &&
>  		git checkout --orphan newnew &&
>  		test_commit new-too &&
> -		git fetch --depth=2
> +		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch --depth=2 &&
> +		grep "fetch-pack< ACK .* ready" ../trace &&
> +		! grep "fetch-pack> done" ../trace
>  	)
>  '
>  
> -- 8< -- 

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> 
> > Reported by Jeff [1]. Junio spotted it right but nobody made any move
> > since then.
> 
> Hrm.  Was I supposed to make any move at that point?  The discussion
> ended by me asking a question "what happens if we did X?" and there
> was no discussion.

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Reported by Jeff [1]. Junio spotted it right but nobody made any move
>> since then.
>
> Hrm.  Was I supposed to make any move at that point?  The discussion
> ended by me asking a question "what happens if we did X?" and there
> was no discussion.

> Duy Nguyen wrote:
>
>> Don't take it the wrong way. I was just summarizing the last round. It
>> surprised me though that this went under my radar. Perhaps a bug
>> tracker is not a bad idea after all (if Jeff went missing, this bug
>> could fall under the crack)
>
> I'm happy to plug
> - http://bugs.debian.org/cgi-bin/pkgreport.cgi?src=git;include=tags:upstream
> - http://packages.qa.debian.org/common/index.html (email subscription link:
>   source package = git; under "Advanced" it's possible to subscribe to
>   bug-tracking system emails and skip e.g. the automated build stuff)
> - https://www.debian.org/Bugs/Reporting (bug reporting interface -
>   unfortunately the important part is buried under "Sending the bug
>   report via e-mail")
> again. :)