[PATCH 0/6] Fix the shallow deepen bug with no-done

[PATCH 0/6] Fix the shallow deepen bug with no-done

[Nguyễn Thái Ngọc Duy]

Reported by Jeff [1]. Junio spotted it right but nobody made any move
since then. The main fix is 6/6. The rest is just updates while I was
looking at it. 2/6 may need fast track though.

[1] http://thread.gmane.org/gmane.comp.version-control.git/219914

Nguyễn Thái Ngọc Duy (6):
  test: rename http fetch and push test files
  t5538: fix default http port
  pack-protocol.txt: clarify 'obj-id' in the last ACK after 'done'
  protocol-capabilities.txt: refer multi_ack_detailed back to pack-protocol.txt
  protocol-capabilities.txt: document no-done
  fetch-pack: fix deepen shallow over smart http with no-done cap

 Documentation/technical/pack-protocol.txt          |  3 ++-
 Documentation/technical/protocol-capabilities.txt  | 18 ++++++++++++++++
 fetch-pack.c                                       |  3 ++-
 t/t5537-fetch-shallow.sh                           | 24 ++++++++++++++++++++++
 t/t5538-push-shallow.sh                            |  2 +-
 ...5540-http-push.sh => t5540-http-push-webdav.sh} |  0
 t/{t5541-http-push.sh => t5541-http-push-smart.sh} |  0
 ...5550-http-fetch.sh => t5550-http-fetch-dumb.sh} |  0
 ...551-http-fetch.sh => t5551-http-fetch-smart.sh} |  0
 9 files changed, 47 insertions(+), 3 deletions(-)
 rename t/{t5540-http-push.sh => t5540-http-push-webdav.sh} (100%)
 rename t/{t5541-http-push.sh => t5541-http-push-smart.sh} (100%)
 rename t/{t5550-http-fetch.sh => t5550-http-fetch-dumb.sh} (100%)
 rename t/{t5551-http-fetch.sh => t5551-http-fetch-smart.sh} (100%)

-- 
1.8.5.2.240.g8478abd

[PATCH 1/6] test: rename http fetch and push test files

[Nguyễn Thái Ngọc Duy]

Make clear which one is for dumb protocol, which one is for smart from
their file name.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/{t5540-http-push.sh => t5540-http-push-webdav.sh}  | 0
 t/{t5541-http-push.sh => t5541-http-push-smart.sh}   | 0
 t/{t5550-http-fetch.sh => t5550-http-fetch-dumb.sh}  | 0
 t/{t5551-http-fetch.sh => t5551-http-fetch-smart.sh} | 0
 4 files changed, 0 insertions(+), 0 deletions(-)
 rename t/{t5540-http-push.sh => t5540-http-push-webdav.sh} (100%)
 rename t/{t5541-http-push.sh => t5541-http-push-smart.sh} (100%)
 rename t/{t5550-http-fetch.sh => t5550-http-fetch-dumb.sh} (100%)
 rename t/{t5551-http-fetch.sh => t5551-http-fetch-smart.sh} (100%)

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push-webdav.sh
similarity index 100%
rename from t/t5540-http-push.sh
rename to t/t5540-http-push-webdav.sh
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push-smart.sh
similarity index 100%
rename from t/t5541-http-push.sh
rename to t/t5541-http-push-smart.sh
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch-dumb.sh
similarity index 100%
rename from t/t5550-http-fetch.sh
rename to t/t5550-http-fetch-dumb.sh
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch-smart.sh
similarity index 100%
rename from t/t5551-http-fetch.sh
rename to t/t5551-http-fetch-smart.sh
-- 
1.8.5.2.240.g8478abd

[PATCH 2/6] t5538: fix default http port

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t5538-push-shallow.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index 0a6e40f..19fe425 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -126,7 +126,7 @@ if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
 	test_done
 fi
 
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5537'}
+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5538'}
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
1.8.5.2.240.g8478abd

[PATCH 3/6] pack-protocol.txt: clarify 'obj-id' in the last ACK after 'done'

[Nguyễn Thái Ngọc Duy]

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
 ACK after 'done' if there is at least one common base and multi_ack or
 multi_ack_detailed is enabled. The server always sends NAK after 'done'
 if there is no common base found.
-- 
1.8.5.2.240.g8478abd

[PATCH 4/6] protocol-capabilities.txt: refer multi_ack_detailed back to pack-protocol.txt

[Nguyễn Thái Ngọc Duy]

pack-protocol.txt explains in detail how multi_ack_detailed works and
what's the difference between no multi_ack, multi_ack and
multi_ack_detailed. No need to repeat here.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/protocol-capabilities.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index e3e7924..cb40ebb 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -69,6 +69,12 @@ ends.
 Without multi_ack the client would have sent that c-b-a chain anyway,
 interleaved with S-R-Q.
 
+multi_ack_detailed
+------------------
+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.
+
 thin-pack
 ---------
 
-- 
1.8.5.2.240.g8478abd

[PATCH 5/6] protocol-capabilities.txt: document no-done

[Nguyễn Thái Ngọc Duy]

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

[PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap

[Nguyễn Thái Ngọc Duy]

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

Re: [PATCH 3/6] pack-protocol.txt: clarify 'obj-id' in the last ACK after 'done'

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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

I'd suggest rewording it to:

    'obj-is' is the object name of the last commit determined to be common

I do not mind too much if it used colloquial "SHA-1" in place of
"object name", but what is common is not the name but the object the
name refers to, so "the last commit" part is a moderately strong
suggestion.

Thanks.

Re: [PATCH 5/6] protocol-capabilities.txt: document no-done

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

Looks good; thanks.

Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap

[Junio C Hamano]

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 ..."?
Lack of a verb makes it hard to guess.

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

Thanks.

Do I understand the situation correctly if I said "The call to
consume-shallow-list has been here from the very beginning, but it
should have been adjusted like this patch when no-done was
introduced."?

>  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

Re: [PATCH 0/6] Fix the shallow deepen bug with no-done

[Junio C Hamano]

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.

> The main fix is 6/6. The rest is just updates while I was
> looking at it. 2/6 may need fast track though.

Re: [PATCH 1/6] test: rename http fetch and push test files

[Jeff King]

On Thu, Feb 06, 2014 at 10:10:34PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Make clear which one is for dumb protocol, which one is for smart from
> their file name.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Yay. This has often bugged me, and I can't believe we went this long
without fixing it.

-Peff

Re: [PATCH 2/6] t5538: fix default http port

[Jeff King]

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.

Yikes. I'm surprised we didn't notice this before, as it would probably
barf intermittently when running the tests in parallel. Perhaps it was
the cause of some intermittent failures that have gone undiagnosed.

Patch looks obviously correct.

-Peff

Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap

[Eric Sunshine]

On Thu, Feb 6, 2014 at 10:10 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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

s/the the/that the/

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

Do you want to use test_seq here?

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

Re: [PATCH 5/6] protocol-capabilities.txt: document no-done

[Jeff King]

On Thu, Feb 06, 2014 at 10:10:38PM +0700, Nguyễn Thái Ngọc Duy wrote:

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

Content looks good. A few minor grammar nits:

> +no-done
> +-------
> +This capability should be only used with smart HTTP protocol. If

split infinitive: s/be only/only be/

I think it would be more customary to say "the smart HTTP protocol",
with the definite article.

> +Without no-done in smart HTTP protocol, the server session would end

Ditto here.

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

s/and the server/before the server/ ?

-Peff

Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap

[Jeff King]

On Thu, Feb 06, 2014 at 10:10:39PM +0700, Nguyễn Thái Ngọc Duy wrote:

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

Thanks for a nice explanation.

> +# 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

s/the the/the/

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

This probably needs to be test_seq for portability.

-Peff

Re: [PATCH 0/6] Fix the shallow deepen bug with no-done

[Jeff King]

On Thu, Feb 06, 2014 at 11:31:02AM -0800, Junio C Hamano wrote:

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

No, you were rightly waiting on me. I queued it on my todo list but
haven't gotten around to it (and hadn't received any reports since the
original, so didn't consider it high priority). I picked it off my todo
list and stuck it on the bug list under "insanely hard" for fun (because
I think the diagnosis probably would have been beyond a first-time git
contributor).

Thanks very much for looking at (and fixing!) this, Duy.

-Peff

Re: [PATCH 0/6] Fix the shallow deepen bug with no-done

[Duy Nguyen]

On Fri, Feb 7, 2014 at 2:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap

[Duy Nguyen]

On Fri, Feb 7, 2014 at 2:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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 ..."?

Yes. Although think again, "both sides" is incorrect. If the server
side is happy, then it'll send "ACK.. ready" to stop the client. The
client can hardly protest.

> Do I understand the situation correctly if I said "The call to
> consume-shallow-list has been here from the very beginning, but it
> should have been adjusted like this patch when no-done was
> introduced."?

It's been there since the introduction of smart http (there are so
many "beginnings" in git). The rest is right.
-- 
Duy

Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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

In addition to two problems Eric and Peff noticed, && chain is
broken between these two pushes.  I initially didn't notice it but
it became obvious after reformatting to fix the indentation.

Here is the difference between the posted series and what I queued
after applying the changes suggested during the review.

Thanks.

 Documentation/technical/pack-protocol.txt         |  4 +--
 Documentation/technical/protocol-capabilities.txt | 10 +++----
 t/t5537-fetch-shallow.sh                          | 34 +++++++++++++----------
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index eb0b8a1..39c6410 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -338,8 +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'. 'obj-id' is the
-last SHA-1 determined to be common. The server only sends
+send a final 'ACK obj-id' or it will send a 'NAK'. 'obj-id' is the object
+name of the last commit determined to be common. The server only sends
 ACK after 'done' if there is at least one common base and multi_ack or
 multi_ack_detailed is enabled. The server always sends NAK after 'done'
 if there is no common base found.
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index cb2f5eb..e174343 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -77,15 +77,15 @@ section "Packfile Negotiation" for more information.
 
 no-done
 -------
-This capability should be only used with smart HTTP protocol. If
+This capability should only be used with the 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.
+Without no-done in the smart HTTP protocol, the server session would
+end and the client has to make another trip to send "done" before
+the server can send the pack. no-done removes the last round and
+thus slightly reduces latency.
 
 thin-pack
 ---------
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index fb11073..1413caf 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -201,26 +201,30 @@ 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
+# 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' '
 	(
-	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 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 fetch --depth=2
+		cd clone &&
+		git checkout --orphan newnew &&
+		test_commit new-too &&
+		git fetch --depth=2
 	)
 '
 

Re: [PATCH 0/6] Fix the shallow deepen bug with no-done

[Jonathan Nieder]

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

Re: [PATCH 0/6] Fix the shallow deepen bug with no-done

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

Then I'd add my bits ;-)

    http://git-blame.blogspot.com/p/leftover-bits.html

Admittedly I haven't touched it for a while, as I usually update it
during a lull, often in the pre-release -rc freeze period, but the
list has been quite active during -rc this cycle.

I probably should start dropping any new topics on the list and find
leftover bits during this cycle.

Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap

[Duy Nguyen]

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

Re: [PATCH 2/6] t5538: fix default http port

[Jeff King]

On Thu, Feb 06, 2014 at 02:35:33PM -0500, Jeff King wrote:

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

-Peff

Re: [PATCH 2/6] t5538: fix default http port

[Duy Nguyen]

On Sat, Feb 8, 2014 at 6:47 AM, Jeff King <peff@peff.net> wrote:
> 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).

Yes! Next stop, attempt to start httpd at start_httpd regardless of
GIT_TEST_HTTPD. If successful, test_set_prereq HTTPD or something that
the following tests can use. If GIT_TEST_HTTPD is set and start_httpd
fails, error out, not set prereq.
-- 
Duy

Re: [PATCH 2/6] t5538: fix default http port

[Jeff King]

On Sat, Feb 08, 2014 at 02:36:11PM +0700, Duy Nguyen wrote:

> > Thinking on this more, I wonder if we should just do something like
> > this:
> [...]
> Yes!

Here it is as a Real Patch™. I just based it on master, so it can
replace your 5537/5538 fix in your series.

> Next stop, attempt to start httpd at start_httpd regardless of
> GIT_TEST_HTTPD. If successful, test_set_prereq HTTPD or something that
> the following tests can use. If GIT_TEST_HTTPD is set and start_httpd
> fails, error out, not set prereq.

I'd be fine with that, as I always run them anyway. The potential
downsides would be:

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

-- >8 --
Subject: [PATCH] tests: auto-set LIB_HTTPD_PORT from test name

We set the default apache port for each of the httpd tests
to the 4-digit test number of the test script. We want these
to remain unique so that the tests do not conflict with each
other when run in parallel.

Instead of doing it manually in each test script, let's just
set it from the test name at run time. This is simpler, and
is one less thing to be updated when test scripts are
renamed (e.g., when being re-rolled or when conflicting
after being merged with another topic).

Incidentally, this fixes a case where t5537 and t5538 used
the same port number (5537), and could conflict with each
other when run in parallel.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-httpd.sh           | 2 +-
 t/t5537-fetch-shallow.sh | 1 -
 t/t5538-push-shallow.sh  | 1 -
 t/t5540-http-push.sh     | 1 -
 t/t5541-http-push.sh     | 1 -
 t/t5550-http-fetch.sh    | 1 -
 t/t5551-http-fetch.sh    | 1 -
 t/t5561-http-backend.sh  | 1 -
 8 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index bfdff2a..b43162e 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -64,7 +64,7 @@ case $(uname) in
 esac
 
 LIB_HTTPD_PATH=${LIB_HTTPD_PATH-"$DEFAULT_HTTPD_PATH"}
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}
+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-${this_test#t}}
 
 TEST_PATH="$TEST_DIRECTORY"/lib-httpd
 HTTPD_ROOT_PATH="$PWD"/httpd
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index b0fa738..adf215a 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -178,7 +178,6 @@ if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
 	test_done
 fi
 
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5537'}
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index 0a6e40f..8e54ac5 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -126,7 +126,6 @@ if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
 	test_done
 fi
 
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5537'}
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 5b0198c..8d7b3c5 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -16,7 +16,6 @@ then
 fi
 
 LIB_HTTPD_DAV=t
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5540'}
 . "$TEST_DIRECTORY"/lib-httpd.sh
 ROOT_PATH="$PWD"
 start_httpd
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index bfd241e..73af16f 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -12,7 +12,6 @@ if test -n "$NO_CURL"; then
 fi
 
 ROOT_PATH="$PWD"
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5541'}
 . "$TEST_DIRECTORY"/lib-httpd.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 start_httpd
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 8392624..1a3a2b6 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -8,7 +8,6 @@ if test -n "$NO_CURL"; then
 	test_done
 fi
 
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index a124efe..e07eaf3 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -8,7 +8,6 @@ if test -n "$NO_CURL"; then
 	test_done
 fi
 
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5551'}
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index b5d7fbc..d23fb02 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -8,7 +8,6 @@ if test -n "$NO_CURL"; then
 	test_done
 fi
 
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5561'}
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
1.8.5.2.500.g8060133

Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

Surely, and thanks for being careful.  Will squash it in.


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

Re: [PATCH 2/6] t5538: fix default http port

[Junio C Hamano]

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.

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

All good points.

Re: [PATCH 2/6] t5538: fix default http port

[Jeff King]

On Mon, Feb 10, 2014 at 10:23:53AM -0800, Junio C Hamano wrote:

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

Thanks. It might be worth squashing in the patch below (or sticking it
on top), to cover git-daemon as well.

AFAICT, that leaves SVN_HTTPD_PORT, which we do not set by default at
all, and the p4 tests, which already do their own similar magic.

The "set GIT_HTTPD_TESTS by default" magic should probably go in a
separate series, so I'll roll them by themselves.

-Peff

Re: [PATCH 2/6] t5538: fix default http port

[Jeff King]

On Mon, Feb 10, 2014 at 02:15:24PM -0500, Jeff King wrote:

> Thanks. It might be worth squashing in the patch below (or sticking it
> on top), to cover git-daemon as well.

Patch would probably be easier to read if I actually included it...

-- >8 --
Subject: [PATCH] tests: auto-set git-daemon port

A recent commit taught lib-httpd to always start apache on
the same port as the numbered tests. Let's do the same for
the git-daemon tests.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-git-daemon.sh   | 2 +-
 t/t5570-git-daemon.sh | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 394b06b..1f22de2 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -22,7 +22,7 @@ then
 	test_done
 fi
 
-LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-'8121'}
+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}}
 
 GIT_DAEMON_PID=
 GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index e061468..6b16379 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -3,7 +3,6 @@
 test_description='test fetching over git protocol'
 . ./test-lib.sh
 
-LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-5570}
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
 start_git_daemon
 
-- 
1.8.5.2.500.g8060133

[PATCH] tests: turn on network daemon tests by default

[Jeff King]

We do not run the httpd nor git-daemon tests by default, as
they are rather heavyweight and require network access
(albeit over localhost). However, it would be nice if more
pepole ran them, for two reasons:

  1. We would get more test coverage on more systems.

  2. The point of the test suite is to find regressions. It
     is very easy to change some of the underlying code and
     break the httpd code without realizing you are even
     affecting it. Running the httpd tests helps find these
     problems sooner (ideally before the patches even hit
     the list).

We still want to leave an "out", though, for people who
really do not want to run them. For that reason, the
GIT_TEST_HTTPD and GIT_TEST_GIT_DAEMON variables are now
tri-state booleans (true/false/auto), so you can say
GIT_TEST_HTTPD=false to turn the tests back off.

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 "true". This makes "auto" a sane
default, as we should not cause failures on setups where the
tests cannot run. But it allows people who use "true" to
catch regressions in their system (e.g., they uninstalled
apache, but were expecting their automated test runs to test
git-httpd, and would want to be notified).

Signed-off-by: Jeff King <peff@peff.net>
---

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

Re: [PATCH] tests: turn on network daemon tests by default

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Will queue, thanks.

I may have originally asked for it for one reason, thinking that one
reason would be enough while having another reason not to run them
as well.  But there would be countless silent others who have been
depending on that choice.

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 am in favor of this change but just pointing out possible fallouts
might be larger than we think.

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

Re: [PATCH] tests: turn on network daemon tests by default

[Jeff King]

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.

-Peff

Re: [PATCH] tests: turn on network daemon tests by default

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

The caller may have to pass the name of the variable:

	GIT_TEST_DAEMON=$(test_tristate GIT_TEST_DAEMON)

and then the callee may become

	test_tristate () {
		variable=$1
                if eval "test x\"\${$variable+isset}\" = xisset"
		then
			# explicitly set
                        eval "value=\$$variable"
                        case "$value" in
			"")
				echo false ;;
                        false | auto)
				echo "$value" ;;
			*)
				echo true ;;
   			esac
		else
			echo auto
		fi
	}

so that

 - Any non-empty string other than the magic strings "false" and
   "auto" continue to mean "please I want to test";

 - Setting the variable explicitly to an empty string will continue
   to mean "no I do not want to test";

 - Leaving the variable unset will continue to mean "I don't really
   care; just follow the default the project gives me".

Re: [PATCH] tests: turn on network daemon tests by default

[Junio C Hamano]

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

Under 'prove' test target, the way it exits causes:

*** prove ***
t5537-fetch-shallow.sh .. Dubious, test returned 1 (wstat 256, 0x100)
All 9 subtests passed

which leads to:

Test Summary Report
-------------------
t5537-fetch-shallow.sh (Wstat: 256 Tests: 9 Failed: 0)
  Non-zero exit status: 1
  Parse errors: No plan found in TAP output


On the 'master' branch without these "auto opt-in" patches,

$ GIT_TEST_HTTPD= 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
skipping remaining tests, git built without http support
# passed all 9 test(s)
1..9

Re: [PATCH] tests: turn on network daemon tests by default

[Jeff King]

On Tue, Feb 11, 2014 at 03:58:27PM -0800, Junio C Hamano wrote:

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

Yes, I didn't really think about build config that works reliably
between both versions (though personally, I think you should be building
with GIT_TEST_GIT_DAEMON=true :) ).

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

Hmm. So you are suggesting that the old code treated "undefined" and
"empty" the same (as "false"). But that in the new code, we would treat
them _differently_, taking undefined to mean "auto" and empty to mean
"false". I suppose that works, but it is rather unfortunate that the end
state we are left with (for all time) makes such a confusing and subtle
distinction.

I think this should work OK with the existing Makefile conventions. That
is, we do not ever set GIT_TEST_HTTPD in the Makefile ourselves, but
rely on it being either unset or set to whatever the user likes (this is
opposed to something like CFLAGS, where the distinction is long gone).

So I'm not excited about it, but I do not think there is any other
loophole through which we can maintain compatibility. If that's
important, I think we have to do it.

> The caller may have to pass the name of the variable:
> 
> 	GIT_TEST_DAEMON=$(test_tristate GIT_TEST_DAEMON)

I don't think that's a big deal. I actually was tempted to just make
this:

  test_normalize_tristate GIT_TEST_DAEMON

in the first place, since you would always want to look at the
normalized value from there on out.

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

Hmm, today I learned about "{foo:+bar}" versus "${foo+bar}". I'm not
sure how that bit of shell trivia escaped me for so many years.

-Peff

Re: [PATCH] tests: turn on network daemon tests by default

[Jeff King]

On Wed, Feb 12, 2014 at 11:06:43AM -0800, Junio C Hamano wrote:

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

I somehow pictured this while reading your email:
http://youtu.be/X4JVcvR7IM0

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

Ah, yeah, I did not notice that t5537 does its own special handling of
GIT_TEST_HTTPD. I think it is wrong to do so, and it is already buggy in
the case that GIT_TEST_HTTPD is set, but apache cannot be started.

E.g., with the current "master":

  $ sudo chmod -x `which apache2`
  $ GIT_TEST_HTTPD=1 ./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.

-Peff

Re: [PATCH] tests: turn on network daemon tests by default

[Junio C Hamano]

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


 t/lib-git-daemon.sh     |  2 +-
 t/lib-httpd.sh          |  2 +-
 t/test-lib-functions.sh | 45 +++++++++++++++++++++++++++------------------
 3 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 36106de..615bf5d 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -16,7 +16,7 @@
 #	stop_git_daemon
 #	test_done
 
-GIT_TEST_GIT_DAEMON=$(test_tristate "$GIT_TEST_GIT_DAEMON")
+test_tristate GIT_TEST_GIT_DAEMON
 if test "$GIT_TEST_GIT_DAEMON" = false
 then
 	skip_all="git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)"
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 583fb14..f9c2e22 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -30,7 +30,7 @@
 # Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
 #
 
-GIT_TEST_HTTPD=$(test_tristate "$GIT_TEST_HTTPD")
+test_tristate GIT_TEST_HTTPD
 if test "$GIT_TEST_HTTPD" = false
 then
 	skip_all="Network testing disabled (unset GIT_TEST_HTTPD to enable)"
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3cc09c0..21c5214 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -716,27 +716,36 @@ 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.:
+# Given a variable $1, normalize the value of it to one of "true",
+# "false", or "auto" and store the result to it.
 #
-#     GIT_TEST_HTTPD=$(test_tristate "$GIT_TEST_HTTPD")
+#     test_tristate GIT_TEST_HTTPD
 #
+# A variable set to an empty string is set to 'false'.
+# A variable set to 'false' or 'auto' keeps its value.
+# Anything else is set to 'true'.
+# An unset variable defaults to 'auto'.
+#
+# The last rule is to allow people to set the variable to an empty
+# string and export it to decline testing the particular feature
+# for versions both before and after this change.  We used to treat
+# both unset and empty variable as a signal for "do not test" and
+# took any non-empty string as "please test".
+
 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
+	if eval "test x\"\${$1+isset}\" = xisset"
+	then
+		# explicitly set
+		eval "
+			case \"\$$1\" in
+			'')	$1=false ;;
+			false | auto) ;;
+			*)	$1=true ;;
+			esac
+		"
+	else
+		eval "$1=auto"
+	fi
 }
 
 # Exit the test suite, either by skipping all remaining tests or by

Re: [PATCH] tests: turn on network daemon tests by default

[Duy Nguyen]

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

[PATCH] t5537: move http tests out to t5539

[Nguyễn Thái Ngọc Duy]

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

Re: [PATCH] tests: turn on network daemon tests by default

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

 - Do we want to do something similar to GIT_TEST_CREDENTIAL_HELPER?

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

We do not run the httpd nor git-daemon tests by default, as
they are rather heavyweight and require network access
(albeit over localhost). However, it would be nice if more
pepole ran them, for two reasons:

  1. We would get more test coverage on more systems.

  2. The point of the test suite is to find regressions. It
     is very easy to change some of the underlying code and
     break the httpd code without realizing you are even
     affecting it. Running the httpd tests helps find these
     problems sooner (ideally before the patches even hit
     the list).

We still want to leave an "out", though, for people who really do
not want to run them. For that reason, the GIT_TEST_HTTPD and
GIT_TEST_GIT_DAEMON variables are now tri-state booleans
(true/false/auto), so you can say GIT_TEST_HTTPD=false to turn the
tests back off.  To support those who want a stable single way to
disable these tests across versions of Git before and after this
change, an empty string explicitly set to these variables is also
taken as "false", so the behaviour changes only for those who:

  a. did not express any preference by leaving these variables
     unset.  They did not test these features before, but now they
     do; or

  b. did express that they want to test these features by setting
     GIT_TEST_FEATURE=false (or any equivalent other ways to tell
     "false" to Git, e.g. "0"), which has been a valid but funny way
     to say that they do want to test the feature only because we
     used to interpret any non-empty string to mean "yes please
     test".  They no longer test that feature.

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
"true". This makes "auto" a sane default, as we should not cause
failures on setups where the tests cannot run. But it allows people
who use "true" to catch regressions in their system (e.g., they
uninstalled apache, but were expecting their automated test runs to
test git-httpd, and would want to be notified).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-git-daemon.sh     |  8 ++++---
 t/lib-httpd.sh          | 22 +++++++++----------
 t/test-lib-functions.sh | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 394b06b..615bf5d 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"
+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 bfdff2a..f9c2e22 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"
+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..b333e3f 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -716,6 +716,64 @@ perl () {
 	command "$PERL_PATH" "$@"
 }
 
+# Is the value one of the various ways to spell a boolean true/false?
+test_normalize_bool () {
+	git -c magic.variable="$1" config --bool magic.variable 2>/dev/null
+}
+
+# Given a variable $1, normalize the value of it to one of "true",
+# "false", or "auto" and store the result to it.
+#
+#     test_tristate GIT_TEST_HTTPD
+#
+# A variable set to an empty string is set to 'false'.
+# A variable set to 'false' or 'auto' keeps its value.
+# Anything else is set to 'true'.
+# An unset variable defaults to 'auto'.
+#
+# The last rule is to allow people to set the variable to an empty
+# string and export it to decline testing the particular feature
+# for versions both before and after this change.  We used to treat
+# both unset and empty variable as a signal for "do not test" and
+# took any non-empty string as "please test".
+
+test_tristate () {
+	if eval "test x\"\${$1+isset}\" = xisset"
+	then
+		# explicitly set
+		eval "
+			case \"\$$1\" in
+			'')	$1=false ;;
+			auto)	;;
+			*)	$1=\$(test_normalize_bool \$$1 || echo true) ;;
+			esac
+		"
+	else
+		eval "$1=auto"
+	fi
+}
+
+# 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.
 
-- 
1.9.0-rc3-260-g4cf525c

Re: [PATCH] t5537: move http tests out to t5539

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

Will queue; thanks.

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

Re: [PATCH] tests: turn on network daemon tests by default

[Jeff King]

On Thu, Feb 13, 2014 at 11:35:13AM -0800, Junio C Hamano wrote:

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

Thanks for picking this up.

> 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

I assume the "something" here is to respect bool options more
consistently? I have no problem with that, but nor do I care too much
about those programs (that is partially laziness, but also partially
that I do not want to deal with introducing a regression).

>  - 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. It would be sensible to hook
contrib/credential to it, though.

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

You probably want to drop this "or empty" or change it to "or unset",
given the magic we do with empty-but-set variables in this version.

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

Patch looks good to me.

-Peff

Re: [PATCH] tests: turn on network daemon tests by default

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yeah, mostly by employing your 'git -c magic.var=X config --bool'
trick and check only for 'false' and 'true', instead of keeping a
hard-coded logic like the lines that hit the above query do.

> I have no problem with that, but nor do I care too much
> about those programs (that is partially laziness, but also partially
> that I do not want to deal with introducing a regression).

True, too ;-)

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

Ah, OK.  I was only grepping for "test -z .*GIT_TEST_".
>> tri-state is "auto" (or empty), but report an error when it is
>
> You probably want to drop this "or empty" or change it to "or unset",

Thanks, I totally missed that.