Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune

["Carlos Martín Nieto" <cmn@elego.de>]

On Thu, 2014-02-27 at 11:21 +0100, Michael Haggerty wrote:
> On 02/27/2014 10:00 AM, Carlos Martín Nieto wrote:
> > From: Carlos Martín Nieto <cmn@dwim.me>
> > 
> > We need to consider that a remote-tracking branch may match more than
> > one rhs of a fetch refspec. In such a case, it is not enough to stop at
> > the first match but look at all of the matches in order to determine
> > whether a head is stale.
> > 
> > To this goal, introduce a variant of query_refspecs which returns all of
> > the matching refspecs and loop over those answers to check for
> > staleness.
> > 
> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> > ---
> > 
> > There is an unfortunate duplication of code here, as
> > query_refspecs_multiple is mostly query_refspecs but we only care
> > about the other side of matching refspecs and disregard the 'force'
> > information which query_refspecs does want.
> > 
> > I thought about putting both together via callbacks and having
> > query_refspecs stop at the first one, but I'm not sure that it would
> > make it easier to read or manage.
> > 
> >  remote.c         | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
> >  t/t5510-fetch.sh |  2 +-
> >  2 files changed, 48 insertions(+), 6 deletions(-)
> > 
> > diff --git a/remote.c b/remote.c
> > index 9f1a8aa..26140c7 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, const char *name,
> >  	return ret;
> >  }
> >  
> > +static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct refspec *query, struct string_list *results)
> > +{
> > +	int i;
> > +	int find_src = !query->src;
> > +
> > +	if (find_src && !query->dst)
> > +		error("query_refspecs_multiple: need either src or dst");
> > +
> > +	for (i = 0; i < ref_count; i++) {
> > +		struct refspec *refspec = &refs[i];
> > +		const char *key = find_src ? refspec->dst : refspec->src;
> > +		const char *value = find_src ? refspec->src : refspec->dst;
> > +		const char *needle = find_src ? query->dst : query->src;
> > +		char **result = find_src ? &query->src : &query->dst;
> > +
> > +		if (!refspec->dst)
> > +			continue;
> > +		if (refspec->pattern) {
> > +			if (match_name_with_pattern(key, needle, value, result)) {
> > +				string_list_append_nodup(results, *result);
> > +			}
> > +		} else if (!strcmp(needle, key)) {
> > +			string_list_append(results, value);
> > +		}
> > +	}
> > +}
> > +
> >  static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query)
> >  {
> >  	int i;
> > @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,
> >  	const unsigned char *sha1, int flags, void *cb_data)
> >  {
> >  	struct stale_heads_info *info = cb_data;
> > +	struct string_list matches = STRING_LIST_INIT_DUP;
> >  	struct refspec query;
> > +	int i, stale = 1;
> >  	memset(&query, 0, sizeof(struct refspec));
> >  	query.dst = (char *)refname;
> >  
> > -	if (query_refspecs(info->refs, info->ref_count, &query))
> > +	query_refspecs_multiple(info->refs, info->ref_count, &query, &matches);
> > +	if (matches.nr == 0)
> >  		return 0; /* No matches */
> >  
> >  	/*
> >  	 * If we did find a suitable refspec and it's not a symref and
> >  	 * it's not in the list of refs that currently exist in that
> > -	 * remote we consider it to be stale.
> > +	 * remote we consider it to be stale. In order to deal with
> > +	 * overlapping refspecs, we need to go over all of the
> > +	 * matching refs.
> >  	 */
> > -	if (!((flags & REF_ISSYMREF) ||
> > -	      string_list_has_string(info->ref_names, query.src))) {
> > +	if (flags & REF_ISSYMREF)
> > +		return 0;
> > +
> > +	for (i = 0; i < matches.nr; i++) {
> > +		if (string_list_has_string(info->ref_names, matches.items[i].string)) {
> > +			stale = 0;
> > +			break;
> > +		}
> > +	}
> > +
> > +	string_list_clear(&matches, 0);
> > +
> > +	if (stale) {
> >  		struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
> >  		hashcpy(ref->new_sha1, sha1);
> >  	}
> >  
> > -	free(query.src);
> >  	return 0;
> >  }
> 
> I didn't have time to review this fully, but I think you are missing
> calls to string_list_clear(&matches) on a couple of code paths.

Yep, you're right. I'll fix this and hold off new version for a bit to
see if there's more input. 

   cmn