Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
flat view
From: Stefan Beller <hidden>
Date: 2018-06-18 19:22:42
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)
quoted hunk ↗ jump to hunk
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)) {
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)?
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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 ;)
quoted hunk ↗ jump to hunk
+ 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