Re: [PATCH 3/4] fetch: honor the user-provided refspecs when pruning refs

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

Carlos Martín Nieto <cmn@elego.de> writes:

> -static int prune_refs(struct transport *transport, struct ref *ref_map)
> +static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
>  {
>  	int result = 0;
> -	struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);
> +	struct ref *ref, *stale_refs = get_stale_heads(ref_map, refs, ref_count);

So in short, get_state_heads() used to take a ref_map and a remote. The
ref_map is what we actually observed from the remote after talking
ls-remote with it. It tried to see if any existing ref in our refspace may
have come from that remote by inspecting the fetch refspec associated with
that remote (and the ones that does not exist anymore are queued in the
stale ref list).

Now get_state_heads() takes a ref_map and <refs, ref_count> (you made the
patch unnecessarily harder to read by swapping the order of parameters).
The latter "pair" roughly corresponds to what the "remote" parameter used
to mean, but instead of using the refspec associated with that remote, we
would use the refspec used for this particular fetch to determine which
refs we have are stale.

> @@ -699,8 +699,12 @@ static int do_fetch(struct transport *transport,
>  		free_refs(ref_map);
>  		return 1;
>  	}
> -	if (prune)
> -		prune_refs(transport, ref_map);
> +	if (prune) {
> +		if (ref_count)
> +			prune_refs(refs, ref_count, ref_map);
> +		else
> +			prune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);
> +	}

And this is consistent to my two paragraph commentary above.

> diff --git a/builtin/remote.c b/builtin/remote.c
> index f2a9c26..79d898b 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -349,7 +349,8 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
>  		else
>  			string_list_append(&states->tracked, abbrev_branch(ref->name));
>  	}
> -	stale_refs = get_stale_heads(states->remote, fetch_map);
> +	stale_refs = get_stale_heads(fetch_map, states->remote->fetch,
> +				     states->remote->fetch_refspec_nr);

So is this.

> diff --git a/remote.c b/remote.c
> index b8ecfa5..13c9153 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1681,36 +1681,84 @@ struct ref *guess_remote_head(const struct ref *head,
>  }
>  
>  struct stale_heads_info {
> -	struct remote *remote;
>  	struct string_list *ref_names;
>  	struct ref **stale_refs_tail;
> +	struct refspec *refs;
> +	int ref_count;
>  };
>  
> +/* Returns 0 on success, -1 if it couldn't find a match in the refspecs. */
> +static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)
> +{
> +	int i;
> +	struct refspec *refspec;

This function replaces the role remote_find_tracking() used to play in the
old code and the difference in the behaviour (except the obvious lack of
"find_src/find_dst") feels gratuitous.

The original code in remote_find_tracking() uses "->pattern" to see if a
pattern match is necessary, but this scans the refspec for an asterisk,
assuring a breakage when the refspec language is updated to understand
other glob magic in the future. Why isn't refspec->pattern used here?

Can't these two functions share more logic?  It appears to me that by
enhancing the logic here a little bit, it may be possible to implement
remote_find_tracking() ed in terms of this function as a helper.

> +	for (i = 0; i < ref_count; ++i) {
> +		refspec = &refs[i];
> +
> +		/* No dst means it can't be used for prunning. */
> +		if (!refspec->dst)
> +			continue;
> +
> +		/*
> +		 * No '*' means that it must match exactly. If it does
> +		 * have it, try to match it against the pattern. If
> +		 * the refspec matches, store the ref name as it would
> +		 * appear in the server in query->src.
> +		 */
> +		if (!strchr(refspec->dst, '*')) {
> +			if (!strcmp(query->dst, refspec->dst)) {
> +				query->src = xstrdup(refspec->src);
> +				return 0;
> +			}
> +		} else if (match_name_with_pattern(refspec->dst, query->dst,
> +						    refspec->src, &query->src)) {
> +			return 0;
> +		}
> +	}
> +
> +	return -1;
> +}