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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>

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

10 more quoted lines

-- 
Brandon Williams

On 06/05, Jonathan Tan wrote:

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.

-- 
Brandon Williams

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

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 <redacted>
---
 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

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 <redacted>
---
 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

On Tue, Jun 5, 2018 at 2:40 PM Jonathan Tan [off-list ref] wrote:

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?

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

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

On Mon, Jun 18, 2018 at 11:30 AM, Stefan Beller [off-list ref] wrote:

quoted

+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 <redacted>
(considering the test_when_finished comment as
an optional nit)

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

On Tue, Jun 5, 2018 at 2:41 PM Jonathan Tan [off-list ref] 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)

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)?

That makes sense so far.

test_when_finished ;)

Makes sense.

Thanks,
Stefan

On Mon, Jun 18, 2018 at 12:15 PM Jonathan Tan [off-list ref] wrote:

On Mon, Jun 18, 2018 at 11:30 AM, Stefan Beller [off-list ref] wrote:

quoted

quoted

+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. :)

quoted

This test is precise and easy to understand; the patch is
Reviewed-by: Stefan Beller <redacted>
(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...

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.

quoted

 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.

quoted

@@ -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.

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.

quoted

quoted

@@ -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.

quoted

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

Hi,

Jonathan Tan wrote:

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

24 more lines

On 07/09, Jonathan Nieder wrote:

Hi,

Jonathan Tan wrote:

quoted

--- 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.

31 more quoted lines

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