{"thread":{"id":"10055","subject":"[PATCH] Merge non-first refs that match first refspec","startedAt":"2007-09-28T03:52:16Z","lastAt":"2007-09-28T21:38:28Z","messageCount":9,"participants":["Daniel Barkalow","Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"54225","messageId":"Pine.LNX.4.64.0709272351010.5926@iabervon.org","threadId":"10055","inReplyTo":null,"subject":"[PATCH] Merge non-first refs that match first refspec","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-09-28T03:52:16Z","receivedAt":"2007-09-28T03:52:16Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"The code only looked at the first ref in the map of refs when looking\nfor matches for the first refspec in the case where there is not\nper-branch configuration of ref to merge. This is often sufficient,\nbut not always. Make the logic clearer and test everything in the map.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\nI ran all the usual tests with this, and it seems like it should fix a bug \nyou saw, but I don't have the test case to make sure.\n\n builtin-fetch.c |    7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 2f639cc..47811c9 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -101,12 +101,13 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\t\t\tif (remote->fetch[i].dst &&\n \t\t\t\t    remote->fetch[i].dst[0])\n \t\t\t\t\t*autotags = 1;\n-\t\t\t\tif (!i && !has_merge && ref_map &&\n-\t\t\t\t    !strcmp(remote->fetch[0].src, ref_map->name))\n-\t\t\t\t\tref_map->merge = 1;\n \t\t\t}\n \t\t\tif (has_merge)\n \t\t\t\tadd_merge_config(&ref_map, remote_refs, branch, &tail);\n+\t\t\telse\n+\t\t\t\tfor (rm = ref_map; rm; rm = rm->next)\n+\t\t\t\t\tif (!strcmp(remote->fetch[0].src, rm->name))\n+\t\t\t\t\t\trm->merge = 1;\n \t\t} else {\n \t\t\tref_map = get_remote_ref(remote_refs, \"HEAD\");\n \t\t\tref_map->merge = 1;\n-- \n1.5.3.2.1041.g2df10\n"},{"id":"54226","messageId":"20070928041509.GU3099@spearce.org","threadId":"10055","inReplyTo":"Pine.LNX.4.64.0709272351010.5926@iabervon.org","subject":"Re: [PATCH] Merge non-first refs that match first refspec","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-28T04:15:09Z","receivedAt":"2007-09-28T04:15:09Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> wrote:\n> The code only looked at the first ref in the map of refs when looking\n> for matches for the first refspec in the case where there is not\n> per-branch configuration of ref to merge. This is often sufficient,\n> but not always. Make the logic clearer and test everything in the map.\n> \n> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>\n> ---\n> I ran all the usual tests with this, and it seems like it should fix a bug \n> you saw, but I don't have the test case to make sure.\n> \n>  builtin-fetch.c |    7 ++++---\n>  1 files changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin-fetch.c b/builtin-fetch.c\n> index 2f639cc..47811c9 100644\n> --- a/builtin-fetch.c\n> +++ b/builtin-fetch.c\n> @@ -101,12 +101,13 @@ static struct ref *get_ref_map(struct transport *transport,\n>  \t\t\t\tif (remote->fetch[i].dst &&\n>  \t\t\t\t    remote->fetch[i].dst[0])\n>  \t\t\t\t\t*autotags = 1;\n> -\t\t\t\tif (!i && !has_merge && ref_map &&\n> -\t\t\t\t    !strcmp(remote->fetch[0].src, ref_map->name))\n> -\t\t\t\t\tref_map->merge = 1;\n>  \t\t\t}\n>  \t\t\tif (has_merge)\n>  \t\t\t\tadd_merge_config(&ref_map, remote_refs, branch, &tail);\n> +\t\t\telse\n> +\t\t\t\tfor (rm = ref_map; rm; rm = rm->next)\n> +\t\t\t\t\tif (!strcmp(remote->fetch[0].src, rm->name))\n> +\t\t\t\t\t\trm->merge = 1;\n>  \t\t} else {\n>  \t\t\tref_map = get_remote_ref(remote_refs, \"HEAD\");\n>  \t\t\tref_map->merge = 1;\n\nHmmph.  I'm not seeing how this fixes the bug.  But if it fixes it,\nit fixes it.\n\nMy understanding of the old code was that it should do what Junio\nwas reporting as broken:\n\n  - when i == 0 this is the first remote.$foo.fetch;\n  - when has_merge == 0 we have no branch.$name.merge;\n  - when ref_map != NULL we have at least one ref from this fetch spec;\n  - when fetch[0].src == ref_map->name it wasn't a wildcard spec;\n\nThe if conditional above was ordered the way it is so we can skip\nthe more expensive operation (the strcmp) most of them time through\nthe loop iteration.\n\nThe only way I can see the old code was failing was if ref_map\nas returned by get_fetch_map() pointed to more than one refspec.\nBut for that to be true then fetch[1].src must have been a wildcard,\nin which case the strcmp() would fail.  So we should only ever\nget one entry, it should be the first entry, and dammit it should\nhave matched.\n\nHow/why are we getting cases where fetch[0].src isn't in the first\nentry in ref_map?  What are those other entries?  Are they possibly\ngoing to also match fetch[0].src and cause more than one branch\nto merge?\n\nBTW, thanks for looking at this.  I didn't have time to get to it\nthis week and now I'm really unlikely to be able to do so until\nafter I get back from San Jose.  I have too many things crammed\ninto this next week. :-\\\n\n-- \nShawn.\n"},{"id":"54229","messageId":"Pine.LNX.4.64.0709280026240.5926@iabervon.org","threadId":"10055","inReplyTo":"20070928041509.GU3099@spearce.org","subject":"Re: [PATCH] Merge non-first refs that match first refspec","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-09-28T04:40:36Z","receivedAt":"2007-09-28T04:40:36Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 28 Sep 2007, Shawn O. Pearce wrote:\n\n> My understanding of the old code was that it should do what Junio\n> was reporting as broken:\n\nAgreed; that's why I thought there must be something wrong with it.\n\n>   - when i == 0 this is the first remote.$foo.fetch;\n>   - when has_merge == 0 we have no branch.$name.merge;\n>   - when ref_map != NULL we have at least one ref from this fetch spec;\n>   - when fetch[0].src == ref_map->name it wasn't a wildcard spec;\n> \n> The if conditional above was ordered the way it is so we can skip\n> the more expensive operation (the strcmp) most of them time through\n> the loop iteration.\n> \n> The only way I can see the old code was failing was if ref_map\n> as returned by get_fetch_map() pointed to more than one refspec.\n> But for that to be true then fetch[1].src must have been a wildcard,\n> in which case the strcmp() would fail.  So we should only ever\n> get one entry, it should be the first entry, and dammit it should\n> have matched.\n\nThat is the analysis that the original code is based on, yes. But it's not \nthe easiest thing to follow, and I misunderstood it earlier this evening \n(prompted by the claim that it wasn't working, mostly). It's probably \nworth checking remote->fetch[0].pattern instead of the strcmp, anyway, \nsince that's clearer and cheaper anyway.\n\n> How/why are we getting cases where fetch[0].src isn't in the first\n> entry in ref_map?  What are those other entries?  Are they possibly\n> going to also match fetch[0].src and cause more than one branch\n> to merge?\n\nBeats me; Junio, what's your test case?\n\n> BTW, thanks for looking at this.  I didn't have time to get to it\n> this week and now I'm really unlikely to be able to do so until\n> after I get back from San Jose.  I have too many things crammed\n> into this next week. :-\\\n\nNo problem. You fixed plenty of my other bugs in this code, and it's \ngetting so close to done.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"54230","messageId":"20070928044824.GV3099@spearce.org","threadId":"10055","inReplyTo":"Pine.LNX.4.64.0709280026240.5926@iabervon.org","subject":"Re: [PATCH] Merge non-first refs that match first refspec","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-28T04:48:24Z","receivedAt":"2007-09-28T04:48:24Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> wrote:\n> Beats me; Junio, what's your test case?\n\nIf I understood him correctly it is this:\n\n\tmkdir foo; cd foo; git init\n\tgit config remote.origin.url git://repo.or.cz/alt-git.git\n\tgit config remote.origin.fetch refs/heads/master:refs/remotes/origin/master\n\tgit config --add remote.origin.fetch refs/heads/maint:refs/remotes/origin/maint\n\tgit pull\n\nWe should see \"master\" listed in .git/FETCH_HEAD as a \"for-merge\"\nand \"maint\" listed as a \"not-for-merge\"...\n\nBut if that remote.origin.fetch was a wildcard spec this shouldn't\nhappen as the results are unpredictable.  But above the user\nexplicitly put master first, so it should be defaulted to.\n\n-- \nShawn.\n"},{"id":"54235","messageId":"Pine.LNX.4.64.0709280108060.5926@iabervon.org","threadId":"10055","inReplyTo":"20070928044824.GV3099@spearce.org","subject":"Re: [PATCH] Merge non-first refs that match first refspec","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-09-28T05:12:02Z","receivedAt":"2007-09-28T05:12:02Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 28 Sep 2007, Shawn O. Pearce wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> wrote:\n> > Beats me; Junio, what's your test case?\n> \n> If I understood him correctly it is this:\n> \n> \tmkdir foo; cd foo; git init\n> \tgit config remote.origin.url git://repo.or.cz/alt-git.git\n> \tgit config remote.origin.fetch refs/heads/master:refs/remotes/origin/master\n> \tgit config --add remote.origin.fetch refs/heads/maint:refs/remotes/origin/maint\n> \tgit pull\n> \n> We should see \"master\" listed in .git/FETCH_HEAD as a \"for-merge\"\n> and \"maint\" listed as a \"not-for-merge\"...\n> \n> But if that remote.origin.fetch was a wildcard spec this shouldn't\n> happen as the results are unpredictable.  But above the user\n> explicitly put master first, so it should be defaulted to.\n\nBut after that sequence, the right thing does happen. So I'm guessing that \nhe has some different sequence that triggers a bug.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"54238","messageId":"20070928052548.GX3099@spearce.org","threadId":"10055","inReplyTo":"Pine.LNX.4.64.0709280108060.5926@iabervon.org","subject":"Re: [PATCH] Merge non-first refs that match first refspec","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-28T05:25:48Z","receivedAt":"2007-09-28T05:25:48Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> wrote:\n> On Fri, 28 Sep 2007, Shawn O. Pearce wrote:\n> \n> > Daniel Barkalow <barkalow@iabervon.org> wrote:\n> > > Beats me; Junio, what's your test case?\n> > \n> > If I understood him correctly it is this:\n> > \n> > \tmkdir foo; cd foo; git init\n> > \tgit config remote.origin.url git://repo.or.cz/alt-git.git\n> > \tgit config remote.origin.fetch refs/heads/master:refs/remotes/origin/master\n> > \tgit config --add remote.origin.fetch refs/heads/maint:refs/remotes/origin/maint\n> > \tgit pull\n> > \n> > We should see \"master\" listed in .git/FETCH_HEAD as a \"for-merge\"\n> > and \"maint\" listed as a \"not-for-merge\"...\n> > \n> > But if that remote.origin.fetch was a wildcard spec this shouldn't\n> > happen as the results are unpredictable.  But above the user\n> > explicitly put master first, so it should be defaulted to.\n> \n> But after that sequence, the right thing does happen. So I'm guessing that \n> he has some different sequence that triggers a bug.\n\nOK.  Then I don't understand what issue Junio identified.  And it\nmakes me feel better knowing that the current code does do the\nright thing in the above case, because that's what I meant for it\nto do when I was last in there...\n\n-- \nShawn.\n"},{"id":"54253","messageId":"7vve9vp1zw.fsf@gitster.siamese.dyndns.org","threadId":"10055","inReplyTo":"Pine.LNX.4.64.0709280026240.5926@iabervon.org","subject":"Re: [PATCH] Merge non-first refs that match first refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-28T09:34:27Z","receivedAt":"2007-09-28T09:34:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> Beats me; Junio, what's your test case?\n\nI can paste tomorrow (it is a clone of git.git at work).  I do\nnot use .git/config but .git/remotes/origin and explicit four\nseparate Pull: lines and going over http.\n"},{"id":"54291","messageId":"7vps02o55q.fsf@gitster.siamese.dyndns.org","threadId":"10055","inReplyTo":"7vve9vp1zw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Merge non-first refs that match first refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-28T21:23:45Z","receivedAt":"2007-09-28T21:23:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n>\n>> Beats me; Junio, what's your test case?\n>\n> I can paste tomorrow (it is a clone of git.git at work).  I do\n> not use .git/config but .git/remotes/origin and explicit four\n> separate Pull: lines and going over http.\n\nHere are the files.  Note that I use traditional layout and\nalways have 'master' checked out when I initiate 'git pull'.\n\n: xyzzy git.git/master; cat .git/config\n[core]\n\tlogallrefupdates = true\n\n[diff]\n\tcolor = auto\n\n[showbranch]\n\tdefault = --topo-order\n\tdefault = master\n\tdefault = next\n\tdefault = pu\n\n[alias]\n\tco = checkout\n: xyzzy git.git/master; cat .git/remotes origin\nURL: http://repo.or.cz/r/alt-git.git/\nPull: master:origin\nPull: next:next\nPull: +pu:pu\nPull: maint:maint\nPull: todo:todo\n: xyzzy git.git/master; git pull\nWarning: No merge candidate found because value of config option\n         \"branch.master.merge\" does not match any remote branch\n         fetched.\nNo changes.\n: xyzzy git.git/master; git version\ngit version 1.5.3.2.1060.gaf79f\n: xyzzy git.git/master; exit\n"},{"id":"54292","messageId":"Pine.LNX.4.64.0709281728310.5926@iabervon.org","threadId":"10055","inReplyTo":"7vps02o55q.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Merge non-first refs that match first refspec","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-09-28T21:38:28Z","receivedAt":"2007-09-28T21:38:28Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 28 Sep 2007, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Daniel Barkalow <barkalow@iabervon.org> writes:\n> >\n> >> Beats me; Junio, what's your test case?\n> >\n> > I can paste tomorrow (it is a clone of git.git at work).  I do\n> > not use .git/config but .git/remotes/origin and explicit four\n> > separate Pull: lines and going over http.\n> \n> Here are the files.  Note that I use traditional layout and\n> always have 'master' checked out when I initiate 'git pull'.\n> \n> : xyzzy git.git/master; cat .git/config\n> [core]\n> \tlogallrefupdates = true\n> \n> [diff]\n> \tcolor = auto\n> \n> [showbranch]\n> \tdefault = --topo-order\n> \tdefault = master\n> \tdefault = next\n> \tdefault = pu\n> \n> [alias]\n> \tco = checkout\n> : xyzzy git.git/master; cat .git/remotes origin\n> URL: http://repo.or.cz/r/alt-git.git/\n> Pull: master:origin\n> Pull: next:next\n> Pull: +pu:pu\n> Pull: maint:maint\n> Pull: todo:todo\n\nThe strcmp fails because the config uses an abbreviation and the server \ndoesn't. Forget my first attempt, and try replacing the strcmp on line 105 \nwith \"!remote->fetch[0].pattern\", which is what we're really checking, \nanyway.\n\n(If this is the first refspec we're on, and we don't have a per-branch \nconfig, and we got a match, and the refspec isn't a pattern, merge it; \nanything that matches according to get_fetch_map is a satisfactory match, \neven if it doesn't look quite the same.)\n\nI'll do up an actual patch after dinner if nobody beats me to it.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}