{"thread":{"id":"9917","subject":"[PATCH 3/5] Correct handling of branch.$name.merge in builtin-fetch","startedAt":"2007-09-18T08:54:53Z","lastAt":"2007-09-18T15:05:37Z","messageCount":2,"participants":["Shawn O. Pearce","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"53423","messageId":"20070918085453.GC5390@spearce.org","threadId":"9917","inReplyTo":null,"subject":"[PATCH 3/5] Correct handling of branch.$name.merge in builtin-fetch","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-18T08:54:53Z","receivedAt":"2007-09-18T08:54:53Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"My prior bug fix for git-push titled \"Don't configure remote \".\" to\nfetch everything to itself\" actually broke t5520 as we were unable\nto evaluate a branch configuration of:\n\n  [branch \"copy\"]\n    remote = .\n    merge = refs/heads/master\n\nas remote \".\" did not have a \"remote...fetch\" configuration entry to\noffer up refs/heads/master as a possible candidate available to be\nfetched and merged.  In shell script git-fetch and prior to the above\nmentioned commit this was hardcoded for a url of \".\" to be the set of\nlocal branches.\n\nChasing down this bug led me to the conclusion that our prior behavior\nwith regards to branch.$name.merge was incorrect.  In the shell script\nbased git-fetch implementation we only fetched and merged a branch if\nit appeared both in branch.$name.merge *and* in remote.$r.fetch, where\n$r = branch.$name.remote.  In other words in the following config file:\n\n  [remote \"origin\"]\n    url = git://git.kernel.org/pub/scm/git/git.git\n    fetch = refs/heads/master:refs/remotes/origin/master\n  [branch \"master\"]\n    remote = origin\n    merge = refs/heads/master\n  [branch \"pu\"]\n    remote = origin\n    merge = refs/heads/pu\n\nAttempting to run `git pull` while on branch \"pu\" would always give\nthe user \"Already up-to-date\" as git-fetch did not fetch pu and thus\ndid not mark it for merge in .git/FETCH_HEAD.  The configured merge\nwould always be ignored and the user would be left scratching her\nconfused head wondering why merge did not work on \"pu\" but worked\nfine on \"master\".\n\nIf we are using the \"default fetch\" specification for the current\nbranch and the current branch has a branch.$name.merge configured\nwe now union it with the list of refs in remote.$r.fetch.  This\nway the above configuration does what the user expects it to do,\nwhich is to fetch only \"master\" by default but when on \"pu\" to\nfetch both \"master\" and \"pu\".\n\nThis uncovered some breakage in the test suite where old-style Cogito\nbranches (.git/branches/$r) did not fetch the branches listed in\n.git/config for merging and thus did not actually merge them if the\nuser tried to use `git pull` on that branch.  Junio and I discussed\nit on list and felt that the union approach here makes more sense to\nDWIM for the end-user than silently ignoring their configured request\nso the test vectors for t5515 have been updated to include for-merge\nlines in .git/FETCH_HEAD where they have been configured for-merge\nin .git/config.\n\nSince we are now performing a union of the fetch specification and\nthe merge specification and we cannot allow a branch to be listed\ntwice (otherwise it comes out twice in .git/FETCH_HEAD) we need to\nperform a double loop here over all of the branch.$name.merge lines\nand try to set their merge flag if we have already schedule that\nbranch for fetching by remote.$r.fetch.  If no match is found then\nwe must add new specifications to fetch the branch but not store it\nas no local tracking branch has been designated.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n builtin-fetch.c                                    |   48 ++++++++++++++-----\n remote.c                                           |   13 ++---\n remote.h                                           |    3 +-\n t/t5515/fetch.br-branches-default-merge            |    1 +\n ...etch.br-branches-default-merge_branches-default |    1 +\n t/t5515/fetch.br-branches-default-octopus          |    2 +\n ...ch.br-branches-default-octopus_branches-default |    2 +\n t/t5515/fetch.br-branches-one-merge                |    1 +\n t/t5515/fetch.br-branches-one-merge_branches-one   |    1 +\n t/t5515/fetch.br-branches-one-octopus              |    1 +\n t/t5515/fetch.br-branches-one-octopus_branches-one |    1 +\n 11 files changed, 51 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 20926e0..670af0b 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -28,20 +28,37 @@ static void unlock_pack_on_signal(int signo)\n \traise(signo);\n }\n \n-static void find_merge_config(struct ref *ref_map, struct remote *remote)\n+static void add_merge_config(struct ref **head,\n+\t\t           struct ref *remote_refs,\n+\t\t           struct branch *branch,\n+\t\t           struct ref ***tail)\n {\n-\tstruct ref *rm = ref_map;\n-\tstruct branch *branch = branch_get(NULL);\n+\tint i;\n \n-\tfor (rm = ref_map; rm; rm = rm->next) {\n-\t\tif (!branch_has_merge_config(branch)) {\n-\t\t\tif (remote && remote->fetch &&\n-\t\t\t    !strcmp(remote->fetch[0].src, rm->name))\n-\t\t\t\trm->merge = 1;\n-\t\t} else {\n-\t\t\tif (branch_merges(branch, rm->name))\n+\tfor (i = 0; i < branch->merge_nr; i++) {\n+\t\tstruct ref *rm, **old_tail = *tail;\n+\t\tstruct refspec refspec;\n+\n+\t\tfor (rm = *head; rm; rm = rm->next) {\n+\t\t\tif (branch_merge_matches(branch, i, rm->name)) {\n \t\t\t\trm->merge = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n+\t\tif (rm)\n+\t\t\tcontinue;\n+\n+\t\t/* Not fetched to a tracking branch?  We need to fetch\n+\t\t * it anyway to allow this branch's \"branch.$name.merge\"\n+\t\t * to be honored by git-pull.\n+\t\t */\n+\t\trefspec.src = branch->merge[i]->src;\n+\t\trefspec.dst = NULL;\n+\t\trefspec.pattern = 0;\n+\t\trefspec.force = 0;\n+\t\tget_fetch_map(remote_refs, &refspec, tail);\n+\t\tfor (rm = *old_tail; rm; rm = rm->next)\n+\t\t\trm->merge = 1;\n \t}\n }\n \n@@ -76,17 +93,22 @@ static struct ref *get_ref_map(struct transport *transport,\n \t} else {\n \t\t/* Use the defaults */\n \t\tstruct remote *remote = transport->remote;\n-\t\tif (remote->fetch_refspec_nr) {\n+\t\tstruct branch *branch = branch_get(NULL);\n+\t\tint has_merge = branch_has_merge_config(branch);\n+\t\tif (remote && (remote->fetch_refspec_nr || has_merge)) {\n \t\t\tfor (i = 0; i < remote->fetch_refspec_nr; i++) {\n \t\t\t\tget_fetch_map(remote_refs, &remote->fetch[i], &tail);\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\tfind_merge_config(ref_map, remote);\n+\t\t\tif (has_merge)\n+\t\t\t\tadd_merge_config(&ref_map, remote_refs, branch, &tail);\n \t\t} else {\n \t\t\tref_map = get_remote_ref(remote_refs, \"HEAD\");\n-\n \t\t\tref_map->merge = 1;\n \t\t}\n \t}\ndiff --git a/remote.c b/remote.c\nindex af3c46b..31e2b70 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -772,16 +772,13 @@ int branch_has_merge_config(struct branch *branch)\n \treturn branch && !!branch->merge;\n }\n \n-int branch_merges(struct branch *branch, const char *refname)\n+int branch_merge_matches(struct branch *branch,\n+\t\t                 int i,\n+\t\t                 const char *refname)\n {\n-\tint i;\n-\tif (!branch)\n+\tif (!branch || i < 0 || i >= branch->merge_nr)\n \t\treturn 0;\n-\tfor (i = 0; i < branch->merge_nr; i++) {\n-\t\tif (ref_matches_abbrev(branch->merge[i]->src, refname))\n-\t\t\treturn 1;\n-\t}\n-\treturn 0;\n+\treturn ref_matches_abbrev(branch->merge[i]->src, refname);\n }\n \n static struct ref *get_expanded_map(struct ref *remote_refs,\ndiff --git a/remote.h b/remote.h\nindex 8994052..b5b558f 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -88,7 +88,6 @@ struct branch {\n struct branch *branch_get(const char *name);\n \n int branch_has_merge_config(struct branch *branch);\n-\n-int branch_merges(struct branch *branch, const char *refname);\n+int branch_merge_matches(struct branch *, int n, const char *);\n \n #endif\ndiff --git a/t/t5515/fetch.br-branches-default-merge b/t/t5515/fetch.br-branches-default-merge\nindex 828bfd8..ca2cc1d 100644\n--- a/t/t5515/fetch.br-branches-default-merge\n+++ b/t/t5515/fetch.br-branches-default-merge\n@@ -1,5 +1,6 @@\n # br-branches-default-merge\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\tbranch 'master' of ../\n+0567da4d5edd2ff4bb292a465ba9e64dcad9536b\t\tbranch 'three' of ../\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\ndiff --git a/t/t5515/fetch.br-branches-default-merge_branches-default b/t/t5515/fetch.br-branches-default-merge_branches-default\nindex f148673..7d947cd 100644\n--- a/t/t5515/fetch.br-branches-default-merge_branches-default\n+++ b/t/t5515/fetch.br-branches-default-merge_branches-default\n@@ -1,5 +1,6 @@\n # br-branches-default-merge branches-default\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\tbranch 'master' of ../\n+0567da4d5edd2ff4bb292a465ba9e64dcad9536b\t\tbranch 'three' of ../\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\ndiff --git a/t/t5515/fetch.br-branches-default-octopus b/t/t5515/fetch.br-branches-default-octopus\nindex bb1a191..ec39c54 100644\n--- a/t/t5515/fetch.br-branches-default-octopus\n+++ b/t/t5515/fetch.br-branches-default-octopus\n@@ -1,5 +1,7 @@\n # br-branches-default-octopus\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\tbranch 'master' of ../\n+8e32a6d901327a23ef831511badce7bf3bf46689\t\tbranch 'one' of ../\n+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8\t\tbranch 'two' of ../\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\ndiff --git a/t/t5515/fetch.br-branches-default-octopus_branches-default b/t/t5515/fetch.br-branches-default-octopus_branches-default\nindex 970fc93..6bf42e2 100644\n--- a/t/t5515/fetch.br-branches-default-octopus_branches-default\n+++ b/t/t5515/fetch.br-branches-default-octopus_branches-default\n@@ -1,5 +1,7 @@\n # br-branches-default-octopus branches-default\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\tbranch 'master' of ../\n+8e32a6d901327a23ef831511badce7bf3bf46689\t\tbranch 'one' of ../\n+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8\t\tbranch 'two' of ../\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\ndiff --git a/t/t5515/fetch.br-branches-one-merge b/t/t5515/fetch.br-branches-one-merge\nindex 24099fd..b4b3b35 100644\n--- a/t/t5515/fetch.br-branches-one-merge\n+++ b/t/t5515/fetch.br-branches-one-merge\n@@ -1,5 +1,6 @@\n # br-branches-one-merge\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\tbranch 'one' of ../\n+0567da4d5edd2ff4bb292a465ba9e64dcad9536b\t\tbranch 'three' of ../\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\ndiff --git a/t/t5515/fetch.br-branches-one-merge_branches-one b/t/t5515/fetch.br-branches-one-merge_branches-one\nindex e4b4fde..2ecef38 100644\n--- a/t/t5515/fetch.br-branches-one-merge_branches-one\n+++ b/t/t5515/fetch.br-branches-one-merge_branches-one\n@@ -1,5 +1,6 @@\n # br-branches-one-merge branches-one\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\tbranch 'one' of ../\n+0567da4d5edd2ff4bb292a465ba9e64dcad9536b\t\tbranch 'three' of ../\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\ndiff --git a/t/t5515/fetch.br-branches-one-octopus b/t/t5515/fetch.br-branches-one-octopus\nindex 53fe808..96e3029 100644\n--- a/t/t5515/fetch.br-branches-one-octopus\n+++ b/t/t5515/fetch.br-branches-one-octopus\n@@ -1,5 +1,6 @@\n # br-branches-one-octopus\n 8e32a6d901327a23ef831511badce7bf3bf46689\t\tbranch 'one' of ../\n+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8\t\tbranch 'two' of ../\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\ndiff --git a/t/t5515/fetch.br-branches-one-octopus_branches-one b/t/t5515/fetch.br-branches-one-octopus_branches-one\nindex 41b18ff..55e0bad 100644\n--- a/t/t5515/fetch.br-branches-one-octopus_branches-one\n+++ b/t/t5515/fetch.br-branches-one-octopus_branches-one\n@@ -1,5 +1,6 @@\n # br-branches-one-octopus branches-one\n 8e32a6d901327a23ef831511badce7bf3bf46689\t\tbranch 'one' of ../\n+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8\t\tbranch 'two' of ../\n 754b754407bf032e9a2f9d5a9ad05ca79a6b228f\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\n-- \n1.5.3.1.1000.g7319b\n"},{"id":"53487","messageId":"Pine.LNX.4.64.0709181034300.5298@iabervon.org","threadId":"9917","inReplyTo":"20070918085453.GC5390@spearce.org","subject":"Re: [PATCH 3/5] Correct handling of branch.$name.merge in builtin-fetch","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-09-18T15:05:37Z","receivedAt":"2007-09-18T15:05:37Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 18 Sep 2007, Shawn O. Pearce wrote:\n\n> My prior bug fix for git-push titled \"Don't configure remote \".\" to\n> fetch everything to itself\" actually broke t5520 as we were unable\n> to evaluate a branch configuration of:\n> \n>   [branch \"copy\"]\n>     remote = .\n>     merge = refs/heads/master\n> \n> as remote \".\" did not have a \"remote...fetch\" configuration entry to\n> offer up refs/heads/master as a possible candidate available to be\n> fetched and merged.  In shell script git-fetch and prior to the above\n> mentioned commit this was hardcoded for a url of \".\" to be the set of\n> local branches.\n\nAh, right. When you removed that, I remembered there being some reason I'd \nput it in, but I couldn't remember what it was, and knew you'd turn it up \nbefore I would.\n\n> Chasing down this bug led me to the conclusion that our prior behavior\n> with regards to branch.$name.merge was incorrect.  In the shell script\n> based git-fetch implementation we only fetched and merged a branch if\n> it appeared both in branch.$name.merge *and* in remote.$r.fetch, where\n> $r = branch.$name.remote.  In other words in the following config file:\n> \n>   [remote \"origin\"]\n>     url = git://git.kernel.org/pub/scm/git/git.git\n>     fetch = refs/heads/master:refs/remotes/origin/master\n>   [branch \"master\"]\n>     remote = origin\n>     merge = refs/heads/master\n>   [branch \"pu\"]\n>     remote = origin\n>     merge = refs/heads/pu\n> \n> Attempting to run `git pull` while on branch \"pu\" would always give\n> the user \"Already up-to-date\" as git-fetch did not fetch pu and thus\n> did not mark it for merge in .git/FETCH_HEAD.  The configured merge\n> would always be ignored and the user would be left scratching her\n> confused head wondering why merge did not work on \"pu\" but worked\n> fine on \"master\".\n> \n> If we are using the \"default fetch\" specification for the current\n> branch and the current branch has a branch.$name.merge configured\n> we now union it with the list of refs in remote.$r.fetch.  This\n> way the above configuration does what the user expects it to do,\n> which is to fetch only \"master\" by default but when on \"pu\" to\n> fetch both \"master\" and \"pu\".\n\nAnd store master, but don't store pu. This looks like the right solution \nto me.\n\n> This uncovered some breakage in the test suite where old-style Cogito\n> branches (.git/branches/$r) did not fetch the branches listed in\n> .git/config for merging and thus did not actually merge them if the\n> user tried to use `git pull` on that branch.  Junio and I discussed\n> it on list and felt that the union approach here makes more sense to\n> DWIM for the end-user than silently ignoring their configured request\n> so the test vectors for t5515 have been updated to include for-merge\n> lines in .git/FETCH_HEAD where they have been configured for-merge\n> in .git/config.\n\nAh, okay. This is one of the things I'd changed, to make it obey the merge \nconfiguration (which meant that it didn't get anything to merge), which \nhad previously been ignored. So there's nothing that's expecting exactly \nthe behavior that you're changing away from, except for that test case.\n\nThis whole series looks good to me.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}