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

[Brandon Williams <bmwill@google.com>]

On 07/09, Jonathan Nieder wrote:
> Hi,
> 
> Jonathan Tan wrote:
> 
> > --- 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)) {
> 
> 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.

-- 
Brandon Williams