[PATCH 0/2] Fix protocol v2 tag following with CLI refspec

[PATCH 0/2] Fix protocol v2 tag following with CLI refspec

[Jonathan Tan]

After the events that led up to Jonathan Nieder's patch fixing fetch by
SHA-1 in protocol v2 [1], while expanding protocol v2's test coverage, I
found a bug with tag following. Patch 2 is a patch that fixes that bug
(and patch 1 is a related but independent test that I had written
beforehand).

[1] https://public-inbox.org/git/20180531072339.GA43435@aiede.svl.corp.google.com/

Jonathan Tan (2):
  t5702: test fetch with multiple refspecs at a time
  fetch: send "refs/tags/" prefix upon CLI refspecs

 builtin/fetch.c        |  2 +-
 t/t5702-protocol-v2.sh | 65 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletion(-)

-- 
2.17.0.768.g1526ddbba1.dirty

[PATCH 1/2] t5702: test fetch with multiple refspecs at a time

[Jonathan Tan]

Extend the protocol v2 tests to also test fetches with multiple refspecs
specified. This also covers the previously uncovered cases of fetching
with prefix matching and fetching by SHA-1.

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

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index a4fe6508b..261e82b0f 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -359,6 +359,53 @@ test_expect_success 'default refspec is used to filter ref when fetchcing' '
 	grep "ref-prefix refs/tags/" log
 '
 
+test_expect_success 'fetch supports various ways of have lines' '
+	rm -rf server client trace &&
+	git init server &&
+	test_commit -C server dwim &&
+	TREE=$(git -C server rev-parse HEAD^{tree}) &&
+	git -C server tag exact \
+		$(git -C server commit-tree -m a "$TREE") &&
+	git -C server tag dwim-unwanted \
+		$(git -C server commit-tree -m b "$TREE") &&
+	git -C server tag exact-unwanted \
+		$(git -C server commit-tree -m c "$TREE") &&
+	git -C server tag prefix1 \
+		$(git -C server commit-tree -m d "$TREE") &&
+	git -C server tag prefix2 \
+		$(git -C server commit-tree -m e "$TREE") &&
+	git -C server tag fetch-by-sha1 \
+		$(git -C server commit-tree -m f "$TREE") &&
+	git -C server tag completely-unrelated \
+		$(git -C server commit-tree -m g "$TREE") &&
+	
+	git init client &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+		fetch "file://$(pwd)/server" \
+		dwim \
+		refs/tags/exact \
+		refs/tags/prefix*:refs/tags/prefix* \
+		"$(git -C server rev-parse fetch-by-sha1)" &&
+
+	# Ensure that the appropriate prefixes are sent (using a sample)
+	grep "fetch> ref-prefix dwim" trace &&
+	grep "fetch> ref-prefix refs/heads/dwim" trace &&
+	grep "fetch> ref-prefix refs/tags/prefix" trace &&
+
+	# Ensure that the correct objects are returned
+	git -C client cat-file -e $(git -C server rev-parse dwim) &&
+	git -C client cat-file -e $(git -C server rev-parse exact) &&
+	git -C client cat-file -e $(git -C server rev-parse prefix1) &&
+	git -C client cat-file -e $(git -C server rev-parse prefix2) &&
+	git -C client cat-file -e $(git -C server rev-parse fetch-by-sha1) &&
+	test_must_fail git -C client cat-file -e \
+		$(git -C server rev-parse dwim-unwanted) &&
+	test_must_fail git -C client cat-file -e \
+		$(git -C server rev-parse exact-unwanted) &&
+	test_must_fail git -C client cat-file -e \
+		$(git -C server rev-parse completely-unrelated)
+'
+
 # Test protocol v2 with 'http://' transport
 #
 . "$TEST_DIRECTORY"/lib-httpd.sh
-- 
2.17.0.768.g1526ddbba1.dirty

[PATCH 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Jonathan Tan]

When performing tag following, in addition to using the server's
"include-tag" capability to send tag objects (and emulating it if the
server does not support that capability), "git fetch" relies upon the
presence of refs/tags/* entries in the initial ref advertisement to
locally create refs pointing to the aforementioned tag objects. When
using protocol v2, refs/tags/* entries in the initial ref advertisement
may be suppressed by a ref-prefix argument, leading to the tag object
being downloaded, but the ref not being created.

Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
prefix when "git fetch" is invoked with no refspecs, but not when "git
fetch" is invoked with refspecs. Extend that functionality to make it
work in both situations.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/fetch.c        |  2 +-
 t/t5702-protocol-v2.sh | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..1f447f1e8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
 		refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
 
 	if (ref_prefixes.argc &&
-	    (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
+	    (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
 		argv_array_push(&ref_prefixes, "refs/tags/");
 	}
 
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 261e82b0f..6733579c1 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '
 		$(git -C server rev-parse completely-unrelated)
 '
 
+test_expect_success 'fetch supports include-tag and tag following' '
+	rm -rf server client trace &&
+	git init server &&
+
+	test_commit -C server to_fetch &&
+	git -C server tag -a annotated_tag -m message &&
+
+	git init client &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+		fetch "$(pwd)/server" to_fetch:to_fetch &&
+
+	grep "fetch> ref-prefix to_fetch" trace &&
+	grep "fetch> ref-prefix refs/tags/" trace &&
+	grep "fetch> include-tag" trace &&
+
+	git -C client cat-file -e $(git -C client rev-parse annotated_tag)
+'
+
 # Test protocol v2 with 'http://' transport
 #
 . "$TEST_DIRECTORY"/lib-httpd.sh
-- 
2.17.0.768.g1526ddbba1.dirty

Re: [PATCH 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Brandon Williams]

On 06/05, Jonathan Tan wrote:
> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.
> 
> Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
> refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
> prefix when "git fetch" is invoked with no refspecs, but not when "git
> fetch" is invoked with refspecs. Extend that functionality to make it
> work in both situations.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>

Test t5702-protocol-v2.sh doesn't pass with this patch.

> ---
>  builtin/fetch.c        |  2 +-
>  t/t5702-protocol-v2.sh | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..1f447f1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
>  		refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
>  
>  	if (ref_prefixes.argc &&
> -	    (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> +	    (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
>  		argv_array_push(&ref_prefixes, "refs/tags/");
>  	}
>  
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 261e82b0f..6733579c1 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '
>  		$(git -C server rev-parse completely-unrelated)
>  '
>  
> +test_expect_success 'fetch supports include-tag and tag following' '
> +	rm -rf server client trace &&
> +	git init server &&
> +
> +	test_commit -C server to_fetch &&
> +	git -C server tag -a annotated_tag -m message &&
> +
> +	git init client &&
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> +		fetch "$(pwd)/server" to_fetch:to_fetch &&
> +
> +	grep "fetch> ref-prefix to_fetch" trace &&
> +	grep "fetch> ref-prefix refs/tags/" trace &&
> +	grep "fetch> include-tag" trace &&
> +
> +	git -C client cat-file -e $(git -C client rev-parse annotated_tag)
> +'
> +
>  # Test protocol v2 with 'http://' transport
>  #
>  . "$TEST_DIRECTORY"/lib-httpd.sh
> -- 
> 2.17.0.768.g1526ddbba1.dirty
> 

-- 
Brandon Williams

Re: [PATCH 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Brandon Williams]

On 06/05, Jonathan Tan wrote:
> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.
> 
> Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
> refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
> prefix when "git fetch" is invoked with no refspecs, but not when "git
> fetch" is invoked with refspecs. Extend that functionality to make it
> work in both situations.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/fetch.c        |  2 +-
>  t/t5702-protocol-v2.sh | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..1f447f1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
>  		refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
>  
>  	if (ref_prefixes.argc &&
> -	    (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> +	    (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
>  		argv_array_push(&ref_prefixes, "refs/tags/");
>  	}

This is difficult...Really I don't think the default should be to follow
tags.  Mostly because this defeats the purpose of ref filtering when a
user only requests the master branch.  Now instead of the server only
sending the master branch, you get the whole tags namespace as well.

>  
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 261e82b0f..6733579c1 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '
>  		$(git -C server rev-parse completely-unrelated)
>  '
>  
> +test_expect_success 'fetch supports include-tag and tag following' '
> +	rm -rf server client trace &&
> +	git init server &&
> +
> +	test_commit -C server to_fetch &&
> +	git -C server tag -a annotated_tag -m message &&
> +
> +	git init client &&
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> +		fetch "$(pwd)/server" to_fetch:to_fetch &&
> +
> +	grep "fetch> ref-prefix to_fetch" trace &&
> +	grep "fetch> ref-prefix refs/tags/" trace &&
> +	grep "fetch> include-tag" trace &&
> +
> +	git -C client cat-file -e $(git -C client rev-parse annotated_tag)
> +'
> +
>  # Test protocol v2 with 'http://' transport
>  #
>  . "$TEST_DIRECTORY"/lib-httpd.sh
> -- 
> 2.17.0.768.g1526ddbba1.dirty
> 

-- 
Brandon Williams

[PATCH v2 0/2] Fix protocol v2 tag following with CLI refspec

[Jonathan Tan]

> Test t5702-protocol-v2.sh doesn't pass with this patch.

Good catch - I've fixed that.

> This is difficult...Really I don't think the default should be to follow
> tags.  Mostly because this defeats the purpose of ref filtering when a
> user only requests the master branch.  Now instead of the server only
> sending the master branch, you get the whole tags namespace as well.

It's true that there is now a performance drop. My instinctive reaction
is to be conservative and preserve the existing behavior, but I'll see
what others on this list think.

It's worth nothing that with ref-in-want (for example, in your latest
series [1]) we might be able to restore performance, because the server
can send refs/tags/X with the packfile instead of sending all
refs/tags/X refs initially to the client.

[1] https://public-inbox.org/git/20180605175144.4225-1-bmwill@google.com/

Jonathan Tan (2):
  t5702: test fetch with multiple refspecs at a time
  fetch: send "refs/tags/" prefix upon CLI refspecs

 builtin/fetch.c        |  2 +-
 t/t5702-protocol-v2.sh | 71 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 69 insertions(+), 4 deletions(-)

-- 
2.17.0.768.g1526ddbba1.dirty

[PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time

[Jonathan Tan]

Extend the protocol v2 tests to also test fetches with multiple refspecs
specified. This also covers the previously uncovered cases of fetching
with prefix matching and fetching by SHA-1.

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

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index a4fe6508b..261e82b0f 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -359,6 +359,53 @@ test_expect_success 'default refspec is used to filter ref when fetchcing' '
 	grep "ref-prefix refs/tags/" log
 '
 
+test_expect_success 'fetch supports various ways of have lines' '
+	rm -rf server client trace &&
+	git init server &&
+	test_commit -C server dwim &&
+	TREE=$(git -C server rev-parse HEAD^{tree}) &&
+	git -C server tag exact \
+		$(git -C server commit-tree -m a "$TREE") &&
+	git -C server tag dwim-unwanted \
+		$(git -C server commit-tree -m b "$TREE") &&
+	git -C server tag exact-unwanted \
+		$(git -C server commit-tree -m c "$TREE") &&
+	git -C server tag prefix1 \
+		$(git -C server commit-tree -m d "$TREE") &&
+	git -C server tag prefix2 \
+		$(git -C server commit-tree -m e "$TREE") &&
+	git -C server tag fetch-by-sha1 \
+		$(git -C server commit-tree -m f "$TREE") &&
+	git -C server tag completely-unrelated \
+		$(git -C server commit-tree -m g "$TREE") &&
+	
+	git init client &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+		fetch "file://$(pwd)/server" \
+		dwim \
+		refs/tags/exact \
+		refs/tags/prefix*:refs/tags/prefix* \
+		"$(git -C server rev-parse fetch-by-sha1)" &&
+
+	# Ensure that the appropriate prefixes are sent (using a sample)
+	grep "fetch> ref-prefix dwim" trace &&
+	grep "fetch> ref-prefix refs/heads/dwim" trace &&
+	grep "fetch> ref-prefix refs/tags/prefix" trace &&
+
+	# Ensure that the correct objects are returned
+	git -C client cat-file -e $(git -C server rev-parse dwim) &&
+	git -C client cat-file -e $(git -C server rev-parse exact) &&
+	git -C client cat-file -e $(git -C server rev-parse prefix1) &&
+	git -C client cat-file -e $(git -C server rev-parse prefix2) &&
+	git -C client cat-file -e $(git -C server rev-parse fetch-by-sha1) &&
+	test_must_fail git -C client cat-file -e \
+		$(git -C server rev-parse dwim-unwanted) &&
+	test_must_fail git -C client cat-file -e \
+		$(git -C server rev-parse exact-unwanted) &&
+	test_must_fail git -C client cat-file -e \
+		$(git -C server rev-parse completely-unrelated)
+'
+
 # Test protocol v2 with 'http://' transport
 #
 . "$TEST_DIRECTORY"/lib-httpd.sh
-- 
2.17.0.768.g1526ddbba1.dirty

[PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Jonathan Tan]

When performing tag following, in addition to using the server's
"include-tag" capability to send tag objects (and emulating it if the
server does not support that capability), "git fetch" relies upon the
presence of refs/tags/* entries in the initial ref advertisement to
locally create refs pointing to the aforementioned tag objects. When
using protocol v2, refs/tags/* entries in the initial ref advertisement
may be suppressed by a ref-prefix argument, leading to the tag object
being downloaded, but the ref not being created.

Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
prefix when "git fetch" is invoked with no refspecs, but not when "git
fetch" is invoked with refspecs. Extend that functionality to make it
work in both situations.

This also necessitates a change another test which tested ref
advertisement filtering using tag refs - since tag refs are sent by
default now, the test has been switched to using branch refs instead.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/fetch.c        |  2 +-
 t/t5702-protocol-v2.sh | 24 +++++++++++++++++++++---
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..1f447f1e8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
 		refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
 
 	if (ref_prefixes.argc &&
-	    (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
+	    (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
 		argv_array_push(&ref_prefixes, "refs/tags/");
 	}
 
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 261e82b0f..b31b6d8d3 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -204,6 +204,7 @@ test_expect_success 'ref advertisment is filtered during fetch using protocol v2
 	test_when_finished "rm -f log" &&
 
 	test_commit -C file_parent three &&
+	git -C file_parent branch unwanted-branch three &&
 
 	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
 		fetch origin master &&
@@ -212,9 +213,8 @@ test_expect_success 'ref advertisment is filtered during fetch using protocol v2
 	git -C file_parent log -1 --format=%s >expect &&
 	test_cmp expect actual &&
 
-	! grep "refs/tags/one" log &&
-	! grep "refs/tags/two" log &&
-	! grep "refs/tags/three" log
+	grep "refs/heads/master" log &&
+	! grep "refs/heads/unwanted-branch" log
 '
 
 test_expect_success 'server-options are sent when fetching' '
@@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '
 		$(git -C server rev-parse completely-unrelated)
 '
 
+test_expect_success 'fetch supports include-tag and tag following' '
+	rm -rf server client trace &&
+	git init server &&
+
+	test_commit -C server to_fetch &&
+	git -C server tag -a annotated_tag -m message &&
+
+	git init client &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+		fetch "$(pwd)/server" to_fetch:to_fetch &&
+
+	grep "fetch> ref-prefix to_fetch" trace &&
+	grep "fetch> ref-prefix refs/tags/" trace &&
+	grep "fetch> include-tag" trace &&
+
+	git -C client cat-file -e $(git -C client rev-parse annotated_tag)
+'
+
 # Test protocol v2 with 'http://' transport
 #
 . "$TEST_DIRECTORY"/lib-httpd.sh
-- 
2.17.0.768.g1526ddbba1.dirty

Re: [PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time

[Stefan Beller]

On Tue, Jun 5, 2018 at 2:40 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> Extend the protocol v2 tests to also test fetches with multiple refspecs
> specified. This also covers the previously uncovered cases of fetching
> with prefix matching and fetching by SHA-1.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  t/t5702-protocol-v2.sh | 47 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index a4fe6508b..261e82b0f 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -359,6 +359,53 @@ test_expect_success 'default refspec is used to filter ref when fetchcing' '
>         grep "ref-prefix refs/tags/" log
>  '
>
> +test_expect_success 'fetch supports various ways of have lines' '
> +       rm -rf server client trace &&

Can we move these deletions to test_when_finished of the previous(?) test
as well as have them here in a test_when_finished line?

> +       git init server &&
> +       test_commit -C server dwim &&
> +       TREE=$(git -C server rev-parse HEAD^{tree}) &&
> +       git -C server tag exact \
> +               $(git -C server commit-tree -m a "$TREE") &&
> +       git -C server tag dwim-unwanted \
> +               $(git -C server commit-tree -m b "$TREE") &&
> +       git -C server tag exact-unwanted \
> +               $(git -C server commit-tree -m c "$TREE") &&
> +       git -C server tag prefix1 \
> +               $(git -C server commit-tree -m d "$TREE") &&
> +       git -C server tag prefix2 \
> +               $(git -C server commit-tree -m e "$TREE") &&
> +       git -C server tag fetch-by-sha1 \
> +               $(git -C server commit-tree -m f "$TREE") &&
> +       git -C server tag completely-unrelated \
> +               $(git -C server commit-tree -m g "$TREE") &&

So you create different commits all with the same tree; no parents;
this seems to convey nicely that this is about testing tags.

> +       git init client &&
> +       GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> +               fetch "file://$(pwd)/server" \
> +               dwim \
> +               refs/tags/exact \
> +               refs/tags/prefix*:refs/tags/prefix* \
> +               "$(git -C server rev-parse fetch-by-sha1)" &&
> +
> +       # Ensure that the appropriate prefixes are sent (using a sample)
> +       grep "fetch> ref-prefix dwim" trace &&
> +       grep "fetch> ref-prefix refs/heads/dwim" trace &&
> +       grep "fetch> ref-prefix refs/tags/prefix" trace &&
> +
> +       # Ensure that the correct objects are returned
> +       git -C client cat-file -e $(git -C server rev-parse dwim) &&
> +       git -C client cat-file -e $(git -C server rev-parse exact) &&
> +       git -C client cat-file -e $(git -C server rev-parse prefix1) &&
> +       git -C client cat-file -e $(git -C server rev-parse prefix2) &&
> +       git -C client cat-file -e $(git -C server rev-parse fetch-by-sha1) &&
> +       test_must_fail git -C client cat-file -e \
> +               $(git -C server rev-parse dwim-unwanted) &&
> +       test_must_fail git -C client cat-file -e \
> +               $(git -C server rev-parse exact-unwanted) &&
> +       test_must_fail git -C client cat-file -e \
> +               $(git -C server rev-parse completely-unrelated)
> +'

This test is precise and easy to understand; the patch is
Reviewed-by: Stefan Beller <sbeller@google.com>
(considering the test_when_finished comment as
an optional nit)

Re: [PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time

[Jonathan Tan]

On Mon, Jun 18, 2018 at 11:30 AM, Stefan Beller <sbeller@google.com> wrote:
>> +test_expect_success 'fetch supports various ways of have lines' '
>> +       rm -rf server client trace &&
>
> Can we move these deletions to test_when_finished of the previous(?) test
> as well as have them here in a test_when_finished line?

I think that deleting them when necessary makes it more explicit, and
this also supports tests where a repository is set up and then used
over multiple tests (e.g. having separate "setup X" and "use X"
tests).

> This test is precise and easy to understand; the patch is
> Reviewed-by: Stefan Beller <sbeller@google.com>
> (considering the test_when_finished comment as
> an optional nit)

Thanks. Do you have any comments about the performance issue that
Brandon brought up?

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Stefan Beller]

On Tue, Jun 5, 2018 at 2:41 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.
>
> Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
> refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
> prefix when "git fetch" is invoked with no refspecs, but not when "git
> fetch" is invoked with refspecs. Extend that functionality to make it
> work in both situations.

okay. Thinking long term, we may want to introduce a capability that
can filter the tag space, e.g. "want-refs-since <date> refs/tags/*"
as a client directive as then the client only asks for new (newly
created/appearing) tags instead of all tags.

> This also necessitates a change another test which tested ref
> advertisement filtering using tag refs -

sounds plausible.

>  since tag refs are sent by
> default now, the test has been switched to using branch refs instead.

That mismatches what I would expect to read below.
I would expect the client to always ask for refs/tags/* now and
instead of the server just giving them.

Oh, the problem is in that other test to restrict the refs/tags
to *not* be sent?

Maybe we can only ask for refs/tags/* if we do not have any
"refs/tags/" on the CLI: if I invoke "git fetch refs/tags/v1"
I would not get an advertisement for refs/tags/v2 but if I omit
all tags from  the refspec, I'd get all its advertisements (v1+v2)



> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/fetch.c        |  2 +-
>  t/t5702-protocol-v2.sh | 24 +++++++++++++++++++++---
>  2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..1f447f1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
>                 refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
>
>         if (ref_prefixes.argc &&
> -           (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> +           (tags == TAGS_SET || tags == TAGS_DEFAULT)) {

Oh, I see. This always asks for refs/tags/ whereas before we only
asked for them if there were *no* refspec given. Maybe we can
change this to

    refspec_any_item_contains("refs/tags/")

instead of always asking for the tags?
(that function would need to be written; I guess for a short term bugfix
this is good enough)

How is the tag following documented (i.e. when is the user least
surprised that we do not tag-follow all and unconditionally)?


>                 argv_array_push(&ref_prefixes, "refs/tags/");
>         }
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 261e82b0f..b31b6d8d3 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -204,6 +204,7 @@ test_expect_success 'ref advertisment is filtered during fetch using protocol v2
>         test_when_finished "rm -f log" &&
>
>         test_commit -C file_parent three &&
> +       git -C file_parent branch unwanted-branch three &&
>
>         GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
>                 fetch origin master &&
> @@ -212,9 +213,8 @@ test_expect_success 'ref advertisment is filtered during fetch using protocol v2
>         git -C file_parent log -1 --format=%s >expect &&
>         test_cmp expect actual &&
>
> -       ! grep "refs/tags/one" log &&
> -       ! grep "refs/tags/two" log &&
> -       ! grep "refs/tags/three" log
> +       grep "refs/heads/master" log &&
> +       ! grep "refs/heads/unwanted-branch" log
>  '

That makes sense so far.

>
>  test_expect_success 'server-options are sent when fetching' '
> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '
>                 $(git -C server rev-parse completely-unrelated)
>  '
>
> +test_expect_success 'fetch supports include-tag and tag following' '
> +       rm -rf server client trace &&

test_when_finished ;)

> +       git init server &&
> +
> +       test_commit -C server to_fetch &&
> +       git -C server tag -a annotated_tag -m message &&
> +
> +       git init client &&
> +       GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> +               fetch "$(pwd)/server" to_fetch:to_fetch &&
> +
> +       grep "fetch> ref-prefix to_fetch" trace &&
> +       grep "fetch> ref-prefix refs/tags/" trace &&
> +       grep "fetch> include-tag" trace &&
> +
> +       git -C client cat-file -e $(git -C client rev-parse annotated_tag)
> +'

Makes sense.

Thanks,
Stefan

Re: [PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time

[Stefan Beller]

On Mon, Jun 18, 2018 at 12:15 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> On Mon, Jun 18, 2018 at 11:30 AM, Stefan Beller <sbeller@google.com> wrote:
> >> +test_expect_success 'fetch supports various ways of have lines' '
> >> +       rm -rf server client trace &&
> >
> > Can we move these deletions to test_when_finished of the previous(?) test
> > as well as have them here in a test_when_finished line?
>
> I think that deleting them when necessary makes it more explicit, and
> this also supports tests where a repository is set up and then used
> over multiple tests (e.g. having separate "setup X" and "use X"
> tests).

and it is necessary at the end of each test, so that we minimize
the dependencies between tests. In an ideal world you could run
any one test in the file alone and would still pass. :)

>
> > This test is precise and easy to understand; the patch is
> > Reviewed-by: Stefan Beller <sbeller@google.com>
> > (considering the test_when_finished comment as
> > an optional nit)
>
> Thanks. Do you have any comments about the performance issue that
> Brandon brought up?

Is that https://public-inbox.org/git/20180605212829.GG158365@google.com/ ?
(otherwise I'd be missing the performance issue)

For now I'd filter out tags only if a specific tag is requested
(i.e. the user given refspec contains "refs/tags/" -> no tag following)
and then later I'd try out a date(?) based capability that asks for
new tags only.

Note that you can create old tags by backdating them, so we'd have
to ask the server for any update in its reflog for refs/tags...

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Jonathan Tan]

> okay. Thinking long term, we may want to introduce a capability that
> can filter the tag space, e.g. "want-refs-since <date> refs/tags/*"
> as a client directive as then the client only asks for new (newly
> created/appearing) tags instead of all tags.

I don't think refs normally have a created/modified time.

> >  since tag refs are sent by
> > default now, the test has been switched to using branch refs instead.
> 
> That mismatches what I would expect to read below.
> I would expect the client to always ask for refs/tags/* now and
> instead of the server just giving them.

I don't understand why there is a mismatch, so I'll just answer all your
questions. Yes, the client indeed always asks for refs/tags/* now.

By the server just giving them, do you mean (1) giving them based on
what is being fetched, or (2) giving them as if refs/tags/* was sent? If
(1), this is possible, but requires the server to precompute all the
commits that would be sent (and this would be an overestimate, since
negotiation has not yet taken place). If (2), I think it's better to let
the client control this (since the client could just as easily want
--no-tags, then sending tags would be wasted).

> Oh, the problem is in that other test to restrict the refs/tags
> to *not* be sent?

The problem is that refs/tags/* is sent by default. Maybe adding
--no-tags would work (I haven't tried it, though), but I think it's
clearer for the test to just operate on branches, than to have --no-tags
and let the reader wonder what --no-tags has to do with what's being
tested.

> Maybe we can only ask for refs/tags/* if we do not have any
> "refs/tags/" on the CLI: if I invoke "git fetch refs/tags/v1"
> I would not get an advertisement for refs/tags/v2 but if I omit
> all tags from  the refspec, I'd get all its advertisements (v1+v2)

This would cause different behavior. For example, if I run "git fetch
refs/heads/master refs/tags/foo", I would expect tag following to work
even if the tag's name does not start with refs/tags/foo.

> > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
> >                 refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
> >
> >         if (ref_prefixes.argc &&
> > -           (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > +           (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> 
> Oh, I see. This always asks for refs/tags/ whereas before we only
> asked for them if there were *no* refspec given. Maybe we can
> change this to
> 
>     refspec_any_item_contains("refs/tags/")
> 
> instead of always asking for the tags?
> (that function would need to be written; I guess for a short term bugfix
> this is good enough)

Same answer as above.

> How is the tag following documented (i.e. when is the user least
> surprised that we do not tag-follow all and unconditionally)?

It's documented under the --no-tags option in the man page for git
fetch. I'm not sure what you mean by the user being surprised.

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Brandon Williams]

On 06/18, Jonathan Tan wrote:
> 
> This would cause different behavior. For example, if I run "git fetch
> refs/heads/master refs/tags/foo", I would expect tag following to work
> even if the tag's name does not start with refs/tags/foo.

I understand that some people *may* want tag following, but really its
confusing from an end user's standpoint.  You can fetch a particular ref
and end up with a bunch of tags that you didn't want.  Aside from that
my biggest issue is with performance.  The ref filtering was added to
cut down on the number of refs sent from the server, now we're basically
adding them all back no matter what (unless a user goes and flips the
default to not fetch tags).

I think we're probably going to need some people other than us to
comment on how this should be handled.

> 
> > > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
> > >                 refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
> > >
> > >         if (ref_prefixes.argc &&
> > > -           (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > > +           (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> > 
> > Oh, I see. This always asks for refs/tags/ whereas before we only
> > asked for them if there were *no* refspec given. Maybe we can
> > change this to
> > 
> >     refspec_any_item_contains("refs/tags/")
> > 
> > instead of always asking for the tags?
> > (that function would need to be written; I guess for a short term bugfix
> > this is good enough)
> 
> Same answer as above.
> 
> > How is the tag following documented (i.e. when is the user least
> > surprised that we do not tag-follow all and unconditionally)?
> 
> It's documented under the --no-tags option in the man page for git
> fetch. I'm not sure what you mean by the user being surprised.

-- 
Brandon Williams

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.

I wonder if it is reasonable to define the protocol v2 semantics of
"include-tag" request a bit differently.

Unlike v0 protocol where the client iterates though the advertised
refs and see if objects at the tip of them, even if they weren't
what the client initially wanted to fetch, to find these unsolicited
followed tag objects, allow the server to say, "I've included some
objects you did not explicitly ask for, and here are the refs/tags/*
names you should place on them", somewhat similar to the way how the
ref-in-want thing would work (i.e. the client does not really ask
for just objects and decides what name to place on them---instead it
lets the server to make part of the decision).

Wouldn't that allow us not having to advertise the whole tags
namespace only to implement the tag following?

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > When performing tag following, in addition to using the server's
> > "include-tag" capability to send tag objects (and emulating it if the
> > server does not support that capability), "git fetch" relies upon the
> > presence of refs/tags/* entries in the initial ref advertisement to
> > locally create refs pointing to the aforementioned tag objects. When
> > using protocol v2, refs/tags/* entries in the initial ref advertisement
> > may be suppressed by a ref-prefix argument, leading to the tag object
> > being downloaded, but the ref not being created.
> 
> I wonder if it is reasonable to define the protocol v2 semantics of
> "include-tag" request a bit differently.
> 
> Unlike v0 protocol where the client iterates though the advertised
> refs and see if objects at the tip of them, even if they weren't
> what the client initially wanted to fetch, to find these unsolicited
> followed tag objects, allow the server to say, "I've included some
> objects you did not explicitly ask for, and here are the refs/tags/*
> names you should place on them", somewhat similar to the way how the
> ref-in-want thing would work (i.e. the client does not really ask
> for just objects and decides what name to place on them---instead it
> lets the server to make part of the decision).
> 
> Wouldn't that allow us not having to advertise the whole tags
> namespace only to implement the tag following?

Yes, it would, but as far as I can tell, it would add an extra burden on
the server to walk all refs requested in the ls-refs call (in order to
determine which tags to send back in the response). Also, this walk must
be done before any negotiation (since this is a ls-refs call), so the
walk is done all the way to the commits without any parents (and so an
overestimate of tags might be sent).

It might be better to have this functionality with ref-in-want, not only
because of its conceptual similarity, but because ref-in-want provides a
way for us to send refs at packfile generation time. So it is more
efficient (since the server has to iterate through all the refs anyway
to include objects for include-tag, it can probably just remember which
refs caused objects to be added and return them), and also, the list of
tags will not include any tags that point to non-sent objects.

So my current inclination is to let v2 "include-tag" have the same
semantics as v0 "include-tag", and only provide automatic sending of the
ref itself when we have ref-in-want.

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>> Wouldn't that allow us not having to advertise the whole tags
>> namespace only to implement the tag following?
>
> Yes, it would, but as far as I can tell, it would add an extra burden on
> the server to walk all refs requested in the ls-refs call (in order to
> determine which tags to send back in the response). Also, this walk must
> be done before any negotiation (since this is a ls-refs call),...

My comment was that I doubt the "must be done" part of the above.
How would refs-in-want be responded where client-supplied "I want
'master' branch---I am not asking for the exact object the first
server I contacted said where the 'master' is at" gets turned into
"So the final value of 'master' among these servers that are not
quite in sync is this" by the one that gives you the pack, not
necessarily the one that responds to ls-refs upon initial contact?
Can't we do something similar, i.e. let the client say "I want tags
that refer to new objects you are going to send me, I do not know
what they are offhand" and the server that actually gives you the
pack to say "here are the tags I ended up including"?  The
"include-tag" process to generate pack with extra objects (i.e. the
tags that point at packed objects) has to involve walking for
reachabliity anyway, so as long as the feature is supported,
somebody has to do the work, and if you want to cut down the
transfer cost of the refs/tags/* enumeration, it needs to happen on
the server end, no?

Or perhaps v2 fetch should implement the automated tag following
without using include-tag and instead as an extended feature of
ref-in-want.  I think that is merely giving a different name to the
same idea outlined above, though ;-)

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >> Wouldn't that allow us not having to advertise the whole tags
> >> namespace only to implement the tag following?
> >
> > Yes, it would, but as far as I can tell, it would add an extra burden on
> > the server to walk all refs requested in the ls-refs call (in order to
> > determine which tags to send back in the response). Also, this walk must
> > be done before any negotiation (since this is a ls-refs call),...
> 
> My comment was that I doubt the "must be done" part of the above.
> How would refs-in-want be responded where client-supplied "I want
> 'master' branch---I am not asking for the exact object the first
> server I contacted said where the 'master' is at" gets turned into
> "So the final value of 'master' among these servers that are not
> quite in sync is this" by the one that gives you the pack, not
> necessarily the one that responds to ls-refs upon initial contact?
> Can't we do something similar, i.e. let the client say "I want tags
> that refer to new objects you are going to send me, I do not know
> what they are offhand" and the server that actually gives you the
> pack to say "here are the tags I ended up including"?  The
> "include-tag" process to generate pack with extra objects (i.e. the
> tags that point at packed objects) has to involve walking for
> reachabliity anyway, so as long as the feature is supported,
> somebody has to do the work, and if you want to cut down the
> transfer cost of the refs/tags/* enumeration, it needs to happen on
> the server end, no?

Ah, I think I see. There are these possible worlds:
 (1) the current world
 (2) no ref-in-want, and upload-pack sends tag information as part of
     its response to ls-refs
 (3) no ref-in-want, but upload-pack can send ref information right
     before the packfile
 (4) ref-in-want, and upload-pack will send ref information right before
     the packfile

I was only thinking about (2) and (4), but I think you are talking about
(3). Yes, that would work, although I don't think it's worth the
protocol churn to do (3) then (4), especially since we already have
ref-in-want patches sent to the mailing list - but I should have
discussed this option in my previous e-mails too.

> Or perhaps v2 fetch should implement the automated tag following
> without using include-tag and instead as an extended feature of
> ref-in-want.  I think that is merely giving a different name to the
> same idea outlined above, though ;-)

Instead of not using include-tag, I would define include-tag in
the presence of want-refs to also include the refs, but I agree with
this solution.

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
>  		refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
>  
>  	if (ref_prefixes.argc &&
> -	    (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> +	    (tags == TAGS_SET || tags == TAGS_DEFAULT)) {

Makes a lot of sense.

This means that if I run

	git fetch origin master

then the ls-refs step will now include refs/tags/* in addition to
refs/heads/master, erasing some of the gain that protocol v2 brought.
But since the user didn't pass --no-tags, that is what the user asked
for.

One could argue that the user didn't *mean* to ask for that.  In other
words, I wonder if the following would better match the user intent:

 - introduce a new tagopt value, --tags=auto or something, that means
   that tags are only followed when no refspec is supplied.  In other
   words,

	git fetch --tags=auto <remote>

   would perform tag auto-following, while

   	git fetch --tags=auto <remote> <branch>

   would act like fetch --no-tags.

 - make --tags=auto the default for new clones.

What do you think?

Some related thoughts on tag auto-following:

It would be tempting to not use tag auto-following at all in the
default configuration and just include refs/tags/*:refs/tags/* in the
default clone refspec.  Unfortunately, that would be a pretty
significant behavior change from the present behavior, since

 - it would fetch tags pointing to objects unrelated to the fetched
   history
 - if we have ref-in-want *with* pattern support, it would mean we
   try to fetch all tags on every fetch.  As discussed in the
   thread [1], this is expensive and difficult to get right.
 - if an unannotated tag is fast-forwarded, it would allow existing
   tags to be updated
 - it interacts poorly with --prune
 - ...

I actually suspect it's a good direction in the long term, but until
we address those downsides (see also the occasional discussions on tag
namespaces), we're not ready for it.  The --tags=auto described above
is a sort of half approximation.

Anyway, this is all a long way of saying that despite the performance
regression I believe this patch heads in the right direction.  The
performance regression is inevitable in what the user is
(unintentionally) requesting, and we can address it separately by
changing the defaults to better match what the user intends to
request.

Thanks,
Jonathan

[1] https://public-inbox.org/git/20180605175144.4225-1-bmwill@google.com/

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Brandon Williams]

On 07/09, Jonathan Nieder wrote:
> Hi,
> 
> Jonathan Tan wrote:
> 
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
> >  		refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
> >  
> >  	if (ref_prefixes.argc &&
> > -	    (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > +	    (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> 
> Makes a lot of sense.
> 
> This means that if I run
> 
> 	git fetch origin master
> 
> then the ls-refs step will now include refs/tags/* in addition to
> refs/heads/master, erasing some of the gain that protocol v2 brought.
> But since the user didn't pass --no-tags, that is what the user asked
> for.
> 
> One could argue that the user didn't *mean* to ask for that.  In other
> words, I wonder if the following would better match the user intent:
> 
>  - introduce a new tagopt value, --tags=auto or something, that means
>    that tags are only followed when no refspec is supplied.  In other
>    words,
> 
> 	git fetch --tags=auto <remote>
> 
>    would perform tag auto-following, while
> 
>    	git fetch --tags=auto <remote> <branch>
> 
>    would act like fetch --no-tags.
> 
>  - make --tags=auto the default for new clones.
> 
> What do you think?
> 
> Some related thoughts on tag auto-following:
> 
> It would be tempting to not use tag auto-following at all in the
> default configuration and just include refs/tags/*:refs/tags/* in the
> default clone refspec.  Unfortunately, that would be a pretty
> significant behavior change from the present behavior, since
> 
>  - it would fetch tags pointing to objects unrelated to the fetched
>    history
>  - if we have ref-in-want *with* pattern support, it would mean we
>    try to fetch all tags on every fetch.  As discussed in the
>    thread [1], this is expensive and difficult to get right.
>  - if an unannotated tag is fast-forwarded, it would allow existing
>    tags to be updated
>  - it interacts poorly with --prune
>  - ...
> 
> I actually suspect it's a good direction in the long term, but until
> we address those downsides (see also the occasional discussions on tag
> namespaces), we're not ready for it.  The --tags=auto described above
> is a sort of half approximation.
> 
> Anyway, this is all a long way of saying that despite the performance
> regression I believe this patch heads in the right direction.  The
> performance regression is inevitable in what the user is
> (unintentionally) requesting, and we can address it separately by
> changing the defaults to better match what the user intends to
> request.

I agree with this observation, though I'm a bit sad about it.  I think
that having tag auto-following the default is a little confusing (and
hurts perf[1] when using proto v2) but since thats the way its always been
we'll have to live with it for now.  I think exploring changing the
defaults might be a good thing to do in the future.  But for now we've
had enough people comment on this lacking functionality that we should
fix it.

[1] Thankfully it doesn't completely undo what protocol v2 did, as we
still are able to eliminate refs/changes or refs/pull or other various
refs which significantly add to the number of refs advertised during
fetch.

-- 
Brandon Williams

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> I agree with this observation, though I'm a bit sad about it.  I think
> that having tag auto-following the default is a little confusing (and
> hurts perf[1] when using proto v2) but since thats the way its always been
> we'll have to live with it for now.  I think exploring changing the
> defaults might be a good thing to do in the future.  But for now we've
> had enough people comment on this lacking functionality that we should
> fix it.
>
> [1] Thankfully it doesn't completely undo what protocol v2 did, as we
> still are able to eliminate refs/changes or refs/pull or other various
> refs which significantly add to the number of refs advertised during
> fetch.

I thought JTan's <20180618231642.174650-1-jonathantanmy@google.com>
showed us a way forward to reduce the overhead even further without
having to be sad ;-).  Am I mistaken?

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Brandon Williams]

On 07/09, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > I agree with this observation, though I'm a bit sad about it.  I think
> > that having tag auto-following the default is a little confusing (and
> > hurts perf[1] when using proto v2) but since thats the way its always been
> > we'll have to live with it for now.  I think exploring changing the
> > defaults might be a good thing to do in the future.  But for now we've
> > had enough people comment on this lacking functionality that we should
> > fix it.
> >
> > [1] Thankfully it doesn't completely undo what protocol v2 did, as we
> > still are able to eliminate refs/changes or refs/pull or other various
> > refs which significantly add to the number of refs advertised during
> > fetch.
> 
> I thought JTan's <20180618231642.174650-1-jonathantanmy@google.com>
> showed us a way forward to reduce the overhead even further without
> having to be sad ;-).  Am I mistaken?

That's true, what Jonathan mentioned there would avoid having to send
"refs/tags/*" when requesting the refs.  The question is do we wait on
implementing that functionality (as another feature to fetch) or do we
fix this now?

-- 
Brandon Williams

Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> On 07/09, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>> 
>> > I agree with this observation, though I'm a bit sad about it.  I think
>> > that having tag auto-following the default is a little confusing (and
>> > hurts perf[1] when using proto v2) but since thats the way its always been
>> > we'll have to live with it for now.  I think exploring changing the
>> > defaults might be a good thing to do in the future.  But for now we've
>> > had enough people comment on this lacking functionality that we should
>> > fix it.
>> >
>> > [1] Thankfully it doesn't completely undo what protocol v2 did, as we
>> > still are able to eliminate refs/changes or refs/pull or other various
>> > refs which significantly add to the number of refs advertised during
>> > fetch.
>> 
>> I thought JTan's <20180618231642.174650-1-jonathantanmy@google.com>
>> showed us a way forward to reduce the overhead even further without
>> having to be sad ;-).  Am I mistaken?
>
> That's true, what Jonathan mentioned there would avoid having to send
> "refs/tags/*" when requesting the refs.  The question is do we wait on
> implementing that functionality (as another feature to fetch) or do we
> fix this now?

It's not like the earlier v2 protocol used to be super efficient and
correct, whose performance benefit is destroyed with this "fix"
irreversibly.  It was a fast but sometimes incorrect implementation,
and we'd protect users of the still-under-development feature with
these two patches while updating the protocol further in order to
become truly efficient and correct, so I do not see it a wrong move
to take a hit like this patch does in the meantime.

What I would see a wrong move would be to leave it very long as-is
after we apply this fix, and declare to flip the default not to
follow tags, using the performance hit as an excuse.

So I do not care too deeply either way, whether we wait to regain
efficiency or taking this safe fix as the first step, as long as it
is fixed in the longer term.  I had an impression that the sentiment
in the thread was that it was OK to accept the inefficiency for now
and fix it later, and I am personally fine with that approach.