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

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

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

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

Sorry for a belated review.

I agree with your analysis of the root-cause of the symptom exposed
by new tests in [1/2] and the proposed solution.

> @@ -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;

Who frees "matches"?  At this point matches.nr != 0 so there must be
something we need to free, no?

> +	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);

In the new code, query_refspecs_multiple() uses the result allocated
by match_name_with_pattern() to the results list, taking it out of
query.src without copying, so losing this free() is the right thing
to do---"matches" must be cleared.

And "string_list matches" is initialized as INIT_DUP, so we can rely
on string_list_clear() to free these strings.

>  	return 0;
>  }

Regarding the seemingly duplicated logic in the new function, I
wonder if the callers of non-duplicated variant may benefit if they
notice there are multiple hits, even if they cannot use more than
one in their context.  That is, what would happen if we changed
these callers to instead of calling query-refspecs call the "multi"
variant, and if that call finds multiple matches, do something about
it (e.g. warn if they use "the first hit" because they are not
acting on later hits, possibly losing information)?

Here is a minor clean-ups, both to fix style and plug leaks, that
can be squashed to this patch.  How does it look?

Thanks.

 remote.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/remote.c b/remote.c
index 26140c7..fde7b52 100644
--- a/remote.c
+++ b/remote.c
@@ -839,9 +839,8 @@ static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct
 		if (!refspec->dst)
 			continue;
 		if (refspec->pattern) {
-			if (match_name_with_pattern(key, needle, value, result)) {
+			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);
 		}
@@ -1989,32 +1988,29 @@ static int get_stale_heads_cb(const char *refname,
 
 	query_refspecs_multiple(info->refs, info->ref_count, &query, &matches);
 	if (matches.nr == 0)
-		return 0; /* No matches */
+		goto clean_exit; /* 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. In order to deal with
+	 * 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)
-		return 0;
+		goto clean_exit;
 
-	for (i = 0; i < matches.nr; i++) {
-		if (string_list_has_string(info->ref_names, matches.items[i].string)) {
+	for (i = 0; stale && 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);
 	}
 
+clean_exit:
+	string_list_clear(&matches, 0);
 	return 0;
 }