[PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches

[PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches

[Jonathan Tan]

This is on the latest master (8feddda32c ("Fifth batch for 2.21",
2019-02-05)) + js/protocol-advertise-multi.

This is a resend of [1], which was built on the result of merging many
branches. Now that most of the branches have been merged to master, I
have rebased it on master. The only branch that I needed to merge was
js/protocol-advertise-multi.

js/protocol-advertise-multi has not been merged to next yet, but the
general idea seems good to me, and I am making review of that patch one
of my priorities. But if anyone is concerned that this set will be
delayed by js/protocol-advertise-multi, I can extract the parts I need
into a patch in this set (most likely just the part that unconditionally
calls ssh with SendEnv in connect.c), although it is difficult to
explain the necessity of that without reference to
js/protocol-advertise-multi. Let me know what you think.

Besides the rebase, this is unchanged from [1]. There is some discussion
there between Ævar, Junio, and I, thinking that it is OK to merge this
even if we didn't eliminate all errors. Right now, only 3 test cases
fail with GIT_TEST_PROTOCOL_VERSION=2.

Patch 1 implements the test variable, patches 2-7 eliminate false
negatives (with explanations), and patch 8 fixes a genuine bug.

[1] https://public-inbox.org/git/cover.1547677183.git.jonathantanmy@google.com/

Jonathan Tan (8):
  tests: define GIT_TEST_PROTOCOL_VERSION
  tests: always test fetch of unreachable with v0
  t5503: fix overspecification of trace expectation
  t5512: compensate for v0 only sending HEAD symrefs
  t5700: only run with protocol version 1
  tests: fix protocol version for overspecifications
  t5552: compensate for v2 filtering ref adv.
  remote-curl: in v2, fill credentials if needed

 protocol.c                           | 17 ++++++++--
 remote-curl.c                        |  9 +++++-
 t/README                             |  3 ++
 t/t5400-send-pack.sh                 |  2 +-
 t/t5500-fetch-pack.sh                |  4 ++-
 t/t5503-tagfollow.sh                 |  2 +-
 t/t5512-ls-remote.sh                 | 18 ++++++++---
 t/t5515-fetch-merge-logic.sh         |  4 +++
 t/t5516-fetch-push.sh                | 22 ++++++++++---
 t/t5539-fetch-http-shallow.sh        |  5 ++-
 t/t5541-http-push-smart.sh           | 14 +++++++--
 t/t5551-http-fetch-smart.sh          | 47 +++++++++++++++++++++-------
 t/t5552-skipping-fetch-negotiator.sh |  5 ++-
 t/t5700-protocol-v1.sh               |  3 ++
 t/t7406-submodule-update.sh          |  5 ++-
 15 files changed, 128 insertions(+), 32 deletions(-)

-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH 1/8] tests: define GIT_TEST_PROTOCOL_VERSION

[Jonathan Tan]

Define a GIT_TEST_PROTOCOL_VERSION environment variable meant to be used
from tests. When set, this ensures protocol.version is at least the
given value, allowing the entire test suite to be run as if this
configuration is in place for all repositories.

As of this patch, all tests pass whether GIT_TEST_PROTOCOL_VERSION is
unset, set to 0, or set to 1. Some tests fail when
GIT_TEST_PROTOCOL_VERSION is set to 2, but this will be dealt with in
subsequent patches.

This is based on work by Ævar Arnfjörð Bjarmason.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 protocol.c                  | 17 +++++++++++++++--
 t/README                    |  3 +++
 t/t5400-send-pack.sh        |  2 +-
 t/t5551-http-fetch-smart.sh |  3 ++-
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/protocol.c b/protocol.c
index 5664bd7a05..c7a735bfa2 100644
--- a/protocol.c
+++ b/protocol.c
@@ -42,6 +42,10 @@ static const char *format_protocol_version(enum protocol_version version)
 enum protocol_version get_protocol_version_config(void)
 {
 	const char *value;
+	enum protocol_version retval = protocol_v0;
+	const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION";
+	const char *git_test_v = getenv(git_test_k);
+
 	if (!git_config_get_string_const("protocol.version", &value)) {
 		enum protocol_version version = parse_protocol_version(value);
 
@@ -49,10 +53,19 @@ enum protocol_version get_protocol_version_config(void)
 			die("unknown value for config 'protocol.version': %s",
 			    value);
 
-		return version;
+		retval = version;
+	}
+
+	if (git_test_v && strlen(git_test_v)) {
+		enum protocol_version env = parse_protocol_version(git_test_v);
+
+		if (env == protocol_unknown_version)
+			die("unknown value for %s: %s", git_test_k, git_test_v);
+		if (retval < env)
+			retval = env;
 	}
 
-	return protocol_v0;
+	return retval;
 }
 
 void register_allowed_protocol_version(enum protocol_version version)
diff --git a/t/README b/t/README
index 25864ec883..21e941eb94 100644
--- a/t/README
+++ b/t/README
@@ -327,6 +327,9 @@ marked strings" in po/README for details.
 GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
 test suite. Accept any boolean values that are accepted by git-config.
 
+GIT_TEST_PROTOCOL_VERSION=<n>, when set, overrides the
+'protocol.version' setting to n if it is less than n.
+
 GIT_TEST_FULL_IN_PACK_ARRAY=<boolean> exercises the uncommon
 pack-objects code path where there are more than 1024 packs even if
 the actual number of packs in repository is below this limit. Accept
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index f1932ea431..571d620aed 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' '
 	$shared .have
 	EOF
 
-	GIT_TRACE_PACKET=$(pwd)/trace \
+	GIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \
 	    git push \
 		--receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \
 		fork HEAD:foo &&
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index a60dd907bd..8f620e0a35 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -44,7 +44,8 @@ test_expect_success 'clone http repository' '
 	< Cache-Control: no-cache, max-age=0, must-revalidate
 	< Content-Type: application/x-git-upload-pack-result
 	EOF
-	GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
+	GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \
+		git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
 	test_cmp file clone/file &&
 	tr '\''\015'\'' Q <err |
 	sed -e "
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH 1/8] tests: define GIT_TEST_PROTOCOL_VERSION

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 06 2019, Jonathan Tan wrote:

> Define a GIT_TEST_PROTOCOL_VERSION environment variable meant to be used
> from tests. When set, this ensures protocol.version is at least the
> given value, allowing the entire test suite to be run as if this
> configuration is in place for all repositories.
>
> As of this patch, all tests pass whether GIT_TEST_PROTOCOL_VERSION is
> unset, set to 0, or set to 1. Some tests fail when
> GIT_TEST_PROTOCOL_VERSION is set to 2, but this will be dealt with in
> subsequent patches.
>
> This is based on work by Ævar Arnfjörð Bjarmason.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  protocol.c                  | 17 +++++++++++++++--
>  t/README                    |  3 +++
>  t/t5400-send-pack.sh        |  2 +-
>  t/t5551-http-fetch-smart.sh |  3 ++-
>  4 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/protocol.c b/protocol.c
> index 5664bd7a05..c7a735bfa2 100644
> --- a/protocol.c
> +++ b/protocol.c
> @@ -42,6 +42,10 @@ static const char *format_protocol_version(enum protocol_version version)
>  enum protocol_version get_protocol_version_config(void)
>  {
>  	const char *value;
> +	enum protocol_version retval = protocol_v0;
> +	const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION";
> +	const char *git_test_v = getenv(git_test_k);
> +
>  	if (!git_config_get_string_const("protocol.version", &value)) {
>  		enum protocol_version version = parse_protocol_version(value);
>
> @@ -49,10 +53,19 @@ enum protocol_version get_protocol_version_config(void)
>  			die("unknown value for config 'protocol.version': %s",
>  			    value);
>
> -		return version;
> +		retval = version;
> +	}
> +
> +	if (git_test_v && strlen(git_test_v)) {
> +		enum protocol_version env = parse_protocol_version(git_test_v);
> +
> +		if (env == protocol_unknown_version)
> +			die("unknown value for %s: %s", git_test_k, git_test_v);
> +		if (retval < env)
> +			retval = env;
>  	}
>
> -	return protocol_v0;
> +	return retval;
>  }
>
>  void register_allowed_protocol_version(enum protocol_version version)
> diff --git a/t/README b/t/README
> index 25864ec883..21e941eb94 100644
> --- a/t/README
> +++ b/t/README
> @@ -327,6 +327,9 @@ marked strings" in po/README for details.
>  GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
>  test suite. Accept any boolean values that are accepted by git-config.
>
> +GIT_TEST_PROTOCOL_VERSION=<n>, when set, overrides the
> +'protocol.version' setting to n if it is less than n.
> +

In my version
(https://public-inbox.org/git/20181213155817.27666-6-avarab@gmail.com/)
I didn't have this "if it is less than n" caveat. I expect that helped
with making some tests that were setting e.g. protocol.version=2 Just
Work, is that the reason for this?

Mine also had more docs here, but maybe telling people that they can use
"env" is too much...


>  GIT_TEST_FULL_IN_PACK_ARRAY=<boolean> exercises the uncommon
>  pack-objects code path where there are more than 1024 packs even if
>  the actual number of packs in repository is below this limit. Accept
> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> index f1932ea431..571d620aed 100755
> --- a/t/t5400-send-pack.sh
> +++ b/t/t5400-send-pack.sh
> @@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' '
>  	$shared .have
>  	EOF
>
> -	GIT_TRACE_PACKET=$(pwd)/trace \
> +	GIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \
>  	    git push \
>  		--receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \
>  		fork HEAD:foo &&
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index a60dd907bd..8f620e0a35 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -44,7 +44,8 @@ test_expect_success 'clone http repository' '
>  	< Cache-Control: no-cache, max-age=0, must-revalidate
>  	< Content-Type: application/x-git-upload-pack-result
>  	EOF
> -	GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
> +	GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \
> +		git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
>  	test_cmp file clone/file &&
>  	tr '\''\015'\'' Q <err |
>  	sed -e "

Re: [PATCH 1/8] tests: define GIT_TEST_PROTOCOL_VERSION

[Jonathan Tan]

> > @@ -327,6 +327,9 @@ marked strings" in po/README for details.
> >  GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
> >  test suite. Accept any boolean values that are accepted by git-config.
> >
> > +GIT_TEST_PROTOCOL_VERSION=<n>, when set, overrides the
> > +'protocol.version' setting to n if it is less than n.
> > +
> 
> In my version
> (https://public-inbox.org/git/20181213155817.27666-6-avarab@gmail.com/)
> I didn't have this "if it is less than n" caveat. I expect that helped
> with making some tests that were setting e.g. protocol.version=2 Just
> Work, is that the reason for this?

Yes, that's right. I thought that there is not much value in testing
tests that are explicitly protocol v2 as another protocol, since there
was a reason in the first place why the test writer wanted to test it
with v2.

> Mine also had more docs here, but maybe telling people that they can use
> "env" is too much...

With the ability to set =0 to effectively disable the option (because
the minimum of 0 and 0/1/2 is 0/1/2), I thought it was less important.

Re: [PATCH 1/8] tests: define GIT_TEST_PROTOCOL_VERSION

[Jeff King]

On Tue, Feb 05, 2019 at 04:21:15PM -0800, Jonathan Tan wrote:

> Define a GIT_TEST_PROTOCOL_VERSION environment variable meant to be used
> from tests. When set, this ensures protocol.version is at least the
> given value, allowing the entire test suite to be run as if this
> configuration is in place for all repositories.
> 
> As of this patch, all tests pass whether GIT_TEST_PROTOCOL_VERSION is
> unset, set to 0, or set to 1. Some tests fail when
> GIT_TEST_PROTOCOL_VERSION is set to 2, but this will be dealt with in
> subsequent patches.

Makes sense. The "at least" part made me scratch my head at first, but
your explanation in response to Ævar made sense.

Two minor nits:

> diff --git a/protocol.c b/protocol.c
> index 5664bd7a05..c7a735bfa2 100644
> --- a/protocol.c
> +++ b/protocol.c
> @@ -42,6 +42,10 @@ static const char *format_protocol_version(enum protocol_version version)
>  enum protocol_version get_protocol_version_config(void)
>  {
>  	const char *value;
> +	enum protocol_version retval = protocol_v0;
> +	const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION";
> +	const char *git_test_v = getenv(git_test_k);

We've discussed recently how the return value from getenv() isn't
stable. It looks like we could assign it much closer to the point-of-use
here (which still isn't 100% foolproof, but I think is something we
could encourage as a general pattern, and mostly works due to our
ring-buffer technique).

I.e., right before this conditional:

> +
> +	if (git_test_v && strlen(git_test_v)) {

It's more idiomatic in our code base to check for a non-empty string as:

  if (git_test_v && *git_test_v)

though obviously that's pretty minor.

-Peff

[PATCH 2/8] tests: always test fetch of unreachable with v0

[Jonathan Tan]

Some tests check that fetching an unreachable object fails, but protocol
v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these
tests are always run using protocol v0.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5500-fetch-pack.sh       |  4 +++-
 t/t5516-fetch-push.sh       | 22 +++++++++++++++++-----
 t/t5551-http-fetch-smart.sh | 10 ++++++++--
 t/t7406-submodule-update.sh |  5 ++++-
 4 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 49c540b1e1..0ef4d6f20c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -636,7 +636,9 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
 	test_commit -C server 6 &&
 
 	git init client &&
-	test_must_fail git -C client fetch-pack ../server \
+	# Some protocol versions (e.g. 2) support fetching
+	# unadvertised objects, so restrict this test to v0.
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
 		$(git -C server rev-parse refs/heads/master^) 2>err &&
 	test_i18ngrep "Server does not allow request for unadvertised object" err
 '
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 37e8e80893..4bfbb79654 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1147,8 +1147,12 @@ test_expect_success 'fetch exact SHA1' '
 		git prune &&
 		test_must_fail git cat-file -t $the_commit &&
 
+		# Some protocol versions (e.g. 2) support fetching
+		# unadvertised objects, so restrict this test to v0.
+
 		# fetching the hidden object should fail by default
-		test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
+		test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+			git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
 		test_i18ngrep "Server does not allow request for unadvertised object" err &&
 		test_must_fail git rev-parse --verify refs/heads/copy &&
 
@@ -1204,7 +1208,10 @@ do
 		mk_empty shallow &&
 		(
 			cd shallow &&
-			test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&
+			# Some protocol versions (e.g. 2) support fetching
+			# unadvertised objects, so restrict this test to v0.
+			test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+				git fetch --depth=1 ../testrepo/.git $SHA1 &&
 			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
 			git fetch --depth=1 ../testrepo/.git $SHA1 &&
 			git cat-file commit $SHA1
@@ -1232,15 +1239,20 @@ do
 		mk_empty shallow &&
 		(
 			cd shallow &&
-			test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&
-			test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&
+			# Some protocol versions (e.g. 2) support fetching
+			# unadvertised objects, so restrict this test to v0.
+			test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+				git fetch ../testrepo/.git $SHA1_3 &&
+			test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+				git fetch ../testrepo/.git $SHA1_1 &&
 			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
 			git fetch ../testrepo/.git $SHA1_1 &&
 			git cat-file commit $SHA1_1 &&
 			test_must_fail git cat-file commit $SHA1_2 &&
 			git fetch ../testrepo/.git $SHA1_2 &&
 			git cat-file commit $SHA1_2 &&
-			test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3
+			test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+				git fetch ../testrepo/.git $SHA1_3
 		)
 	'
 done
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8f620e0a35..d2661eb338 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -308,7 +308,10 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
 
 	git init --bare test_reachable.git &&
 	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
-	test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+	# Some protocol versions (e.g. 2) support fetching
+	# unadvertised objects, so restrict this test to v0.
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+		git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
 '
 
 test_expect_success 'test allowanysha1inwant with unreachable' '
@@ -327,7 +330,10 @@ test_expect_success 'test allowanysha1inwant with unreachable' '
 
 	git init --bare test_reachable.git &&
 	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
-	test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
+	# Some protocol versions (e.g. 2) support fetching
+	# unadvertised objects, so restrict this test to v0.
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+		git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
 
 	git -C "$server" config uploadpack.allowanysha1inwant 1 &&
 	git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e87164aa8f..c973278300 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -943,7 +943,10 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
 		cd super3 &&
 		sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
 		mv -f .gitmodules.tmp .gitmodules &&
-		test_must_fail git submodule update --init --depth=1 2>actual &&
+		# Some protocol versions (e.g. 2) support fetching
+		# unadvertised objects, so restrict this test to v0.
+		test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+			git submodule update --init --depth=1 2>actual &&
 		test_i18ngrep "Direct fetching of that commit failed." actual &&
 		git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
 		git submodule update --init --depth=1 >actual &&
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH 2/8] tests: always test fetch of unreachable with v0

[Jeff King]

On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote:

> Some tests check that fetching an unreachable object fails, but protocol
> v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these
> tests are always run using protocol v0.

I think this is reasonable, but just musing on a few things:

  1. Are we sure going forward that we want to retain this behavior? I
     feel like we discussed this on the list recently with no real
     conclusion, but I'm having trouble digging it up in the archive.

  2. If it does change, is there any way we could automatically find
     spots like this that would then need to be undone? I cannot think
     of a good solution, and I don't think it's a show-stopper not to
     have one, but I thought I'd put it forward as a concept.

-Peff

Re: [PATCH 2/8] tests: always test fetch of unreachable with v0

[Jonathan Tan]

> On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote:
> 
> > Some tests check that fetching an unreachable object fails, but protocol
> > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these
> > tests are always run using protocol v0.
> 
> I think this is reasonable, but just musing on a few things:
> 
>   1. Are we sure going forward that we want to retain this behavior? I
>      feel like we discussed this on the list recently with no real
>      conclusion, but I'm having trouble digging it up in the archive.

One such discussion is here:
https://public-inbox.org/git/20181214101232.GC13465@sigill.intra.peff.net/

>   2. If it does change, is there any way we could automatically find
>      spots like this that would then need to be undone? I cannot think
>      of a good solution, and I don't think it's a show-stopper not to
>      have one, but I thought I'd put it forward as a concept.

We can do so now either by "blaming" one and finding the originating
commit, or by searching for "support fetching unadvertised objects" (I
used the same comment everywhere in the commit [1] so that people can do
this), but I don't know how to enforce this for future work. (We can add
a special variable, but I think it's unnecessary and we can't enforce
that people use that new special variable instead of
GIT_TEST_PROTOCOL_VERSION anyway.)

[1] https://public-inbox.org/git/9b9061985202ec022cc562637d7f62ea599e7d8c.1549411880.git.jonathantanmy@google.com/

Re: [PATCH 2/8] tests: always test fetch of unreachable with v0

[Jeff King]

On Thu, Feb 14, 2019 at 11:58:25AM -0800, Jonathan Tan wrote:

> > On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote:
> > 
> > > Some tests check that fetching an unreachable object fails, but protocol
> > > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these
> > > tests are always run using protocol v0.
> > 
> > I think this is reasonable, but just musing on a few things:
> > 
> >   1. Are we sure going forward that we want to retain this behavior? I
> >      feel like we discussed this on the list recently with no real
> >      conclusion, but I'm having trouble digging it up in the archive.
> 
> One such discussion is here:
> https://public-inbox.org/git/20181214101232.GC13465@sigill.intra.peff.net/

Thanks, that was what I was thinking of, though there doesn't seem to
have been much discussion in response. :)

> >   2. If it does change, is there any way we could automatically find
> >      spots like this that would then need to be undone? I cannot think
> >      of a good solution, and I don't think it's a show-stopper not to
> >      have one, but I thought I'd put it forward as a concept.
> 
> We can do so now either by "blaming" one and finding the originating
> commit, or by searching for "support fetching unadvertised objects" (I
> used the same comment everywhere in the commit [1] so that people can do
> this), but I don't know how to enforce this for future work. (We can add
> a special variable, but I think it's unnecessary and we can't enforce
> that people use that new special variable instead of
> GIT_TEST_PROTOCOL_VERSION anyway.)

Yeah, I think we can find them once we know they need fixing. I was more
wondering whether we would remember to do so. I.e., is there a way the
test suite could remind us when our assumptions change. I can't think of
an easy way to do so, though.

-Peff

Re: [PATCH 2/8] tests: always test fetch of unreachable with v0

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Feb 14, 2019 at 11:58:25AM -0800, Jonathan Tan wrote:
>
>> > On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote:
>> > 
>> > > Some tests check that fetching an unreachable object fails, but protocol
>> > > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these
>> > > tests are always run using protocol v0.
>> > 
>> > I think this is reasonable, but just musing on a few things:
>> > 
>> >   1. Are we sure going forward that we want to retain this behavior? I
>> >      feel like we discussed this on the list recently with no real
>> >      conclusion, but I'm having trouble digging it up in the archive.
>> 
>> One such discussion is here:
>> https://public-inbox.org/git/20181214101232.GC13465@sigill.intra.peff.net/
>
> Thanks, that was what I was thinking of, though there doesn't seem to
> have been much discussion in response. :)
>
>> >   2. If it does change, is there any way we could automatically find
>> >      spots like this that would then need to be undone? I cannot think
>> >      of a good solution, and I don't think it's a show-stopper not to
>> >      have one, but I thought I'd put it forward as a concept.
>> 
>> We can do so now either by "blaming" one and finding the originating
>> commit, or by searching for "support fetching unadvertised objects" (I
>> used the same comment everywhere in the commit [1] so that people can do
>> this), but I don't know how to enforce this for future work. (We can add
>> a special variable, but I think it's unnecessary and we can't enforce
>> that people use that new special variable instead of
>> GIT_TEST_PROTOCOL_VERSION anyway.)
>
> Yeah, I think we can find them once we know they need fixing. I was more
> wondering whether we would remember to do so. I.e., is there a way the
> test suite could remind us when our assumptions change. I can't think of
> an easy way to do so, though.

Perhaps looking it a different way may help.  Instead of saying "v2
will not protect unreachable objects, so this test must be run with
v0", think of it like "Git ought to protect unreachable objects, so
test with different versions of protocols to make sure all satisfy
the requirement---for now, v2 is known to be broken, so write it
with test_expect_failure".  IOW, have one test for each version,
some of them may document a known breakage.

Re: [PATCH 2/8] tests: always test fetch of unreachable with v0

[Jeff King]

On Fri, Feb 22, 2019 at 12:47:42PM -0800, Junio C Hamano wrote:

> >> We can do so now either by "blaming" one and finding the originating
> >> commit, or by searching for "support fetching unadvertised objects" (I
> >> used the same comment everywhere in the commit [1] so that people can do
> >> this), but I don't know how to enforce this for future work. (We can add
> >> a special variable, but I think it's unnecessary and we can't enforce
> >> that people use that new special variable instead of
> >> GIT_TEST_PROTOCOL_VERSION anyway.)
> >
> > Yeah, I think we can find them once we know they need fixing. I was more
> > wondering whether we would remember to do so. I.e., is there a way the
> > test suite could remind us when our assumptions change. I can't think of
> > an easy way to do so, though.
> 
> Perhaps looking it a different way may help.  Instead of saying "v2
> will not protect unreachable objects, so this test must be run with
> v0", think of it like "Git ought to protect unreachable objects, so
> test with different versions of protocols to make sure all satisfy
> the requirement---for now, v2 is known to be broken, so write it
> with test_expect_failure".  IOW, have one test for each version,
> some of them may document a known breakage.

Ah, yeah, that would work. It means writing out the test twice, once for
each protocol version. But if we're explicitly expecting two different
behaviors, then that's exactly what we ought to be doing.

I'm still not sure if there's consensus on whether the v2 behavior is a
problem or not, though.

-Peff

[PATCH 3/8] t5503: fix overspecification of trace expectation

[Jonathan Tan]

In order to extract the wants from a trace, a loop in t5503 currently
breaks if "0000" is found. This works for protocol v0 and v1, but not
v2. Instead, teach t5503 to look specifically for the "want" string,
which is compatible with all protocols.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5503-tagfollow.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 4ca48f0276..6041a4dd32 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -47,7 +47,7 @@ get_needs () {
 	test -s "$1" &&
 	perl -alne '
 		next unless $F[1] eq "upload-pack<";
-		last if $F[2] eq "0000";
+		next unless $F[2] eq "want";
 		print $F[2], " ", $F[3];
 	' "$1"
 }
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH 4/8] t5512: compensate for v0 only sending HEAD symrefs

[Jonathan Tan]

Protocol v2 supports sending non-HEAD symrefs, but this is not true of
protocol v0. Some tests expect protocol v0 behavior, so fix them to use
protocol v0.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5512-ls-remote.sh | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ced15ae122..e3c4a48c85 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -223,7 +223,9 @@ test_expect_success 'ls-remote --symref' '
 	$(git rev-parse refs/tags/mark1.10)	refs/tags/mark1.10
 	$(git rev-parse refs/tags/mark1.2)	refs/tags/mark1.2
 	EOF
-	git ls-remote --symref >actual &&
+	# Protocol v2 supports sending symrefs for refs other than HEAD, so use
+	# protocol v0 here.
+	GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref >actual &&
 	test_cmp expect actual
 '
 
@@ -232,7 +234,9 @@ test_expect_success 'ls-remote with filtered symref (refname)' '
 	ref: refs/heads/master	HEAD
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	HEAD
 	EOF
-	git ls-remote --symref . HEAD >actual &&
+	# Protocol v2 supports sending symrefs for refs other than HEAD, so use
+	# protocol v0 here.
+	GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . HEAD >actual &&
 	test_cmp expect actual
 '
 
@@ -243,7 +247,9 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/foo
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
 	EOF
-	git ls-remote --symref --heads . >actual &&
+	# Protocol v2 supports sending symrefs for refs other than HEAD, so use
+	# protocol v0 here.
+	GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&
 	test_cmp expect actual
 '
 
@@ -252,9 +258,11 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/foo
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
 	EOF
-	git ls-remote --symref --heads . >actual &&
+	# Protocol v2 supports sending symrefs for refs other than HEAD, so use
+	# protocol v0 here.
+	GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&
 	test_cmp expect actual &&
-	git ls-remote --symref . "refs/heads/*" >actual &&
+	GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . "refs/heads/*" >actual &&
 	test_cmp expect actual
 '
 
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH 5/8] t5700: only run with protocol version 1

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5700-protocol-v1.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index 2e56c79233..9857bd0ecb 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -4,6 +4,9 @@ test_description='test git wire-protocol transition'
 
 TEST_NO_CREATE_REPO=1
 
+# This is a protocol-specific test.
+GIT_TEST_PROTOCOL_VERSION=
+
 . ./test-lib.sh
 
 # Test protocol v1 with 'git://' transport
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH 6/8] tests: fix protocol version for overspecifications

[Jonathan Tan]

These tests are also marked with a NEEDSWORK comment.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5515-fetch-merge-logic.sh  |  4 ++++
 t/t5539-fetch-http-shallow.sh |  5 ++++-
 t/t5541-http-push-smart.sh    | 14 ++++++++++++--
 t/t5551-http-fetch-smart.sh   | 34 ++++++++++++++++++++++++++--------
 4 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
index 36b0dbc01c..e55d8474ef 100755
--- a/t/t5515-fetch-merge-logic.sh
+++ b/t/t5515-fetch-merge-logic.sh
@@ -6,6 +6,10 @@
 
 test_description='Merge logic in fetch'
 
+# NEEDSWORK: If the overspecification of the expected result is reduced, we
+# might be able to run this test in all protocol versions.
+GIT_TEST_PROTOCOL_VERSION=
+
 . ./test-lib.sh
 
 LF='
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 5fbf67c446..cdb687b93a 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -67,7 +67,10 @@ test_expect_success 'no shallow lines after receiving ACK ready' '
 		cd clone &&
 		git checkout --orphan newnew &&
 		test_commit new-too &&
-		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch --depth=2 &&
+		# NEEDSWORK: If the overspecification of the expected result is reduced, we
+		# might be able to run this test in all protocol versions.
+		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" GIT_TEST_PROTOCOL_VERSION= \
+			git fetch --depth=2 &&
 		grep "fetch-pack< ACK .* ready" ../trace &&
 		! grep "fetch-pack> done" ../trace
 	)
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 5475afc052..0e3055ab98 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -47,7 +47,12 @@ test_expect_success 'no empty path components' '
 	cd "$ROOT_PATH" &&
 	git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
 
-	check_access_log exp
+	# NEEDSWORK: If the overspecification of the expected result is reduced, we
+	# might be able to run this test in all protocol versions.
+	if test -z "$GIT_TEST_PROTOCOL_VERSION"
+	then
+		check_access_log exp
+	fi
 '
 
 test_expect_success 'clone remote repository' '
@@ -128,7 +133,12 @@ GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
 POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
 EOF
 test_expect_success 'used receive-pack service' '
-	check_access_log exp
+	# NEEDSWORK: If the overspecification of the expected result is reduced, we
+	# might be able to run this test in all protocol versions.
+	if test -z "$GIT_TEST_PROTOCOL_VERSION"
+	then
+		check_access_log exp
+	fi
 '
 
 test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index d2661eb338..e8ed185b33 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -82,12 +82,18 @@ test_expect_success 'clone http repository' '
 		/^< Content-Length: /d
 		/^< Transfer-Encoding: /d
 	" >actual &&
-	sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
-			actual >actual.smudged &&
-	test_cmp exp actual.smudged &&
 
-	grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
-	test_line_count = 2 actual.gzip
+	# NEEDSWORK: If the overspecification of the expected result is reduced, we
+	# might be able to run this test in all protocol versions.
+	if test -z "$GIT_TEST_PROTOCOL_VERSION"
+	then
+		sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
+				actual >actual.smudged &&
+		test_cmp exp actual.smudged &&
+
+		grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
+		test_line_count = 2 actual.gzip
+	fi
 '
 
 test_expect_success 'fetch changes via http' '
@@ -105,7 +111,13 @@ test_expect_success 'used upload-pack service' '
 	GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
 	POST /smart/repo.git/git-upload-pack HTTP/1.1 200
 	EOF
-	check_access_log exp
+
+	# NEEDSWORK: If the overspecification of the expected result is reduced, we
+	# might be able to run this test in all protocol versions.
+	if test -z "$GIT_TEST_PROTOCOL_VERSION"
+	then
+		check_access_log exp
+	fi
 '
 
 test_expect_success 'follow redirects (301)' '
@@ -217,8 +229,14 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
 	git config http.cookiefile cookies.txt &&
 	git config http.savecookies true &&
 	git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
-	tail -3 cookies.txt | sort >cookies_tail.txt &&
-	test_cmp expect_cookies.txt cookies_tail.txt
+
+	# NEEDSWORK: If the overspecification of the expected result is reduced, we
+	# might be able to run this test in all protocol versions.
+	if test -z "$GIT_TEST_PROTOCOL_VERSION"
+	then
+		tail -3 cookies.txt | sort >cookies_tail.txt &&
+		test_cmp expect_cookies.txt cookies_tail.txt
+	fi
 '
 
 test_expect_success 'transfer.hiderefs works over smart-http' '
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH 7/8] t5552: compensate for v2 filtering ref adv.

[Jonathan Tan]

Protocol v2 filters the ref advertisement, but protocol v0 does not. A
test in t5552 uses the ref advertisement, so fix it to use protocol v0.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5552-skipping-fetch-negotiator.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 30857b84a8..8a14be51a1 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -127,7 +127,10 @@ test_expect_success 'use ref advertisement to filter out commits' '
 	# not need to send any ancestors of "c3", but we still need to send "c3"
 	# itself.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	trace_fetch client origin to_fetch &&
+
+	# The ref advertisement itself is filtered when protocol v2 is used, so
+	# use v0.
+	GIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch &&
 	have_sent c5 c4^ c2side &&
 	have_not_sent c4 c4^^ c4^^^
 '
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH 8/8] remote-curl: in v2, fill credentials if needed

[Jonathan Tan]

In post_rpc(), remote-curl calls credential_fill() if HTTP_REAUTH is
returned, but this is not true in proxy_request(). Do this in
proxy_request() too.

When t5551 is run using GIT_TEST_PROTOCOL_VERSION=2, one of the tests
that used to fail now pass.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 remote-curl.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 293bcdb95b..437a8e76d8 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1295,7 +1295,9 @@ static size_t proxy_out(char *buffer, size_t eltsize,
 static int proxy_request(struct proxy_state *p)
 {
 	struct active_request_slot *slot;
+	int err;
 
+retry:
 	slot = get_active_slot();
 
 	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
@@ -1312,7 +1314,12 @@ static int proxy_request(struct proxy_state *p)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p);
 
-	if (run_slot(slot, NULL) != HTTP_OK)
+	err = run_slot(slot, NULL);
+	if (err == HTTP_REAUTH) {
+		credential_fill(&http_auth);
+		goto retry;
+	}
+	if (err != HTTP_OK)
 		return -1;
 
 	return 0;
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH 8/8] remote-curl: in v2, fill credentials if needed

[Jeff King]

On Tue, Feb 05, 2019 at 04:21:22PM -0800, Jonathan Tan wrote:

> In post_rpc(), remote-curl calls credential_fill() if HTTP_REAUTH is
> returned, but this is not true in proxy_request(). Do this in
> proxy_request() too.

Can we do this as a general rule? If we look at the code in post_rpc(),
there are two cases: when large_request is set and when it is not.

When it's not, we have the whole request in a buffer, and we can happily
resend it.

But when it's not, we cannot restart it, because we'll have thrown away
some of the data. So we send an initial probe_rpc() as a sanity check.
If that works and we later get a 401 on the real request, we still fail
anyway.

In the case of proxy_request(), we don't know ahead of time whether the
request is large or not; we just proxy the data through. And we don't do
the probe thing at all. So wouldn't we dropping some data for the
follow-up request?

-Peff

Re: [PATCH 8/8] remote-curl: in v2, fill credentials if needed

[Jonathan Tan]

> On Tue, Feb 05, 2019 at 04:21:22PM -0800, Jonathan Tan wrote:
> 
> > In post_rpc(), remote-curl calls credential_fill() if HTTP_REAUTH is
> > returned, but this is not true in proxy_request(). Do this in
> > proxy_request() too.
> 
> Can we do this as a general rule? If we look at the code in post_rpc(),
> there are two cases: when large_request is set and when it is not.
> 
> When it's not, we have the whole request in a buffer, and we can happily
> resend it.
> 
> But when it's not, we cannot restart it, because we'll have thrown away
> some of the data. So we send an initial probe_rpc() as a sanity check.
> If that works and we later get a 401 on the real request, we still fail
> anyway.
> 
> In the case of proxy_request(), we don't know ahead of time whether the
> request is large or not; we just proxy the data through. And we don't do
> the probe thing at all. So wouldn't we dropping some data for the
> follow-up request?

Thanks - I'll look into this. Maybe the best way is to somehow make the
v2 code path also use post_rpc() - I'll see if that's possible.

In the meantime, do you have any other opinions on the other patches,
besides introducing a prereq [1]? I don't have any strong opinions for
or against this, so I didn't reply, but I slightly prefer not having the
prereq so that test readers and authors don't need to juggle so many
variables in their heads.

If everything looks good, I'll suggest that we drop this patch from this
patch set for me to work on it independently. (Having said that, this
patch set is based on js/protocol-advertise-multi, which is still under
review, so it is not so urgent.)

[1] https://public-inbox.org/git/20190206213458.GC12737@sigill.intra.peff.net/

Re: [PATCH 8/8] remote-curl: in v2, fill credentials if needed

[Jeff King]

On Mon, Feb 11, 2019 at 11:20:54AM -0800, Jonathan Tan wrote:

> > In the case of proxy_request(), we don't know ahead of time whether the
> > request is large or not; we just proxy the data through. And we don't do
> > the probe thing at all. So wouldn't we dropping some data for the
> > follow-up request?
> 
> Thanks - I'll look into this. Maybe the best way is to somehow make the
> v2 code path also use post_rpc() - I'll see if that's possible.

Yeah, that makes sense.

> In the meantime, do you have any other opinions on the other patches,
> besides introducing a prereq [1]? I don't have any strong opinions for
> or against this, so I didn't reply, but I slightly prefer not having the
> prereq so that test readers and authors don't need to juggle so many
> variables in their heads.

I think Ævar convinced me that the way you've done it is the right way,
so ignore my earlier comments. :)

I just took another pass and left a few very minor nits, but it all
looks good overall.

> If everything looks good, I'll suggest that we drop this patch from this
> patch set for me to work on it independently. (Having said that, this
> patch set is based on js/protocol-advertise-multi, which is still under
> review, so it is not so urgent.)

Yeah, that's fine by me (with or without my nits addressed).

-Peff

Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches

[Jeff King]

On Tue, Feb 05, 2019 at 04:21:14PM -0800, Jonathan Tan wrote:

> This is on the latest master (8feddda32c ("Fifth batch for 2.21",
> 2019-02-05)) + js/protocol-advertise-multi.
> 
> This is a resend of [1], which was built on the result of merging many
> branches. Now that most of the branches have been merged to master, I
> have rebased it on master. The only branch that I needed to merge was
> js/protocol-advertise-multi.

Thanks for working on this. With the exception of the final patch, this
all seems pretty sane to me from a quick look.

There is one thing that your test patches made me wonder. When we have
to make an exception to a test (i.e., that doesn't work under v2), you
do it by unsetting GIT_TEST_PROTOCOL_VERSION in the environment. That
means we'll actually run the test, but not with the version that the
caller specified.

I wonder if it would be more obvious what's going on if we instead had a
prereq like:

  test_expect_success !PROTO_V2 'ls-remote --symref' '
     ...
  '

and just skipped those tests entirely (and in a way that appears in the
TAP output).

I think it would also future-proof us a bit for v2 becoming the default
(i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean "use v2").

I dunno. It probably doesn't matter all that much, so it may not be
worth going back and changing at this point. Just a thought.

-Peff

Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 06 2019, Jeff King wrote:

> On Tue, Feb 05, 2019 at 04:21:14PM -0800, Jonathan Tan wrote:
>
>> This is on the latest master (8feddda32c ("Fifth batch for 2.21",
>> 2019-02-05)) + js/protocol-advertise-multi.
>>
>> This is a resend of [1], which was built on the result of merging many
>> branches. Now that most of the branches have been merged to master, I
>> have rebased it on master. The only branch that I needed to merge was
>> js/protocol-advertise-multi.
>
> Thanks for working on this. With the exception of the final patch, this
> all seems pretty sane to me from a quick look.
>
> There is one thing that your test patches made me wonder. When we have
> to make an exception to a test (i.e., that doesn't work under v2), you
> do it by unsetting GIT_TEST_PROTOCOL_VERSION in the environment. That
> means we'll actually run the test, but not with the version that the
> caller specified.
>
> I wonder if it would be more obvious what's going on if we instead had a
> prereq like:
>
>   test_expect_success !PROTO_V2 'ls-remote --symref' '
>      ...
>   '
>
> and just skipped those tests entirely (and in a way that appears in the
> TAP output).
>
> I think it would also future-proof us a bit for v2 becoming the default
> (i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean "use v2").
>
> I dunno. It probably doesn't matter all that much, so it may not be
> worth going back and changing at this point. Just a thought.

So far we've had the convention that these GIT_TEST_* variables,
e.g. the one for the commit graph, work the same way. Thus we guarantee
that we get (in theory) 100% coverage even when running the tests in
this special mode. I think it's better to keep it as-is.

Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches

[Jeff King]

On Wed, Feb 06, 2019 at 10:52:15PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > I wonder if it would be more obvious what's going on if we instead had a
> > prereq like:
> >
> >   test_expect_success !PROTO_V2 'ls-remote --symref' '
> >      ...
> >   '
> >
> > and just skipped those tests entirely (and in a way that appears in the
> > TAP output).
> >
> > I think it would also future-proof us a bit for v2 becoming the default
> > (i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean "use v2").
> >
> > I dunno. It probably doesn't matter all that much, so it may not be
> > worth going back and changing at this point. Just a thought.
> 
> So far we've had the convention that these GIT_TEST_* variables,
> e.g. the one for the commit graph, work the same way. Thus we guarantee
> that we get (in theory) 100% coverage even when running the tests in
> this special mode. I think it's better to keep it as-is.

But what's the point of that? Don't you always have to run the test
suite _twice_, once with the special variable and once without?
Otherwise, you are not testing one case or the other.

Or are you arguing that one might set many special variables in one go
(to prefer running the suite only twice, instead of 2^N times). In which
case we are better off running the test (as opposed to skipping it), as
it might use one of the _other_ special variables besides
GIT_TEST_PROTOCOL_VERSION.

I can buy that line of reasoning. It still doesn't cover all cases that
a true 2^N test would, but that clearly isn't going to be practical.

-Peff

Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 06 2019, Jeff King wrote:

> On Wed, Feb 06, 2019 at 10:52:15PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I wonder if it would be more obvious what's going on if we instead had a
>> > prereq like:
>> >
>> >   test_expect_success !PROTO_V2 'ls-remote --symref' '
>> >      ...
>> >   '
>> >
>> > and just skipped those tests entirely (and in a way that appears in the
>> > TAP output).
>> >
>> > I think it would also future-proof us a bit for v2 becoming the default
>> > (i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean "use v2").
>> >
>> > I dunno. It probably doesn't matter all that much, so it may not be
>> > worth going back and changing at this point. Just a thought.
>>
>> So far we've had the convention that these GIT_TEST_* variables,
>> e.g. the one for the commit graph, work the same way. Thus we guarantee
>> that we get (in theory) 100% coverage even when running the tests in
>> this special mode. I think it's better to keep it as-is.
>
> But what's the point of that? Don't you always have to run the test
> suite _twice_, once with the special variable and once without?
> Otherwise, you are not testing one case or the other.
>
> Or are you arguing that one might set many special variables in one go
> (to prefer running the suite only twice, instead of 2^N times). In which
> case we are better off running the test (as opposed to skipping it), as
> it might use one of the _other_ special variables besides
> GIT_TEST_PROTOCOL_VERSION.
>
> I can buy that line of reasoning. It still doesn't cover all cases that
> a true 2^N test would, but that clearly isn't going to be practical.

Maybe I'm misunderstanding what you're proposing, but as an example,
let's say the test suite is just these two tests:

    test_expect_success 'some unrelated thing' '...'
    test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...'

And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test
with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason,

I'd still like both tests to be run, not just 1/2 with
GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly
testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a
GIT_TEST_PROTOCOL_VERSION=1.

IOW the point of these tests is to piggy-back on the tests that *aren't*
aware of the feature you're trying to test. So
e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the
commit graph, and *also* those tests that are explicitly aware of the
commit graph, including those that for some reason would want to test
for the case where it isn't enabled (to e.g. test that --contains works
without the graph).

Otherwise I can't say "I care more about GIT_TEST_COMMIT_GRAPH=true than
not", and run just one test run with that, because I'll have blind spots
in the commit graph tests themselves, and would then need to do another
run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage.

Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches

[Jeff King]

On Wed, Feb 06, 2019 at 11:20:32PM +0100, Ævar Arnfjörð Bjarmason wrote:

> >> So far we've had the convention that these GIT_TEST_* variables,
> >> e.g. the one for the commit graph, work the same way. Thus we guarantee
> >> that we get (in theory) 100% coverage even when running the tests in
> >> this special mode. I think it's better to keep it as-is.
> >
> > But what's the point of that? Don't you always have to run the test
> > suite _twice_, once with the special variable and once without?
> > Otherwise, you are not testing one case or the other.
> >
> > Or are you arguing that one might set many special variables in one go
> > (to prefer running the suite only twice, instead of 2^N times). In which
> > case we are better off running the test (as opposed to skipping it), as
> > it might use one of the _other_ special variables besides
> > GIT_TEST_PROTOCOL_VERSION.
> >
> > I can buy that line of reasoning. It still doesn't cover all cases that
> > a true 2^N test would, but that clearly isn't going to be practical.
> 
> Maybe I'm misunderstanding what you're proposing, but as an example,
> let's say the test suite is just these two tests:
> 
>     test_expect_success 'some unrelated thing' '...'
>     test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...'
> 
> And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test
> with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason,
> 
> I'd still like both tests to be run, not just 1/2 with
> GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly
> testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a
> GIT_TEST_PROTOCOL_VERSION=1.

But that's my "why". The second test will run identically in both runs,
regardless of your setting of GIT_TEST_PROTOCOL_VERSION. So there's
value if you're only running the suite once in getting full coverage,
but if you are literally going to run it with and without, then you're
running the exact same code twice for no reason. And you have to run it
both with and without, since otherwise all of the _other_ tests aren't
seeing both options.

> IOW the point of these tests is to piggy-back on the tests that *aren't*
> aware of the feature you're trying to test. So
> e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the
> commit graph, and *also* those tests that are explicitly aware of the
> commit graph, including those that for some reason would want to test
> for the case where it isn't enabled (to e.g. test that --contains works
> without the graph).
> 
> Otherwise I can't say "I care more about GIT_TEST_COMMIT_GRAPH=true than
> not", and run just one test run with that, because I'll have blind spots
> in the commit graph tests themselves, and would then need to do another
> run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage.

So if we are still talking about the same 2-test setup above, which only
has a GIT_TEST_PROTOCOL_VERSION override, then yeah, I get the point of
being able to run a "stock" test, and then one with _both_ of the flags
(GIT_TEST_PROTOCOL_VERSION and GIT_TEST_COMMIT_GRAPH) because you don't
quite know how each test will interact with all of the "other" flags.

So now I'm not 100% sure I understand what you're talking about, but I
think maybe we are actually in agreement. ;)

-Peff

Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 07 2019, Jeff King wrote:

> On Wed, Feb 06, 2019 at 11:20:32PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> >> So far we've had the convention that these GIT_TEST_* variables,
>> >> e.g. the one for the commit graph, work the same way. Thus we guarantee
>> >> that we get (in theory) 100% coverage even when running the tests in
>> >> this special mode. I think it's better to keep it as-is.
>> >
>> > But what's the point of that? Don't you always have to run the test
>> > suite _twice_, once with the special variable and once without?
>> > Otherwise, you are not testing one case or the other.
>> >
>> > Or are you arguing that one might set many special variables in one go
>> > (to prefer running the suite only twice, instead of 2^N times). In which
>> > case we are better off running the test (as opposed to skipping it), as
>> > it might use one of the _other_ special variables besides
>> > GIT_TEST_PROTOCOL_VERSION.
>> >
>> > I can buy that line of reasoning. It still doesn't cover all cases that
>> > a true 2^N test would, but that clearly isn't going to be practical.
>>
>> Maybe I'm misunderstanding what you're proposing, but as an example,
>> let's say the test suite is just these two tests:
>>
>>     test_expect_success 'some unrelated thing' '...'
>>     test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...'
>>
>> And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test
>> with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason,
>>
>> I'd still like both tests to be run, not just 1/2 with
>> GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly
>> testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a
>> GIT_TEST_PROTOCOL_VERSION=1.
>
> But that's my "why". The second test will run identically in both runs,
> regardless of your setting of GIT_TEST_PROTOCOL_VERSION. So there's
> value if you're only running the suite once in getting full coverage,
> but if you are literally going to run it with and without, then you're
> running the exact same code twice for no reason. And you have to run it
> both with and without, since otherwise all of the _other_ tests aren't
> seeing both options.

Yeah, by always running the 2nd test regardless of what
GIT_TEST_PROTOCOL_VERSION=* is set to we're wasting CPU if we know we're
going to run both with GIT_TEST_PROTOCOL_VERSION=1 and
GIT_TEST_PROTOCOL_VERSION=2.

But we don't know that, and in terms of CPU & time the tests that rely
on any given GIT_TEST_* flag are such a tiny part of the test suite,
that I think it's fine to run them twice in such a scenario to guard
against the case when we just run in one more or the other, and not
both.

>> IOW the point of these tests is to piggy-back on the tests that *aren't*
>> aware of the feature you're trying to test. So
>> e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the
>> commit graph, and *also* those tests that are explicitly aware of the
>> commit graph, including those that for some reason would want to test
>> for the case where it isn't enabled (to e.g. test that --contains works
>> without the graph).
>>
>> Otherwise I can't say "I care more about GIT_TEST_COMMIT_GRAPH=true than
>> not", and run just one test run with that, because I'll have blind spots
>> in the commit graph tests themselves, and would then need to do another
>> run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage.
>
> So if we are still talking about the same 2-test setup above, which only
> has a GIT_TEST_PROTOCOL_VERSION override, then yeah, I get the point of
> being able to run a "stock" test, and then one with _both_ of the flags
> (GIT_TEST_PROTOCOL_VERSION and GIT_TEST_COMMIT_GRAPH) because you don't
> quite know how each test will interact with all of the "other" flags.

Yeah that's another reason not to skip them, although we could imagine a
prereq where we skip the v2 tests if GIT_TEST_PROTOCOL_VERSION=1 is set
*and* no other GIT_TEST_*=* flag is set, but I think that would also be
a bad idea.

> So now I'm not 100% sure I understand what you're talking about, but I
> think maybe we are actually in agreement. ;)

I think there's two ways to view these GIT_TEST_FOO=BAR facilities:

 1. Run all tests with "FOO" set to "BAR", skip those (via prereq) we
    know would break in that mode.

 2. Ditto, but if a test says "I'm a test for FOO=!BAR" leave it alone.

#2 is what we have now. I read your
<20190206213458.GC12737@sigill.intra.peff.net> as suggesting that we
should move to #1.

You're correct that moving to #1 would force us to more exhaustively
mark things so that if the default moves to "BAR" we just have to flip a
switch.