threads / patch / 10055

patchMerge non-first refs that match first refspec

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

## tl;dr

9 messages between Sep 28, 2007 and Sep 28, 2007. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Daniel Barkalow· Sep 28, 2007, 03:52 UTC · lore

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(-)
Show changes to builtin-fetch.c +4 −3
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
Shawn O. Pearce· Sep 28, 2007, 04:15 UTC · re: Daniel Barkalow · lore

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

Daniel Barkalow <barkalow@iabervon.org> wrote:
Show 34 quoted lines
> 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.
Daniel Barkalow· Sep 28, 2007, 04:40 UTC · re: Shawn O. Pearce · lore

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

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.
Show 15 quoted lines
>   - 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*
Shawn O. Pearce· Sep 28, 2007, 04:48 UTC · re: Daniel Barkalow · lore

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

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.
Daniel Barkalow· Sep 28, 2007, 05:12 UTC · re: Shawn O. Pearce · lore

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

On Fri, 28 Sep 2007, Shawn O. Pearce wrote:
Show 17 quoted lines
> 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*
Shawn O. Pearce· Sep 28, 2007, 05:25 UTC · re: Daniel Barkalow · lore

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

Daniel Barkalow <barkalow@iabervon.org> wrote:
Show 22 quoted lines
> 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.
Junio C Hamano· Sep 28, 2007, 09:34 UTC · re: Daniel Barkalow · lore

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

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.

Junio C Hamano· Sep 28, 2007, 21:23 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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
Daniel Barkalow· Sep 28, 2007, 21:38 UTC · re: Junio C Hamano · lore

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

On Fri, 28 Sep 2007, Junio C Hamano wrote:
Show 35 quoted lines
> 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*

← back to recent threads