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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 9, 2018, 17:38 UTC
Message-ID: <20180709173813.GA14196@aiede.svl.corp.google.com>
In-Reply-To: <c6910161aab1f383b5721bdc91969baad8c10a66.1528234587.git.jonathantanmy@google.com>

Hi,

Jonathan Tan wrote:

Show 8 quoted lines

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

Thanks, Jonathan

[1] https://public-inbox.org/git/20180605175144.4225-1-bmwill@google.com/