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

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

[Daniel Barkalow]

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;
-- 
1.5.3.2.1041.g2df10

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

[Shawn O. Pearce]

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.

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

[Daniel Barkalow]

On Fri, 28 Sep 2007, Shawn O. Pearce wrote:

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

Agreed; that's why I thought there must be something wrong with it.

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

That is the analysis that the original code is based on, yes. But it's not 
the easiest thing to follow, and I misunderstood it earlier this evening 
(prompted by the claim that it wasn't working, mostly). It's probably 
worth checking remote->fetch[0].pattern instead of the strcmp, anyway, 
since that's clearer and cheaper anyway.

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

Beats me; Junio, what's your test case?

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

No problem. You fixed plenty of my other bugs in this code, and it's 
getting so close to done.

	-Daniel
*This .sig left intentionally blank*

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

[Shawn O. Pearce]

Daniel Barkalow <barkalow@iabervon.org> wrote:
> Beats me; Junio, what's your test case?

If I understood him correctly it is this:

	mkdir foo; cd foo; git init
	git config remote.origin.url git://repo.or.cz/alt-git.git
	git config remote.origin.fetch refs/heads/master:refs/remotes/origin/master
	git config --add remote.origin.fetch refs/heads/maint:refs/remotes/origin/maint
	git pull

We should see "master" listed in .git/FETCH_HEAD as a "for-merge"
and "maint" listed as a "not-for-merge"...

But if that remote.origin.fetch was a wildcard spec this shouldn't
happen as the results are unpredictable.  But above the user
explicitly put master first, so it should be defaulted to.

-- 
Shawn.

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

[Daniel Barkalow]

On Fri, 28 Sep 2007, Shawn O. Pearce wrote:

> Daniel Barkalow <barkalow@iabervon.org> wrote:
> > Beats me; Junio, what's your test case?
> 
> If I understood him correctly it is this:
> 
> 	mkdir foo; cd foo; git init
> 	git config remote.origin.url git://repo.or.cz/alt-git.git
> 	git config remote.origin.fetch refs/heads/master:refs/remotes/origin/master
> 	git config --add remote.origin.fetch refs/heads/maint:refs/remotes/origin/maint
> 	git pull
> 
> We should see "master" listed in .git/FETCH_HEAD as a "for-merge"
> and "maint" listed as a "not-for-merge"...
> 
> But if that remote.origin.fetch was a wildcard spec this shouldn't
> happen as the results are unpredictable.  But above the user
> explicitly put master first, so it should be defaulted to.

But after that sequence, the right thing does happen. So I'm guessing that 
he has some different sequence that triggers a bug.

	-Daniel
*This .sig left intentionally blank*

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

[Shawn O. Pearce]

Daniel Barkalow <barkalow@iabervon.org> wrote:
> On Fri, 28 Sep 2007, Shawn O. Pearce wrote:
> 
> > Daniel Barkalow <barkalow@iabervon.org> wrote:
> > > Beats me; Junio, what's your test case?
> > 
> > If I understood him correctly it is this:
> > 
> > 	mkdir foo; cd foo; git init
> > 	git config remote.origin.url git://repo.or.cz/alt-git.git
> > 	git config remote.origin.fetch refs/heads/master:refs/remotes/origin/master
> > 	git config --add remote.origin.fetch refs/heads/maint:refs/remotes/origin/maint
> > 	git pull
> > 
> > We should see "master" listed in .git/FETCH_HEAD as a "for-merge"
> > and "maint" listed as a "not-for-merge"...
> > 
> > But if that remote.origin.fetch was a wildcard spec this shouldn't
> > happen as the results are unpredictable.  But above the user
> > explicitly put master first, so it should be defaulted to.
> 
> But after that sequence, the right thing does happen. So I'm guessing that 
> he has some different sequence that triggers a bug.

OK.  Then I don't understand what issue Junio identified.  And it
makes me feel better knowing that the current code does do the
right thing in the above case, because that's what I meant for it
to do when I was last in there...

-- 
Shawn.

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> Beats me; Junio, what's your test case?

I can paste tomorrow (it is a clone of git.git at work).  I do
not use .git/config but .git/remotes/origin and explicit four
separate Pull: lines and going over http.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Daniel Barkalow <barkalow@iabervon.org> writes:
>
>> Beats me; Junio, what's your test case?
>
> I can paste tomorrow (it is a clone of git.git at work).  I do
> not use .git/config but .git/remotes/origin and explicit four
> separate Pull: lines and going over http.

Here are the files.  Note that I use traditional layout and
always have 'master' checked out when I initiate 'git pull'.

: xyzzy git.git/master; cat .git/config
[core]
	logallrefupdates = true

[diff]
	color = auto

[showbranch]
	default = --topo-order
	default = master
	default = next
	default = pu

[alias]
	co = checkout
: xyzzy git.git/master; cat .git/remotes origin
URL: http://repo.or.cz/r/alt-git.git/
Pull: master:origin
Pull: next:next
Pull: +pu:pu
Pull: maint:maint
Pull: todo:todo
: xyzzy git.git/master; git pull
Warning: No merge candidate found because value of config option
         "branch.master.merge" does not match any remote branch
         fetched.
No changes.
: xyzzy git.git/master; git version
git version 1.5.3.2.1060.gaf79f
: xyzzy git.git/master; exit

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

[Daniel Barkalow]

On Fri, 28 Sep 2007, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Daniel Barkalow <barkalow@iabervon.org> writes:
> >
> >> Beats me; Junio, what's your test case?
> >
> > I can paste tomorrow (it is a clone of git.git at work).  I do
> > not use .git/config but .git/remotes/origin and explicit four
> > separate Pull: lines and going over http.
> 
> Here are the files.  Note that I use traditional layout and
> always have 'master' checked out when I initiate 'git pull'.
> 
> : xyzzy git.git/master; cat .git/config
> [core]
> 	logallrefupdates = true
> 
> [diff]
> 	color = auto
> 
> [showbranch]
> 	default = --topo-order
> 	default = master
> 	default = next
> 	default = pu
> 
> [alias]
> 	co = checkout
> : xyzzy git.git/master; cat .git/remotes origin
> URL: http://repo.or.cz/r/alt-git.git/
> Pull: master:origin
> Pull: next:next
> Pull: +pu:pu
> Pull: maint:maint
> Pull: todo:todo

The strcmp fails because the config uses an abbreviation and the server 
doesn't. Forget my first attempt, and try replacing the strcmp on line 105 
with "!remote->fetch[0].pattern", which is what we're really checking, 
anyway.

(If this is the first refspec we're on, and we don't have a per-branch 
config, and we got a match, and the refspec isn't a pattern, merge it; 
anything that matches according to get_fetch_map is a satisfactory match, 
even if it doesn't look quite the same.)

I'll do up an actual patch after dinner if nobody beats me to it.

	-Daniel
*This .sig left intentionally blank*