Re: [PATCH 11/15] fetch --prune: prune only based on explicit refspecs

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> ...
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Everything in the proposed log message made sense to me.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d4d93c9..83c1700 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2086,7 +2086,7 @@ remote.<name>.vcs::
>  remote.<name>.prune::
>  	When set to true, fetching from this remote by default will also
>  	remove any remote-tracking branches which no longer exist on the
> -	remote (as if the `--prune` option was give on the command line).
> +	remote (as if the `--prune` option was given on the command line).

Shouldn't we stop saying "branches" here?

> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 0e6d2ac..5d12219 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -41,8 +41,14 @@ ifndef::git-pull[]
>  
>  -p::
>  --prune::
> -	After fetching, remove any remote-tracking branches which
> -	no longer exist	on the remote.
> +	After fetching, remove any remote-tracking branches that

Likewise.  This is a lot more important than the one in
remote.<name>.prune documentation, as the next sentence "Tags are
not subject to ..." implies that they fall into the same category as
what gets pruned here, i.e. "remote-tracking branches" in the above
sentence, but nobody calls refs/tags/v1.0.0 a "remote-tracking
branch" even if it came from your 'origin'.

> +	no longer exist	on the remote.  Tags are not subject to
> +	pruning in the usual case that they are fetched because of the
> +	--tags option or remote.<name>.tagopt.  

We should mention the most usual case tags are fetched, before
mentioning the case the unusual option "--tags" was used from the
command line or .tagopt configuration was used.  Namely, when the
tags are automatically followed.

> +     However, if tags are
> +	fetched due to an explicit refspec (either on the command line
> +	or in the remote configuration, for example if the remote was
> +	cloned with the --mirror option), then they are also subject
> +	to pruning.

Very good.

> @@ -63,7 +69,10 @@ ifndef::git-pull[]
>  --tags::
>  	This is a short-hand requesting that all tags be fetched from
>  	the remote in addition to whatever else is being fetched.  It
> -	is similar to using the refspec `refs/tags/*:refs/tags/*`.
> +	is similar to using the refspec `refs/tags/*:refs/tags/*`,
> +	except that it doesn't subject tags to pruning, regardless of
> +	a --prune option or the configuration settings of fetch.prune
> +	or remote.<name>.prune.

Using --tags is not similar to using refs/tags/*:refs/tags/* after
the previous patch already; "git fetch origin --tags" and "git fetch
origin refs/tags/*:refs/tags/*" are vastly different and that was
the whole point of the previous step.  And that "calling something
not so similar similar" needs to be fixed further here to clarify
that they are not similar in yet another way.

We should just lose "It is similar to using" from 10/15 and start
over, perhaps?  Add the first paragraph of the below in 10/15 and
add the rest in 11/15, or something.

	--tags::
		Request that all tags be fetched from the remote
		under the same name (i.e. `refs/tags/X` is created in
		our repository by copying their `refs/tags/X`), in
		addition to whatever is fetched by the same `git
		fetch` command without this option on the command
		line.
	+
        When `refs/tags/*` hierarchy from the remote is copied only
        because this option was given, they are not subject to be
	pruned when `--prune` option (or configuration variables
	like `fetch.prune` or `remote.<name>.prune`) is in effect.

That would make it clear that they are subject to pruning when --mirror
or an explicit refspec refs/tags/*:refs/tags/* is given, as tags are
not fetched "only because of --tags" in such cases.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 7edb1ea..47b63a7 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -829,38 +829,17 @@ static int do_fetch(struct transport *transport,
>  		goto cleanup;
>  	}
>  	if (prune) {
> -		struct refspec *prune_refspecs;
> -		int prune_refspec_count;
> -
> +		/*
> +		 * We only prune based on refspecs specified
> +		 * explicitly (via command line or configuration); we
> +		 * don't care whether --tags was specified.
> +		 */
>  		if (ref_count) {
> -			prune_refspecs = refs;
> -			prune_refspec_count = ref_count;
> -		} else {
> -			prune_refspecs = transport->remote->fetch;
> -			prune_refspec_count = transport->remote->fetch_refspec_nr;
> -		}
> -
> -		if (tags == TAGS_SET) {
> -			/*
> -			 * --tags was specified.  Pretend that the user also
> -			 * gave us the canonical tags refspec
> -			 */
> -			const char *tags_str = "refs/tags/*:refs/tags/*";
> -			struct refspec *tags_refspec, *refspec;
> -
> -			/* Copy the refspec and add the tags to it */
> -			refspec = xcalloc(prune_refspec_count + 1, sizeof(*refspec));
> -			tags_refspec = parse_fetch_refspec(1, &tags_str);
> -			memcpy(refspec, prune_refspecs, prune_refspec_count * sizeof(*refspec));
> -			memcpy(&refspec[prune_refspec_count], tags_refspec, sizeof(*refspec));
> -
> -			prune_refs(refspec, prune_refspec_count + 1, ref_map);
> -
> -			/* The rest of the strings belong to fetch_one */
> -			free_refspec(1, tags_refspec);
> -			free(refspec);
> +			prune_refs(refs, ref_count, ref_map);
>  		} else {
> -			prune_refs(prune_refspecs, prune_refspec_count, ref_map);
> +			prune_refs(transport->remote->fetch,
> +				   transport->remote->fetch_refspec_nr,
> +				   ref_map);
>  		}
>  	}

Nice.