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

[Jonathan Tan <jonathantanmy@google.com>]

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

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

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