Re: [PATCH] Merge non-first refs that match first refspec

["Shawn O. Pearce" <spearce@spearce.org>]

Daniel Barkalow <barkalow@iabervon.org> wrote:
> The code only looked at the first ref in the map of refs when looking
> for matches for the first refspec in the case where there is not
> per-branch configuration of ref to merge. This is often sufficient,
> but not always. Make the logic clearer and test everything in the map.
> 
> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> ---
> I ran all the usual tests with this, and it seems like it should fix a bug 
> you saw, but I don't have the test case to make sure.
> 
>  builtin-fetch.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index 2f639cc..47811c9 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> @@ -101,12 +101,13 @@ static struct ref *get_ref_map(struct transport *transport,
>  				if (remote->fetch[i].dst &&
>  				    remote->fetch[i].dst[0])
>  					*autotags = 1;
> -				if (!i && !has_merge && ref_map &&
> -				    !strcmp(remote->fetch[0].src, ref_map->name))
> -					ref_map->merge = 1;
>  			}
>  			if (has_merge)
>  				add_merge_config(&ref_map, remote_refs, branch, &tail);
> +			else
> +				for (rm = ref_map; rm; rm = rm->next)
> +					if (!strcmp(remote->fetch[0].src, rm->name))
> +						rm->merge = 1;
>  		} else {
>  			ref_map = get_remote_ref(remote_refs, "HEAD");
>  			ref_map->merge = 1;

Hmmph.  I'm not seeing how this fixes the bug.  But if it fixes it,
it fixes it.

My understanding of the old code was that it should do what Junio
was reporting as broken:

  - when i == 0 this is the first remote.$foo.fetch;
  - when has_merge == 0 we have no branch.$name.merge;
  - when ref_map != NULL we have at least one ref from this fetch spec;
  - when fetch[0].src == ref_map->name it wasn't a wildcard spec;

The if conditional above was ordered the way it is so we can skip
the more expensive operation (the strcmp) most of them time through
the loop iteration.

The only way I can see the old code was failing was if ref_map
as returned by get_fetch_map() pointed to more than one refspec.
But for that to be true then fetch[1].src must have been a wildcard,
in which case the strcmp() would fail.  So we should only ever
get one entry, it should be the first entry, and dammit it should
have matched.

How/why are we getting cases where fetch[0].src isn't in the first
entry in ref_map?  What are those other entries?  Are they possibly
going to also match fetch[0].src and cause more than one branch
to merge?

BTW, thanks for looking at this.  I didn't have time to get to it
this week and now I'm really unlikely to be able to do so until
after I get back from San Jose.  I have too many things crammed
into this next week. :-\

-- 
Shawn.