Fix protocol v2 tag following with CLI refspec

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 <[email protected]>

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

builtin/fetch.c +1 -1

index ea5b9669a..1f447f1e8 100644
@@ -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/");
 	}
 

t/t5702-protocol-v2.sh +18 -0

index 261e82b0f..6733579c1 100755
@@ -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

Signature · 1 line

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 <[email protected]>

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

builtin/fetch.c +0 -0

index ea5b9669a..1f447f1e8 100644

t/t5702-protocol-v2.sh +18 -0

index 261e82b0f..6733579c1 100755
@@ -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

Signature · 1 line

2.17.0.768.g1526ddbba1.dirty

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

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.

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 <[email protected]>

---
 t/t5702-protocol-v2.sh | 47 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

t/t5702-protocol-v2.sh +0 -0

index a4fe6508b..261e82b0f 100755

t/t5702-protocol-v2.sh +7 -0

+       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") &&

t/t5702-protocol-v2.sh +16 -0

+       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) &&

t/t5702-protocol-v2.sh test_expect_success 'default refspec is used to filter ref when fetchcing' ' · line 363

Quoted 2 lines

t/t5702-protocol-v2.sh +2 -0

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

This test is precise and easy to understand; the patch is

Reviewed-by Stefan Beller <[email protected]>

(considering the test_when_finished comment as
an optional nit)

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 <[email protected]>

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

builtin/fetch.c +0 -0

index ea5b9669a..1f447f1e8 100644

builtin/fetch.c +0 -0

                argv_array_push(&ref_prefixes, "refs/tags/");
        }
 

t/t5702-protocol-v2.sh +1 -0

index 261e82b0f..b31b6d8d3 100755
@@ -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 &&

t/t5702-protocol-v2.sh +5 -0

+       git init server &&
+
+       test_commit -C server to_fetch &&
+       git -C server tag -a annotated_tag -m message &&
+

Quoted 3 lines · Stefan Beller

Quoted 2 lines

t/t5702-protocol-v2.sh +2 -0

+test_expect_success 'fetch supports various ways of have lines' '
+       rm -rf server client trace &&

Quoted 2 lines

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

Quoted 5 lines

This test is precise and easy to understand; the patch is

Reviewed-by Stefan Beller <[email protected]>

(considering the test_when_finished comment as
an optional nit)

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

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.

Quoted 2 lines

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)

builtin/fetch.c static struct ref *get_ref_map(struct transport *transport, · line 362

Quoted 6 lines

builtin/fetch.c +1 -1

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

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 12 lines

builtin/fetch.c static struct ref *get_ref_map(struct transport *transport, · line 362

Quoted 7 lines

builtin/fetch.c +1 -1

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

Quoted 5 lines

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 2 lines

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.

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.

Quoted 8 lines · 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.

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?

Quoted 2 lines

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

Quoted 7 lines · Jonathan Tan

Quoted 2 lines

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

Hi,

builtin/fetch.c static struct ref *get_ref_map(struct transport *transport, · line 362

Quoted 8 lines · Jonathan Tan

builtin/fetch.c +1 -1

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

Quoted 8 lines · Brandon Williams

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.

Quoted 4 lines

[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 <[email protected]> showed us a way forward to reduce the overhead even further without having to be sad ;-). Am I mistaken?

Quoted 15 lines · Junio C Hamano

Quoted 12 lines · Brandon Williams

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.

Quoted 3 lines

I thought JTan's <[email protected]> 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?