{"thread":{"id":"50406","subject":"[PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches","startedAt":"2019-02-06T00:21:28Z","lastAt":"2019-02-23T13:25:12Z","messageCount":26,"participants":["Jonathan Tan","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"368553","messageId":"cover.1549411880.git.jonathantanmy@google.com","threadId":"50406","inReplyTo":null,"subject":"[PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-06T00:21:14Z","receivedAt":"2019-02-06T00:21:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is on the latest master (8feddda32c (\"Fifth batch for 2.21\",\n2019-02-05)) + js/protocol-advertise-multi.\n\nThis is a resend of [1], which was built on the result of merging many\nbranches. Now that most of the branches have been merged to master, I\nhave rebased it on master. The only branch that I needed to merge was\njs/protocol-advertise-multi.\n\njs/protocol-advertise-multi has not been merged to next yet, but the\ngeneral idea seems good to me, and I am making review of that patch one\nof my priorities. But if anyone is concerned that this set will be\ndelayed by js/protocol-advertise-multi, I can extract the parts I need\ninto a patch in this set (most likely just the part that unconditionally\ncalls ssh with SendEnv in connect.c), although it is difficult to\nexplain the necessity of that without reference to\njs/protocol-advertise-multi. Let me know what you think.\n\nBesides the rebase, this is unchanged from [1]. There is some discussion\nthere between Ævar, Junio, and I, thinking that it is OK to merge this\neven if we didn't eliminate all errors. Right now, only 3 test cases\nfail with GIT_TEST_PROTOCOL_VERSION=2.\n\nPatch 1 implements the test variable, patches 2-7 eliminate false\nnegatives (with explanations), and patch 8 fixes a genuine bug.\n\n[1] https://public-inbox.org/git/cover.1547677183.git.jonathantanmy@google.com/\n\nJonathan Tan (8):\n  tests: define GIT_TEST_PROTOCOL_VERSION\n  tests: always test fetch of unreachable with v0\n  t5503: fix overspecification of trace expectation\n  t5512: compensate for v0 only sending HEAD symrefs\n  t5700: only run with protocol version 1\n  tests: fix protocol version for overspecifications\n  t5552: compensate for v2 filtering ref adv.\n  remote-curl: in v2, fill credentials if needed\n\n protocol.c                           | 17 ++++++++--\n remote-curl.c                        |  9 +++++-\n t/README                             |  3 ++\n t/t5400-send-pack.sh                 |  2 +-\n t/t5500-fetch-pack.sh                |  4 ++-\n t/t5503-tagfollow.sh                 |  2 +-\n t/t5512-ls-remote.sh                 | 18 ++++++++---\n t/t5515-fetch-merge-logic.sh         |  4 +++\n t/t5516-fetch-push.sh                | 22 ++++++++++---\n t/t5539-fetch-http-shallow.sh        |  5 ++-\n t/t5541-http-push-smart.sh           | 14 +++++++--\n t/t5551-http-fetch-smart.sh          | 47 +++++++++++++++++++++-------\n t/t5552-skipping-fetch-negotiator.sh |  5 ++-\n t/t5700-protocol-v1.sh               |  3 ++\n t/t7406-submodule-update.sh          |  5 ++-\n 15 files changed, 128 insertions(+), 32 deletions(-)\n\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"368554","messageId":"6e0c6aa9a71d4192591ed406735684cd15a0e3b9.1549411880.git.jonathantanmy@google.com","threadId":"50406","inReplyTo":"cover.1549411880.git.jonathantanmy@google.com","subject":"[PATCH 1/8] tests: define GIT_TEST_PROTOCOL_VERSION","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-06T00:21:15Z","receivedAt":"2019-02-06T00:21:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Define a GIT_TEST_PROTOCOL_VERSION environment variable meant to be used\nfrom tests. When set, this ensures protocol.version is at least the\ngiven value, allowing the entire test suite to be run as if this\nconfiguration is in place for all repositories.\n\nAs of this patch, all tests pass whether GIT_TEST_PROTOCOL_VERSION is\nunset, set to 0, or set to 1. Some tests fail when\nGIT_TEST_PROTOCOL_VERSION is set to 2, but this will be dealt with in\nsubsequent patches.\n\nThis is based on work by Ævar Arnfjörð Bjarmason.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n protocol.c                  | 17 +++++++++++++++--\n t/README                    |  3 +++\n t/t5400-send-pack.sh        |  2 +-\n t/t5551-http-fetch-smart.sh |  3 ++-\n 4 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/protocol.c b/protocol.c\nindex 5664bd7a05..c7a735bfa2 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -42,6 +42,10 @@ static const char *format_protocol_version(enum protocol_version version)\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+\n \tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n \t\tenum protocol_version version = parse_protocol_version(value);\n \n@@ -49,10 +53,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\treturn version;\n+\t\tretval = version;\n+\t}\n+\n+\tif (git_test_v && strlen(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}\n \n-\treturn protocol_v0;\n+\treturn retval;\n }\n \n void register_allowed_protocol_version(enum protocol_version version)\ndiff --git a/t/README b/t/README\nindex 25864ec883..21e941eb94 100644\n--- a/t/README\n+++ b/t/README\n@@ -327,6 +327,9 @@ marked strings\" in po/README for 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+\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 the actual number of packs in repository is below this limit. Accept\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex f1932ea431..571d620aed 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 \\\n+\tGIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \\\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/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex a60dd907bd..8f620e0a35 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -44,7 +44,8 @@ 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 clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n+\tGIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \\\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 \tsed -e \"\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"368555","messageId":"9b9061985202ec022cc562637d7f62ea599e7d8c.1549411880.git.jonathantanmy@google.com","threadId":"50406","inReplyTo":"cover.1549411880.git.jonathantanmy@google.com","subject":"[PATCH 2/8] tests: always test fetch of unreachable with v0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-06T00:21:16Z","receivedAt":"2019-02-06T00:21:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Some tests check that fetching an unreachable object fails, but protocol\nv2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these\ntests are always run using protocol v0.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5500-fetch-pack.sh       |  4 +++-\n t/t5516-fetch-push.sh       | 22 +++++++++++++++++-----\n t/t5551-http-fetch-smart.sh | 10 ++++++++--\n t/t7406-submodule-update.sh |  5 ++++-\n 4 files changed, 32 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 49c540b1e1..0ef4d6f20c 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -636,7 +636,9 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a\n \ttest_commit -C server 6 &&\n \n \tgit init client &&\n-\ttest_must_fail git -C client fetch-pack ../server \\\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 \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/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 37e8e80893..4bfbb79654 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1147,8 +1147,12 @@ test_expect_success 'fetch exact SHA1' '\n \t\tgit prune &&\n \t\ttest_must_fail git cat-file -t $the_commit &&\n \n+\t\t# Some protocol versions (e.g. 2) support fetching\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 git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&\n+\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\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 \n@@ -1204,7 +1208,10 @@ do\n \t\tmk_empty shallow &&\n \t\t(\n \t\t\tcd shallow &&\n-\t\t\ttest_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&\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\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 \t\t\tgit cat-file commit $SHA1\n@@ -1232,15 +1239,20 @@ do\n \t\tmk_empty shallow &&\n \t\t(\n \t\t\tcd shallow &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&\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 ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\t\tgit fetch ../testrepo/.git $SHA1_3 &&\n+\t\t\ttest_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \\\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 \t\t\tgit cat-file commit $SHA1_1 &&\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 ok=sigpipe git fetch ../testrepo/.git $SHA1_3\n+\t\t\ttest_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\t\tgit fetch ../testrepo/.git $SHA1_3\n \t\t)\n \t'\n done\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 8f620e0a35..d2661eb338 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -308,7 +308,10 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '\n \n \tgit init --bare test_reachable.git &&\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n-\ttest_must_fail git -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\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+\t\tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\n '\n \n test_expect_success 'test allowanysha1inwant with unreachable' '\n@@ -327,7 +330,10 @@ test_expect_success 'test allowanysha1inwant with unreachable' '\n \n \tgit init --bare test_reachable.git &&\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n-\ttest_must_fail git -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\" &&\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+\t\tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\" &&\n \n \tgit -C \"$server\" config uploadpack.allowanysha1inwant 1 &&\n \tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex e87164aa8f..c973278300 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -943,7 +943,10 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n \t\tcd super3 &&\n \t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n \t\tmv -f .gitmodules.tmp .gitmodules &&\n-\t\ttest_must_fail git submodule update --init --depth=1 2>actual &&\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\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 \t\tgit submodule update --init --depth=1 >actual &&\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"368556","messageId":"84b3464c07a827a7aa3fd427ab97754b107886a7.1549411880.git.jonathantanmy@google.com","threadId":"50406","inReplyTo":"cover.1549411880.git.jonathantanmy@google.com","subject":"[PATCH 3/8] t5503: fix overspecification of trace expectation","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-06T00:21:17Z","receivedAt":"2019-02-06T00:21:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In order to extract the wants from a trace, a loop in t5503 currently\nbreaks if \"0000\" is found. This works for protocol v0 and v1, but not\nv2. Instead, teach t5503 to look specifically for the \"want\" string,\nwhich is compatible with all protocols.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5503-tagfollow.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex 4ca48f0276..6041a4dd32 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -47,7 +47,7 @@ get_needs () {\n \ttest -s \"$1\" &&\n \tperl -alne '\n \t\tnext unless $F[1] eq \"upload-pack<\";\n-\t\tlast if $F[2] eq \"0000\";\n+\t\tnext unless $F[2] eq \"want\";\n \t\tprint $F[2], \" \", $F[3];\n \t' \"$1\"\n }\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"368557","messageId":"05b1b94c479760c17d38beafc3d45ad9a5a08d0d.1549411880.git.jonathantanmy@google.com","threadId":"50406","inReplyTo":"cover.1549411880.git.jonathantanmy@google.com","subject":"[PATCH 4/8] t5512: compensate for v0 only sending HEAD symrefs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-06T00:21:18Z","receivedAt":"2019-02-06T00:21:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Protocol v2 supports sending non-HEAD symrefs, but this is not true of\nprotocol v0. Some tests expect protocol v0 behavior, so fix them to use\nprotocol v0.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5512-ls-remote.sh | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex ced15ae122..e3c4a48c85 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -223,7 +223,9 @@ test_expect_success 'ls-remote --symref' '\n \t$(git rev-parse refs/tags/mark1.10)\trefs/tags/mark1.10\n \t$(git rev-parse refs/tags/mark1.2)\trefs/tags/mark1.2\n \tEOF\n-\tgit ls-remote --symref >actual &&\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 \ttest_cmp expect actual\n '\n \n@@ -232,7 +234,9 @@ test_expect_success 'ls-remote with filtered symref (refname)' '\n \tref: refs/heads/master\tHEAD\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\tHEAD\n \tEOF\n-\tgit ls-remote --symref . HEAD >actual &&\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 \ttest_cmp expect actual\n '\n \n@@ -243,7 +247,9 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/foo\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/master\n \tEOF\n-\tgit ls-remote --symref --heads . >actual &&\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 \ttest_cmp expect actual\n '\n \n@@ -252,9 +258,11 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/foo\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/master\n \tEOF\n-\tgit ls-remote --symref --heads . >actual &&\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 \ttest_cmp expect actual &&\n-\tgit ls-remote --symref . \"refs/heads/*\" >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . \"refs/heads/*\" >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"368558","messageId":"f6cc28bf17d34c4b54a75dea6bd3eaccc3b17853.1549411880.git.jonathantanmy@google.com","threadId":"50406","inReplyTo":"cover.1549411880.git.jonathantanmy@google.com","subject":"[PATCH 5/8] t5700: only run with protocol version 1","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-06T00:21:19Z","receivedAt":"2019-02-06T00:21:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5700-protocol-v1.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 2e56c79233..9857bd0ecb 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -4,6 +4,9 @@ test_description='test git wire-protocol transition'\n \n TEST_NO_CREATE_REPO=1\n \n+# This is a protocol-specific test.\n+GIT_TEST_PROTOCOL_VERSION=\n+\n . ./test-lib.sh\n \n # Test protocol v1 with 'git://' transport\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"368559","messageId":"8569ae986ee99ec92c2d844b6092664a8b47ad06.1549411880.git.jonathantanmy@google.com","threadId":"50406","inReplyTo":"cover.1549411880.git.jonathantanmy@google.com","subject":"[PATCH 6/8] tests: fix protocol version for overspecifications","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-06T00:21:20Z","receivedAt":"2019-02-06T00:21:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"These tests are also marked with a NEEDSWORK comment.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5515-fetch-merge-logic.sh  |  4 ++++\n t/t5539-fetch-http-shallow.sh |  5 ++++-\n t/t5541-http-push-smart.sh    | 14 ++++++++++++--\n t/t5551-http-fetch-smart.sh   | 34 ++++++++++++++++++++++++++--------\n 4 files changed, 46 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh\nindex 36b0dbc01c..e55d8474ef 100755\n--- a/t/t5515-fetch-merge-logic.sh\n+++ b/t/t5515-fetch-merge-logic.sh\n@@ -6,6 +6,10 @@\n \n 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+\n . ./test-lib.sh\n \n LF='\ndiff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\nindex 5fbf67c446..cdb687b93a 100755\n--- a/t/t5539-fetch-http-shallow.sh\n+++ b/t/t5539-fetch-http-shallow.sh\n@@ -67,7 +67,10 @@ test_expect_success 'no shallow lines after receiving ACK ready' '\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\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\t\tgit fetch --depth=2 &&\n \t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n \t\t! grep \"fetch-pack> done\" ../trace\n \t)\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 5475afc052..0e3055ab98 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -47,7 +47,12 @@ test_expect_success 'no empty path components' '\n \tcd \"$ROOT_PATH\" &&\n \tgit clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&\n \n-\tcheck_access_log exp\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+\tthen\n+\t\tcheck_access_log exp\n+\tfi\n '\n \n test_expect_success 'clone remote repository' '\n@@ -128,7 +133,12 @@ GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200\n POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200\n EOF\n test_expect_success 'used receive-pack service' '\n-\tcheck_access_log exp\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+\tthen\n+\t\tcheck_access_log exp\n+\tfi\n '\n \n test_http_push_nonff \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git \\\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex d2661eb338..e8ed185b33 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -82,12 +82,18 @@ test_expect_success 'clone http repository' '\n \t\t/^< Content-Length: /d\n \t\t/^< Transfer-Encoding: /d\n \t\" >actual &&\n-\tsed -e \"s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/\" \\\n-\t\t\tactual >actual.smudged &&\n-\ttest_cmp exp actual.smudged &&\n \n-\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n-\ttest_line_count = 2 actual.gzip\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+\tthen\n+\t\tsed -e \"s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/\" \\\n+\t\t\t\tactual >actual.smudged &&\n+\t\ttest_cmp exp actual.smudged &&\n+\n+\t\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n+\t\ttest_line_count = 2 actual.gzip\n+\tfi\n '\n \n test_expect_success 'fetch changes via http' '\n@@ -105,7 +111,13 @@ test_expect_success 'used upload-pack service' '\n \tGET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n \tPOST /smart/repo.git/git-upload-pack HTTP/1.1 200\n \tEOF\n-\tcheck_access_log exp\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+\tthen\n+\t\tcheck_access_log exp\n+\tfi\n '\n \n test_expect_success 'follow redirects (301)' '\n@@ -217,8 +229,14 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set\n \tgit config http.cookiefile cookies.txt &&\n \tgit config http.savecookies true &&\n \tgit ls-remote $HTTPD_URL/smart_cookies/repo.git master &&\n-\ttail -3 cookies.txt | sort >cookies_tail.txt &&\n-\ttest_cmp expect_cookies.txt cookies_tail.txt\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+\tthen\n+\t\ttail -3 cookies.txt | sort >cookies_tail.txt &&\n+\t\ttest_cmp expect_cookies.txt cookies_tail.txt\n+\tfi\n '\n \n test_expect_success 'transfer.hiderefs works over smart-http' '\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"368560","messageId":"2e8ad7ab247e6b14c0583f0800a228e95bcda095.1549411880.git.jonathantanmy@google.com","threadId":"50406","inReplyTo":"cover.1549411880.git.jonathantanmy@google.com","subject":"[PATCH 7/8] t5552: compensate for v2 filtering ref adv.","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-06T00:21:21Z","receivedAt":"2019-02-06T00:21:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Protocol v2 filters the ref advertisement, but protocol v0 does not. A\ntest in t5552 uses the ref advertisement, so fix it to use protocol v0.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5552-skipping-fetch-negotiator.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex 30857b84a8..8a14be51a1 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -127,7 +127,10 @@ test_expect_success 'use ref advertisement to filter out commits' '\n \t# not need to send any ancestors of \"c3\", but we still need to send \"c3\"\n \t# itself.\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\ttrace_fetch client origin to_fetch &&\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 \thave_sent c5 c4^ c2side &&\n \thave_not_sent c4 c4^^ c4^^^\n '\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"368561","messageId":"8d5ff2fc224e2ce7981bcae492de02a622889208.1549411880.git.jonathantanmy@google.com","threadId":"50406","inReplyTo":"cover.1549411880.git.jonathantanmy@google.com","subject":"[PATCH 8/8] remote-curl: in v2, fill credentials if needed","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-06T00:21:22Z","receivedAt":"2019-02-06T00:21:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In post_rpc(), remote-curl calls credential_fill() if HTTP_REAUTH is\nreturned, but this is not true in proxy_request(). Do this in\nproxy_request() too.\n\nWhen t5551 is run using GIT_TEST_PROTOCOL_VERSION=2, one of the tests\nthat used to fail now pass.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n remote-curl.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 293bcdb95b..437a8e76d8 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1295,7 +1295,9 @@ static size_t proxy_out(char *buffer, size_t eltsize,\n static int proxy_request(struct proxy_state *p)\n {\n \tstruct active_request_slot *slot;\n+\tint err;\n \n+retry:\n \tslot = get_active_slot();\n \n \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n@@ -1312,7 +1314,12 @@ static int proxy_request(struct proxy_state *p)\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p);\n \n-\tif (run_slot(slot, NULL) != HTTP_OK)\n+\terr = run_slot(slot, NULL);\n+\tif (err == HTTP_REAUTH) {\n+\t\tcredential_fill(&http_auth);\n+\t\tgoto retry;\n+\t}\n+\tif (err != HTTP_OK)\n \t\treturn -1;\n \n \treturn 0;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"368642","messageId":"20190206212928.GB12737@sigill.intra.peff.net","threadId":"50406","inReplyTo":"8d5ff2fc224e2ce7981bcae492de02a622889208.1549411880.git.jonathantanmy@google.com","subject":"Re: [PATCH 8/8] remote-curl: in v2, fill credentials if needed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T21:29:29Z","receivedAt":"2019-02-06T21:29:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 05, 2019 at 04:21:22PM -0800, Jonathan Tan wrote:\n\n> In post_rpc(), remote-curl calls credential_fill() if HTTP_REAUTH is\n> returned, but this is not true in proxy_request(). Do this in\n> proxy_request() too.\n\nCan we do this as a general rule? If we look at the code in post_rpc(),\nthere are two cases: when large_request is set and when it is not.\n\nWhen it's not, we have the whole request in a buffer, and we can happily\nresend it.\n\nBut when it's not, we cannot restart it, because we'll have thrown away\nsome of the data. So we send an initial probe_rpc() as a sanity check.\nIf that works and we later get a 401 on the real request, we still fail\nanyway.\n\nIn the case of proxy_request(), we don't know ahead of time whether the\nrequest is large or not; we just proxy the data through. And we don't do\nthe probe thing at all. So wouldn't we dropping some data for the\nfollow-up request?\n\n-Peff\n"},{"id":"368643","messageId":"20190206213458.GC12737@sigill.intra.peff.net","threadId":"50406","inReplyTo":"cover.1549411880.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T21:34:58Z","receivedAt":"2019-02-06T21:35:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 05, 2019 at 04:21:14PM -0800, Jonathan Tan wrote:\n\n> This is on the latest master (8feddda32c (\"Fifth batch for 2.21\",\n> 2019-02-05)) + js/protocol-advertise-multi.\n> \n> This is a resend of [1], which was built on the result of merging many\n> branches. Now that most of the branches have been merged to master, I\n> have rebased it on master. The only branch that I needed to merge was\n> js/protocol-advertise-multi.\n\nThanks for working on this. With the exception of the final patch, this\nall seems pretty sane to me from a quick look.\n\nThere is one thing that your test patches made me wonder. When we have\nto make an exception to a test (i.e., that doesn't work under v2), you\ndo it by unsetting GIT_TEST_PROTOCOL_VERSION in the environment. That\nmeans we'll actually run the test, but not with the version that the\ncaller specified.\n\nI wonder if it would be more obvious what's going on if we instead had a\nprereq like:\n\n  test_expect_success !PROTO_V2 'ls-remote --symref' '\n     ...\n  '\n\nand just skipped those tests entirely (and in a way that appears in the\nTAP output).\n\nI think it would also future-proof us a bit for v2 becoming the default\n(i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean \"use v2\").\n\nI dunno. It probably doesn't matter all that much, so it may not be\nworth going back and changing at this point. Just a thought.\n\n-Peff\n"},{"id":"368644","messageId":"87ef8klh4g.fsf@evledraar.gmail.com","threadId":"50406","inReplyTo":"20190206213458.GC12737@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-06T21:52:15Z","receivedAt":"2019-02-06T21:52:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 06 2019, Jeff King wrote:\n\n> On Tue, Feb 05, 2019 at 04:21:14PM -0800, Jonathan Tan wrote:\n>\n>> This is on the latest master (8feddda32c (\"Fifth batch for 2.21\",\n>> 2019-02-05)) + js/protocol-advertise-multi.\n>>\n>> This is a resend of [1], which was built on the result of merging many\n>> branches. Now that most of the branches have been merged to master, I\n>> have rebased it on master. The only branch that I needed to merge was\n>> js/protocol-advertise-multi.\n>\n> Thanks for working on this. With the exception of the final patch, this\n> all seems pretty sane to me from a quick look.\n>\n> There is one thing that your test patches made me wonder. When we have\n> to make an exception to a test (i.e., that doesn't work under v2), you\n> do it by unsetting GIT_TEST_PROTOCOL_VERSION in the environment. That\n> means we'll actually run the test, but not with the version that the\n> caller specified.\n>\n> I wonder if it would be more obvious what's going on if we instead had a\n> prereq like:\n>\n>   test_expect_success !PROTO_V2 'ls-remote --symref' '\n>      ...\n>   '\n>\n> and just skipped those tests entirely (and in a way that appears in the\n> TAP output).\n>\n> I think it would also future-proof us a bit for v2 becoming the default\n> (i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean \"use v2\").\n>\n> I dunno. It probably doesn't matter all that much, so it may not be\n> worth going back and changing at this point. Just a thought.\n\nSo far we've had the convention that these GIT_TEST_* variables,\ne.g. the one for the commit graph, work the same way. Thus we guarantee\nthat we get (in theory) 100% coverage even when running the tests in\nthis special mode. I think it's better to keep it as-is.\n"},{"id":"368645","messageId":"87d0o4lgtr.fsf@evledraar.gmail.com","threadId":"50406","inReplyTo":"6e0c6aa9a71d4192591ed406735684cd15a0e3b9.1549411880.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/8] tests: define GIT_TEST_PROTOCOL_VERSION","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-06T21:58:40Z","receivedAt":"2019-02-06T21:58:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 06 2019, Jonathan Tan wrote:\n\n> Define a GIT_TEST_PROTOCOL_VERSION environment variable meant to be used\n> from tests. When set, this ensures protocol.version is at least the\n> given value, allowing the entire test suite to be run as if this\n> configuration is in place for all repositories.\n>\n> As of this patch, all tests pass whether GIT_TEST_PROTOCOL_VERSION is\n> unset, set to 0, or set to 1. Some tests fail when\n> GIT_TEST_PROTOCOL_VERSION is set to 2, but this will be dealt with in\n> subsequent patches.\n>\n> This is based on work by Ævar Arnfjörð Bjarmason.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  protocol.c                  | 17 +++++++++++++++--\n>  t/README                    |  3 +++\n>  t/t5400-send-pack.sh        |  2 +-\n>  t/t5551-http-fetch-smart.sh |  3 ++-\n>  4 files changed, 21 insertions(+), 4 deletions(-)\n>\n> diff --git a/protocol.c b/protocol.c\n> index 5664bd7a05..c7a735bfa2 100644\n> --- a/protocol.c\n> +++ b/protocol.c\n> @@ -42,6 +42,10 @@ static const char *format_protocol_version(enum protocol_version version)\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> +\n>  \tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n>  \t\tenum protocol_version version = parse_protocol_version(value);\n>\n> @@ -49,10 +53,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\treturn version;\n> +\t\tretval = version;\n> +\t}\n> +\n> +\tif (git_test_v && strlen(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}\n>\n> -\treturn protocol_v0;\n> +\treturn retval;\n>  }\n>\n>  void register_allowed_protocol_version(enum protocol_version version)\n> diff --git a/t/README b/t/README\n> index 25864ec883..21e941eb94 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -327,6 +327,9 @@ marked strings\" in po/README for 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> +\n\nIn my version\n(https://public-inbox.org/git/20181213155817.27666-6-avarab@gmail.com/)\nI didn't have this \"if it is less than n\" caveat. I expect that helped\nwith making some tests that were setting e.g. protocol.version=2 Just\nWork, is that the reason for this?\n\nMine also had more docs here, but maybe telling people that they can use\n\"env\" is too much...\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>  the actual number of packs in repository is below this limit. Accept\n> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\n> index f1932ea431..571d620aed 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 \\\n> +\tGIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \\\n>  \t    git push \\\n>  \t\t--receive-pack=\"unset GIT_TRACE_PACKET; git-receive-pack\" \\\n>  \t\tfork HEAD:foo &&\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index a60dd907bd..8f620e0a35 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -44,7 +44,8 @@ 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 clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n> +\tGIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \\\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>  \tsed -e \"\n"},{"id":"368647","messageId":"20190206221008.GB15378@sigill.intra.peff.net","threadId":"50406","inReplyTo":"87ef8klh4g.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T22:10:09Z","receivedAt":"2019-02-06T22:10:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2019 at 10:52:15PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > I wonder if it would be more obvious what's going on if we instead had a\n> > prereq like:\n> >\n> >   test_expect_success !PROTO_V2 'ls-remote --symref' '\n> >      ...\n> >   '\n> >\n> > and just skipped those tests entirely (and in a way that appears in the\n> > TAP output).\n> >\n> > I think it would also future-proof us a bit for v2 becoming the default\n> > (i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean \"use v2\").\n> >\n> > I dunno. It probably doesn't matter all that much, so it may not be\n> > worth going back and changing at this point. Just a thought.\n> \n> So far we've had the convention that these GIT_TEST_* variables,\n> e.g. the one for the commit graph, work the same way. Thus we guarantee\n> that we get (in theory) 100% coverage even when running the tests in\n> this special mode. I think it's better to keep it as-is.\n\nBut what's the point of that? Don't you always have to run the test\nsuite _twice_, once with the special variable and once without?\nOtherwise, you are not testing one case or the other.\n\nOr are you arguing that one might set many special variables in one go\n(to prefer running the suite only twice, instead of 2^N times). In which\ncase we are better off running the test (as opposed to skipping it), as\nit might use one of the _other_ special variables besides\nGIT_TEST_PROTOCOL_VERSION.\n\nI can buy that line of reasoning. It still doesn't cover all cases that\na true 2^N test would, but that clearly isn't going to be practical.\n\n-Peff\n"},{"id":"368650","messageId":"87bm3olftb.fsf@evledraar.gmail.com","threadId":"50406","inReplyTo":"20190206221008.GB15378@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-06T22:20:32Z","receivedAt":"2019-02-06T22:20:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 06 2019, Jeff King wrote:\n\n> On Wed, Feb 06, 2019 at 10:52:15PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > I wonder if it would be more obvious what's going on if we instead had a\n>> > prereq like:\n>> >\n>> >   test_expect_success !PROTO_V2 'ls-remote --symref' '\n>> >      ...\n>> >   '\n>> >\n>> > and just skipped those tests entirely (and in a way that appears in the\n>> > TAP output).\n>> >\n>> > I think it would also future-proof us a bit for v2 becoming the default\n>> > (i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean \"use v2\").\n>> >\n>> > I dunno. It probably doesn't matter all that much, so it may not be\n>> > worth going back and changing at this point. Just a thought.\n>>\n>> So far we've had the convention that these GIT_TEST_* variables,\n>> e.g. the one for the commit graph, work the same way. Thus we guarantee\n>> that we get (in theory) 100% coverage even when running the tests in\n>> this special mode. I think it's better to keep it as-is.\n>\n> But what's the point of that? Don't you always have to run the test\n> suite _twice_, once with the special variable and once without?\n> Otherwise, you are not testing one case or the other.\n>\n> Or are you arguing that one might set many special variables in one go\n> (to prefer running the suite only twice, instead of 2^N times). In which\n> case we are better off running the test (as opposed to skipping it), as\n> it might use one of the _other_ special variables besides\n> GIT_TEST_PROTOCOL_VERSION.\n>\n> I can buy that line of reasoning. It still doesn't cover all cases that\n> a true 2^N test would, but that clearly isn't going to be practical.\n\nMaybe I'm misunderstanding what you're proposing, but as an example,\nlet's say the test suite is just these two tests:\n\n    test_expect_success 'some unrelated thing' '...'\n    test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...'\n\nAnd GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test\nwith GIT_TEST_PROTOCOL_VERSION=1 for whatever reason,\n\nI'd still like both tests to be run, not just 1/2 with\nGIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly\ntesting for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a\nGIT_TEST_PROTOCOL_VERSION=1.\n\nIOW the point of these tests is to piggy-back on the tests that *aren't*\naware of the feature you're trying to test. So\ne.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the\ncommit graph, and *also* those tests that are explicitly aware of the\ncommit graph, including those that for some reason would want to test\nfor the case where it isn't enabled (to e.g. test that --contains works\nwithout the graph).\n\nOtherwise I can't say \"I care more about GIT_TEST_COMMIT_GRAPH=true than\nnot\", and run just one test run with that, because I'll have blind spots\nin the commit graph tests themselves, and would then need to do another\nrun with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage.\n"},{"id":"368653","messageId":"20190206230813.GC19901@sigill.intra.peff.net","threadId":"50406","inReplyTo":"87bm3olftb.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T23:08:14Z","receivedAt":"2019-02-06T23:10:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2019 at 11:20:32PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> >> So far we've had the convention that these GIT_TEST_* variables,\n> >> e.g. the one for the commit graph, work the same way. Thus we guarantee\n> >> that we get (in theory) 100% coverage even when running the tests in\n> >> this special mode. I think it's better to keep it as-is.\n> >\n> > But what's the point of that? Don't you always have to run the test\n> > suite _twice_, once with the special variable and once without?\n> > Otherwise, you are not testing one case or the other.\n> >\n> > Or are you arguing that one might set many special variables in one go\n> > (to prefer running the suite only twice, instead of 2^N times). In which\n> > case we are better off running the test (as opposed to skipping it), as\n> > it might use one of the _other_ special variables besides\n> > GIT_TEST_PROTOCOL_VERSION.\n> >\n> > I can buy that line of reasoning. It still doesn't cover all cases that\n> > a true 2^N test would, but that clearly isn't going to be practical.\n> \n> Maybe I'm misunderstanding what you're proposing, but as an example,\n> let's say the test suite is just these two tests:\n> \n>     test_expect_success 'some unrelated thing' '...'\n>     test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...'\n> \n> And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test\n> with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason,\n> \n> I'd still like both tests to be run, not just 1/2 with\n> GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly\n> testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a\n> GIT_TEST_PROTOCOL_VERSION=1.\n\nBut that's my \"why\". The second test will run identically in both runs,\nregardless of your setting of GIT_TEST_PROTOCOL_VERSION. So there's\nvalue if you're only running the suite once in getting full coverage,\nbut if you are literally going to run it with and without, then you're\nrunning the exact same code twice for no reason. And you have to run it\nboth with and without, since otherwise all of the _other_ tests aren't\nseeing both options.\n\n> IOW the point of these tests is to piggy-back on the tests that *aren't*\n> aware of the feature you're trying to test. So\n> e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the\n> commit graph, and *also* those tests that are explicitly aware of the\n> commit graph, including those that for some reason would want to test\n> for the case where it isn't enabled (to e.g. test that --contains works\n> without the graph).\n> \n> Otherwise I can't say \"I care more about GIT_TEST_COMMIT_GRAPH=true than\n> not\", and run just one test run with that, because I'll have blind spots\n> in the commit graph tests themselves, and would then need to do another\n> run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage.\n\nSo if we are still talking about the same 2-test setup above, which only\nhas a GIT_TEST_PROTOCOL_VERSION override, then yeah, I get the point of\nbeing able to run a \"stock\" test, and then one with _both_ of the flags\n(GIT_TEST_PROTOCOL_VERSION and GIT_TEST_COMMIT_GRAPH) because you don't\nquite know how each test will interact with all of the \"other\" flags.\n\nSo now I'm not 100% sure I understand what you're talking about, but I\nthink maybe we are actually in agreement. ;)\n\n-Peff\n"},{"id":"368656","messageId":"20190207000116.55126-1-jonathantanmy@google.com","threadId":"50406","inReplyTo":"87d0o4lgtr.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/8] tests: define GIT_TEST_PROTOCOL_VERSION","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-07T00:01:16Z","receivedAt":"2019-02-07T00:01:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > @@ -327,6 +327,9 @@ marked strings\" in po/README for 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> > +\n> \n> In my version\n> (https://public-inbox.org/git/20181213155817.27666-6-avarab@gmail.com/)\n> I didn't have this \"if it is less than n\" caveat. I expect that helped\n> with making some tests that were setting e.g. protocol.version=2 Just\n> Work, is that the reason for this?\n\nYes, that's right. I thought that there is not much value in testing\ntests that are explicitly protocol v2 as another protocol, since there\nwas a reason in the first place why the test writer wanted to test it\nwith v2.\n\n> Mine also had more docs here, but maybe telling people that they can use\n> \"env\" is too much...\n\nWith the ability to set =0 to effectively disable the option (because\nthe minimum of 0 and 0/1/2 is 0/1/2), I thought it was less important.\n"},{"id":"368701","messageId":"877eeblvp5.fsf@evledraar.gmail.com","threadId":"50406","inReplyTo":"20190206230813.GC19901@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-07T10:49:42Z","receivedAt":"2019-02-07T10:49:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 07 2019, Jeff King wrote:\n\n> On Wed, Feb 06, 2019 at 11:20:32PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> >> So far we've had the convention that these GIT_TEST_* variables,\n>> >> e.g. the one for the commit graph, work the same way. Thus we guarantee\n>> >> that we get (in theory) 100% coverage even when running the tests in\n>> >> this special mode. I think it's better to keep it as-is.\n>> >\n>> > But what's the point of that? Don't you always have to run the test\n>> > suite _twice_, once with the special variable and once without?\n>> > Otherwise, you are not testing one case or the other.\n>> >\n>> > Or are you arguing that one might set many special variables in one go\n>> > (to prefer running the suite only twice, instead of 2^N times). In which\n>> > case we are better off running the test (as opposed to skipping it), as\n>> > it might use one of the _other_ special variables besides\n>> > GIT_TEST_PROTOCOL_VERSION.\n>> >\n>> > I can buy that line of reasoning. It still doesn't cover all cases that\n>> > a true 2^N test would, but that clearly isn't going to be practical.\n>>\n>> Maybe I'm misunderstanding what you're proposing, but as an example,\n>> let's say the test suite is just these two tests:\n>>\n>>     test_expect_success 'some unrelated thing' '...'\n>>     test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...'\n>>\n>> And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test\n>> with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason,\n>>\n>> I'd still like both tests to be run, not just 1/2 with\n>> GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly\n>> testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a\n>> GIT_TEST_PROTOCOL_VERSION=1.\n>\n> But that's my \"why\". The second test will run identically in both runs,\n> regardless of your setting of GIT_TEST_PROTOCOL_VERSION. So there's\n> value if you're only running the suite once in getting full coverage,\n> but if you are literally going to run it with and without, then you're\n> running the exact same code twice for no reason. And you have to run it\n> both with and without, since otherwise all of the _other_ tests aren't\n> seeing both options.\n\nYeah, by always running the 2nd test regardless of what\nGIT_TEST_PROTOCOL_VERSION=* is set to we're wasting CPU if we know we're\ngoing to run both with GIT_TEST_PROTOCOL_VERSION=1 and\nGIT_TEST_PROTOCOL_VERSION=2.\n\nBut we don't know that, and in terms of CPU & time the tests that rely\non any given GIT_TEST_* flag are such a tiny part of the test suite,\nthat I think it's fine to run them twice in such a scenario to guard\nagainst the case when we just run in one more or the other, and not\nboth.\n\n>> IOW the point of these tests is to piggy-back on the tests that *aren't*\n>> aware of the feature you're trying to test. So\n>> e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the\n>> commit graph, and *also* those tests that are explicitly aware of the\n>> commit graph, including those that for some reason would want to test\n>> for the case where it isn't enabled (to e.g. test that --contains works\n>> without the graph).\n>>\n>> Otherwise I can't say \"I care more about GIT_TEST_COMMIT_GRAPH=true than\n>> not\", and run just one test run with that, because I'll have blind spots\n>> in the commit graph tests themselves, and would then need to do another\n>> run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage.\n>\n> So if we are still talking about the same 2-test setup above, which only\n> has a GIT_TEST_PROTOCOL_VERSION override, then yeah, I get the point of\n> being able to run a \"stock\" test, and then one with _both_ of the flags\n> (GIT_TEST_PROTOCOL_VERSION and GIT_TEST_COMMIT_GRAPH) because you don't\n> quite know how each test will interact with all of the \"other\" flags.\n\nYeah that's another reason not to skip them, although we could imagine a\nprereq where we skip the v2 tests if GIT_TEST_PROTOCOL_VERSION=1 is set\n*and* no other GIT_TEST_*=* flag is set, but I think that would also be\na bad idea.\n\n> So now I'm not 100% sure I understand what you're talking about, but I\n> think maybe we are actually in agreement. ;)\n\nI think there's two ways to view these GIT_TEST_FOO=BAR facilities:\n\n 1. Run all tests with \"FOO\" set to \"BAR\", skip those (via prereq) we\n    know would break in that mode.\n\n 2. Ditto, but if a test says \"I'm a test for FOO=!BAR\" leave it alone.\n\n#2 is what we have now. I read your\n<20190206213458.GC12737@sigill.intra.peff.net> as suggesting that we\nshould move to #1.\n\nYou're correct that moving to #1 would force us to more exhaustively\nmark things so that if the default moves to \"BAR\" we just have to flip a\nswitch.\n"},{"id":"369026","messageId":"20190211192054.80935-1-jonathantanmy@google.com","threadId":"50406","inReplyTo":"20190206212928.GB12737@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] remote-curl: in v2, fill credentials if needed","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-11T19:20:54Z","receivedAt":"2019-02-11T19:21:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Tue, Feb 05, 2019 at 04:21:22PM -0800, Jonathan Tan wrote:\n> \n> > In post_rpc(), remote-curl calls credential_fill() if HTTP_REAUTH is\n> > returned, but this is not true in proxy_request(). Do this in\n> > proxy_request() too.\n> \n> Can we do this as a general rule? If we look at the code in post_rpc(),\n> there are two cases: when large_request is set and when it is not.\n> \n> When it's not, we have the whole request in a buffer, and we can happily\n> resend it.\n> \n> But when it's not, we cannot restart it, because we'll have thrown away\n> some of the data. So we send an initial probe_rpc() as a sanity check.\n> If that works and we later get a 401 on the real request, we still fail\n> anyway.\n> \n> In the case of proxy_request(), we don't know ahead of time whether the\n> request is large or not; we just proxy the data through. And we don't do\n> the probe thing at all. So wouldn't we dropping some data for the\n> follow-up request?\n\nThanks - I'll look into this. Maybe the best way is to somehow make the\nv2 code path also use post_rpc() - I'll see if that's possible.\n\nIn the meantime, do you have any other opinions on the other patches,\nbesides introducing a prereq [1]? I don't have any strong opinions for\nor against this, so I didn't reply, but I slightly prefer not having the\nprereq so that test readers and authors don't need to juggle so many\nvariables in their heads.\n\nIf everything looks good, I'll suggest that we drop this patch from this\npatch set for me to work on it independently. (Having said that, this\npatch set is based on js/protocol-advertise-multi, which is still under\nreview, so it is not so urgent.)\n\n[1] https://public-inbox.org/git/20190206213458.GC12737@sigill.intra.peff.net/\n"},{"id":"369030","messageId":"20190211201959.GA9072@sigill.intra.peff.net","threadId":"50406","inReplyTo":"6e0c6aa9a71d4192591ed406735684cd15a0e3b9.1549411880.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/8] tests: define GIT_TEST_PROTOCOL_VERSION","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-11T20:20:00Z","receivedAt":"2019-02-11T20:20:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 05, 2019 at 04:21:15PM -0800, Jonathan Tan wrote:\n\n> Define a GIT_TEST_PROTOCOL_VERSION environment variable meant to be used\n> from tests. When set, this ensures protocol.version is at least the\n> given value, allowing the entire test suite to be run as if this\n> configuration is in place for all repositories.\n> \n> As of this patch, all tests pass whether GIT_TEST_PROTOCOL_VERSION is\n> unset, set to 0, or set to 1. Some tests fail when\n> GIT_TEST_PROTOCOL_VERSION is set to 2, but this will be dealt with in\n> subsequent patches.\n\nMakes sense. The \"at least\" part made me scratch my head at first, but\nyour explanation in response to Ævar made sense.\n\nTwo minor nits:\n\n> diff --git a/protocol.c b/protocol.c\n> index 5664bd7a05..c7a735bfa2 100644\n> --- a/protocol.c\n> +++ b/protocol.c\n> @@ -42,6 +42,10 @@ static const char *format_protocol_version(enum protocol_version version)\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\nWe've discussed recently how the return value from getenv() isn't\nstable. It looks like we could assign it much closer to the point-of-use\nhere (which still isn't 100% foolproof, but I think is something we\ncould encourage as a general pattern, and mostly works due to our\nring-buffer technique).\n\nI.e., right before this conditional:\n\n> +\n> +\tif (git_test_v && strlen(git_test_v)) {\n\nIt's more idiomatic in our code base to check for a non-empty string as:\n\n  if (git_test_v && *git_test_v)\n\nthough obviously that's pretty minor.\n\n-Peff\n"},{"id":"369031","messageId":"20190211203011.GB9072@sigill.intra.peff.net","threadId":"50406","inReplyTo":"9b9061985202ec022cc562637d7f62ea599e7d8c.1549411880.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/8] tests: always test fetch of unreachable with v0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-11T20:30:11Z","receivedAt":"2019-02-11T20:30:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote:\n\n> Some tests check that fetching an unreachable object fails, but protocol\n> v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these\n> tests are always run using protocol v0.\n\nI think this is reasonable, but just musing on a few things:\n\n  1. Are we sure going forward that we want to retain this behavior? I\n     feel like we discussed this on the list recently with no real\n     conclusion, but I'm having trouble digging it up in the archive.\n\n  2. If it does change, is there any way we could automatically find\n     spots like this that would then need to be undone? I cannot think\n     of a good solution, and I don't think it's a show-stopper not to\n     have one, but I thought I'd put it forward as a concept.\n\n-Peff\n"},{"id":"369033","messageId":"20190211203827.GA9010@sigill.intra.peff.net","threadId":"50406","inReplyTo":"20190211192054.80935-1-jonathantanmy@google.com","subject":"Re: [PATCH 8/8] remote-curl: in v2, fill credentials if needed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-11T20:38:27Z","receivedAt":"2019-02-11T20:38:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 11, 2019 at 11:20:54AM -0800, Jonathan Tan wrote:\n\n> > In the case of proxy_request(), we don't know ahead of time whether the\n> > request is large or not; we just proxy the data through. And we don't do\n> > the probe thing at all. So wouldn't we dropping some data for the\n> > follow-up request?\n> \n> Thanks - I'll look into this. Maybe the best way is to somehow make the\n> v2 code path also use post_rpc() - I'll see if that's possible.\n\nYeah, that makes sense.\n\n> In the meantime, do you have any other opinions on the other patches,\n> besides introducing a prereq [1]? I don't have any strong opinions for\n> or against this, so I didn't reply, but I slightly prefer not having the\n> prereq so that test readers and authors don't need to juggle so many\n> variables in their heads.\n\nI think Ævar convinced me that the way you've done it is the right way,\nso ignore my earlier comments. :)\n\nI just took another pass and left a few very minor nits, but it all\nlooks good overall.\n\n> If everything looks good, I'll suggest that we drop this patch from this\n> patch set for me to work on it independently. (Having said that, this\n> patch set is based on js/protocol-advertise-multi, which is still under\n> review, so it is not so urgent.)\n\nYeah, that's fine by me (with or without my nits addressed).\n\n-Peff\n"},{"id":"369333","messageId":"20190214195825.125751-1-jonathantanmy@google.com","threadId":"50406","inReplyTo":"20190211203011.GB9072@sigill.intra.peff.net","subject":"Re: [PATCH 2/8] tests: always test fetch of unreachable with v0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-14T19:58:25Z","receivedAt":"2019-02-14T19:58:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote:\n> \n> > Some tests check that fetching an unreachable object fails, but protocol\n> > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these\n> > tests are always run using protocol v0.\n> \n> I think this is reasonable, but just musing on a few things:\n> \n>   1. Are we sure going forward that we want to retain this behavior? I\n>      feel like we discussed this on the list recently with no real\n>      conclusion, but I'm having trouble digging it up in the archive.\n\nOne such discussion is here:\nhttps://public-inbox.org/git/20181214101232.GC13465@sigill.intra.peff.net/\n\n>   2. If it does change, is there any way we could automatically find\n>      spots like this that would then need to be undone? I cannot think\n>      of a good solution, and I don't think it's a show-stopper not to\n>      have one, but I thought I'd put it forward as a concept.\n\nWe can do so now either by \"blaming\" one and finding the originating\ncommit, or by searching for \"support fetching unadvertised objects\" (I\nused the same comment everywhere in the commit [1] so that people can do\nthis), but I don't know how to enforce this for future work. (We can add\na special variable, but I think it's unnecessary and we can't enforce\nthat people use that new special variable instead of\nGIT_TEST_PROTOCOL_VERSION anyway.)\n\n[1] https://public-inbox.org/git/9b9061985202ec022cc562637d7f62ea599e7d8c.1549411880.git.jonathantanmy@google.com/\n"},{"id":"369809","messageId":"20190221134954.GC21406@sigill.intra.peff.net","threadId":"50406","inReplyTo":"20190214195825.125751-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/8] tests: always test fetch of unreachable with v0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-21T13:49:54Z","receivedAt":"2019-02-21T13:49:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 14, 2019 at 11:58:25AM -0800, Jonathan Tan wrote:\n\n> > On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote:\n> > \n> > > Some tests check that fetching an unreachable object fails, but protocol\n> > > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these\n> > > tests are always run using protocol v0.\n> > \n> > I think this is reasonable, but just musing on a few things:\n> > \n> >   1. Are we sure going forward that we want to retain this behavior? I\n> >      feel like we discussed this on the list recently with no real\n> >      conclusion, but I'm having trouble digging it up in the archive.\n> \n> One such discussion is here:\n> https://public-inbox.org/git/20181214101232.GC13465@sigill.intra.peff.net/\n\nThanks, that was what I was thinking of, though there doesn't seem to\nhave been much discussion in response. :)\n\n> >   2. If it does change, is there any way we could automatically find\n> >      spots like this that would then need to be undone? I cannot think\n> >      of a good solution, and I don't think it's a show-stopper not to\n> >      have one, but I thought I'd put it forward as a concept.\n> \n> We can do so now either by \"blaming\" one and finding the originating\n> commit, or by searching for \"support fetching unadvertised objects\" (I\n> used the same comment everywhere in the commit [1] so that people can do\n> this), but I don't know how to enforce this for future work. (We can add\n> a special variable, but I think it's unnecessary and we can't enforce\n> that people use that new special variable instead of\n> GIT_TEST_PROTOCOL_VERSION anyway.)\n\nYeah, I think we can find them once we know they need fixing. I was more\nwondering whether we would remember to do so. I.e., is there a way the\ntest suite could remind us when our assumptions change. I can't think of\nan easy way to do so, though.\n\n-Peff\n"},{"id":"369978","messageId":"xmqqimxbpn2p.fsf@gitster-ct.c.googlers.com","threadId":"50406","inReplyTo":"20190221134954.GC21406@sigill.intra.peff.net","subject":"Re: [PATCH 2/8] tests: always test fetch of unreachable with v0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-22T20:47:42Z","receivedAt":"2019-02-22T20:47:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Feb 14, 2019 at 11:58:25AM -0800, Jonathan Tan wrote:\n>\n>> > On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote:\n>> > \n>> > > Some tests check that fetching an unreachable object fails, but protocol\n>> > > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these\n>> > > tests are always run using protocol v0.\n>> > \n>> > I think this is reasonable, but just musing on a few things:\n>> > \n>> >   1. Are we sure going forward that we want to retain this behavior? I\n>> >      feel like we discussed this on the list recently with no real\n>> >      conclusion, but I'm having trouble digging it up in the archive.\n>> \n>> One such discussion is here:\n>> https://public-inbox.org/git/20181214101232.GC13465@sigill.intra.peff.net/\n>\n> Thanks, that was what I was thinking of, though there doesn't seem to\n> have been much discussion in response. :)\n>\n>> >   2. If it does change, is there any way we could automatically find\n>> >      spots like this that would then need to be undone? I cannot think\n>> >      of a good solution, and I don't think it's a show-stopper not to\n>> >      have one, but I thought I'd put it forward as a concept.\n>> \n>> We can do so now either by \"blaming\" one and finding the originating\n>> commit, or by searching for \"support fetching unadvertised objects\" (I\n>> used the same comment everywhere in the commit [1] so that people can do\n>> this), but I don't know how to enforce this for future work. (We can add\n>> a special variable, but I think it's unnecessary and we can't enforce\n>> that people use that new special variable instead of\n>> GIT_TEST_PROTOCOL_VERSION anyway.)\n>\n> Yeah, I think we can find them once we know they need fixing. I was more\n> wondering whether we would remember to do so. I.e., is there a way the\n> test suite could remind us when our assumptions change. I can't think of\n> an easy way to do so, though.\n\nPerhaps looking it a different way may help.  Instead of saying \"v2\nwill not protect unreachable objects, so this test must be run with\nv0\", think of it like \"Git ought to protect unreachable objects, so\ntest with different versions of protocols to make sure all satisfy\nthe requirement---for now, v2 is known to be broken, so write it\nwith test_expect_failure\".  IOW, have one test for each version,\nsome of them may document a known breakage.\n\n\n"},{"id":"370017","messageId":"20190223132509.GC22403@sigill.intra.peff.net","threadId":"50406","inReplyTo":"xmqqimxbpn2p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/8] tests: always test fetch of unreachable with v0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-23T13:25:09Z","receivedAt":"2019-02-23T13:25:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2019 at 12:47:42PM -0800, Junio C Hamano wrote:\n\n> >> We can do so now either by \"blaming\" one and finding the originating\n> >> commit, or by searching for \"support fetching unadvertised objects\" (I\n> >> used the same comment everywhere in the commit [1] so that people can do\n> >> this), but I don't know how to enforce this for future work. (We can add\n> >> a special variable, but I think it's unnecessary and we can't enforce\n> >> that people use that new special variable instead of\n> >> GIT_TEST_PROTOCOL_VERSION anyway.)\n> >\n> > Yeah, I think we can find them once we know they need fixing. I was more\n> > wondering whether we would remember to do so. I.e., is there a way the\n> > test suite could remind us when our assumptions change. I can't think of\n> > an easy way to do so, though.\n> \n> Perhaps looking it a different way may help.  Instead of saying \"v2\n> will not protect unreachable objects, so this test must be run with\n> v0\", think of it like \"Git ought to protect unreachable objects, so\n> test with different versions of protocols to make sure all satisfy\n> the requirement---for now, v2 is known to be broken, so write it\n> with test_expect_failure\".  IOW, have one test for each version,\n> some of them may document a known breakage.\n\nAh, yeah, that would work. It means writing out the test twice, once for\neach protocol version. But if we're explicitly expecting two different\nbehaviors, then that's exactly what we ought to be doing.\n\nI'm still not sure if there's consensus on whether the v2 behavior is a\nproblem or not, though.\n\n-Peff\n"}]}