{"thread":{"id":"35815","subject":"[PATCH 0/6] Fix the shallow deepen bug with no-done","startedAt":"2014-02-06T15:10:33Z","lastAt":"2014-02-14T16:13:24Z","messageCount":44,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Jeff King","Eric Sunshine","Duy Nguyen","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"234367","messageId":"1391699439-22781-1-git-send-email-pclouds@gmail.com","threadId":"35815","inReplyTo":null,"subject":"[PATCH 0/6] Fix the shallow deepen bug with no-done","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-06T15:10:33Z","receivedAt":"2014-02-06T15:10:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Reported by Jeff [1]. Junio spotted it right but nobody made any move\nsince then. The main fix is 6/6. The rest is just updates while I was\nlooking at it. 2/6 may need fast track though.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/219914\n\nNguyễn Thái Ngọc Duy (6):\n test: rename http fetch and push test files\n t5538: fix default http port\n pack-protocol.txt: clarify 'obj-id' in the last ACK after 'done'\n protocol-capabilities.txt: refer multi_ack_detailed back to pack-protocol.txt\n protocol-capabilities.txt: document no-done\n fetch-pack: fix deepen shallow over smart http with no-done cap\n\n Documentation/technical/pack-protocol.txt | 3 ++-\n Documentation/technical/protocol-capabilities.txt | 18 ++++++++++++++++\n fetch-pack.c | 3 ++-\n t/t5537-fetch-shallow.sh | 24 ++++++++++++++++++++++\n t/t5538-push-shallow.sh | 2 +-\n ...5540-http-push.sh => t5540-http-push-webdav.sh} | 0\n t/{t5541-http-push.sh => t5541-http-push-smart.sh} | 0\n ...5550-http-fetch.sh => t5550-http-fetch-dumb.sh} | 0\n ...551-http-fetch.sh => t5551-http-fetch-smart.sh} | 0\n 9 files changed, 47 insertions(+), 3 deletions(-)\n rename t/{t5540-http-push.sh => t5540-http-push-webdav.sh} (100%)\n rename t/{t5541-http-push.sh => t5541-http-push-smart.sh} (100%)\n rename t/{t5550-http-fetch.sh => t5550-http-fetch-dumb.sh} (100%)\n rename t/{t5551-http-fetch.sh => t5551-http-fetch-smart.sh} (100%)\n\n-- \n1.8.5.2.240.g8478abd\n"},{"id":"234368","messageId":"1391699439-22781-2-git-send-email-pclouds@gmail.com","threadId":"35815","inReplyTo":"1391699439-22781-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/6] test: rename http fetch and push test files","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-06T15:10:34Z","receivedAt":"2014-02-06T15:10:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Make clear which one is for dumb protocol, which one is for smart from\ntheir file name.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n t/{t5540-http-push.sh => t5540-http-push-webdav.sh} | 0\n t/{t5541-http-push.sh => t5541-http-push-smart.sh} | 0\n t/{t5550-http-fetch.sh => t5550-http-fetch-dumb.sh} | 0\n t/{t5551-http-fetch.sh => t5551-http-fetch-smart.sh} | 0\n 4 files changed, 0 insertions(+), 0 deletions(-)\n rename t/{t5540-http-push.sh => t5540-http-push-webdav.sh} (100%)\n rename t/{t5541-http-push.sh => t5541-http-push-smart.sh} (100%)\n rename t/{t5550-http-fetch.sh => t5550-http-fetch-dumb.sh} (100%)\n rename t/{t5551-http-fetch.sh => t5551-http-fetch-smart.sh} (100%)\n\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push-webdav.sh\nsimilarity index 100%\nrename from t/t5540-http-push.sh\nrename to t/t5540-http-push-webdav.sh\ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push-smart.sh\nsimilarity index 100%\nrename from t/t5541-http-push.sh\nrename to t/t5541-http-push-smart.sh\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch-dumb.sh\nsimilarity index 100%\nrename from t/t5550-http-fetch.sh\nrename to t/t5550-http-fetch-dumb.sh\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch-smart.sh\nsimilarity index 100%\nrename from t/t5551-http-fetch.sh\nrename to t/t5551-http-fetch-smart.sh\n-- \n1.8.5.2.240.g8478abd\n"},{"id":"234369","messageId":"1391699439-22781-3-git-send-email-pclouds@gmail.com","threadId":"35815","inReplyTo":"1391699439-22781-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/6] t5538: fix default http port","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-06T15:10:35Z","receivedAt":"2014-02-06T15:10:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Originally I had t5537 use port 5536 and 5538 use port 5537(!). Then\nJeff found my fault so I changed port in t5537 from 5536 to 5537 in\n3b32a7c (t5537: fix incorrect expectation in test case 10 -\n2014-01-08). Which makes it worse because now both t5537 and t5538\nboth use the same port. Fix it.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n t/t5538-push-shallow.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh\nindex 0a6e40f..19fe425 100755\n--- a/t/t5538-push-shallow.sh\n+++ b/t/t5538-push-shallow.sh\n@@ -126,7 +126,7 @@ if test -n \"$NO_CURL\" -o -z \"$GIT_TEST_HTTPD\"; then\n \ttest_done\n fi\n \n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5537'}\n+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5538'}\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n1.8.5.2.240.g8478abd\n"},{"id":"234371","messageId":"1391699439-22781-4-git-send-email-pclouds@gmail.com","threadId":"35815","inReplyTo":"1391699439-22781-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/6] pack-protocol.txt: clarify 'obj-id' in the last ACK after 'done'","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-06T15:10:36Z","receivedAt":"2014-02-06T15:10:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"It's introduced in 1bd8c8f (git-upload-pack: Support the multi_ack\nprotocol - 2005-10-28) but probably better documented in the commit\nmessage of 78affc4 (Add multi_ack_detailed capability to\nfetch-pack/upload-pack - 2009-10-30)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/technical/pack-protocol.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex c73b62f..eb0b8a1 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -338,7 +338,8 @@ during a prior round. This helps to ensure that at least one common\n ancestor is found before we give up entirely.\n \n Once the 'done' line is read from the client, the server will either\n-send a final 'ACK obj-id' or it will send a 'NAK'. The server only sends\n+send a final 'ACK obj-id' or it will send a 'NAK'. 'obj-id' is the\n+last SHA-1 determined to be common. The server only sends\n ACK after 'done' if there is at least one common base and multi_ack or\n multi_ack_detailed is enabled. The server always sends NAK after 'done'\n if there is no common base found.\n-- \n1.8.5.2.240.g8478abd\n"},{"id":"234370","messageId":"1391699439-22781-5-git-send-email-pclouds@gmail.com","threadId":"35815","inReplyTo":"1391699439-22781-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/6] protocol-capabilities.txt: refer multi_ack_detailed back to pack-protocol.txt","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-06T15:10:37Z","receivedAt":"2014-02-06T15:10:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"pack-protocol.txt explains in detail how multi_ack_detailed works and\nwhat's the difference between no multi_ack, multi_ack and\nmulti_ack_detailed. No need to repeat here.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/technical/protocol-capabilities.txt | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex e3e7924..cb40ebb 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -69,6 +69,12 @@ ends.\n Without multi_ack the client would have sent that c-b-a chain anyway,\n interleaved with S-R-Q.\n \n+multi_ack_detailed\n+------------------\n+This is an extension of multi_ack that permits client to better\n+understand the server's in-memory state. See pack-protocol.txt,\n+section \"Packfile Negotiation\" for more information.\n+\n thin-pack\n ---------\n \n-- \n1.8.5.2.240.g8478abd\n"},{"id":"234372","messageId":"1391699439-22781-6-git-send-email-pclouds@gmail.com","threadId":"35815","inReplyTo":"1391699439-22781-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 5/6] protocol-capabilities.txt: document no-done","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-06T15:10:38Z","receivedAt":"2014-02-06T15:10:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"See 3e63b21 (upload-pack: Implement no-done capability - 2011-03-14)\nand 761ecf0 (fetch-pack: Implement no-done capability - 2011-03-14)\nfor more information.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/technical/protocol-capabilities.txt | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex cb40ebb..cb2f5eb 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -75,6 +75,18 @@ This is an extension of multi_ack that permits client to better\n understand the server's in-memory state. See pack-protocol.txt,\n section \"Packfile Negotiation\" for more information.\n \n+no-done\n+-------\n+This capability should be only used with smart HTTP protocol. If\n+multi_ack_detailed and no-done are both present, then the sender is\n+free to immediately send a pack following its first \"ACK obj-id ready\"\n+message.\n+\n+Without no-done in smart HTTP protocol, the server session would end\n+and the client has to make another trip to send \"done\" and the server\n+can send the pack. no-done removes the last round and thus slightly\n+reduces latency.\n+\n thin-pack\n ---------\n \n-- \n1.8.5.2.240.g8478abd\n"},{"id":"234373","messageId":"1391699439-22781-7-git-send-email-pclouds@gmail.com","threadId":"35815","inReplyTo":"1391699439-22781-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-06T15:10:39Z","receivedAt":"2014-02-06T15:10:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In smart http, upload-pack adds new shallow lines at the beginning of\neach rpc response. Only shallow lines from the first rpc call are\nuseful. After that they are thrown away. It's designed this way\nbecause upload-pack is stateless and has no idea when its shallow\nlines are helpful or not.\n\nSo after refs are negotiated with multi_ack_detailed and both sides\nhappy. The server sends \"ACK obj-id ready\", terminates the rpc call\nand waits for the final rpc round. The client sends \"done\". The server\nsends another response, which also has shallow lines at the beginning,\nand the last \"ACK obj-id\" line.\n\nWhen no-done is active, the last round is cut out, the server sends\n\"ACK obj-id ready\" and \"ACK obj-id\" in the same rpc\nresponse. fetch-pack is updated to recognize this and not send\n\"done\". However it still tries to consume shallow lines, which are\nnever sent.\n\nUpdate the code, make sure to skip consuming shallow lines when\nno-done is enabled.\n\nReported-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n fetch-pack.c | 3 ++-\n t/t5537-fetch-shallow.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 90fdd49..f061f1f 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -439,7 +439,8 @@ done:\n \t}\n \tstrbuf_release(&req_buf);\n \n-\tconsume_shallow_list(args, fd[0]);\n+\tif (!got_ready || !no_done)\n+\t\tconsume_shallow_list(args, fd[0]);\n \twhile (flushes || multi_ack) {\n \t\tint ack = get_ack(fd[0], result_sha1);\n \t\tif (ack) {\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex b0fa738..fb11073 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -200,5 +200,29 @@ EOF\n \t)\n '\n \n+# This test is tricky. We need large enough \"have\"s that fetch-pack\n+# will put pkt-flush in between. Then we need a \"have\" the the server\n+# does not have, it'll send \"ACK %s ready\"\n+test_expect_success 'add more commits' '\n+\t(\n+\tcd shallow &&\n+\tfor i in $(seq 10); do\n+\tgit checkout --orphan unrelated$i &&\n+\ttest_commit unrelated$i >/dev/null &&\n+\tgit push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" refs/heads/unrelated$i:refs/heads/unrelated$i\n+\tgit push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i\n+\tdone &&\n+\tgit checkout master &&\n+\ttest_commit new &&\n+\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" master\n+\t) &&\n+\t(\n+\tcd clone &&\n+\tgit checkout --orphan newnew &&\n+\ttest_commit new-too &&\n+\tgit fetch --depth=2\n+\t)\n+'\n+\n stop_httpd\n test_done\n-- \n1.8.5.2.240.g8478abd\n"},{"id":"234382","messageId":"xmqqlhxoysuw.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"1391699439-22781-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/6] pack-protocol.txt: clarify 'obj-id' in the last ACK after 'done'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-06T18:54:31Z","receivedAt":"2014-02-06T18:54:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> It's introduced in 1bd8c8f (git-upload-pack: Support the multi_ack\n> protocol - 2005-10-28) but probably better documented in the commit\n> message of 78affc4 (Add multi_ack_detailed capability to\n> fetch-pack/upload-pack - 2009-10-30)\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Documentation/technical/pack-protocol.txt | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index c73b62f..eb0b8a1 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -338,7 +338,8 @@ during a prior round. This helps to ensure that at least one common\n> ancestor is found before we give up entirely.\n> \n> Once the 'done' line is read from the client, the server will either\n> -send a final 'ACK obj-id' or it will send a 'NAK'. The server only sends\n> +send a final 'ACK obj-id' or it will send a 'NAK'. 'obj-id' is the\n> +last SHA-1 determined to be common. The server only sends\n\nI'd suggest rewording it to:\n\n 'obj-is' is the object name of the last commit determined to be common\n\nI do not mind too much if it used colloquial \"SHA-1\" in place of\n\"object name\", but what is common is not the name but the object the\nname refers to, so \"the last commit\" part is a moderately strong\nsuggestion.\n\nThanks.\n"},{"id":"234383","messageId":"xmqqha8cyssh.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"1391699439-22781-6-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 5/6] protocol-capabilities.txt: document no-done","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-06T18:55:58Z","receivedAt":"2014-02-06T18:55:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> See 3e63b21 (upload-pack: Implement no-done capability - 2011-03-14)\n> and 761ecf0 (fetch-pack: Implement no-done capability - 2011-03-14)\n> for more information.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Documentation/technical/protocol-capabilities.txt | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\n> index cb40ebb..cb2f5eb 100644\n> --- a/Documentation/technical/protocol-capabilities.txt\n> +++ b/Documentation/technical/protocol-capabilities.txt\n> @@ -75,6 +75,18 @@ This is an extension of multi_ack that permits client to better\n> understand the server's in-memory state. See pack-protocol.txt,\n> section \"Packfile Negotiation\" for more information.\n> \n> +no-done\n> +-------\n> +This capability should be only used with smart HTTP protocol. If\n> +multi_ack_detailed and no-done are both present, then the sender is\n> +free to immediately send a pack following its first \"ACK obj-id ready\"\n> +message.\n> +\n> +Without no-done in smart HTTP protocol, the server session would end\n> +and the client has to make another trip to send \"done\" and the server\n> +can send the pack. no-done removes the last round and thus slightly\n> +reduces latency.\n> +\n> thin-pack\n> ---------\n\nLooks good; thanks.\n"},{"id":"234384","messageId":"xmqqbnykyrtl.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"1391699439-22781-7-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-06T19:16:54Z","receivedAt":"2014-02-06T19:16:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> In smart http, upload-pack adds new shallow lines at the beginning of\n> each rpc response. Only shallow lines from the first rpc call are\n> useful. After that they are thrown away. It's designed this way\n> because upload-pack is stateless and has no idea when its shallow\n> lines are helpful or not.\n>\n> So after refs are negotiated with multi_ack_detailed and both sides\n> happy. The server sends \"ACK obj-id ready\", terminates the rpc call\n\nIs the above \"... and both sides are happy, the server sends ...\"?\nLack of a verb makes it hard to guess.\n\n> and waits for the final rpc round. The client sends \"done\". The server\n> sends another response, which also has shallow lines at the beginning,\n> and the last \"ACK obj-id\" line.\n>\n> When no-done is active, the last round is cut out, the server sends\n> \"ACK obj-id ready\" and \"ACK obj-id\" in the same rpc\n> response. fetch-pack is updated to recognize this and not send\n> \"done\". However it still tries to consume shallow lines, which are\n> never sent.\n>\n> Update the code, make sure to skip consuming shallow lines when\n> no-done is enabled.\n>\n> Reported-by: Jeff King \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n\nThanks.\n\nDo I understand the situation correctly if I said \"The call to\nconsume-shallow-list has been here from the very beginning, but it\nshould have been adjusted like this patch when no-done was\nintroduced.\"?\n\n> fetch-pack.c | 3 ++-\n> t/t5537-fetch-shallow.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 26 insertions(+), 1 deletion(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 90fdd49..f061f1f 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -439,7 +439,8 @@ done:\n> \t}\n> \tstrbuf_release(&req_buf);\n> \n> -\tconsume_shallow_list(args, fd[0]);\n> +\tif (!got_ready || !no_done)\n> +\t\tconsume_shallow_list(args, fd[0]);\n> \twhile (flushes || multi_ack) {\n> \t\tint ack = get_ack(fd[0], result_sha1);\n> \t\tif (ack) {\n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index b0fa738..fb11073 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -200,5 +200,29 @@ EOF\n> \t)\n> '\n> \n> +# This test is tricky. We need large enough \"have\"s that fetch-pack\n> +# will put pkt-flush in between. Then we need a \"have\" the the server\n> +# does not have, it'll send \"ACK %s ready\"\n> +test_expect_success 'add more commits' '\n> +\t(\n> +\tcd shallow &&\n> +\tfor i in $(seq 10); do\n> +\tgit checkout --orphan unrelated$i &&\n> +\ttest_commit unrelated$i >/dev/null &&\n> +\tgit push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" refs/heads/unrelated$i:refs/heads/unrelated$i\n> +\tgit push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i\n> +\tdone &&\n> +\tgit checkout master &&\n> +\ttest_commit new &&\n> +\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" master\n> +\t) &&\n> +\t(\n> +\tcd clone &&\n> +\tgit checkout --orphan newnew &&\n> +\ttest_commit new-too &&\n> +\tgit fetch --depth=2\n> +\t)\n> +'\n> +\n> stop_httpd\n> test_done\n"},{"id":"234385","messageId":"xmqq4n4cyr61.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"1391699439-22781-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 0/6] Fix the shallow deepen bug with no-done","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-06T19:31:02Z","receivedAt":"2014-02-06T19:31:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Reported by Jeff [1]. Junio spotted it right but nobody made any move\n> since then.\n\nHrm. Was I supposed to make any move at that point? The discussion\nended by me asking a question \"what happens if we did X?\" and there\nwas no discussion.\n\n> The main fix is 6/6. The rest is just updates while I was\n> looking at it. 2/6 may need fast track though.\n"},{"id":"234386","messageId":"20140206193339.GA14552@sigill.intra.peff.net","threadId":"35815","inReplyTo":"1391699439-22781-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/6] test: rename http fetch and push test files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-06T19:33:40Z","receivedAt":"2014-02-06T19:33:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 06, 2014 at 10:10:34PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Make clear which one is for dumb protocol, which one is for smart from\n> their file name.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nYay. This has often bugged me, and I can't believe we went this long\nwithout fixing it.\n\n-Peff\n"},{"id":"234387","messageId":"20140206193533.GB14552@sigill.intra.peff.net","threadId":"35815","inReplyTo":"1391699439-22781-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/6] t5538: fix default http port","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-06T19:35:36Z","receivedAt":"2014-02-06T19:35:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 06, 2014 at 10:10:35PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Originally I had t5537 use port 5536 and 5538 use port 5537(!). Then\n> Jeff found my fault so I changed port in t5537 from 5536 to 5537 in\n> 3b32a7c (t5537: fix incorrect expectation in test case 10 -\n> 2014-01-08). Which makes it worse because now both t5537 and t5538\n> both use the same port. Fix it.\n\nYikes. I'm surprised we didn't notice this before, as it would probably\nbarf intermittently when running the tests in parallel. Perhaps it was\nthe cause of some intermittent failures that have gone undiagnosed.\n\nPatch looks obviously correct.\n\n-Peff\n"},{"id":"234388","messageId":"CAPig+cSXEc_A3KUMvbFsuVZGfsXt1eU9kHO078LLp1HjL5ZkiQ@mail.gmail.com","threadId":"35815","inReplyTo":"1391699439-22781-7-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-06T19:35:58Z","receivedAt":"2014-02-06T19:35:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 6, 2014 at 10:10 AM, Nguyễn Thái Ngọc Duy wrote:\n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index b0fa738..fb11073 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -200,5 +200,29 @@ EOF\n> )\n> '\n>\n> +# This test is tricky. We need large enough \"have\"s that fetch-pack\n> +# will put pkt-flush in between. Then we need a \"have\" the the server\n\ns/the the/that the/\n\n> +# does not have, it'll send \"ACK %s ready\"\n> +test_expect_success 'add more commits' '\n> + (\n> + cd shallow &&\n> + for i in $(seq 10); do\n\nDo you want to use test_seq here?\n\n> + git checkout --orphan unrelated$i &&\n> + test_commit unrelated$i >/dev/null &&\n> + git push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" refs/heads/unrelated$i:refs/heads/unrelated$i\n> + git push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i\n> + done &&\n> + git checkout master &&\n> + test_commit new &&\n> + git push \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" master\n> + ) &&\n> + (\n> + cd clone &&\n> + git checkout --orphan newnew &&\n> + test_commit new-too &&\n> + git fetch --depth=2\n> + )\n> +'\n> +\n> stop_httpd\n> test_done\n> --\n> 1.8.5.2.240.g8478abd\n"},{"id":"234389","messageId":"20140206194000.GC14552@sigill.intra.peff.net","threadId":"35815","inReplyTo":"1391699439-22781-6-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 5/6] protocol-capabilities.txt: document no-done","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-06T19:40:01Z","receivedAt":"2014-02-06T19:40:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 06, 2014 at 10:10:38PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> See 3e63b21 (upload-pack: Implement no-done capability - 2011-03-14)\n> and 761ecf0 (fetch-pack: Implement no-done capability - 2011-03-14)\n> for more information.\n\nContent looks good. A few minor grammar nits:\n\n> +no-done\n> +-------\n> +This capability should be only used with smart HTTP protocol. If\n\nsplit infinitive: s/be only/only be/\n\nI think it would be more customary to say \"the smart HTTP protocol\",\nwith the definite article.\n\n> +Without no-done in smart HTTP protocol, the server session would end\n\nDitto here.\n\n> +and the client has to make another trip to send \"done\" and the server\n> +can send the pack. no-done removes the last round and thus slightly\n\ns/and the server/before the server/ ?\n\n-Peff\n"},{"id":"234390","messageId":"20140206194249.GD14552@sigill.intra.peff.net","threadId":"35815","inReplyTo":"1391699439-22781-7-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-06T19:42:59Z","receivedAt":"2014-02-06T19:42:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 06, 2014 at 10:10:39PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> In smart http, upload-pack adds new shallow lines at the beginning of\n> each rpc response. Only shallow lines from the first rpc call are\n> useful. After that they are thrown away. It's designed this way\n> because upload-pack is stateless and has no idea when its shallow\n> lines are helpful or not.\n> \n> So after refs are negotiated with multi_ack_detailed and both sides\n> happy. The server sends \"ACK obj-id ready\", terminates the rpc call\n> and waits for the final rpc round. The client sends \"done\". The server\n> sends another response, which also has shallow lines at the beginning,\n> and the last \"ACK obj-id\" line.\n> \n> When no-done is active, the last round is cut out, the server sends\n> \"ACK obj-id ready\" and \"ACK obj-id\" in the same rpc\n> response. fetch-pack is updated to recognize this and not send\n> \"done\". However it still tries to consume shallow lines, which are\n> never sent.\n> \n> Update the code, make sure to skip consuming shallow lines when\n> no-done is enabled.\n\nThanks for a nice explanation.\n\n> +# This test is tricky. We need large enough \"have\"s that fetch-pack\n> +# will put pkt-flush in between. Then we need a \"have\" the the server\n\ns/the the/the/\n\n> +# does not have, it'll send \"ACK %s ready\"\n> +test_expect_success 'add more commits' '\n> +\t(\n> +\tcd shallow &&\n> +\tfor i in $(seq 10); do\n\nThis probably needs to be test_seq for portability.\n\n-Peff\n"},{"id":"234391","messageId":"20140206194405.GA14478@sigill.intra.peff.net","threadId":"35815","inReplyTo":"xmqq4n4cyr61.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/6] Fix the shallow deepen bug with no-done","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-06T19:44:06Z","receivedAt":"2014-02-06T19:44:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 06, 2014 at 11:31:02AM -0800, Junio C Hamano wrote:\n\n> Nguyễn Thái Ngọc Duy writes:\n> \n> > Reported by Jeff [1]. Junio spotted it right but nobody made any move\n> > since then.\n> \n> Hrm. Was I supposed to make any move at that point? The discussion\n> ended by me asking a question \"what happens if we did X?\" and there\n> was no discussion.\n\nNo, you were rightly waiting on me. I queued it on my todo list but\nhaven't gotten around to it (and hadn't received any reports since the\noriginal, so didn't consider it high priority). I picked it off my todo\nlist and stuck it on the bug list under \"insanely hard\" for fun (because\nI think the diagnosis probably would have been beyond a first-time git\ncontributor).\n\nThanks very much for looking at (and fixing!) this, Duy.\n\n-Peff\n"},{"id":"234430","messageId":"CACsJy8AdOdVCma8FYeDM0kTd=Bgo-atpXmxFvmK4rzH6jOZEzQ@mail.gmail.com","threadId":"35815","inReplyTo":"xmqq4n4cyr61.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/6] Fix the shallow deepen bug with no-done","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-07T00:47:23Z","receivedAt":"2014-02-07T00:47:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 7, 2014 at 2:31 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Reported by Jeff [1]. Junio spotted it right but nobody made any move\n>> since then.\n>\n> Hrm. Was I supposed to make any move at that point? The discussion\n> ended by me asking a question \"what happens if we did X?\" and there\n> was no discussion.\n\nDon't take it the wrong way. I was just summarizing the last round. It\nsurprised me though that this went under my radar. Perhaps a bug\ntracker is not a bad idea after all (if Jeff went missing, this bug\ncould fall under the crack)\n-- \nDuy\n"},{"id":"234431","messageId":"CACsJy8C_xzPyEF8npz3iQ2hvNFvZNRKwMKy5Tu8QuT3O=TPZpg@mail.gmail.com","threadId":"35815","inReplyTo":"xmqqbnykyrtl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-07T00:52:06Z","receivedAt":"2014-02-07T00:52:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 7, 2014 at 2:16 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> In smart http, upload-pack adds new shallow lines at the beginning of\n>> each rpc response. Only shallow lines from the first rpc call are\n>> useful. After that they are thrown away. It's designed this way\n>> because upload-pack is stateless and has no idea when its shallow\n>> lines are helpful or not.\n>>\n>> So after refs are negotiated with multi_ack_detailed and both sides\n>> happy. The server sends \"ACK obj-id ready\", terminates the rpc call\n>\n> Is the above \"... and both sides are happy, the server sends ...\"?\n\nYes. Although think again, \"both sides\" is incorrect. If the server\nside is happy, then it'll send \"ACK.. ready\" to stop the client. The\nclient can hardly protest.\n\n> Do I understand the situation correctly if I said \"The call to\n> consume-shallow-list has been here from the very beginning, but it\n> should have been adjusted like this patch when no-done was\n> introduced.\"?\n\nIt's been there since the introduction of smart http (there are so\nmany \"beginnings\" in git). The rest is right.\n-- \nDuy\n"},{"id":"234458","messageId":"xmqqppmyvm3f.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"1391699439-22781-7-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T18:01:08Z","receivedAt":"2014-02-07T18:01:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index b0fa738..fb11073 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -200,5 +200,29 @@ EOF\n> \t)\n> '\n> \n> +# This test is tricky. We need large enough \"have\"s that fetch-pack\n> +# will put pkt-flush in between. Then we need a \"have\" the the server\n> +# does not have, it'll send \"ACK %s ready\"\n> +test_expect_success 'add more commits' '\n> +\t(\n> +\tcd shallow &&\n> +\tfor i in $(seq 10); do\n> +\tgit checkout --orphan unrelated$i &&\n> +\ttest_commit unrelated$i >/dev/null &&\n> +\tgit push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" refs/heads/unrelated$i:refs/heads/unrelated$i\n> +\tgit push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i\n\nIn addition to two problems Eric and Peff noticed, && chain is\nbroken between these two pushes. I initially didn't notice it but\nit became obvious after reformatting to fix the indentation.\n\nHere is the difference between the posted series and what I queued\nafter applying the changes suggested during the review.\n\nThanks.\n\n Documentation/technical/pack-protocol.txt | 4 +--\n Documentation/technical/protocol-capabilities.txt | 10 +++----\n t/t5537-fetch-shallow.sh | 34 +++++++++++++----------\n 3 files changed, 26 insertions(+), 22 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex eb0b8a1..39c6410 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -338,8 +338,8 @@ during a prior round. This helps to ensure that at least one common\n ancestor is found before we give up entirely.\n \n Once the 'done' line is read from the client, the server will either\n-send a final 'ACK obj-id' or it will send a 'NAK'. 'obj-id' is the\n-last SHA-1 determined to be common. The server only sends\n+send a final 'ACK obj-id' or it will send a 'NAK'. 'obj-id' is the object\n+name of the last commit determined to be common. The server only sends\n ACK after 'done' if there is at least one common base and multi_ack or\n multi_ack_detailed is enabled. The server always sends NAK after 'done'\n if there is no common base found.\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex cb2f5eb..e174343 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -77,15 +77,15 @@ section \"Packfile Negotiation\" for more information.\n \n no-done\n -------\n-This capability should be only used with smart HTTP protocol. If\n+This capability should only be used with the smart HTTP protocol. If\n multi_ack_detailed and no-done are both present, then the sender is\n free to immediately send a pack following its first \"ACK obj-id ready\"\n message.\n \n-Without no-done in smart HTTP protocol, the server session would end\n-and the client has to make another trip to send \"done\" and the server\n-can send the pack. no-done removes the last round and thus slightly\n-reduces latency.\n+Without no-done in the smart HTTP protocol, the server session would\n+end and the client has to make another trip to send \"done\" before\n+the server can send the pack. no-done removes the last round and\n+thus slightly reduces latency.\n \n thin-pack\n ---------\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex fb11073..1413caf 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -201,26 +201,30 @@ EOF\n '\n \n # This test is tricky. We need large enough \"have\"s that fetch-pack\n-# will put pkt-flush in between. Then we need a \"have\" the the server\n+# will put pkt-flush in between. Then we need a \"have\" the server\n # does not have, it'll send \"ACK %s ready\"\n test_expect_success 'add more commits' '\n \t(\n-\tcd shallow &&\n-\tfor i in $(seq 10); do\n-\tgit checkout --orphan unrelated$i &&\n-\ttest_commit unrelated$i >/dev/null &&\n-\tgit push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" refs/heads/unrelated$i:refs/heads/unrelated$i\n-\tgit push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i\n-\tdone &&\n-\tgit checkout master &&\n-\ttest_commit new &&\n-\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" master\n+\t\tcd shallow &&\n+\t\tfor i in $(test_seq 10)\n+\t\tdo\n+\t\t\tgit checkout --orphan unrelated$i &&\n+\t\t\ttest_commit unrelated$i &&\n+\t\t\tgit push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n+\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i &&\n+\t\t\tgit push -q ../clone/.git \\\n+\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i ||\n+\t\t\texit 1\n+\t\tdone &&\n+\t\tgit checkout master &&\n+\t\ttest_commit new &&\n+\t\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" master\n \t) &&\n \t(\n-\tcd clone &&\n-\tgit checkout --orphan newnew &&\n-\ttest_commit new-too &&\n-\tgit fetch --depth=2\n+\t\tcd clone &&\n+\t\tgit checkout --orphan newnew &&\n+\t\ttest_commit new-too &&\n+\t\tgit fetch --depth=2\n \t)\n '\n \n"},{"id":"234461","messageId":"20140207192031.GY30398@google.com","threadId":"35815","inReplyTo":"CACsJy8AdOdVCma8FYeDM0kTd=Bgo-atpXmxFvmK4rzH6jOZEzQ@mail.gmail.com","subject":"Re: [PATCH 0/6] Fix the shallow deepen bug with no-done","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-02-07T19:20:31Z","receivedAt":"2014-02-07T19:20:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Duy Nguyen wrote:\n\n> Don't take it the wrong way. I was just summarizing the last round. It\n> surprised me though that this went under my radar. Perhaps a bug\n> tracker is not a bad idea after all (if Jeff went missing, this bug\n> could fall under the crack)\n\nI'm happy to plug\n- http://bugs.debian.org/cgi-bin/pkgreport.cgi?src=git;include=tags:upstream\n- http://packages.qa.debian.org/common/index.html (email subscription link:\n source package = git; under \"Advanced\" it's possible to subscribe to\n bug-tracking system emails and skip e.g. the automated build stuff)\n- https://www.debian.org/Bugs/Reporting (bug reporting interface -\n unfortunately the important part is buried under \"Sending the bug\n report via e-mail\")\nagain. :)\n"},{"id":"234462","messageId":"xmqqbnyivggb.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"20140207192031.GY30398@google.com","subject":"Re: [PATCH 0/6] Fix the shallow deepen bug with no-done","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T20:03:00Z","receivedAt":"2014-02-07T20:03:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Duy Nguyen wrote:\n>\n>> Don't take it the wrong way. I was just summarizing the last round. It\n>> surprised me though that this went under my radar. Perhaps a bug\n>> tracker is not a bad idea after all (if Jeff went missing, this bug\n>> could fall under the crack)\n>\n> I'm happy to plug\n> - http://bugs.debian.org/cgi-bin/pkgreport.cgi?src=git;include=tags:upstream\n> - http://packages.qa.debian.org/common/index.html (email subscription link:\n> source package = git; under \"Advanced\" it's possible to subscribe to\n> bug-tracking system emails and skip e.g. the automated build stuff)\n> - https://www.debian.org/Bugs/Reporting (bug reporting interface -\n> unfortunately the important part is buried under \"Sending the bug\n> report via e-mail\")\n> again. :)\n\nThen I'd add my bits ;-)\n\n http://git-blame.blogspot.com/p/leftover-bits.html\n\nAdmittedly I haven't touched it for a while, as I usually update it\nduring a lull, often in the pre-release -rc freeze period, but the\nlist has been quite active during -rc this cycle.\n\nI probably should start dropping any new topics on the list and find\nleftover bits during this cycle.\n"},{"id":"234483","messageId":"20140207233906.GA5791@lanh","threadId":"35815","inReplyTo":"xmqqppmyvm3f.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-07T23:39:06Z","receivedAt":"2014-02-07T23:39:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 07, 2014 at 10:01:08AM -0800, Junio C Hamano wrote:\n> Here is the difference between the posted series and what I queued\n> after applying the changes suggested during the review.\n> \n> Thanks.\n\nI was going to send a reroll after the received comments. Could you\nput this on top of 6/6, just to make sure future changes in t5537\n(maybe more or less commits created..) does not change the test\nbehavior?\n\nIt fixes the test name too. I originally thought, ok let's create\ncommits in one test and do fetch in another. But it ended up in the\nsame test and I forgot to update test name.\n\n-- 8< --\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 1413caf..b300383 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -203,7 +203,7 @@ EOF\n # This test is tricky. We need large enough \"have\"s that fetch-pack\n # will put pkt-flush in between. Then we need a \"have\" the server\n # does not have, it'll send \"ACK %s ready\"\n-test_expect_success 'add more commits' '\n+test_expect_success 'no shallow lines after receiving ACK ready' '\n \t(\n \t\tcd shallow &&\n \t\tfor i in $(test_seq 10)\n@@ -224,7 +224,9 @@ test_expect_success 'add more commits' '\n \t\tcd clone &&\n \t\tgit checkout --orphan newnew &&\n \t\ttest_commit new-too &&\n-\t\tgit fetch --depth=2\n+\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" git fetch --depth=2 &&\n+\t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n+\t\t! grep \"fetch-pack> done\" ../trace\n \t)\n '\n \n-- 8< -- \n"},{"id":"234484","messageId":"20140207234727.GA17520@sigill.intra.peff.net","threadId":"35815","inReplyTo":"20140206193533.GB14552@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t5538: fix default http port","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-07T23:47:27Z","receivedAt":"2014-02-07T23:47:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 06, 2014 at 02:35:33PM -0500, Jeff King wrote:\n\n> On Thu, Feb 06, 2014 at 10:10:35PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> \n> > Originally I had t5537 use port 5536 and 5538 use port 5537(!). Then\n> > Jeff found my fault so I changed port in t5537 from 5536 to 5537 in\n> > 3b32a7c (t5537: fix incorrect expectation in test case 10 -\n> > 2014-01-08). Which makes it worse because now both t5537 and t5538\n> > both use the same port. Fix it.\n> [...]\n\nThinking on this more, I wonder if we should just do something like\nthis:\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex bfdff2a..c82b4ee 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -64,7 +64,9 @@ case $(uname) in\n esac\n \n LIB_HTTPD_PATH=${LIB_HTTPD_PATH-\"$DEFAULT_HTTPD_PATH\"}\n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}\n+if test -z \"$LIB_HTTPD_PORT\"; then\n+\tLIB_HTTPD_PORT=${this_test#t}\n+fi\n \n TEST_PATH=\"$TEST_DIRECTORY\"/lib-httpd\n HTTPD_ROOT_PATH=\"$PWD\"/httpd\n\nand drop the manual LIB_HTTPD_PORT setting in each of the test scripts.\nThat would prevent this type of error in the future, and people can\nstill override if they want to.\n\nAny test scripts that are not numbered would need to set it, but we\nshould not have any of those (and if we did, the failure mode would be\nOK, as Apache would barf on the bogus port number).\n\n-Peff\n"},{"id":"234493","messageId":"CACsJy8CLR4qerWPWs+0TWxnTWidm_VLtYExjKqAOLOSNK0chaA@mail.gmail.com","threadId":"35815","inReplyTo":"20140207234727.GA17520@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t5538: fix default http port","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-08T07:36:11Z","receivedAt":"2014-02-08T07:36:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 8, 2014 at 6:47 AM, Jeff King wrote:\n> Thinking on this more, I wonder if we should just do something like\n> this:\n>\n> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\n> index bfdff2a..c82b4ee 100644\n> --- a/t/lib-httpd.sh\n> +++ b/t/lib-httpd.sh\n> @@ -64,7 +64,9 @@ case $(uname) in\n> esac\n>\n> LIB_HTTPD_PATH=${LIB_HTTPD_PATH-\"$DEFAULT_HTTPD_PATH\"}\n> -LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}\n> +if test -z \"$LIB_HTTPD_PORT\"; then\n> + LIB_HTTPD_PORT=${this_test#t}\n> +fi\n>\n> TEST_PATH=\"$TEST_DIRECTORY\"/lib-httpd\n> HTTPD_ROOT_PATH=\"$PWD\"/httpd\n>\n> and drop the manual LIB_HTTPD_PORT setting in each of the test scripts.\n> That would prevent this type of error in the future, and people can\n> still override if they want to.\n>\n> Any test scripts that are not numbered would need to set it, but we\n> should not have any of those (and if we did, the failure mode would be\n> OK, as Apache would barf on the bogus port number).\n\nYes! Next stop, attempt to start httpd at start_httpd regardless of\nGIT_TEST_HTTPD. If successful, test_set_prereq HTTPD or something that\nthe following tests can use. If GIT_TEST_HTTPD is set and start_httpd\nfails, error out, not set prereq.\n-- \nDuy\n"},{"id":"234582","messageId":"20140210143948.GA21242@sigill.intra.peff.net","threadId":"35815","inReplyTo":"CACsJy8CLR4qerWPWs+0TWxnTWidm_VLtYExjKqAOLOSNK0chaA@mail.gmail.com","subject":"Re: [PATCH 2/6] t5538: fix default http port","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-10T14:39:48Z","receivedAt":"2014-02-10T14:39:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 08, 2014 at 02:36:11PM +0700, Duy Nguyen wrote:\n\n> > Thinking on this more, I wonder if we should just do something like\n> > this:\n> [...]\n> Yes!\n\nHere it is as a Real Patch™. I just based it on master, so it can\nreplace your 5537/5538 fix in your series.\n\n> Next stop, attempt to start httpd at start_httpd regardless of\n> GIT_TEST_HTTPD. If successful, test_set_prereq HTTPD or something that\n> the following tests can use. If GIT_TEST_HTTPD is set and start_httpd\n> fails, error out, not set prereq.\n\nI'd be fine with that, as I always run them anyway. The potential\ndownsides would be:\n\n 1. Is there anybody who has apache installed who would _not_ want to\n bring it up for the test?\n\n 2. Is there anybody for whom the failure mode of bringing up apache\n would be unpleasant (e.g., if it hangs the tests or something)?\n\nFor (1), we could perhaps have a GIT_NO_TEST_HTTPD to avoid it.\n\nFor (2), I suspect we may need to make our error handling more robust,\nbut getting people to run it is the first step to figuring out what the\nproblems are.\n\nIf we go this route, we should probably do the same for\nGIT_TEST_GIT_DAEMON in t5570 (and for that matter, we should probably do\nthe same for the port numbers).\n\n-- >8 --\nSubject: [PATCH] tests: auto-set LIB_HTTPD_PORT from test name\n\nWe set the default apache port for each of the httpd tests\nto the 4-digit test number of the test script. We want these\nto remain unique so that the tests do not conflict with each\nother when run in parallel.\n\nInstead of doing it manually in each test script, let's just\nset it from the test name at run time. This is simpler, and\nis one less thing to be updated when test scripts are\nrenamed (e.g., when being re-rolled or when conflicting\nafter being merged with another topic).\n\nIncidentally, this fixes a case where t5537 and t5538 used\nthe same port number (5537), and could conflict with each\nother when run in parallel.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd.sh | 2 +-\n t/t5537-fetch-shallow.sh | 1 -\n t/t5538-push-shallow.sh | 1 -\n t/t5540-http-push.sh | 1 -\n t/t5541-http-push.sh | 1 -\n t/t5550-http-fetch.sh | 1 -\n t/t5551-http-fetch.sh | 1 -\n t/t5561-http-backend.sh | 1 -\n 8 files changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex bfdff2a..b43162e 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -64,7 +64,7 @@ case $(uname) in\n esac\n \n LIB_HTTPD_PATH=${LIB_HTTPD_PATH-\"$DEFAULT_HTTPD_PATH\"}\n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}\n+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-${this_test#t}}\n \n TEST_PATH=\"$TEST_DIRECTORY\"/lib-httpd\n HTTPD_ROOT_PATH=\"$PWD\"/httpd\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex b0fa738..adf215a 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -178,7 +178,6 @@ if test -n \"$NO_CURL\" -o -z \"$GIT_TEST_HTTPD\"; then\n \ttest_done\n fi\n \n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5537'}\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \ndiff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh\nindex 0a6e40f..8e54ac5 100755\n--- a/t/t5538-push-shallow.sh\n+++ b/t/t5538-push-shallow.sh\n@@ -126,7 +126,6 @@ if test -n \"$NO_CURL\" -o -z \"$GIT_TEST_HTTPD\"; then\n \ttest_done\n fi\n \n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5537'}\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex 5b0198c..8d7b3c5 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -16,7 +16,6 @@ then\n fi\n \n LIB_HTTPD_DAV=t\n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5540'}\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n ROOT_PATH=\"$PWD\"\n start_httpd\ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\nindex bfd241e..73af16f 100755\n--- a/t/t5541-http-push.sh\n+++ b/t/t5541-http-push.sh\n@@ -12,7 +12,6 @@ if test -n \"$NO_CURL\"; then\n fi\n \n ROOT_PATH=\"$PWD\"\n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5541'}\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n start_httpd\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 8392624..1a3a2b6 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -8,7 +8,6 @@ if test -n \"$NO_CURL\"; then\n \ttest_done\n fi\n \n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex a124efe..e07eaf3 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -8,7 +8,6 @@ if test -n \"$NO_CURL\"; then\n \ttest_done\n fi\n \n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5551'}\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \ndiff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh\nindex b5d7fbc..d23fb02 100755\n--- a/t/t5561-http-backend.sh\n+++ b/t/t5561-http-backend.sh\n@@ -8,7 +8,6 @@ if test -n \"$NO_CURL\"; then\n \ttest_done\n fi\n \n-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5561'}\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n1.8.5.2.500.g8060133\n"},{"id":"234586","messageId":"xmqq1tzau8z7.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"20140207233906.GA5791@lanh","subject":"Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-10T18:18:52Z","receivedAt":"2014-02-10T18:18:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Feb 07, 2014 at 10:01:08AM -0800, Junio C Hamano wrote:\n>> Here is the difference between the posted series and what I queued\n>> after applying the changes suggested during the review.\n>> \n>> Thanks.\n>\n> I was going to send a reroll after the received comments. Could you\n> put this on top of 6/6, just to make sure future changes in t5537\n> (maybe more or less commits created..) does not change the test\n> behavior?\n>\n> It fixes the test name too. I originally thought, ok let's create\n> commits in one test and do fetch in another. But it ended up in the\n> same test and I forgot to update test name.\n\nSurely, and thanks for being careful. Will squash it in.\n\n\n> -- 8< --\n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index 1413caf..b300383 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -203,7 +203,7 @@ EOF\n> # This test is tricky. We need large enough \"have\"s that fetch-pack\n> # will put pkt-flush in between. Then we need a \"have\" the server\n> # does not have, it'll send \"ACK %s ready\"\n> -test_expect_success 'add more commits' '\n> +test_expect_success 'no shallow lines after receiving ACK ready' '\n> \t(\n> \t\tcd shallow &&\n> \t\tfor i in $(test_seq 10)\n> @@ -224,7 +224,9 @@ test_expect_success 'add more commits' '\n> \t\tcd clone &&\n> \t\tgit checkout --orphan newnew &&\n> \t\ttest_commit new-too &&\n> -\t\tgit fetch --depth=2\n> +\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" git fetch --depth=2 &&\n> +\t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n> +\t\t! grep \"fetch-pack> done\" ../trace\n> \t)\n> '\n> \n> -- 8< -- \n"},{"id":"234587","messageId":"xmqqwqh2su6e.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"20140210143948.GA21242@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t5538: fix default http port","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-10T18:23:53Z","receivedAt":"2014-02-10T18:23:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here it is as a Real Patch™. I just based it on master, so it can\n> replace your 5537/5538 fix in your series.\n\nThanks, looks good. Will put this at the bottom and rebuild the\nnd/http-fetch-shallow-fix series on top.\n\n> 1. Is there anybody who has apache installed who would _not_ want to\n> bring it up for the test?\n>\n> 2. Is there anybody for whom the failure mode of bringing up apache\n> would be unpleasant (e.g., if it hangs the tests or something)?\n>\n> For (1), we could perhaps have a GIT_NO_TEST_HTTPD to avoid it.\n>\n> For (2), I suspect we may need to make our error handling more robust,\n> but getting people to run it is the first step to figuring out what the\n> problems are.\n>\n> If we go this route, we should probably do the same for\n> GIT_TEST_GIT_DAEMON in t5570 (and for that matter, we should probably do\n> the same for the port numbers).\n\nAll good points.\n"},{"id":"234593","messageId":"20140210191521.GA3112@sigill.intra.peff.net","threadId":"35815","inReplyTo":"xmqqwqh2su6e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/6] t5538: fix default http port","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-10T19:15:24Z","receivedAt":"2014-02-10T19:15:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 10, 2014 at 10:23:53AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Here it is as a Real Patch™. I just based it on master, so it can\n> > replace your 5537/5538 fix in your series.\n> \n> Thanks, looks good. Will put this at the bottom and rebuild the\n> nd/http-fetch-shallow-fix series on top.\n\nThanks. It might be worth squashing in the patch below (or sticking it\non top), to cover git-daemon as well.\n\nAFAICT, that leaves SVN_HTTPD_PORT, which we do not set by default at\nall, and the p4 tests, which already do their own similar magic.\n\nThe \"set GIT_HTTPD_TESTS by default\" magic should probably go in a\nseparate series, so I'll roll them by themselves.\n\n-Peff\n"},{"id":"234594","messageId":"20140210191637.GA3820@sigill.intra.peff.net","threadId":"35815","inReplyTo":"20140210191521.GA3112@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t5538: fix default http port","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-10T19:16:38Z","receivedAt":"2014-02-10T19:16:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 10, 2014 at 02:15:24PM -0500, Jeff King wrote:\n\n> Thanks. It might be worth squashing in the patch below (or sticking it\n> on top), to cover git-daemon as well.\n\nPatch would probably be easier to read if I actually included it...\n\n-- >8 --\nSubject: [PATCH] tests: auto-set git-daemon port\n\nA recent commit taught lib-httpd to always start apache on\nthe same port as the numbered tests. Let's do the same for\nthe git-daemon tests.\n\nSigned-off-by: Jeff King \n---\n t/lib-git-daemon.sh | 2 +-\n t/t5570-git-daemon.sh | 1 -\n 2 files changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 394b06b..1f22de2 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -22,7 +22,7 @@ then\n \ttest_done\n fi\n \n-LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-'8121'}\n+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}}\n \n GIT_DAEMON_PID=\n GIT_DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex e061468..6b16379 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -3,7 +3,6 @@\n test_description='test fetching over git protocol'\n . ./test-lib.sh\n \n-LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-5570}\n . \"$TEST_DIRECTORY\"/lib-git-daemon.sh\n start_git_daemon\n \n-- \n1.8.5.2.500.g8060133\n"},{"id":"234600","messageId":"20140210212931.GA16154@sigill.intra.peff.net","threadId":"35815","inReplyTo":"20140210191521.GA3112@sigill.intra.peff.net","subject":"[PATCH] tests: turn on network daemon tests by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-10T21:29:37Z","receivedAt":"2014-02-10T21:29:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not run the httpd nor git-daemon tests by default, as\nthey are rather heavyweight and require network access\n(albeit over localhost). However, it would be nice if more\npepole ran them, for two reasons:\n\n 1. We would get more test coverage on more systems.\n\n 2. The point of the test suite is to find regressions. It\n is very easy to change some of the underlying code and\n break the httpd code without realizing you are even\n affecting it. Running the httpd tests helps find these\n problems sooner (ideally before the patches even hit\n the list).\n\nWe still want to leave an \"out\", though, for people who\nreally do not want to run them. For that reason, the\nGIT_TEST_HTTPD and GIT_TEST_GIT_DAEMON variables are now\ntri-state booleans (true/false/auto), so you can say\nGIT_TEST_HTTPD=false to turn the tests back off.\n\nIn addition, we are forgiving of common setup failures\n(e.g., you do not have apache installed, or have an old\nversion) when the tri-state is \"auto\" (or empty), but report\nan error when it is \"true\". This makes \"auto\" a sane\ndefault, as we should not cause failures on setups where the\ntests cannot run. But it allows people who use \"true\" to\ncatch regressions in their system (e.g., they uninstalled\napache, but were expecting their automated test runs to test\ngit-httpd, and would want to be notified).\n\nSigned-off-by: Jeff King \n---\n\nI dug in the history to see if there was any reasoning given for the\ncurrent \"off by default\" setting. It looks like Junio asked for it when\nthe original http-push tests were added, and everything else just\nfollowed that. The reasoning there was basically \"they're heavyweight\nand we might not be able to run them\", but hopefully having the option\nvariable will make that OK.\n\n t/lib-git-daemon.sh | 8 +++++---\n t/lib-httpd.sh | 22 +++++++++++-----------\n t/test-lib-functions.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 60 insertions(+), 14 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 1f22de2..e623bef 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -16,9 +16,10 @@\n #\tstop_git_daemon\n #\ttest_done\n \n-if test -z \"$GIT_TEST_GIT_DAEMON\"\n+GIT_TEST_GIT_DAEMON=$(test_tristate \"$GIT_TEST_GIT_DAEMON\")\n+if test \"$GIT_TEST_GIT_DAEMON\" = false\n then\n-\tskip_all=\"git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)\"\n+\tskip_all=\"git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)\"\n \ttest_done\n fi\n \n@@ -58,7 +59,8 @@ start_git_daemon() {\n \t\tkill \"$GIT_DAEMON_PID\"\n \t\twait \"$GIT_DAEMON_PID\"\n \t\ttrap 'die' EXIT\n-\t\terror \"git daemon failed to start\"\n+\t\ttest_skip_or_die $GIT_TEST_GIT_DAEMON \\\n+\t\t\t\"git daemon failed to start\"\n \tfi\n }\n \ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex b43162e..bb900ca 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -30,9 +30,10 @@\n # Copyright (c) 2008 Clemens Buchacher \n #\n \n-if test -z \"$GIT_TEST_HTTPD\"\n+GIT_TEST_HTTPD=$(test_tristate \"$GIT_TEST_HTTPD\")\n+if test \"$GIT_TEST_HTTPD\" = false\n then\n-\tskip_all=\"Network testing disabled (define GIT_TEST_HTTPD to enable)\"\n+\tskip_all=\"Network testing disabled (unset GIT_TEST_HTTPD to enable)\"\n \ttest_done\n fi\n \n@@ -76,8 +77,7 @@ GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export GIT_VALGRIND_OPTIONS\n \n if ! test -x \"$LIB_HTTPD_PATH\"\n then\n-\tskip_all=\"skipping test, no web server found at '$LIB_HTTPD_PATH'\"\n-\ttest_done\n+\ttest_skip_or_die $GIT_TEST_HTTPD \"no web server found at '$LIB_HTTPD_PATH'\"\n fi\n \n HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \\\n@@ -89,19 +89,20 @@ then\n \tthen\n \t\tif ! test $HTTPD_VERSION -ge 2\n \t\tthen\n-\t\t\tskip_all=\"skipping test, at least Apache version 2 is required\"\n-\t\t\ttest_done\n+\t\t\ttest_skip_or_die $GIT_TEST_HTTPD \\\n+\t\t\t\t\"at least Apache version 2 is required\"\n \t\tfi\n \t\tif ! test -d \"$DEFAULT_HTTPD_MODULE_PATH\"\n \t\tthen\n-\t\t\tskip_all=\"Apache module directory not found. Skipping tests.\"\n-\t\t\ttest_done\n+\t\t\ttest_skip_or_die $GIT_TEST_HTTPD \\\n+\t\t\t\t\"Apache module directory not found\"\n \t\tfi\n \n \t\tLIB_HTTPD_MODULE_PATH=\"$DEFAULT_HTTPD_MODULE_PATH\"\n \tfi\n else\n-\terror \"Could not identify web server at '$LIB_HTTPD_PATH'\"\n+\ttest_skip_or_die $GIT_TEST_HTTPD \\\n+\t\t\"Could not identify web server at '$LIB_HTTPD_PATH'\"\n fi\n \n prepare_httpd() {\n@@ -155,9 +156,8 @@ start_httpd() {\n \t\t>&3 2>&4\n \tif test $? -ne 0\n \tthen\n-\t\tskip_all=\"skipping test, web server setup failed\"\n \t\ttrap 'die' EXIT\n-\t\ttest_done\n+\t\ttest_skip_or_die $GIT_TEST_HTTPD \"web server setup failed\"\n \tfi\n }\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex aeae3ca..3cc09c0 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -716,6 +716,50 @@ perl () {\n \tcommand \"$PERL_PATH\" \"$@\"\n }\n \n+# Normalize the value given in $1 to one of \"true\", \"false\", or \"auto\". The\n+# result is written to stdout. E.g.:\n+#\n+# GIT_TEST_HTTPD=$(test_tristate \"$GIT_TEST_HTTPD\")\n+#\n+test_tristate () {\n+\tcase \"$1\" in\n+\t\"\"|auto)\n+\t\techo auto\n+\t\t;;\n+\t*)\n+\t\t# Rely on git-config to handle all the variants of\n+\t\t# true/1/on/etc that we allow.\n+\t\tif ! git -c magic.hack=\"$1\" config --bool magic.hack 2>/dev/null\n+\t\tthen\n+\t\t\t# If git-config failed, it was some non-bool value like\n+\t\t\t# YesPlease. Default this to \"true\" for historical\n+\t\t\t# compatibility.\n+\t\t\techo true\n+\t\tfi\n+\tesac\n+}\n+\n+# Exit the test suite, either by skipping all remaining tests or by\n+# exiting with an error. If \"$1\" is \"auto\", we then we assume we were\n+# opportunistically trying to set up some tests and we skip. If it is\n+# \"true\", then we report a failure.\n+#\n+# The error/skip message should be given by $2.\n+#\n+test_skip_or_die () {\n+\tcase \"$1\" in\n+\tauto)\n+\t\tskip_all=$2\n+\t\ttest_done\n+\t\t;;\n+\ttrue)\n+\t\terror \"$2\"\n+\t\t;;\n+\t*)\n+\t\terror \"BUG: test tristate is '$1' (real error: $2)\"\n+\tesac\n+}\n+\n # The following mingw_* functions obey POSIX shell syntax, but are actually\n # bash scripts, and are meant to be used only with bash on Windows.\n \n-- \n1.8.5.2.500.g8060133\n"},{"id":"234626","messageId":"xmqqa9dxpgw9.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"20140210212931.GA16154@sigill.intra.peff.net","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-11T19:51:18Z","receivedAt":"2014-02-11T19:51:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I dug in the history to see if there was any reasoning given for the\n> current \"off by default\" setting. It looks like Junio asked for it when\n> the original http-push tests were added, and everything else just\n> followed that. The reasoning there was basically \"they're heavyweight\n> and we might not be able to run them\", but hopefully having the option\n> variable will make that OK.\n\nWill queue, thanks.\n\nI may have originally asked for it for one reason, thinking that one\nreason would be enough while having another reason not to run them\nas well. But there would be countless silent others who have been\ndepending on that choice.\n\nThose who run buildfarms may want to disable the networking test if\nthe buildfarms are not isolated well, for example. They have to be\ntold somewhere that now they need to explicitly disable these tests\nand how.\n\nI am in favor of this change but just pointing out possible fallouts\nmight be larger than we think.\n\n> t/lib-git-daemon.sh | 8 +++++---\n> t/lib-httpd.sh | 22 +++++++++++-----------\n> t/test-lib-functions.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 60 insertions(+), 14 deletions(-)\n>\n> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\n> index 1f22de2..e623bef 100644\n> --- a/t/lib-git-daemon.sh\n> +++ b/t/lib-git-daemon.sh\n> @@ -16,9 +16,10 @@\n> #\tstop_git_daemon\n> #\ttest_done\n> \n> -if test -z \"$GIT_TEST_GIT_DAEMON\"\n> +GIT_TEST_GIT_DAEMON=$(test_tristate \"$GIT_TEST_GIT_DAEMON\")\n> +if test \"$GIT_TEST_GIT_DAEMON\" = false\n> then\n> -\tskip_all=\"git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)\"\n> +\tskip_all=\"git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)\"\n> \ttest_done\n> fi\n> \n> @@ -58,7 +59,8 @@ start_git_daemon() {\n> \t\tkill \"$GIT_DAEMON_PID\"\n> \t\twait \"$GIT_DAEMON_PID\"\n> \t\ttrap 'die' EXIT\n> -\t\terror \"git daemon failed to start\"\n> +\t\ttest_skip_or_die $GIT_TEST_GIT_DAEMON \\\n> +\t\t\t\"git daemon failed to start\"\n> \tfi\n> }\n> \n> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\n> index b43162e..bb900ca 100644\n> --- a/t/lib-httpd.sh\n> +++ b/t/lib-httpd.sh\n> @@ -30,9 +30,10 @@\n> # Copyright (c) 2008 Clemens Buchacher \n> #\n> \n> -if test -z \"$GIT_TEST_HTTPD\"\n> +GIT_TEST_HTTPD=$(test_tristate \"$GIT_TEST_HTTPD\")\n> +if test \"$GIT_TEST_HTTPD\" = false\n> then\n> -\tskip_all=\"Network testing disabled (define GIT_TEST_HTTPD to enable)\"\n> +\tskip_all=\"Network testing disabled (unset GIT_TEST_HTTPD to enable)\"\n> \ttest_done\n> fi\n> \n> @@ -76,8 +77,7 @@ GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export GIT_VALGRIND_OPTIONS\n> \n> if ! test -x \"$LIB_HTTPD_PATH\"\n> then\n> -\tskip_all=\"skipping test, no web server found at '$LIB_HTTPD_PATH'\"\n> -\ttest_done\n> +\ttest_skip_or_die $GIT_TEST_HTTPD \"no web server found at '$LIB_HTTPD_PATH'\"\n> fi\n> \n> HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \\\n> @@ -89,19 +89,20 @@ then\n> \tthen\n> \t\tif ! test $HTTPD_VERSION -ge 2\n> \t\tthen\n> -\t\t\tskip_all=\"skipping test, at least Apache version 2 is required\"\n> -\t\t\ttest_done\n> +\t\t\ttest_skip_or_die $GIT_TEST_HTTPD \\\n> +\t\t\t\t\"at least Apache version 2 is required\"\n> \t\tfi\n> \t\tif ! test -d \"$DEFAULT_HTTPD_MODULE_PATH\"\n> \t\tthen\n> -\t\t\tskip_all=\"Apache module directory not found. Skipping tests.\"\n> -\t\t\ttest_done\n> +\t\t\ttest_skip_or_die $GIT_TEST_HTTPD \\\n> +\t\t\t\t\"Apache module directory not found\"\n> \t\tfi\n> \n> \t\tLIB_HTTPD_MODULE_PATH=\"$DEFAULT_HTTPD_MODULE_PATH\"\n> \tfi\n> else\n> -\terror \"Could not identify web server at '$LIB_HTTPD_PATH'\"\n> +\ttest_skip_or_die $GIT_TEST_HTTPD \\\n> +\t\t\"Could not identify web server at '$LIB_HTTPD_PATH'\"\n> fi\n> \n> prepare_httpd() {\n> @@ -155,9 +156,8 @@ start_httpd() {\n> \t\t>&3 2>&4\n> \tif test $? -ne 0\n> \tthen\n> -\t\tskip_all=\"skipping test, web server setup failed\"\n> \t\ttrap 'die' EXIT\n> -\t\ttest_done\n> +\t\ttest_skip_or_die $GIT_TEST_HTTPD \"web server setup failed\"\n> \tfi\n> }\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index aeae3ca..3cc09c0 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -716,6 +716,50 @@ perl () {\n> \tcommand \"$PERL_PATH\" \"$@\"\n> }\n> \n> +# Normalize the value given in $1 to one of \"true\", \"false\", or \"auto\". The\n> +# result is written to stdout. E.g.:\n> +#\n> +# GIT_TEST_HTTPD=$(test_tristate \"$GIT_TEST_HTTPD\")\n> +#\n> +test_tristate () {\n> +\tcase \"$1\" in\n> +\t\"\"|auto)\n> +\t\techo auto\n> +\t\t;;\n> +\t*)\n> +\t\t# Rely on git-config to handle all the variants of\n> +\t\t# true/1/on/etc that we allow.\n> +\t\tif ! git -c magic.hack=\"$1\" config --bool magic.hack 2>/dev/null\n> +\t\tthen\n> +\t\t\t# If git-config failed, it was some non-bool value like\n> +\t\t\t# YesPlease. Default this to \"true\" for historical\n> +\t\t\t# compatibility.\n> +\t\t\techo true\n> +\t\tfi\n> +\tesac\n> +}\n> +\n> +# Exit the test suite, either by skipping all remaining tests or by\n> +# exiting with an error. If \"$1\" is \"auto\", we then we assume we were\n> +# opportunistically trying to set up some tests and we skip. If it is\n> +# \"true\", then we report a failure.\n> +#\n> +# The error/skip message should be given by $2.\n> +#\n> +test_skip_or_die () {\n> +\tcase \"$1\" in\n> +\tauto)\n> +\t\tskip_all=$2\n> +\t\ttest_done\n> +\t\t;;\n> +\ttrue)\n> +\t\terror \"$2\"\n> +\t\t;;\n> +\t*)\n> +\t\terror \"BUG: test tristate is '$1' (real error: $2)\"\n> +\tesac\n> +}\n> +\n> # The following mingw_* functions obey POSIX shell syntax, but are actually\n> # bash scripts, and are meant to be used only with bash on Windows.\n"},{"id":"234628","messageId":"20140211200445.GA27946@sigill.intra.peff.net","threadId":"35815","inReplyTo":"xmqqa9dxpgw9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-11T20:04:45Z","receivedAt":"2014-02-11T20:04:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 11, 2014 at 11:51:18AM -0800, Junio C Hamano wrote:\n\n> Those who run buildfarms may want to disable the networking test if\n> the buildfarms are not isolated well, for example. They have to be\n> told somewhere that now they need to explicitly disable these tests\n> and how.\n\nI think they should be OK. The daemons run on the loopback interface, so\nthere is hopefully not a security implication. If multiple buildfarms\nare sharing the same loopback space (e.g., running in separate\ndirectories on the same machine), the \"auto\" setting should degrade\ngracefully. One daemon will \"win\" the setup, and the tests will run, and\non the other, they will be skipped.\n\n> I am in favor of this change but just pointing out possible fallouts\n> might be larger than we think.\n\nAgreed, but I think the only way to know the size of those fallouts is\nto try it and see who complains. I would not normally be so cavalier\nwith git itself, but I think for the test infrastructure, we have a\nsmall, tech-savvy audience that can help us iterate on it without too\nmuch pain.\n\n-Peff\n"},{"id":"234631","messageId":"xmqqzjlxnqvw.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"20140211200445.GA27946@sigill.intra.peff.net","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-11T23:58:27Z","receivedAt":"2014-02-11T23:58:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 11, 2014 at 11:51:18AM -0800, Junio C Hamano wrote:\n>\n>> Those who run buildfarms may want to disable the networking test if\n>> the buildfarms are not isolated well, for example. They have to be\n>> told somewhere that now they need to explicitly disable these tests\n>> and how.\n>\n> I think they should be OK. The daemons run on the loopback interface, so\n> there is hopefully not a security implication. If multiple buildfarms\n> are sharing the same loopback space (e.g., running in separate\n> directories on the same machine), the \"auto\" setting should degrade\n> gracefully. One daemon will \"win\" the setup, and the tests will run, and\n> on the other, they will be skipped.\n>\n>> I am in favor of this change but just pointing out possible fallouts\n>> might be larger than we think.\n>\n> Agreed, but I think the only way to know the size of those fallouts is\n> to try it and see who complains. I would not normally be so cavalier\n> with git itself, but I think for the test infrastructure, we have a\n> small, tech-savvy audience that can help us iterate on it without too\n> much pain.\n\nSure. One immediate complaint is that I would probably need to do\nsomething like this in the build automation:\n\n\tif testing a branch without this patch\n then\n\t\t: do nothing\n\telse\n\t\tGIT_TEST_GIT_DAEMON=false\n\tfi\n\nArguably, it is the fault of the current/original code that treated\n*any* non-empty value that is set in the environment variable as\n\"true\"---if it paid attention to GIT_TEST_GIT_DAEMON=NoThanks, we\nwouldn't have to have a workaround like this.\n\nI wonder if GIT_TEST_X=$(test_tristate \"$GIT_TEST_X\") pattern can be\nmade a bit more friendly, though. For example, can we behave\ndifferently depending on the reason why $GIT_TEST_X is empty?\n\n - People who have *not* been opting in to the expensive tests have\n not done anything special; GIT_TEST_X environment variable did\n not exist for them (i.e. unset), and we used to skip when\n \"$GIT_TEST_X\" is an empty string.\n\n - We want to encourage people who do not care to run these tests.\n If people do not do anything, their $GIT_TEST_X will continue to\n be an empty string without GIT_TEST_X variable in the\n environment.\n\nIf we let people who *do* want to opt out of the expensive tests by\nexplicitly setting $GIT_TEST_X to an empty string in the new scheme,\nwouldn't the transition go a lot smoother?\n\nThe caller may have to pass the name of the variable:\n\n\tGIT_TEST_DAEMON=$(test_tristate GIT_TEST_DAEMON)\n\nand then the callee may become\n\n\ttest_tristate () {\n\t\tvariable=$1\n if eval \"test x\\\"\\${$variable+isset}\\\" = xisset\"\n\t\tthen\n\t\t\t# explicitly set\n eval \"value=\\$$variable\"\n case \"$value\" in\n\t\t\t\"\")\n\t\t\t\techo false ;;\n false | auto)\n\t\t\t\techo \"$value\" ;;\n\t\t\t*)\n\t\t\t\techo true ;;\n \t\t\tesac\n\t\telse\n\t\t\techo auto\n\t\tfi\n\t}\n\nso that\n\n - Any non-empty string other than the magic strings \"false\" and\n \"auto\" continue to mean \"please I want to test\";\n\n - Setting the variable explicitly to an empty string will continue\n to mean \"no I do not want to test\";\n\n - Leaving the variable unset will continue to mean \"I don't really\n care; just follow the default the project gives me\".\n"},{"id":"234680","messageId":"xmqq61oknoak.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"20140211200445.GA27946@sigill.intra.peff.net","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-12T19:06:43Z","receivedAt":"2014-02-12T19:06:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Agreed, but I think the only way to know the size of those fallouts is\n> to try it and see who complains. I would not normally be so cavalier\n> with git itself, but I think for the test infrastructure, we have a\n> small, tech-savvy audience that can help us iterate on it without too\n> much pain.\n\nThere is another.\n\n$ GIT_TEST_HTTPD=false sh t5537-fetch-shallow.sh \nok 1 - setup\nok 2 - setup shallow clone\nok 3 - clone from shallow clone\nok 4 - fetch from shallow clone\nok 5 - fetch --depth from shallow clone\nok 6 - fetch --unshallow from shallow clone\nok 7 - fetch something upstream has but hidden by clients shallow boundaries\nok 8 - fetch that requires changes in .git/shallow is filtered\nok 9 - fetch --update-shallow\nerror: Can't use skip_all after running some tests\n\nUnder 'prove' test target, the way it exits causes:\n\n*** prove ***\nt5537-fetch-shallow.sh .. Dubious, test returned 1 (wstat 256, 0x100)\nAll 9 subtests passed\n\nwhich leads to:\n\nTest Summary Report\n-------------------\nt5537-fetch-shallow.sh (Wstat: 256 Tests: 9 Failed: 0)\n Non-zero exit status: 1\n Parse errors: No plan found in TAP output\n\n\nOn the 'master' branch without these \"auto opt-in\" patches,\n\n$ GIT_TEST_HTTPD= sh t5537-fetch-shallow.sh \nok 1 - setup\nok 2 - setup shallow clone\nok 3 - clone from shallow clone\nok 4 - fetch from shallow clone\nok 5 - fetch --depth from shallow clone\nok 6 - fetch --unshallow from shallow clone\nok 7 - fetch something upstream has but hidden by clients shallow boundaries\nok 8 - fetch that requires changes in .git/shallow is filtered\nok 9 - fetch --update-shallow\nskipping remaining tests, git built without http support\n# passed all 9 test(s)\n1..9\n"},{"id":"234693","messageId":"20140212214753.GA6799@sigill.intra.peff.net","threadId":"35815","inReplyTo":"xmqqzjlxnqvw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-12T21:47:53Z","receivedAt":"2014-02-12T21:47:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 11, 2014 at 03:58:27PM -0800, Junio C Hamano wrote:\n\n> Sure. One immediate complaint is that I would probably need to do\n> something like this in the build automation:\n> \n> \tif testing a branch without this patch\n> then\n> \t\t: do nothing\n> \telse\n> \t\tGIT_TEST_GIT_DAEMON=false\n> \tfi\n> \n> Arguably, it is the fault of the current/original code that treated\n> *any* non-empty value that is set in the environment variable as\n> \"true\"---if it paid attention to GIT_TEST_GIT_DAEMON=NoThanks, we\n> wouldn't have to have a workaround like this.\n\nYes, I didn't really think about build config that works reliably\nbetween both versions (though personally, I think you should be building\nwith GIT_TEST_GIT_DAEMON=true :) ).\n\n> I wonder if GIT_TEST_X=$(test_tristate \"$GIT_TEST_X\") pattern can be\n> made a bit more friendly, though. For example, can we behave\n> differently depending on the reason why $GIT_TEST_X is empty?\n> \n> - People who have *not* been opting in to the expensive tests have\n> not done anything special; GIT_TEST_X environment variable did\n> not exist for them (i.e. unset), and we used to skip when\n> \"$GIT_TEST_X\" is an empty string.\n> \n> - We want to encourage people who do not care to run these tests.\n> If people do not do anything, their $GIT_TEST_X will continue to\n> be an empty string without GIT_TEST_X variable in the\n> environment.\n> \n> If we let people who *do* want to opt out of the expensive tests by\n> explicitly setting $GIT_TEST_X to an empty string in the new scheme,\n> wouldn't the transition go a lot smoother?\n\nHmm. So you are suggesting that the old code treated \"undefined\" and\n\"empty\" the same (as \"false\"). But that in the new code, we would treat\nthem _differently_, taking undefined to mean \"auto\" and empty to mean\n\"false\". I suppose that works, but it is rather unfortunate that the end\nstate we are left with (for all time) makes such a confusing and subtle\ndistinction.\n\nI think this should work OK with the existing Makefile conventions. That\nis, we do not ever set GIT_TEST_HTTPD in the Makefile ourselves, but\nrely on it being either unset or set to whatever the user likes (this is\nopposed to something like CFLAGS, where the distinction is long gone).\n\nSo I'm not excited about it, but I do not think there is any other\nloophole through which we can maintain compatibility. If that's\nimportant, I think we have to do it.\n\n> The caller may have to pass the name of the variable:\n> \n> \tGIT_TEST_DAEMON=$(test_tristate GIT_TEST_DAEMON)\n\nI don't think that's a big deal. I actually was tempted to just make\nthis:\n\n test_normalize_tristate GIT_TEST_DAEMON\n\nin the first place, since you would always want to look at the\nnormalized value from there on out.\n\n> and then the callee may become\n> \n> \ttest_tristate () {\n> \t\tvariable=$1\n> if eval \"test x\\\"\\${$variable+isset}\\\" = xisset\"\n\nHmm, today I learned about \"{foo:+bar}\" versus \"${foo+bar}\". I'm not\nsure how that bit of shell trivia escaped me for so many years.\n\n-Peff\n"},{"id":"234696","messageId":"20140212221258.GB6799@sigill.intra.peff.net","threadId":"35815","inReplyTo":"xmqq61oknoak.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-12T22:12:58Z","receivedAt":"2014-02-12T22:12:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 12, 2014 at 11:06:43AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Agreed, but I think the only way to know the size of those fallouts is\n> > to try it and see who complains. I would not normally be so cavalier\n> > with git itself, but I think for the test infrastructure, we have a\n> > small, tech-savvy audience that can help us iterate on it without too\n> > much pain.\n> \n> There is another.\n\nI somehow pictured this while reading your email:\nhttp://youtu.be/X4JVcvR7IM0\n\n> $ GIT_TEST_HTTPD=false sh t5537-fetch-shallow.sh \n> ok 1 - setup\n> ok 2 - setup shallow clone\n> ok 3 - clone from shallow clone\n> ok 4 - fetch from shallow clone\n> ok 5 - fetch --depth from shallow clone\n> ok 6 - fetch --unshallow from shallow clone\n> ok 7 - fetch something upstream has but hidden by clients shallow boundaries\n> ok 8 - fetch that requires changes in .git/shallow is filtered\n> ok 9 - fetch --update-shallow\n> error: Can't use skip_all after running some tests\n\nAh, yeah, I did not notice that t5537 does its own special handling of\nGIT_TEST_HTTPD. I think it is wrong to do so, and it is already buggy in\nthe case that GIT_TEST_HTTPD is set, but apache cannot be started.\n\nE.g., with the current \"master\":\n\n $ sudo chmod -x `which apache2`\n $ GIT_TEST_HTTPD=1 ./t5537-fetch-shallow.sh\n ok 1 - setup\n ok 2 - setup shallow clone\n ok 3 - clone from shallow clone\n ok 4 - fetch from shallow clone\n ok 5 - fetch --depth from shallow clone\n ok 6 - fetch --unshallow from shallow clone\n ok 7 - fetch something upstream has but hidden by clients shallow boundaries\n ok 8 - fetch that requires changes in .git/shallow is filtered\n ok 9 - fetch --update-shallow\n error: Can't use skip_all after running some tests\n\nlib-httpd was never designed to be included from anywhere except the\nbeginning of the file. But that wouldn't be right for t5537, because it\nwants to run some of the tests, even if apache setup fails. The right\nway to do it is probably to have lib-httpd do all of its work in a lazy\nprereq. I don't know how clunky that will end up, though; it might be\nsimpler to just move the shallow http test into one of the http-fetch\nscripts.\n\n-Peff\n"},{"id":"234697","messageId":"xmqq38jom037.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"20140212214753.GA6799@sigill.intra.peff.net","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-12T22:34:52Z","receivedAt":"2014-02-12T22:34:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> test_normalize_tristate GIT_TEST_DAEMON\n\nHeh, great minds think alike. This is what I am playing with,\nwithout committing (because I do like your \"ask config if this is a\nkind of various boolean 'false' representations, which I haven't\nmanaged to add to it).\n\n\n t/lib-git-daemon.sh | 2 +-\n t/lib-httpd.sh | 2 +-\n t/test-lib-functions.sh | 45 +++++++++++++++++++++++++++------------------\n 3 files changed, 29 insertions(+), 20 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 36106de..615bf5d 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -16,7 +16,7 @@\n #\tstop_git_daemon\n #\ttest_done\n \n-GIT_TEST_GIT_DAEMON=$(test_tristate \"$GIT_TEST_GIT_DAEMON\")\n+test_tristate GIT_TEST_GIT_DAEMON\n if test \"$GIT_TEST_GIT_DAEMON\" = false\n then\n \tskip_all=\"git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)\"\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 583fb14..f9c2e22 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -30,7 +30,7 @@\n # Copyright (c) 2008 Clemens Buchacher \n #\n \n-GIT_TEST_HTTPD=$(test_tristate \"$GIT_TEST_HTTPD\")\n+test_tristate GIT_TEST_HTTPD\n if test \"$GIT_TEST_HTTPD\" = false\n then\n \tskip_all=\"Network testing disabled (unset GIT_TEST_HTTPD to enable)\"\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 3cc09c0..21c5214 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -716,27 +716,36 @@ perl () {\n \tcommand \"$PERL_PATH\" \"$@\"\n }\n \n-# Normalize the value given in $1 to one of \"true\", \"false\", or \"auto\". The\n-# result is written to stdout. E.g.:\n+# Given a variable $1, normalize the value of it to one of \"true\",\n+# \"false\", or \"auto\" and store the result to it.\n #\n-# GIT_TEST_HTTPD=$(test_tristate \"$GIT_TEST_HTTPD\")\n+# test_tristate GIT_TEST_HTTPD\n #\n+# A variable set to an empty string is set to 'false'.\n+# A variable set to 'false' or 'auto' keeps its value.\n+# Anything else is set to 'true'.\n+# An unset variable defaults to 'auto'.\n+#\n+# The last rule is to allow people to set the variable to an empty\n+# string and export it to decline testing the particular feature\n+# for versions both before and after this change. We used to treat\n+# both unset and empty variable as a signal for \"do not test\" and\n+# took any non-empty string as \"please test\".\n+\n test_tristate () {\n-\tcase \"$1\" in\n-\t\"\"|auto)\n-\t\techo auto\n-\t\t;;\n-\t*)\n-\t\t# Rely on git-config to handle all the variants of\n-\t\t# true/1/on/etc that we allow.\n-\t\tif ! git -c magic.hack=\"$1\" config --bool magic.hack 2>/dev/null\n-\t\tthen\n-\t\t\t# If git-config failed, it was some non-bool value like\n-\t\t\t# YesPlease. Default this to \"true\" for historical\n-\t\t\t# compatibility.\n-\t\t\techo true\n-\t\tfi\n-\tesac\n+\tif eval \"test x\\\"\\${$1+isset}\\\" = xisset\"\n+\tthen\n+\t\t# explicitly set\n+\t\teval \"\n+\t\t\tcase \\\"\\$$1\\\" in\n+\t\t\t'')\t$1=false ;;\n+\t\t\tfalse | auto) ;;\n+\t\t\t*)\t$1=true ;;\n+\t\t\tesac\n+\t\t\"\n+\telse\n+\t\teval \"$1=auto\"\n+\tfi\n }\n \n # Exit the test suite, either by skipping all remaining tests or by\n"},{"id":"234705","messageId":"CACsJy8Do0CRhhC+YvXSmihczOMDT9UpA4a4mEQ1z5MbR3TSXRw@mail.gmail.com","threadId":"35815","inReplyTo":"20140212221258.GB6799@sigill.intra.peff.net","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-13T01:22:50Z","receivedAt":"2014-02-13T01:22:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 13, 2014 at 5:12 AM, Jeff King wrote:\n> lib-httpd was never designed to be included from anywhere except the\n> beginning of the file. But that wouldn't be right for t5537, because it\n> wants to run some of the tests, even if apache setup fails. The right\n> way to do it is probably to have lib-httpd do all of its work in a lazy\n> prereq. I don't know how clunky that will end up, though; it might be\n> simpler to just move the shallow http test into one of the http-fetch\n> scripts.\n\nI'll move it out later.\n-- \nDuy\n"},{"id":"234726","messageId":"1392297674-4291-1-git-send-email-pclouds@gmail.com","threadId":"35815","inReplyTo":"CACsJy8Do0CRhhC+YvXSmihczOMDT9UpA4a4mEQ1z5MbR3TSXRw@mail.gmail.com","subject":"[PATCH] t5537: move http tests out to t5539","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-13T13:21:14Z","receivedAt":"2014-02-13T13:21:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"start_httpd is supposed to be at the beginning of the test file, not\nthe middle of it. The \"test_seq\" line in \"no shallow lines..\" test is\nupdated to compensate missing refs that are there in t5537, but not in\nthe new t5539.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n On Thu, Feb 13, 2014 at 8:22 AM, Duy Nguyen wrote:\n > On Thu, Feb 13, 2014 at 5:12 AM, Jeff King wrote:\n >> lib-httpd was never designed to be included from anywhere except the\n >> beginning of the file. But that wouldn't be right for t5537, because it\n >> wants to run some of the tests, even if apache setup fails. The right\n >> way to do it is probably to have lib-httpd do all of its work in a lazy\n >> prereq. I don't know how clunky that will end up, though; it might be\n >> simpler to just move the shallow http test into one of the http-fetch\n >> scripts.\n >\n > I'll move it out later.\n \n Here it is, on top of nd/http-fetch-shallow-fix because the new test\n in t5537 is picky and a simple merge resolution wouldn't do it.\n\n t/t5537-fetch-shallow.sh | 57 -----------------------\n t/t5539-fetch-http-shallow.sh (new +x) | 82 ++++++++++++++++++++++++++++++++++\n 2 files changed, 82 insertions(+), 57 deletions(-)\n\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 098f220..3ae9092 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -173,61 +173,4 @@ EOF\n \t)\n '\n \n-if test -n \"$NO_CURL\" -o -z \"$GIT_TEST_HTTPD\"; then\n-\tsay 'skipping remaining tests, git built without http support'\n-\ttest_done\n-fi\n-\n-. \"$TEST_DIRECTORY\"/lib-httpd.sh\n-start_httpd\n-\n-test_expect_success 'clone http repository' '\n-\tgit clone --bare --no-local shallow \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\tgit clone $HTTPD_URL/smart/repo.git clone &&\n-\t(\n-\tcd clone &&\n-\tgit fsck &&\n-\tgit log --format=%s origin/master >actual &&\n-\tcat <expect &&\n-7\n-6\n-5\n-4\n-3\n-EOF\n-\ttest_cmp expect actual\n-\t)\n-'\n-\n-# This test is tricky. We need large enough \"have\"s that fetch-pack\n-# will put pkt-flush in between. Then we need a \"have\" the server\n-# does not have, it'll send \"ACK %s ready\"\n-test_expect_success 'no shallow lines after receiving ACK ready' '\n-\t(\n-\t\tcd shallow &&\n-\t\tfor i in $(test_seq 10)\n-\t\tdo\n-\t\t\tgit checkout --orphan unrelated$i &&\n-\t\t\ttest_commit unrelated$i &&\n-\t\t\tgit push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n-\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i &&\n-\t\t\tgit push -q ../clone/.git \\\n-\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i ||\n-\t\t\texit 1\n-\t\tdone &&\n-\t\tgit checkout master &&\n-\t\ttest_commit new &&\n-\t\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" master\n-\t) &&\n-\t(\n-\t\tcd clone &&\n-\t\tgit checkout --orphan newnew &&\n-\t\ttest_commit new-too &&\n-\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" git fetch --depth=2 &&\n-\t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n-\t\t! grep \"fetch-pack> done\" ../trace\n-\t)\n-'\n-\n-stop_httpd\n test_done\ndiff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\nnew file mode 100755\nindex 0000000..94553e1\n--- /dev/null\n+++ b/t/t5539-fetch-http-shallow.sh\n@@ -0,0 +1,82 @@\n+#!/bin/sh\n+\n+test_description='fetch/clone from a shallow clone over http'\n+\n+. ./test-lib.sh\n+\n+if test -n \"$NO_CURL\"; then\n+\tskip_all='skipping test, git built without http support'\n+\ttest_done\n+fi\n+\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+commit() {\n+\techo \"$1\" >tracked &&\n+\tgit add tracked &&\n+\tgit commit -m \"$1\"\n+}\n+\n+test_expect_success 'setup shallow clone' '\n+\tcommit 1 &&\n+\tcommit 2 &&\n+\tcommit 3 &&\n+\tcommit 4 &&\n+\tcommit 5 &&\n+\tcommit 6 &&\n+\tcommit 7 &&\n+\tgit clone --no-local --depth=5 .git shallow &&\n+\tgit config --global transfer.fsckObjects true\n+'\n+\n+test_expect_success 'clone http repository' '\n+\tgit clone --bare --no-local shallow \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit clone $HTTPD_URL/smart/repo.git clone &&\n+\t(\n+\tcd clone &&\n+\tgit fsck &&\n+\tgit log --format=%s origin/master >actual &&\n+\tcat <expect &&\n+7\n+6\n+5\n+4\n+3\n+EOF\n+\ttest_cmp expect actual\n+\t)\n+'\n+\n+# This test is tricky. We need large enough \"have\"s that fetch-pack\n+# will put pkt-flush in between. Then we need a \"have\" the server\n+# does not have, it'll send \"ACK %s ready\"\n+test_expect_success 'no shallow lines after receiving ACK ready' '\n+\t(\n+\t\tcd shallow &&\n+\t\tfor i in $(test_seq 15)\n+\t\tdo\n+\t\t\tgit checkout --orphan unrelated$i &&\n+\t\t\ttest_commit unrelated$i &&\n+\t\t\tgit push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n+\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i &&\n+\t\t\tgit push -q ../clone/.git \\\n+\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i ||\n+\t\t\texit 1\n+\t\tdone &&\n+\t\tgit checkout master &&\n+\t\ttest_commit new &&\n+\t\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" master\n+\t) &&\n+\t(\n+\t\tcd clone &&\n+\t\tgit checkout --orphan newnew &&\n+\t\ttest_commit new-too &&\n+\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" git fetch --depth=2 &&\n+\t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n+\t\t! grep \"fetch-pack> done\" ../trace\n+\t)\n+'\n+\n+stop_httpd\n+test_done\n-- \n1.8.5.2.240.g8478abd\n"},{"id":"234740","messageId":"xmqqfvnmlsb2.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"xmqq38jom037.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-13T19:35:13Z","receivedAt":"2014-02-13T19:35:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> test_normalize_tristate GIT_TEST_DAEMON\n>\n> Heh, great minds think alike. This is what I am playing with,\n> without committing (because I do like your \"ask config if this is a\n> kind of various boolean 'false' representations, which I haven't\n> managed to add to it).\n\nAnd this is with the \"ask config\" helper.\n\nTwo tangents.\n\n - We may want to do something similar in cvsserver and git-gui to\n make them more robust.\n\n $ git grep -e true --and -e 1 --and -e yes\n\n - Do we want to do something similar to GIT_TEST_CREDENTIAL_HELPER?\n\n-- >8 --\nFrom: Jeff King \nDate: Mon, 10 Feb 2014 16:29:37 -0500\nSubject: [PATCH] tests: turn on network daemon tests by default\n\nWe do not run the httpd nor git-daemon tests by default, as\nthey are rather heavyweight and require network access\n(albeit over localhost). However, it would be nice if more\npepole ran them, for two reasons:\n\n 1. We would get more test coverage on more systems.\n\n 2. The point of the test suite is to find regressions. It\n is very easy to change some of the underlying code and\n break the httpd code without realizing you are even\n affecting it. Running the httpd tests helps find these\n problems sooner (ideally before the patches even hit\n the list).\n\nWe still want to leave an \"out\", though, for people who really do\nnot want to run them. For that reason, the GIT_TEST_HTTPD and\nGIT_TEST_GIT_DAEMON variables are now tri-state booleans\n(true/false/auto), so you can say GIT_TEST_HTTPD=false to turn the\ntests back off. To support those who want a stable single way to\ndisable these tests across versions of Git before and after this\nchange, an empty string explicitly set to these variables is also\ntaken as \"false\", so the behaviour changes only for those who:\n\n a. did not express any preference by leaving these variables\n unset. They did not test these features before, but now they\n do; or\n\n b. did express that they want to test these features by setting\n GIT_TEST_FEATURE=false (or any equivalent other ways to tell\n \"false\" to Git, e.g. \"0\"), which has been a valid but funny way\n to say that they do want to test the feature only because we\n used to interpret any non-empty string to mean \"yes please\n test\". They no longer test that feature.\n\nIn addition, we are forgiving of common setup failures (e.g., you do\nnot have apache installed, or have an old version) when the\ntri-state is \"auto\" (or empty), but report an error when it is\n\"true\". This makes \"auto\" a sane default, as we should not cause\nfailures on setups where the tests cannot run. But it allows people\nwho use \"true\" to catch regressions in their system (e.g., they\nuninstalled apache, but were expecting their automated test runs to\ntest git-httpd, and would want to be notified).\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n t/lib-git-daemon.sh | 8 ++++---\n t/lib-httpd.sh | 22 +++++++++----------\n t/test-lib-functions.sh | 58 +++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 74 insertions(+), 14 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 394b06b..615bf5d 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -16,9 +16,10 @@\n #\tstop_git_daemon\n #\ttest_done\n \n-if test -z \"$GIT_TEST_GIT_DAEMON\"\n+test_tristate GIT_TEST_GIT_DAEMON\n+if test \"$GIT_TEST_GIT_DAEMON\" = false\n then\n-\tskip_all=\"git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)\"\n+\tskip_all=\"git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)\"\n \ttest_done\n fi\n \n@@ -58,7 +59,8 @@ start_git_daemon() {\n \t\tkill \"$GIT_DAEMON_PID\"\n \t\twait \"$GIT_DAEMON_PID\"\n \t\ttrap 'die' EXIT\n-\t\terror \"git daemon failed to start\"\n+\t\ttest_skip_or_die $GIT_TEST_GIT_DAEMON \\\n+\t\t\t\"git daemon failed to start\"\n \tfi\n }\n \ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex bfdff2a..f9c2e22 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -30,9 +30,10 @@\n # Copyright (c) 2008 Clemens Buchacher \n #\n \n-if test -z \"$GIT_TEST_HTTPD\"\n+test_tristate GIT_TEST_HTTPD\n+if test \"$GIT_TEST_HTTPD\" = false\n then\n-\tskip_all=\"Network testing disabled (define GIT_TEST_HTTPD to enable)\"\n+\tskip_all=\"Network testing disabled (unset GIT_TEST_HTTPD to enable)\"\n \ttest_done\n fi\n \n@@ -76,8 +77,7 @@ GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export GIT_VALGRIND_OPTIONS\n \n if ! test -x \"$LIB_HTTPD_PATH\"\n then\n-\tskip_all=\"skipping test, no web server found at '$LIB_HTTPD_PATH'\"\n-\ttest_done\n+\ttest_skip_or_die $GIT_TEST_HTTPD \"no web server found at '$LIB_HTTPD_PATH'\"\n fi\n \n HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \\\n@@ -89,19 +89,20 @@ then\n \tthen\n \t\tif ! test $HTTPD_VERSION -ge 2\n \t\tthen\n-\t\t\tskip_all=\"skipping test, at least Apache version 2 is required\"\n-\t\t\ttest_done\n+\t\t\ttest_skip_or_die $GIT_TEST_HTTPD \\\n+\t\t\t\t\"at least Apache version 2 is required\"\n \t\tfi\n \t\tif ! test -d \"$DEFAULT_HTTPD_MODULE_PATH\"\n \t\tthen\n-\t\t\tskip_all=\"Apache module directory not found. Skipping tests.\"\n-\t\t\ttest_done\n+\t\t\ttest_skip_or_die $GIT_TEST_HTTPD \\\n+\t\t\t\t\"Apache module directory not found\"\n \t\tfi\n \n \t\tLIB_HTTPD_MODULE_PATH=\"$DEFAULT_HTTPD_MODULE_PATH\"\n \tfi\n else\n-\terror \"Could not identify web server at '$LIB_HTTPD_PATH'\"\n+\ttest_skip_or_die $GIT_TEST_HTTPD \\\n+\t\t\"Could not identify web server at '$LIB_HTTPD_PATH'\"\n fi\n \n prepare_httpd() {\n@@ -155,9 +156,8 @@ start_httpd() {\n \t\t>&3 2>&4\n \tif test $? -ne 0\n \tthen\n-\t\tskip_all=\"skipping test, web server setup failed\"\n \t\ttrap 'die' EXIT\n-\t\ttest_done\n+\t\ttest_skip_or_die $GIT_TEST_HTTPD \"web server setup failed\"\n \tfi\n }\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex aeae3ca..b333e3f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -716,6 +716,64 @@ perl () {\n \tcommand \"$PERL_PATH\" \"$@\"\n }\n \n+# Is the value one of the various ways to spell a boolean true/false?\n+test_normalize_bool () {\n+\tgit -c magic.variable=\"$1\" config --bool magic.variable 2>/dev/null\n+}\n+\n+# Given a variable $1, normalize the value of it to one of \"true\",\n+# \"false\", or \"auto\" and store the result to it.\n+#\n+# test_tristate GIT_TEST_HTTPD\n+#\n+# A variable set to an empty string is set to 'false'.\n+# A variable set to 'false' or 'auto' keeps its value.\n+# Anything else is set to 'true'.\n+# An unset variable defaults to 'auto'.\n+#\n+# The last rule is to allow people to set the variable to an empty\n+# string and export it to decline testing the particular feature\n+# for versions both before and after this change. We used to treat\n+# both unset and empty variable as a signal for \"do not test\" and\n+# took any non-empty string as \"please test\".\n+\n+test_tristate () {\n+\tif eval \"test x\\\"\\${$1+isset}\\\" = xisset\"\n+\tthen\n+\t\t# explicitly set\n+\t\teval \"\n+\t\t\tcase \\\"\\$$1\\\" in\n+\t\t\t'')\t$1=false ;;\n+\t\t\tauto)\t;;\n+\t\t\t*)\t$1=\\$(test_normalize_bool \\$$1 || echo true) ;;\n+\t\t\tesac\n+\t\t\"\n+\telse\n+\t\teval \"$1=auto\"\n+\tfi\n+}\n+\n+# Exit the test suite, either by skipping all remaining tests or by\n+# exiting with an error. If \"$1\" is \"auto\", we then we assume we were\n+# opportunistically trying to set up some tests and we skip. If it is\n+# \"true\", then we report a failure.\n+#\n+# The error/skip message should be given by $2.\n+#\n+test_skip_or_die () {\n+\tcase \"$1\" in\n+\tauto)\n+\t\tskip_all=$2\n+\t\ttest_done\n+\t\t;;\n+\ttrue)\n+\t\terror \"$2\"\n+\t\t;;\n+\t*)\n+\t\terror \"BUG: test tristate is '$1' (real error: $2)\"\n+\tesac\n+}\n+\n # The following mingw_* functions obey POSIX shell syntax, but are actually\n # bash scripts, and are meant to be used only with bash on Windows.\n \n-- \n1.9.0-rc3-260-g4cf525c\n"},{"id":"234744","messageId":"xmqqy51ekbws.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"1392297674-4291-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] t5537: move http tests out to t5539","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-13T20:14:43Z","receivedAt":"2014-02-13T20:14:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> start_httpd is supposed to be at the beginning of the test file, not\n> the middle of it. The \"test_seq\" line in \"no shallow lines..\" test is\n> updated to compensate missing refs that are there in t5537, but not in\n> the new t5539.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> On Thu, Feb 13, 2014 at 8:22 AM, Duy Nguyen wrote:\n> > On Thu, Feb 13, 2014 at 5:12 AM, Jeff King wrote:\n> >> lib-httpd was never designed to be included from anywhere except the\n> >> beginning of the file. But that wouldn't be right for t5537, because it\n> >> wants to run some of the tests, even if apache setup fails. The right\n> >> way to do it is probably to have lib-httpd do all of its work in a lazy\n> >> prereq. I don't know how clunky that will end up, though; it might be\n> >> simpler to just move the shallow http test into one of the http-fetch\n> >> scripts.\n> >\n> > I'll move it out later.\n> \n> Here it is, on top of nd/http-fetch-shallow-fix because the new test\n> in t5537 is picky and a simple merge resolution wouldn't do it.\n\nWill queue; thanks.\n\n>\n> t/t5537-fetch-shallow.sh | 57 -----------------------\n> t/t5539-fetch-http-shallow.sh (new +x) | 82 ++++++++++++++++++++++++++++++++++\n> 2 files changed, 82 insertions(+), 57 deletions(-)\n>\n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index 098f220..3ae9092 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -173,61 +173,4 @@ EOF\n> \t)\n> '\n> \n> -if test -n \"$NO_CURL\" -o -z \"$GIT_TEST_HTTPD\"; then\n> -\tsay 'skipping remaining tests, git built without http support'\n> -\ttest_done\n> -fi\n> -\n> -. \"$TEST_DIRECTORY\"/lib-httpd.sh\n> -start_httpd\n> -\n> -test_expect_success 'clone http repository' '\n> -\tgit clone --bare --no-local shallow \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> -\tgit clone $HTTPD_URL/smart/repo.git clone &&\n> -\t(\n> -\tcd clone &&\n> -\tgit fsck &&\n> -\tgit log --format=%s origin/master >actual &&\n> -\tcat <expect &&\n> -7\n> -6\n> -5\n> -4\n> -3\n> -EOF\n> -\ttest_cmp expect actual\n> -\t)\n> -'\n> -\n> -# This test is tricky. We need large enough \"have\"s that fetch-pack\n> -# will put pkt-flush in between. Then we need a \"have\" the server\n> -# does not have, it'll send \"ACK %s ready\"\n> -test_expect_success 'no shallow lines after receiving ACK ready' '\n> -\t(\n> -\t\tcd shallow &&\n> -\t\tfor i in $(test_seq 10)\n> -\t\tdo\n> -\t\t\tgit checkout --orphan unrelated$i &&\n> -\t\t\ttest_commit unrelated$i &&\n> -\t\t\tgit push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n> -\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i &&\n> -\t\t\tgit push -q ../clone/.git \\\n> -\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i ||\n> -\t\t\texit 1\n> -\t\tdone &&\n> -\t\tgit checkout master &&\n> -\t\ttest_commit new &&\n> -\t\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" master\n> -\t) &&\n> -\t(\n> -\t\tcd clone &&\n> -\t\tgit checkout --orphan newnew &&\n> -\t\ttest_commit new-too &&\n> -\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" git fetch --depth=2 &&\n> -\t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n> -\t\t! grep \"fetch-pack> done\" ../trace\n> -\t)\n> -'\n> -\n> -stop_httpd\n> test_done\n> diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\n> new file mode 100755\n> index 0000000..94553e1\n> --- /dev/null\n> +++ b/t/t5539-fetch-http-shallow.sh\n> @@ -0,0 +1,82 @@\n> +#!/bin/sh\n> +\n> +test_description='fetch/clone from a shallow clone over http'\n> +\n> +. ./test-lib.sh\n> +\n> +if test -n \"$NO_CURL\"; then\n> +\tskip_all='skipping test, git built without http support'\n> +\ttest_done\n> +fi\n> +\n> +. \"$TEST_DIRECTORY\"/lib-httpd.sh\n> +start_httpd\n> +\n> +commit() {\n> +\techo \"$1\" >tracked &&\n> +\tgit add tracked &&\n> +\tgit commit -m \"$1\"\n> +}\n> +\n> +test_expect_success 'setup shallow clone' '\n> +\tcommit 1 &&\n> +\tcommit 2 &&\n> +\tcommit 3 &&\n> +\tcommit 4 &&\n> +\tcommit 5 &&\n> +\tcommit 6 &&\n> +\tcommit 7 &&\n> +\tgit clone --no-local --depth=5 .git shallow &&\n> +\tgit config --global transfer.fsckObjects true\n> +'\n> +\n> +test_expect_success 'clone http repository' '\n> +\tgit clone --bare --no-local shallow \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> +\tgit clone $HTTPD_URL/smart/repo.git clone &&\n> +\t(\n> +\tcd clone &&\n> +\tgit fsck &&\n> +\tgit log --format=%s origin/master >actual &&\n> +\tcat <expect &&\n> +7\n> +6\n> +5\n> +4\n> +3\n> +EOF\n> +\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> +# This test is tricky. We need large enough \"have\"s that fetch-pack\n> +# will put pkt-flush in between. Then we need a \"have\" the server\n> +# does not have, it'll send \"ACK %s ready\"\n> +test_expect_success 'no shallow lines after receiving ACK ready' '\n> +\t(\n> +\t\tcd shallow &&\n> +\t\tfor i in $(test_seq 15)\n> +\t\tdo\n> +\t\t\tgit checkout --orphan unrelated$i &&\n> +\t\t\ttest_commit unrelated$i &&\n> +\t\t\tgit push -q \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n> +\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i &&\n> +\t\t\tgit push -q ../clone/.git \\\n> +\t\t\t\trefs/heads/unrelated$i:refs/heads/unrelated$i ||\n> +\t\t\texit 1\n> +\t\tdone &&\n> +\t\tgit checkout master &&\n> +\t\ttest_commit new &&\n> +\t\tgit push \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" master\n> +\t) &&\n> +\t(\n> +\t\tcd clone &&\n> +\t\tgit checkout --orphan newnew &&\n> +\t\ttest_commit new-too &&\n> +\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" git fetch --depth=2 &&\n> +\t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n> +\t\t! grep \"fetch-pack> done\" ../trace\n> +\t)\n> +'\n> +\n> +stop_httpd\n> +test_done\n"},{"id":"234760","messageId":"20140214095841.GA27161@sigill.intra.peff.net","threadId":"35815","inReplyTo":"xmqqfvnmlsb2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-14T09:58:42Z","receivedAt":"2014-02-14T09:58:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 13, 2014 at 11:35:13AM -0800, Junio C Hamano wrote:\n\n> >> test_normalize_tristate GIT_TEST_DAEMON\n> >\n> > Heh, great minds think alike. This is what I am playing with,\n> > without committing (because I do like your \"ask config if this is a\n> > kind of various boolean 'false' representations, which I haven't\n> > managed to add to it).\n> \n> And this is with the \"ask config\" helper.\n\nThanks for picking this up.\n\n> Two tangents.\n> \n> - We may want to do something similar in cvsserver and git-gui to\n> make them more robust.\n> \n> $ git grep -e true --and -e 1 --and -e yes\n\nI assume the \"something\" here is to respect bool options more\nconsistently? I have no problem with that, but nor do I care too much\nabout those programs (that is partially laziness, but also partially\nthat I do not want to deal with introducing a regression).\n\n> - Do we want to do something similar to GIT_TEST_CREDENTIAL_HELPER?\n\nNo, it is not a boolean. It is a bit of a hack, but it is meant to be\nused like:\n\n GIT_TEST_CREDENTIAL_HELPER=foo ./t0303-*\n\nto test some random git-credential-foo you have in your PATH. There is\nnothing to run \"by default\" there. It would be sensible to hook\ncontrib/credential to it, though.\n\n> -- >8 --\n> From: Jeff King \n> Date: Mon, 10 Feb 2014 16:29:37 -0500\n> Subject: [PATCH] tests: turn on network daemon tests by default\n> [...]\n> In addition, we are forgiving of common setup failures (e.g., you do\n> not have apache installed, or have an old version) when the\n> tri-state is \"auto\" (or empty), but report an error when it is\n\nYou probably want to drop this \"or empty\" or change it to \"or unset\",\ngiven the magic we do with empty-but-set variables in this version.\n\n> ---\n> t/lib-git-daemon.sh | 8 ++++---\n> t/lib-httpd.sh | 22 +++++++++----------\n> t/test-lib-functions.sh | 58 +++++++++++++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 74 insertions(+), 14 deletions(-)\n\nPatch looks good to me.\n\n-Peff\n"},{"id":"234772","messageId":"xmqqbny9k6zf.fsf@gitster.dls.corp.google.com","threadId":"35815","inReplyTo":"20140214095841.GA27161@sigill.intra.peff.net","subject":"Re: [PATCH] tests: turn on network daemon tests by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-14T16:13:24Z","receivedAt":"2014-02-14T16:13:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> - We may want to do something similar in cvsserver and git-gui to\n>> make them more robust.\n>> \n>> $ git grep -e true --and -e 1 --and -e yes\n>\n> I assume the \"something\" here is to respect bool options more\n> consistently?\n\nYeah, mostly by employing your 'git -c magic.var=X config --bool'\ntrick and check only for 'false' and 'true', instead of keeping a\nhard-coded logic like the lines that hit the above query do.\n\n> I have no problem with that, but nor do I care too much\n> about those programs (that is partially laziness, but also partially\n> that I do not want to deal with introducing a regression).\n\nTrue, too ;-)\n\n>> - Do we want to do something similar to GIT_TEST_CREDENTIAL_HELPER?\n>\n> No, it is not a boolean. It is a bit of a hack, but it is meant to be\n> used like:\n>\n> GIT_TEST_CREDENTIAL_HELPER=foo ./t0303-*\n>\n> to test some random git-credential-foo you have in your PATH. There is\n> nothing to run \"by default\" there.\n\nAh, OK. I was only grepping for \"test -z .*GIT_TEST_\".\n>> tri-state is \"auto\" (or empty), but report an error when it is\n>\n> You probably want to drop this \"or empty\" or change it to \"or unset\",\n\nThanks, I totally missed that.\n"}]}