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

[Stefan Beller <sbeller@google.com>]

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