{"thread":{"id":"52513","subject":"[PATCH 0/5] Enable protocol v2 by default","startedAt":"2019-12-24T00:58:20Z","lastAt":"2019-12-26T23:12:56Z","messageCount":17,"participants":["Jonathan Nieder","Derrick Stolee","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"388836","messageId":"20191224005816.GC38316@google.com","threadId":"52513","inReplyTo":null,"subject":"[PATCH 0/5] Enable protocol v2 by default","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-24T00:58:16Z","receivedAt":"2019-12-24T00:58:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThe Git users at $DAYJOB have been using protocol v2 as a default for\n~1.5 years now and others have been also reporting good experiences\nwith it, so it seems like a good time to propose bumping the default\nversion.  It produces a significant performance improvement when\nfetching from repositories with many refs, such as\nhttps://chromium.googlesource.com/chromium/src.\n\nThis only affects the client, not the server.  (The server already\ndefaults to supporting protocol v2.)\n\nThis could go in 2.25 (most of the \"next\" population is likely already\nusing protocol.version=2, so the -rc period would be one of the better\nways to expand the user population using this) or could cook in \"next\"\nfor a cycle.  Either is fine by me.\n\nThoughts of all kinds welcome, as always.\n\nJonathan Nieder (5):\n  fetch test: use more robust test for filtered objects\n  config doc: protocol.version is not experimental\n  test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate\n  protocol test: let protocol.version override GIT_TEST_PROTOCOL_VERSION\n  fetch: default to protocol version 2\n\n Documentation/config/protocol.txt    |  9 ++++-----\n protocol.c                           | 11 +++++------\n t/README                             |  4 ++--\n t/t5400-send-pack.sh                 |  2 +-\n t/t5500-fetch-pack.sh                | 23 ++++++++++++++++-------\n t/t5512-ls-remote.sh                 | 10 +++++-----\n t/t5515-fetch-merge-logic.sh         |  3 ++-\n t/t5516-fetch-push.sh                | 12 ++++++------\n t/t5539-fetch-http-shallow.sh        |  2 +-\n t/t5541-http-push-smart.sh           |  4 ++--\n t/t5551-http-fetch-smart.sh          | 12 ++++++------\n t/t5552-skipping-fetch-negotiator.sh |  2 +-\n t/t5700-protocol-v1.sh               |  3 ++-\n t/t7406-submodule-update.sh          |  2 +-\n 14 files changed, 54 insertions(+), 45 deletions(-)\n\nbase-commit: 12029dc57db23baef008e77db1909367599210ee\n"},{"id":"388837","messageId":"20191224005907.GD38316@google.com","threadId":"52513","inReplyTo":"20191224005816.GC38316@google.com","subject":"[PATCH 1/5] fetch test: use more robust test for filtered objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-24T00:59:07Z","receivedAt":"2019-12-24T00:59:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"\"git cat-file -e\" uses has_object_file, which can fetch from promisor\nremotes when an object is missing.  These tests end up checking that\nthat fetch fails instead of for the object being missing.\n\nBy luck, the tests pass anyway:\n\n- in one of these tests (\"filtering by size\"), the fetch fails because\n  (in protocol v0) the server does not support fetches by SHA-1\n\n- in the second, the object is present but the test could pass even if\n  it weren't if the fetch succeeds\n\n- in the third, the test sets extensions.partialClone to \"arbitrary\n  string\" so that when it tries to fetch, it looks up the \"arbitrary\n  string\" remote which does not exist\n\nUse \"git rev-list --objects --missing=allow-any\", so that the tests\npass for the right reason.\n\nNoticed while testing with protocol v2, which allows fetching by sha1\nby default, causing the first fetch to succeed and the test to fail.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n t/t5500-fetch-pack.sh | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 6b97923964..964930b2d2 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -917,7 +917,10 @@ test_expect_success 'filtering by size' '\n \tgit -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&\n \n \t# Ensure that object is not inadvertently fetched\n-\ttest_must_fail git -C client cat-file -e $(git hash-object server/one.t)\n+\tcommit=$(git -C server rev-parse HEAD) &&\n+\tblob=$(git hash-object server/one.t) &&\n+\tgit -C client rev-list --objects --missing=allow-any \"$commit\" >oids &&\n+\t! grep \"$blob\" oids\n '\n \n test_expect_success 'filtering by size has no effect if support for it is not advertised' '\n@@ -929,7 +932,10 @@ test_expect_success 'filtering by size has no effect if support for it is not ad\n \tgit -C client fetch-pack --filter=blob:limit=0 ../server HEAD 2> err &&\n \n \t# Ensure that object is fetched\n-\tgit -C client cat-file -e $(git hash-object server/one.t) &&\n+\tcommit=$(git -C server rev-parse HEAD) &&\n+\tblob=$(git hash-object server/one.t) &&\n+\tgit -C client rev-list --objects --missing=allow-any \"$commit\" >oids &&\n+\tgrep \"$blob\" oids &&\n \n \ttest_i18ngrep \"filtering not recognized by server\" err\n '\n@@ -951,9 +957,11 @@ fetch_filter_blob_limit_zero () {\n \tgit -C client fetch --filter=blob:limit=0 origin HEAD:somewhere &&\n \n \t# Ensure that commit is fetched, but blob is not\n-\ttest_config -C client extensions.partialclone \"arbitrary string\" &&\n-\tgit -C client cat-file -e $(git -C \"$SERVER\" rev-parse two) &&\n-\ttest_must_fail git -C client cat-file -e $(git hash-object \"$SERVER/two.t\")\n+\tcommit=$(git -C \"$SERVER\" rev-parse two) &&\n+\tblob=$(git hash-object server/two.t) &&\n+\tgit -C client rev-list --objects --missing=allow-any \"$commit\" >oids &&\n+\tgrep \"$commit\" oids &&\n+\t! grep \"$blob\" oids\n }\n \n test_expect_success 'fetch with --filter=blob:limit=0' '\n-- \n2.24.1.735.g03f4e72817\n\n"},{"id":"388838","messageId":"20191224010000.GE38316@google.com","threadId":"52513","inReplyTo":"20191224005816.GC38316@google.com","subject":"[PATCH 2/5] config doc: protocol.version is not experimental","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-24T01:00:00Z","receivedAt":"2019-12-24T01:00:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Git's protocol version 2 has been working well in production for over\na year.  Simplify documentation by no longer referring to it as\nexperimental.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n Documentation/config/protocol.txt | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config/protocol.txt b/Documentation/config/protocol.txt\nindex bfccc07491..0b40141613 100644\n--- a/Documentation/config/protocol.txt\n+++ b/Documentation/config/protocol.txt\n@@ -45,11 +45,10 @@ The protocol names currently used by git are:\n --\n \n protocol.version::\n-\tExperimental. If set, clients will attempt to communicate with a\n-\tserver using the specified protocol version.  If unset, no\n-\tattempt will be made by the client to communicate using a\n-\tparticular protocol version, this results in protocol version 0\n-\tbeing used.\n+\tIf set, clients will attempt to communicate with a server\n+\tusing the specified protocol version.  If the server does\n+\tnot support it, communication falls back to version 0.\n+\tIf unset, the default is `0`.\n \tSupported versions:\n +\n --\n-- \n2.24.1.735.g03f4e72817\n\n"},{"id":"388839","messageId":"20191224010110.GF38316@google.com","threadId":"52513","inReplyTo":"20191224005816.GC38316@google.com","subject":"[PATCH 3/5] test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-24T01:01:10Z","receivedAt":"2019-12-24T01:01:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Since 8cbeba0632 (tests: define GIT_TEST_PROTOCOL_VERSION,\n2019-02-25), it has been possible to run tests with a newer protocol\nversion by setting the GIT_TEST_PROTOCOL_VERSION envvar to a version\nnumber.  Tests that assume protocol v0 handle this by explicitly\nsetting\n\n\tGIT_TEST_PROTOCOL_VERSION=\n\nor similar constructs like 'test -z \"$GIT_TEST_PROTOCOL_VERSION\" ||\nreturn 0' to declare that they only handle the default (v0) protocol.\n\nThe emphasis there is a bit off: it would be clearer to specify\nGIT_TEST_PROTOCOL_VERSION=0 to inform the reader that these tests are\nspecifically testing and relying on details of protocol v0.  Do so.\n\nThis way, a reader does not need to know what the default protocol\nversion is, and the tests can continue to work when the default\nprotocol version used by Git advances past v0.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n t/t5400-send-pack.sh                 |  2 +-\n t/t5500-fetch-pack.sh                |  5 +++--\n t/t5512-ls-remote.sh                 | 10 +++++-----\n t/t5515-fetch-merge-logic.sh         |  3 ++-\n t/t5516-fetch-push.sh                | 12 ++++++------\n t/t5539-fetch-http-shallow.sh        |  2 +-\n t/t5541-http-push-smart.sh           |  4 ++--\n t/t5551-http-fetch-smart.sh          | 12 ++++++------\n t/t5552-skipping-fetch-negotiator.sh |  2 +-\n t/t5700-protocol-v1.sh               |  3 ++-\n t/t7406-submodule-update.sh          |  2 +-\n 11 files changed, 30 insertions(+), 27 deletions(-)\n\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 571d620aed..b84618c925 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' '\n \t$shared .have\n \tEOF\n \n-\tGIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \\\n+\tGIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION=0 \\\n \t    git push \\\n \t\t--receive-pack=\"unset GIT_TRACE_PACKET; git-receive-pack\" \\\n \t\tfork HEAD:foo &&\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 964930b2d2..baa1a99f45 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -440,11 +440,12 @@ test_expect_success 'setup tests for the --stdin parameter' '\n '\n \n test_expect_success 'setup fetch refs from cmdline v[12]' '\n+\tcp -r client client0 &&\n \tcp -r client client1 &&\n \tcp -r client client2\n '\n \n-for version in '' 1 2\n+for version in '' 0 1 2\n do\n \ttest_expect_success \"protocol.version=$version fetch refs from cmdline\" \"\n \t\t(\n@@ -638,7 +639,7 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a\n \tgit init client &&\n \t# Some protocol versions (e.g. 2) support fetching\n \t# unadvertised objects, so restrict this test to v0.\n-\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \\\n+\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -C client fetch-pack ../server \\\n \t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n \ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n '\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex d7b9f9078f..459d1fcddf 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -225,7 +225,7 @@ test_expect_success 'ls-remote --symref' '\n \tEOF\n \t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n \t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -236,7 +236,7 @@ test_expect_success 'ls-remote with filtered symref (refname)' '\n \tEOF\n \t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n \t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . HEAD >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -249,7 +249,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '\n \tEOF\n \t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n \t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -260,9 +260,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \tEOF\n \t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n \t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&\n \ttest_cmp expect actual &&\n-\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . \"refs/heads/*\" >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . \"refs/heads/*\" >actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh\nindex 961eb35c99..a9a7d50d3e 100755\n--- a/t/t5515-fetch-merge-logic.sh\n+++ b/t/t5515-fetch-merge-logic.sh\n@@ -8,7 +8,8 @@ test_description='Merge logic in fetch'\n \n # NEEDSWORK: If the overspecification of the expected result is reduced, we\n # might be able to run this test in all protocol versions.\n-GIT_TEST_PROTOCOL_VERSION=\n+GIT_TEST_PROTOCOL_VERSION=0\n+export GIT_TEST_PROTOCOL_VERSION\n \n . ./test-lib.sh\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex c81ca360ac..f12cbef097 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1151,7 +1151,7 @@ test_expect_success 'fetch exact SHA1' '\n \t\t# unadvertised objects, so restrict this test to v0.\n \n \t\t# fetching the hidden object should fail by default\n-\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\t\tgit fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&\n \t\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err &&\n \t\ttest_must_fail git rev-parse --verify refs/heads/copy &&\n@@ -1210,7 +1210,7 @@ do\n \t\t\tcd shallow &&\n \t\t\t# Some protocol versions (e.g. 2) support fetching\n \t\t\t# unadvertised objects, so restrict this test to v0.\n-\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\t\t\tgit fetch --depth=1 ../testrepo/.git $SHA1 &&\n \t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n \t\t\tgit fetch --depth=1 ../testrepo/.git $SHA1 &&\n@@ -1241,9 +1241,9 @@ do\n \t\t\tcd shallow &&\n \t\t\t# Some protocol versions (e.g. 2) support fetching\n \t\t\t# unadvertised objects, so restrict this test to v0.\n-\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\t\t\tgit fetch ../testrepo/.git $SHA1_3 &&\n-\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\t\t\tgit fetch ../testrepo/.git $SHA1_1 &&\n \t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_1 &&\n@@ -1251,7 +1251,7 @@ do\n \t\t\ttest_must_fail git cat-file commit $SHA1_2 &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_2 &&\n \t\t\tgit cat-file commit $SHA1_2 &&\n-\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\t\t\tgit fetch ../testrepo/.git $SHA1_3 2>err &&\n \t\t\ttest_i18ngrep \"remote error:.*not our ref.*$SHA1_3\\$\" err\n \t\t)\n@@ -1291,7 +1291,7 @@ test_expect_success 'peeled advertisements are not considered ref tips' '\n \tgit -C testrepo commit --allow-empty -m two &&\n \tgit -C testrepo tag -m foo mytag HEAD^ &&\n \toid=$(git -C testrepo rev-parse mytag^{commit}) &&\n-\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\tgit fetch testrepo $oid 2>err &&\n \ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n '\ndiff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\nindex b4ad81f006..c0d02dee89 100755\n--- a/t/t5539-fetch-http-shallow.sh\n+++ b/t/t5539-fetch-http-shallow.sh\n@@ -69,7 +69,7 @@ test_expect_success 'no shallow lines after receiving ACK ready' '\n \t\ttest_commit new-too &&\n \t\t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n \t\t# might be able to run this test in all protocol versions.\n-\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" GIT_TEST_PROTOCOL_VERSION= \\\n+\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\t\tgit fetch --depth=2 &&\n \t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n \t\t! grep \"fetch-pack> done\" ../trace\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 4c970787b0..23be8ce92d 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -49,7 +49,7 @@ test_expect_success 'no empty path components' '\n \n \t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n \t# might be able to run this test in all protocol versions.\n-\tif test -z \"$GIT_TEST_PROTOCOL_VERSION\"\n+\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n \tthen\n \t\tcheck_access_log exp\n \tfi\n@@ -135,7 +135,7 @@ EOF\n test_expect_success 'used receive-pack service' '\n \t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n \t# might be able to run this test in all protocol versions.\n-\tif test -z \"$GIT_TEST_PROTOCOL_VERSION\"\n+\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n \tthen\n \t\tcheck_access_log exp\n \tfi\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex e38e543867..6788aeface 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -43,7 +43,7 @@ test_expect_success 'clone http repository' '\n \t< Cache-Control: no-cache, max-age=0, must-revalidate\n \t< Content-Type: application/x-git-upload-pack-result\n \tEOF\n-\tGIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \\\n+\tGIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\tgit clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n \ttest_cmp file clone/file &&\n \ttr '\\''\\015'\\'' Q <err |\n@@ -84,7 +84,7 @@ test_expect_success 'clone http repository' '\n \n \t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n \t# might be able to run this test in all protocol versions.\n-\tif test -z \"$GIT_TEST_PROTOCOL_VERSION\"\n+\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n \tthen\n \t\tsed -e \"s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/\" \\\n \t\t\t\tactual >actual.smudged &&\n@@ -113,7 +113,7 @@ test_expect_success 'used upload-pack service' '\n \n \t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n \t# might be able to run this test in all protocol versions.\n-\tif test -z \"$GIT_TEST_PROTOCOL_VERSION\"\n+\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n \tthen\n \t\tcheck_access_log exp\n \tfi\n@@ -241,7 +241,7 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set\n \n \t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n \t# might be able to run this test in all protocol versions.\n-\tif test -z \"$GIT_TEST_PROTOCOL_VERSION\"\n+\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n \tthen\n \t\ttail -3 cookies.txt | sort >cookies_tail.txt &&\n \t\ttest_cmp expect_cookies.txt cookies_tail.txt\n@@ -336,7 +336,7 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n \t# Some protocol versions (e.g. 2) support fetching\n \t# unadvertised objects, so restrict this test to v0.\n-\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\n '\n \n@@ -358,7 +358,7 @@ test_expect_success 'test allowanysha1inwant with unreachable' '\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n \t# Some protocol versions (e.g. 2) support fetching\n \t# unadvertised objects, so restrict this test to v0.\n-\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\" &&\n \n \tgit -C \"$server\" config uploadpack.allowanysha1inwant 1 &&\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex f70cbcc9ca..a2a5e0743f 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -107,7 +107,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n \n \t# The ref advertisement itself is filtered when protocol v2 is used, so\n \t# use v0.\n-\tGIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 trace_fetch client origin to_fetch &&\n \thave_sent c5 c4^ c2side &&\n \thave_not_sent c4 c4^^ c4^^^\n '\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 2571eb90b7..022901b9eb 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -5,7 +5,8 @@ test_description='test git wire-protocol transition'\n TEST_NO_CREATE_REPO=1\n \n # This is a protocol-specific test.\n-GIT_TEST_PROTOCOL_VERSION=\n+GIT_TEST_PROTOCOL_VERSION=0\n+export GIT_TEST_PROTOCOL_VERSION\n \n . ./test-lib.sh\n \ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 7478f7ab7e..4fb447a143 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -960,7 +960,7 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n \t\tmv -f .gitmodules.tmp .gitmodules &&\n \t\t# Some protocol versions (e.g. 2) support fetching\n \t\t# unadvertised objects, so restrict this test to v0.\n-\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n \t\t\tgit submodule update --init --depth=1 2>actual &&\n \t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n \t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n-- \n2.24.1.735.g03f4e72817\n\n"},{"id":"388840","messageId":"20191224010228.GG38316@google.com","threadId":"52513","inReplyTo":"20191224005816.GC38316@google.com","subject":"[PATCH 4/5] protocol test: let protocol.version override GIT_TEST_PROTOCOL_VERSION","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-24T01:02:28Z","receivedAt":"2019-12-24T01:02:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The GIT_TEST_PROTOCOL_VERSION environment variable can be used to\nupgrade the version of Git protocol used in tests.  If both\nGIT_TEST_PROTOCOL_VERSION and 'protocol.version' are set, the higher\nvalue wins.\n\nFor usage within tests, these semantics are too complex.  Instead,\nalways use the value from protocol.version configuration when it is\nset, falling back to GIT_TEST_PROTOCOL_VERSION.  This way, the envvar\nprovides a reliable preview of what will happen if the default\nprotocol version is changed.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nI'd like to remove the built-in support for GIT_TEST_PROTOCOL_VERSION\naltogether and replace it with support in the test harness for setting\nprotocol.version to the specified value, but that can wait for a\nfollowup another day.\n\n protocol.c | 11 +++++------\n t/README   |  4 ++--\n 2 files changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/protocol.c b/protocol.c\nindex 9741f05750..d390391eba 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -17,9 +17,8 @@ static enum protocol_version parse_protocol_version(const char *value)\n enum protocol_version get_protocol_version_config(void)\n {\n \tconst char *value;\n-\tenum protocol_version retval = protocol_v0;\n \tconst char *git_test_k = \"GIT_TEST_PROTOCOL_VERSION\";\n-\tconst char *git_test_v = getenv(git_test_k);\n+\tconst char *git_test_v;\n \n \tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n \t\tenum protocol_version version = parse_protocol_version(value);\n@@ -28,19 +27,19 @@ enum protocol_version get_protocol_version_config(void)\n \t\t\tdie(\"unknown value for config 'protocol.version': %s\",\n \t\t\t    value);\n \n-\t\tretval = version;\n+\t\treturn version;\n \t}\n \n+\tgit_test_v = getenv(git_test_k);\n \tif (git_test_v && *git_test_v) {\n \t\tenum protocol_version env = parse_protocol_version(git_test_v);\n \n \t\tif (env == protocol_unknown_version)\n \t\t\tdie(\"unknown value for %s: %s\", git_test_k, git_test_v);\n-\t\tif (retval < env)\n-\t\t\tretval = env;\n+\t\treturn env;\n \t}\n \n-\treturn retval;\n+\treturn protocol_v0;\n }\n \n enum protocol_version determine_protocol_version_server(void)\ndiff --git a/t/README b/t/README\nindex caa125ba9a..9afd61e3ca 100644\n--- a/t/README\n+++ b/t/README\n@@ -352,8 +352,8 @@ details.\n GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole\n test suite. Accept any boolean values that are accepted by git-config.\n \n-GIT_TEST_PROTOCOL_VERSION=<n>, when set, overrides the\n-'protocol.version' setting to n if it is less than n.\n+GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'\n+default to n.\n \n GIT_TEST_FULL_IN_PACK_ARRAY=<boolean> exercises the uncommon\n pack-objects code path where there are more than 1024 packs even if\n-- \n2.24.1.735.g03f4e72817\n\n"},{"id":"388841","messageId":"20191224010415.GH38316@google.com","threadId":"52513","inReplyTo":"20191224005816.GC38316@google.com","subject":"[PATCH 5/5] fetch: default to protocol version 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-24T01:04:15Z","receivedAt":"2019-12-24T01:04:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The Git users at $DAYJOB have been using protocol v2 as a default for\n~1.5 years now and others have been also reporting good experiences\nwith it, so it seems like a good time to bump the default version.  It\nproduces a significant performance improvement when fetching from\nrepositories with many refs, such as\nhttps://chromium.googlesource.com/chromium/src.\n\nThis only affects the client, not the server.  (The server already\ndefaults to supporting protocol v2.)  The protocol change is backward\ncompatible, so this should produce no significant effect when\ncontacting servers that only speak protocol v0.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nThat's the end of the series.  Thanks for reading.\n\nThoughts?\n\n Documentation/config/protocol.txt | 2 +-\n protocol.c                        | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/protocol.txt b/Documentation/config/protocol.txt\nindex 0b40141613..756591d77b 100644\n--- a/Documentation/config/protocol.txt\n+++ b/Documentation/config/protocol.txt\n@@ -48,7 +48,7 @@ protocol.version::\n \tIf set, clients will attempt to communicate with a server\n \tusing the specified protocol version.  If the server does\n \tnot support it, communication falls back to version 0.\n-\tIf unset, the default is `0`.\n+\tIf unset, the default is `2`.\n \tSupported versions:\n +\n --\ndiff --git a/protocol.c b/protocol.c\nindex d390391eba..803bef5c87 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -39,7 +39,7 @@ enum protocol_version get_protocol_version_config(void)\n \t\treturn env;\n \t}\n \n-\treturn protocol_v0;\n+\treturn protocol_v2;\n }\n \n enum protocol_version determine_protocol_version_server(void)\n-- \n2.24.1.735.g03f4e72817\n\n"},{"id":"388919","messageId":"a50a4ed5-5135-597a-fd37-9a907030b59f@gmail.com","threadId":"52513","inReplyTo":"20191224005907.GD38316@google.com","subject":"Re: [PATCH 1/5] fetch test: use more robust test for filtered objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-26T14:29:00Z","receivedAt":"2019-12-26T14:29:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/23/2019 7:59 PM, Jonathan Nieder wrote:\n>  \t# Ensure that object is not inadvertently fetched\n> -\ttest_must_fail git -C client cat-file -e $(git hash-object server/one.t)\n> +\tcommit=$(git -C server rev-parse HEAD) &&\n> +\tblob=$(git hash-object server/one.t) &&\n> +\tgit -C client rev-list --objects --missing=allow-any \"$commit\" >oids &&\n> +\t! grep \"$blob\" oids\n>  '\n>  \n>  test_expect_success 'filtering by size has no effect if support for it is not advertised' '\n> @@ -929,7 +932,10 @@ test_expect_success 'filtering by size has no effect if support for it is not ad\n>  \tgit -C client fetch-pack --filter=blob:limit=0 ../server HEAD 2> err &&\n>  \n>  \t# Ensure that object is fetched\n> -\tgit -C client cat-file -e $(git hash-object server/one.t) &&\n> +\tcommit=$(git -C server rev-parse HEAD) &&\n> +\tblob=$(git hash-object server/one.t) &&\n> +\tgit -C client rev-list --objects --missing=allow-any \"$commit\" >oids &&\n> +\tgrep \"$blob\" oids &&\n>  \n>  \ttest_i18ngrep \"filtering not recognized by server\" err\n>  '\n> @@ -951,9 +957,11 @@ fetch_filter_blob_limit_zero () {\n>  \tgit -C client fetch --filter=blob:limit=0 origin HEAD:somewhere &&\n>  \n>  \t# Ensure that commit is fetched, but blob is not\n> -\ttest_config -C client extensions.partialclone \"arbitrary string\" &&\n> -\tgit -C client cat-file -e $(git -C \"$SERVER\" rev-parse two) &&\n> -\ttest_must_fail git -C client cat-file -e $(git hash-object \"$SERVER/two.t\")\n> +\tcommit=$(git -C \"$SERVER\" rev-parse two) &&\n> +\tblob=$(git hash-object server/two.t) &&\n> +\tgit -C client rev-list --objects --missing=allow-any \"$commit\" >oids &&\n> +\tgrep \"$commit\" oids &&\n> +\t! grep \"$blob\" oids\n\nAt first glance, I saw this and thought the \"three is many\" rule would imply\nthe boilerplate in these tests should be de-duplicated with a function.\nHowever, the use of the \"commit\" and \"blob\" variables is different in each\ntest, so I did not find a convenient way to make this simpler.\n\nThanks,\n-Stolee\n\n"},{"id":"388920","messageId":"0a4f064b-8260-1662-2ead-b2e2c930d706@gmail.com","threadId":"52513","inReplyTo":"20191224005816.GC38316@google.com","subject":"Re: [PATCH 0/5] Enable protocol v2 by default","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-26T14:30:47Z","receivedAt":"2019-12-26T14:30:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/23/2019 7:58 PM, Jonathan Nieder wrote:\n> Hi,\n> \n> The Git users at $DAYJOB have been using protocol v2 as a default for\n> ~1.5 years now and others have been also reporting good experiences\n> with it, so it seems like a good time to propose bumping the default\n> version.  It produces a significant performance improvement when\n> fetching from repositories with many refs, such as\n> https://chromium.googlesource.com/chromium/src.\n\nThe benefits of protocol v2 are very clear, assuming the server\nsupports it. And I'm pretty sure there is no downside, as a v0\nserver continues responding to the v2 request without any extra\nround trips to agree on protocol.\n\n> This only affects the client, not the server.  (The server already\n> defaults to supporting protocol v2.)\n> \n> This could go in 2.25 (most of the \"next\" population is likely already\n> using protocol.version=2, so the -rc period would be one of the better\n> ways to expand the user population using this) or could cook in \"next\"\n> for a cycle.  Either is fine by me.\n\nI have no firm opinion on when this lands. The code change is much simpler\nthan I would have thought, and perhaps we had enough testing of the protocol\nby experts.\n\nThis series looks good to me.\n\nThanks,\n-Stolee\n"},{"id":"388941","messageId":"xmqqfth6lwgl.fsf@gitster-ct.c.googlers.com","threadId":"52513","inReplyTo":"20191224010110.GF38316@google.com","subject":"Re: [PATCH 3/5] test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-26T19:26:34Z","receivedAt":"2019-12-26T19:26:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\n> index f70cbcc9ca..a2a5e0743f 100755\n> --- a/t/t5552-skipping-fetch-negotiator.sh\n> +++ b/t/t5552-skipping-fetch-negotiator.sh\n> @@ -107,7 +107,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n>  \n>  \t# The ref advertisement itself is filtered when protocol v2 is used, so\n>  \t# use v0.\n> -\tGIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch &&\n> +\tGIT_TEST_PROTOCOL_VERSION=0 trace_fetch client origin to_fetch &&\n\nDidn't this trigger \"FOO=bar shell_func\" test lint for you?\n\n"},{"id":"388942","messageId":"20191226195357.GA170890@google.com","threadId":"52513","inReplyTo":"xmqqfth6lwgl.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 0/2] avoid use of \"VAR= cmd\" with a shell function (Re: [PATCH 3/5] test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-26T19:53:57Z","receivedAt":"2019-12-26T19:54:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\n>> index f70cbcc9ca..a2a5e0743f 100755\n>> --- a/t/t5552-skipping-fetch-negotiator.sh\n>> +++ b/t/t5552-skipping-fetch-negotiator.sh\n>> @@ -107,7 +107,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n>>  \n>>  \t# The ref advertisement itself is filtered when protocol v2 is used, so\n>>  \t# use v0.\n>> -\tGIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch &&\n>> +\tGIT_TEST_PROTOCOL_VERSION=0 trace_fetch client origin to_fetch &&\n>\n> Didn't this trigger \"FOO=bar shell_func\" test lint for you?\n\nIt does indeed.  Here are some preparatory patches to handle that.\n\nJonathan Nieder (2):\n  fetch test: avoid use of \"VAR= cmd\" with a shell function\n  t/check-non-portable-shell: detect \"FOO= shell_func\", too\n\n t/check-non-portable-shell.pl        | 2 +-\n t/t5552-skipping-fetch-negotiator.sh | 6 +++++-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\nbase-commit: 99c33bed562b41de6ce9bd3fd561303d39645048\n"},{"id":"388943","messageId":"20191226195510.GB170890@google.com","threadId":"52513","inReplyTo":"20191226195357.GA170890@google.com","subject":"[PATCH 1/2] fetch test: avoid use of \"VAR= cmd\" with a shell function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-26T19:55:10Z","receivedAt":"2019-12-26T19:55:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Just like assigning a nonempty value, assigning an empty value to a\nshell variable when calling a function produces non-portable behavior:\nin some shells, the assignment lasts for the duration of the function\ninvocation, and in others, it persists after the function returns.\n\nUse an explicit subshell with the envvar exported to make the behavior\nconsistent across shells and crystal clear.\n\nAll previous instances of this pattern used \"VAR=value\" (with nonempty\n`value`), which is already diagnosed automatically by \"make test-lint\"\nsince a0a630192d (t/check-non-portable-shell: detect \"FOO=bar\nshell_func\", 2018-07-13).\n\nNoticed using an improved \"make test-lint\".\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n t/t5552-skipping-fetch-negotiator.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex f70cbcc9ca..a452fe32fa 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -107,7 +107,11 @@ test_expect_success 'use ref advertisement to filter out commits' '\n \n \t# The ref advertisement itself is filtered when protocol v2 is used, so\n \t# use v0.\n-\tGIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch &&\n+\t(\n+\t\tGIT_TEST_PROTOCOL_VERSION= &&\n+\t\texport GIT_TEST_PROTOCOL_VERSION &&\n+\t\ttrace_fetch client origin to_fetch\n+\t) &&\n \thave_sent c5 c4^ c2side &&\n \thave_not_sent c4 c4^^ c4^^^\n '\n-- \n2.24.1.735.g03f4e72817\n\n"},{"id":"388944","messageId":"20191226195747.GC170890@google.com","threadId":"52513","inReplyTo":"20191226195357.GA170890@google.com","subject":"[PATCH 2/2] t/check-non-portable-shell: detect \"FOO= shell_func\", too","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-26T19:57:47Z","receivedAt":"2019-12-26T19:57:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Just like assigning a nonempty value, assigning an empty value to a\nshell variable when calling a function produces non-portable behavior:\nin some shells, the assignment lasts for the duration of the function\ninvocation, and in others, it persists after the function returns.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nIf it would be useful for me to send a copy of the \"Enable protocol v2\nby default\" series rebased on top of this, let me know.\n\nThanks again for catching it.\n\nSincerely,\nJonathan\n\n t/check-non-portable-shell.pl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex 38bfeebd88..fd3303552b 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -46,7 +46,7 @@ sub err {\n \t/(?:\\$\\(seq|^\\s*seq\\b)/ and err 'seq is not portable (use test_seq)';\n \t/\\bgrep\\b.*--file\\b/ and err 'grep --file FILE is not portable (use grep -f FILE)';\n \t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (use FOO=bar && export FOO)';\n-\t/^\\s*([A-Z0-9_]+=(\\w+|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n+\t/^\\s*([A-Z0-9_]+=(\\w*|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n \t\terr '\"FOO=bar shell_func\" assignment extends beyond \"shell_func\"';\n \t$line = '';\n \t# this resets our $. for each file\n-- \n2.24.1.735.g03f4e72817\n\n"},{"id":"388946","messageId":"xmqqblruluje.fsf@gitster-ct.c.googlers.com","threadId":"52513","inReplyTo":"20191226195747.GC170890@google.com","subject":"Re: [PATCH 2/2] t/check-non-portable-shell: detect \"FOO= shell_func\", too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-26T20:08:05Z","receivedAt":"2019-12-26T20:08:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Just like assigning a nonempty value, assigning an empty value to a\n> shell variable when calling a function produces non-portable behavior:\n> in some shells, the assignment lasts for the duration of the function\n> invocation, and in others, it persists after the function returns.\n> ...\n> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\n> index 38bfeebd88..fd3303552b 100755\n> --- a/t/check-non-portable-shell.pl\n> +++ b/t/check-non-portable-shell.pl\n> @@ -46,7 +46,7 @@ sub err {\n>  \t/(?:\\$\\(seq|^\\s*seq\\b)/ and err 'seq is not portable (use test_seq)';\n>  \t/\\bgrep\\b.*--file\\b/ and err 'grep --file FILE is not portable (use grep -f FILE)';\n>  \t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (use FOO=bar && export FOO)';\n> -\t/^\\s*([A-Z0-9_]+=(\\w+|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n> +\t/^\\s*([A-Z0-9_]+=(\\w*|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n\nThanks for a quick fix.\n\nIt is kind-of surprising that there was only one existing offender.\n"},{"id":"388947","messageId":"xmqq7e2ilu1j.fsf@gitster-ct.c.googlers.com","threadId":"52513","inReplyTo":"20191226195747.GC170890@google.com","subject":"Re: [PATCH 2/2] t/check-non-portable-shell: detect \"FOO= shell_func\", too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-26T20:18:48Z","receivedAt":"2019-12-26T20:25:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Just like assigning a nonempty value, assigning an empty value to a\n> shell variable when calling a function produces non-portable behavior:\n> in some shells, the assignment lasts for the duration of the function\n> invocation, and in others, it persists after the function returns.\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> If it would be useful for me to send a copy of the \"Enable protocol v2\n> by default\" series rebased on top of this, let me know.\n\nWhen rebased, t5552 passes (including the test lint) at the \"request\nv0 explicitly for some tests\" step now.\n\nThe tip of \"promote proto v2 to default\" series fails at 5552.5\nwith or without these two patches, though.\n"},{"id":"388948","messageId":"CAPig+cTPQROOvXVPxL2bv0FT1FZs+XcWq6rHkhRE8vsTQJsHCA@mail.gmail.com","threadId":"52513","inReplyTo":"20191226195747.GC170890@google.com","subject":"Re: [PATCH 2/2] t/check-non-portable-shell: detect \"FOO= shell_func\", too","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-26T20:39:31Z","receivedAt":"2019-12-26T20:39:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 26, 2019 at 2:57 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Just like assigning a nonempty value, assigning an empty value to a\n> shell variable when calling a function produces non-portable behavior:\n> in some shells, the assignment lasts for the duration of the function\n> invocation, and in others, it persists after the function returns.\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\n> @@ -46,7 +46,7 @@ sub err {\n> -       /^\\s*([A-Z0-9_]+=(\\w+|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n> +       /^\\s*([A-Z0-9_]+=(\\w*|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n>                 err '\"FOO=bar shell_func\" assignment extends beyond \"shell_func\"';\n\nThanks, the change makes sense. I suspect that I simply overlooked\nthis case when implementing this.\n\nAcked-by: me\n"},{"id":"388962","messageId":"20191226223727.GB186931@google.com","threadId":"52513","inReplyTo":"xmqq7e2ilu1j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] t/check-non-portable-shell: detect \"FOO= shell_func\", too","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-26T22:37:27Z","receivedAt":"2019-12-26T22:37:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> Just like assigning a nonempty value, assigning an empty value to a\n>> shell variable when calling a function produces non-portable behavior:\n>> in some shells, the assignment lasts for the duration of the function\n>> invocation, and in others, it persists after the function returns.\n>>\n>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n>> ---\n>> If it would be useful for me to send a copy of the \"Enable protocol v2\n>> by default\" series rebased on top of this, let me know.\n>\n> When rebased, t5552 passes (including the test lint) at the \"request\n> v0 explicitly for some tests\" step now.\n>\n> The tip of \"promote proto v2 to default\" series fails at 5552.5\n> with or without these two patches, though.\n\nOh, subtle.  With shells that leak variable assignments after a\nfunction returns (such as bash when run as 'sh'), 5552.5 was running\nwith GIT_TEST_PROTOCOL_VERSION=0, masking the issue.\n\nIn protocol v2, there is no \"stateful\" mode: negotiation always uses\nthe stateless-rpc path, and the stateless-rpc path involves more care\nto avoid chatter during negotiation (since request size increases with\neach round).\n\nThis is why b1.c14 and b1.c9 don't show up in the v2 trace.  Processing\nthe trace with \"git name-rev --stdin\" yields\n\n packet:        fetch> want 184bd23dc533e1e63153e7e181411bd29acca918\n packet:        fetch> have f65fc9b4d5c1cb76494a7f8df0230d8d29a33e67 (tags/b8.c19)\n packet:        fetch> have 334d40a157dec5d93023976c30cd22b24bdc279a (tags/b7.c19)\n[...]\n packet:        fetch> have e3496f08debed7528bd7e4c4a12b71d1a99d697f (tags/b1.c19)\n packet:        fetch> have e7bb01cb25bebd0341c9d62f4c7e929a99b6ed4b (tags/b8.c17)\n packet:        fetch> have 7f5656e94770d527d4f909fd5e2ea274ec63177a (tags/b7.c17)\n[...]\n packet:        fetch> have 17639a004fe8511fe1de57dd9ddabf2ee0de902d (tags/b1.c17)\n packet:        fetch> 0000\n packet:        fetch< acknowledgments\n packet:        fetch< ACK e3496f08debed7528bd7e4c4a12b71d1a99d697f (tags/b1.c19)\n packet:        fetch< ACK 17639a004fe8511fe1de57dd9ddabf2ee0de902d (tags/b1.c17)\n\nBy comparison, with protocol v0 over stateful bidirectional\ntransports, there's an additional round-trip folded in:\n\n packet:        fetch> have f65fc9b4d5c1cb76494a7f8df0230d8d29a33e67 (tags/b8.c19)\n packet:        fetch> have 334d40a157dec5d93023976c30cd22b24bdc279a (tags/b7.c19)\n[...]\n packet:        fetch> have e3496f08debed7528bd7e4c4a12b71d1a99d697f (tags/b1.c19)\n packet:        fetch> have e7bb01cb25bebd0341c9d62f4c7e929a99b6ed4b (tags/b8.c17)\n packet:        fetch> have 7f5656e94770d527d4f909fd5e2ea274ec63177a (tags/b7.c17)\n[...]\n packet:        fetch> have 17639a004fe8511fe1de57dd9ddabf2ee0de902d (tags/b1.c17)\n packet:        fetch> 0000\n packet:        fetch> have a1d75daa2f482f89171f092778da506803e54531 (tags/b8.c14)\n packet:        fetch> have b2e9b68d2650b77283421888be8a950c18bab29d (tags/b7.c14)\n[...]\n packet:        fetch> have b89f6499d7cee40ef422edb15433a10f82de0206 (tags/b1.c14)\n packet:        fetch> have e4190b433240834c895347214d29426a094f2fe2 (tags/b8.c9)\n packet:        fetch> have 5f1aa7f016defcf74e5e1d4991342987c9d4b447 (tags/b7.c9)\n[...]\n packet:        fetch> have b76868e654ce45adb9e06f638e48a72556843361 (tags/b1.c9)\n packet:        fetch> 0000\n packet:        fetch< ACK e3496f08debed7528bd7e4c4a12b71d1a99d697f (tags/b1.c19) common\n packet:        fetch< ACK 17639a004fe8511fe1de57dd9ddabf2ee0de902d (tags/b1.c17) common\n\nPatch coming in a moment to force v0 here with a comment.\n\nThanks,\nJonathan\n"},{"id":"388963","messageId":"20191226231251.GC186931@google.com","threadId":"52513","inReplyTo":"xmqq7e2ilu1j.fsf@gitster-ct.c.googlers.com","subject":"[PATCH jn/test-lint-one-shot-export-to-shell-function] fetch test: mark test of \"skipping\" haves as v0-only","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-26T23:12:51Z","receivedAt":"2019-12-26T23:12:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Since 633a53179e (fetch test: avoid use of \"VAR= cmd\" with a shell\nfunction, 2019-12-26), t5552.5 (do not send \"have\" with ancestors of\ncommits that server ACKed) fails when run with\nGIT_TEST_PROTOCOL_VERSION=2.\n\nThe cause:\n\nThe progression of \"have\"s sent in negotiation depends on whether we\nare using a stateless RPC based transport or a stateful bidirectional\none (see for example 44d8dc54e7, \"Fix potential local deadlock during\nfetch-pack\", 2011-03-29).  In protocol v2, all transports are\nstateless transports, while in protocol v0, transports such as local\naccess and ssh are stateful.\n\nIn stateful transports, the number of \"have\"s to send multiplies by\ntwo each round until we reach PIPESAFE_FLUSH (that is, 32), and then\nit increases by PIPESAFE_FLUSH each round.  In stateless transport,\nthe count multiplies by two each round until we reach LARGE_FLUSH\n(which is 16384) and then multiplies by 1.1 each round after that.\n\nMoreover, in stateful transports, as fetch-pack.c explains:\n\n\tWe keep one window \"ahead\" of the other side, and will wait\n\tfor an ACK only on the next one.\n\nThis affects t5552.5 because it looks for \"have\"s from the negotiator\nthat appear in that second window.  With protocol version 2, the\nsecond window never arrives, and the test fails.\n\nUntil 633a53179e (2019-12-26), a previous test in the same file\ncontained\n\n\tGIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch\n\nIn many common shells (e.g. bash when run as \"sh\"), the setting of\nGIT_TEST_PROTOCOL_VERSION to the empty string lasts beyond the\nintended duration of the trace_fetch invocation.  This causes it to\noverride the GIT_TEST_PROTOCOL_VERSION setting that was passed in to\nthe test during the remainder of the test script, so t5552.5 never got\nrun using protocol v2 on those shells, regardless of the\nGIT_TEST_PROTOCOL_VERSION setting from the environment.  633a53179e\nfixed that, revealing the failing test.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJunio C Hamano wrote:\n\n> The tip of \"promote proto v2 to default\" series fails at 5552.5\n> with or without these two patches, though.\n\nHere's the promised fix, against\njn/test-lint-one-shot-export-to-shell-function.  Thanks again.\n\n t/t5552-skipping-fetch-negotiator.sh | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex a452fe32fa..8f25f4b31f 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -173,7 +173,17 @@ test_expect_success 'do not send \"have\" with ancestors of commits that server AC\n \ttest_commit -C server commit-on-b1 &&\n \n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\ttrace_fetch client \"$(pwd)/server\" to_fetch &&\n+\n+\t# NEEDSWORK: The number of \"have\"s sent depends on whether the transport\n+\t# is stateful. If the overspecification of the result were reduced, this\n+\t# test could be used for both stateful and stateless transports.\n+\t(\n+\t\t# Force protocol v0, in which local transport is stateful (in\n+\t\t# protocol v2 it is stateless).\n+\t\tGIT_TEST_PROTOCOL_VERSION=0 &&\n+\t\texport GIT_TEST_PROTOCOL_VERSION &&\n+\t\ttrace_fetch client \"$(pwd)/server\" to_fetch\n+\t) &&\n \tgrep \"  fetch\" trace &&\n \n \t# fetch-pack sends 2 requests each containing 16 \"have\" lines before\n-- \n2.24.1.735.g03f4e72817\n\n"}]}