Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On Tue, Jun 5, 2018 at 2:41 PM Jonathan Tan <jonathantanmy@google.com> wrote:
Show 15 quoted lines
>
> 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 -
> 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)
Show 16 quoted lines
> 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)?
Show 25 quoted lines
> 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
> '
Show 8 quoted lines
>
> 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 &&
Show 15 quoted lines
> + 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
