{"thread":{"id":"28206","subject":"[PATCH] submodule: Demonstrate known breakage during recursive merge","startedAt":"2011-08-24T13:59:50Z","lastAt":"2011-08-26T19:30:56Z","messageCount":25,"participants":["Brad King","Heiko Voigt","Junio C Hamano","Sverre Rabbelier"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174152","messageId":"680d2679c3275c01152500760311b5f96a93ea62.1314193375.git.brad.king@kitware.com","threadId":"28206","inReplyTo":null,"subject":"[PATCH] submodule: Demonstrate known breakage during recursive merge","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2011-08-24T13:59:50Z","receivedAt":"2011-08-24T13:59:50Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"Since commit 68d03e4a (Implement automatic fast-forward merge for\nsubmodules, 2010-07-07) we try to suggest submodule commits that resolve\na conflict.  Consider a true recursive merge case\n\n    b---bc\n   / \\ /\n  o   X\n   \\ / \\\n    c---cb\n\nin which the two heads themselves (bc,cb) had resolved a submodule\nconflict (i.e. reference different commits than their parents).  The\nsubmodule merge search runs during the temporary merge of the two merge\nbases (b,c) and prints out a suggestion that is not meaningful to the\nuser.  Then during the main merge the submodule merge search runs again\nbut dies with the message\n\n  fatal: --ancestry-path given but there are no bottom commits\n\nwhile trying to enumerate candidates.  Demonstrate this known breakage\nwith a new test in t7405-submodule-merge covering the case.\n\nSigned-off-by: Brad King <brad.king@kitware.com>\n---\nHi folks,\n\nA co-worker encountered this problem last week in a real project.\nThe merge dies and does not leave the submodule conflict in the index.\nWe were able to work around it by moving the submodule out of the way\nand resolving the conflict by hand.  Then I ran the merge again to\ndebug the problem and narrowed it down to this case.\n\nBTW, if one adds the line\n\n  rm -rf sub && mkdir sub &&\n\njust above the line\n\n  test_must_fail git merge top-bc\n\nthen the test passes because no submodule merge search runs.  Therefore\nthis test can be switched to expect success when the problem is fixed.\n\nBrad\n\n t/t7405-submodule-merge.sh |   51 ++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 51 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex a8fb30b..8f6f2d6 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -228,4 +228,55 @@ test_expect_success 'merging with a modify/modify conflict between merge bases'\n \tgit merge d\n '\n \n+# canonical criss-cross history in top and submodule\n+test_expect_success 'setup for recursive merge with submodule' '\n+\tmkdir merge-recursive &&\n+\t(cd merge-recursive &&\n+\t git init &&\n+\t mkdir sub &&\n+\t (cd sub &&\n+\t  git init &&\n+\t  test_commit a &&\n+\t  git checkout -b sub-b master &&\n+\t  test_commit b &&\n+\t  git checkout -b sub-c master &&\n+\t  test_commit c &&\n+\t  git checkout -b sub-bc sub-b &&\n+\t  git merge sub-c &&\n+\t  git checkout -b sub-cb sub-c &&\n+\t  git merge sub-b &&\n+\t  git checkout master) &&\n+\t git add sub &&\n+\t git commit -m a &&\n+\t git checkout -b top-b master &&\n+\t (cd sub && git checkout sub-b) &&\n+\t git add sub &&\n+\t git commit -m b &&\n+\t git checkout -b top-c master &&\n+\t (cd sub && git checkout sub-c) &&\n+\t git add sub &&\n+\t git commit -m c &&\n+\t git checkout -b top-bc top-b &&\n+\t git merge -s ours -n top-c &&\n+\t (cd sub && git checkout sub-bc) &&\n+\t git add sub &&\n+\t git commit -m bc &&\n+\t git checkout -b top-cb top-c &&\n+\t git merge -s ours -n top-b &&\n+\t (cd sub && git checkout sub-cb) &&\n+\t git add sub &&\n+\t git commit -m cb)\n+'\n+\n+# merge should leave submodule unmerged in index\n+test_expect_failure 'recursive merge with submodule' '\n+\t(cd merge-recursive &&\n+\t test_must_fail git merge top-bc &&\n+\t echo \"160000 $(git rev-parse top-cb:sub) 2\tsub\" > expect2 &&\n+\t echo \"160000 $(git rev-parse top-bc:sub) 3\tsub\" > expect3 &&\n+\t git ls-files -u > actual &&\n+\t grep \"$(cat expect2)\" actual > /dev/null &&\n+\t grep \"$(cat expect3)\" actual > /dev/null)\n+'\n+\n test_done\n-- \n1.7.4.4\n"},{"id":"174177","messageId":"20110824191438.GA45292@book.hvoigt.net","threadId":"28206","inReplyTo":"680d2679c3275c01152500760311b5f96a93ea62.1314193375.git.brad.king@kitware.com","subject":"Re: [PATCH] submodule: Demonstrate known breakage during recursive merge","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-24T19:14:38Z","receivedAt":"2011-08-24T19:14:38Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nthanks for finding this subtle bug!\n\nOn Wed, Aug 24, 2011 at 09:59:50AM -0400, Brad King wrote:\n> Since commit 68d03e4a (Implement automatic fast-forward merge for\n> submodules, 2010-07-07) we try to suggest submodule commits that resolve\n> a conflict.  Consider a true recursive merge case\n> \n>     b---bc\n>    / \\ /\n>   o   X\n>    \\ / \\\n>     c---cb\n\nAnd here is a patch[1] that you can apply on top of yours which should fix\nthis. An extra pair of merge machinery knowing eyes appreciated. Its a\nlittle bit workaroundish so if anymore has an idea how to fix this in\nnicer way, please tell me.\n\n[1]--8<----\nFrom: Heiko Voigt <hvoigt@hvoigt.net>\nSubject: [PATCH] protect submodule merge search against multiple calls for\n the same path\n\nWhen multiple merge-bases are found for two commits to be merged the\nmerge machinery will ask twice for a merge resolution. Currently its not\npossible to use the revision-walking api for walking the same commits\nmultiple times. Since the result will not change we can simply fail\nhere if we are asked for a resolution of the same path again.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n submodule.c                |    9 +++++++++\n t/t7405-submodule-merge.sh |    2 +-\n 2 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 1ba9646..a4af08e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -646,6 +646,7 @@ int merge_submodule(unsigned char result[20], const char *path,\n \t\t    const unsigned char base[20], const unsigned char a[20],\n \t\t    const unsigned char b[20])\n {\n+\tstatic char last_path[PATH_MAX] = {'\\0'};\n \tstruct commit *commit_base, *commit_a, *commit_b;\n \tint parent_count;\n \tstruct object_array merges;\n@@ -699,6 +700,13 @@ int merge_submodule(unsigned char result[20], const char *path,\n \t * user needs to confirm the resolution.\n \t */\n \n+\t/* in case of multiple merge-bases the merge algorithm will ask\n+\t * again for a resolution. We should not search twice for the\n+\t * same path.\n+\t */\n+\tif (!strcmp(path, last_path))\n+\t\treturn 0;\n+\n \t/* find commit which merges them */\n \tparent_count = find_first_merges(&merges, path, commit_a, commit_b);\n \tswitch (parent_count) {\n@@ -726,6 +734,7 @@ int merge_submodule(unsigned char result[20], const char *path,\n \t\t\tprint_commit((struct commit *) merges.objects[i].item);\n \t}\n \n+\tmemcpy(last_path, path, strlen(path) + 1);\n \tfree(merges.objects);\n \treturn 0;\n }\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 8f6f2d6..603fb72 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '\n '\n \n # merge should leave submodule unmerged in index\n-test_expect_failure 'recursive merge with submodule' '\n+test_expect_success 'recursive merge with submodule' '\n \t(cd merge-recursive &&\n \t test_must_fail git merge top-bc &&\n \t echo \"160000 $(git rev-parse top-cb:sub) 2\tsub\" > expect2 &&\n-- \n1.7.6.551.g4266ca\n"},{"id":"174178","messageId":"7vty96obo9.fsf@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"20110824191438.GA45292@book.hvoigt.net","subject":"Re: [PATCH] submodule: Demonstrate known breakage during recursive merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-24T19:24:22Z","receivedAt":"2011-08-24T19:24:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> ... Its a\n> little bit workaroundish so if anymore has an idea how to fix this in\n> nicer way, please tell me.\n>\n> [1]--8<----\n> From: Heiko Voigt <hvoigt@hvoigt.net>\n> Subject: [PATCH] protect submodule merge search against multiple calls for\n>  the same path\n>\n> When multiple merge-bases are found for two commits to be merged the\n> merge machinery will ask twice for a merge resolution. Currently its not\n> possible to use the revision-walking api for walking the same commits\n> multiple times.\n\nI have been suspecting that most of this should be done in a separate\nhelper program that is run via run_command() interface, without\ncontaminating the object pool the main merge process has with data from\nthe submodule object store to begin with (i.e. add_submodule_odb() and\neverything below should go). Wouldn't it be a lot cleaner solution?\n"},{"id":"174180","messageId":"20110824194618.GD45292@book.hvoigt.net","threadId":"28206","inReplyTo":"7vty96obo9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] submodule: Demonstrate known breakage during recursive merge","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-24T19:46:18Z","receivedAt":"2011-08-24T19:46:18Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Aug 24, 2011 at 12:24:22PM -0700, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n> > ... Its a\n> > little bit workaroundish so if anymore has an idea how to fix this in\n> > nicer way, please tell me.\n> >\n> > [1]--8<----\n> > From: Heiko Voigt <hvoigt@hvoigt.net>\n> > Subject: [PATCH] protect submodule merge search against multiple calls for\n> >  the same path\n> >\n> > When multiple merge-bases are found for two commits to be merged the\n> > merge machinery will ask twice for a merge resolution. Currently its not\n> > possible to use the revision-walking api for walking the same commits\n> > multiple times.\n> \n> I have been suspecting that most of this should be done in a separate\n> helper program that is run via run_command() interface, without\n> contaminating the object pool the main merge process has with data from\n> the submodule object store to begin with (i.e. add_submodule_odb() and\n> everything below should go). Wouldn't it be a lot cleaner solution?\n\nHmm, I would like to keep it in process. Since there are platforms where\nspawning new processes is very slow. If just the revision-walking is an\nissue: I am currently working on extending it to be able to walk again,\nbecause we also need that for the recursive push series.\n\nBut even if we are able to search twice or more (in or out process) we\nwould give the advice that there are multiple possible resolutions\nfor each merge base. For the merge search we do not take the bases into\naccount so the outcome will not change. So what I think would be\ncleaner (also UI wise) is to remember whether we already gave advice and\nnot do it a second time.\n\nCheers Heiko\n"},{"id":"174181","messageId":"4E5558BB.4040307@kitware.com","threadId":"28206","inReplyTo":"20110824194618.GD45292@book.hvoigt.net","subject":"Re: [PATCH] submodule: Demonstrate known breakage during recursive merge","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2011-08-24T20:02:03Z","receivedAt":"2011-08-24T20:02:03Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"On 8/24/2011 3:14 PM, Heiko Voigt wrote:\n > thanks for finding this subtle bug!\n\nThanks for looking at it!\n\nOn 8/24/2011 3:46 PM, Heiko Voigt wrote:\n> For the merge search we do not take the bases into\n> account so the outcome will not change.\n\nThe test case creates history like this:\n\n >     b---bc\n >    / \\ /\n >   o   X\n >    \\ / \\\n >     c---cb\n\nwhere b, c, bc, and cb all reference different submodule commits.\n\nIsn't the merge search asked to search for a descendant of \"b:sub\" and \"c:sub\"\nduring the recursive part of the merge and then \"bc:sub\" and \"cb:sub\" during\nthe primary merge?  Might those results be different?\n\nAs for the UI part, I think the user would be interested only in the search\nresults for the primary merge between HEAD and MERGE_HEAD.  Results from the\nintermediate merges might not make sense.\n\nThanks,\n-Brad\n"},{"id":"174186","messageId":"20110824202721.GF45292@book.hvoigt.net","threadId":"28206","inReplyTo":"4E5558BB.4040307@kitware.com","subject":"Re: [PATCH] submodule: Demonstrate known breakage during recursive merge","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-24T20:27:22Z","receivedAt":"2011-08-24T20:27:22Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Aug 24, 2011 at 04:02:03PM -0400, Brad King wrote:\n> On 8/24/2011 3:14 PM, Heiko Voigt wrote:\n> > thanks for finding this subtle bug!\n>\n> Thanks for looking at it!\n>\n> On 8/24/2011 3:46 PM, Heiko Voigt wrote:\n>> For the merge search we do not take the bases into\n>> account so the outcome will not change.\n>\n> The test case creates history like this:\n>\n> >     b---bc\n> >    / \\ /\n> >   o   X\n> >    \\ / \\\n> >     c---cb\n>\n> where b, c, bc, and cb all reference different submodule commits.\n>\n> Isn't the merge search asked to search for a descendant of \"b:sub\" and \"c:sub\"\n> during the recursive part of the merge and then \"bc:sub\" and \"cb:sub\" during\n> the primary merge?  Might those results be different?\n\nThe merge is quite simple. All it does is check whether both changes\nbase->a or base->b point forward in the submodule. Then it checks\nwhether a is contained in b or the other way around. This is the only\ncase in which it will succeed automatically.\n\nSupposing you merge bc into cb:\nIf I understand the situation correctly, the above is done first with\na := cb:sub, b := bc:sub, base := b:sub and then another time with\nbase := c:sub.\n\nFor the suggestion part only bc and cb are taken into account. That is\nwe search for the first commit in the submodule refs which contains both\nbc:sub and cb:sub.\n\n>\n> As for the UI part, I think the user would be interested only in the search\n> results for the primary merge between HEAD and MERGE_HEAD.  Results from the\n> intermediate merges might not make sense.\n\nAs stated above since bc:sub and cb:sub will not change in between two\nsearches the result for the suggestion will be the same. What I meant\nwas that the same result would be output twice (or more).\n\nCheers Heiko\n"},{"id":"174189","messageId":"4E5561D2.2080206@kitware.com","threadId":"28206","inReplyTo":"20110824202721.GF45292@book.hvoigt.net","subject":"Re: [PATCH] submodule: Demonstrate known breakage during recursive merge","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2011-08-24T20:40:50Z","receivedAt":"2011-08-24T20:40:50Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"On 8/24/2011 4:27 PM, Heiko Voigt wrote:\n>>>      b---bc\n>>>     / \\ /\n>>>    o   X\n>>>     \\ / \\\n>>>      c---cb\n[snip]\n> Supposing you merge bc into cb:\n> If I understand the situation correctly, the above is done first with\n> a := cb:sub, b := bc:sub, base := b:sub and then another time with\n> base := c:sub.\n\nWhen merging bc and cb there are two merge bases: b and c.  The recursive\nmerge strategy first performs a \"virtual\" merge between b and c and uses\nthe result as a fictional merge base between bc and cb.  Currently the\nsubmodule merge search runs during the \"virtual\" merge and gives advice.\nThen it later dies while trying to search during the \"real\" merge.\n\nAfter applying my patch, try this:\n\n  $ cd t && ./t7405-submodule-merge.sh --verbose\n  ...\n  Merging:\n  8cbd0fb cb\n  virtual top-bc\n  found 2 common ancestor(s):\n  f6b4d5a b\n  4d9cfab c\n    Merging:\n    f6b4d5a b\n    4d9cfab c\n    found 1 common ancestor(s):\n    a2ff72f a\n  warning: Failed to merge submodule sub (multiple merges found)\n   806049692f8921101f2e7223852e3bd74f7187c8: > Merge branch 'sub-c' into sub-bc\n   db70dfacda48ce55365256a58eaf89b7da87cbe7: > Merge branch 'sub-b' into sub-cb\n    Auto-merging sub\n    CONFLICT (submodule): Merge conflict in sub\n  fatal: --ancestry-path given but there are no bottom commits\n\nOne can see that the advice given talks about merging \"b:sub\" and \"c:sub\"\nand the suggested commits are actually \"bc:sub\" and \"cb:sub\".  This advice\nis not useful to someone mergeing bc and cb.\n\n-Brad\n"},{"id":"174194","messageId":"20110824213205.GI45292@book.hvoigt.net","threadId":"28206","inReplyTo":"4E5561D2.2080206@kitware.com","subject":"Re: [PATCH] submodule: Demonstrate known breakage during recursive merge","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-24T21:32:05Z","receivedAt":"2011-08-24T21:32:05Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Wed, Aug 24, 2011 at 04:40:50PM -0400, Brad King wrote:\n> On 8/24/2011 4:27 PM, Heiko Voigt wrote:\n>>>>      b---bc\n>>>>     / \\ /\n>>>>    o   X\n>>>>     \\ / \\\n>>>>      c---cb\n> [snip]\n>> Supposing you merge bc into cb:\n>> If I understand the situation correctly, the above is done first with\n>> a := cb:sub, b := bc:sub, base := b:sub and then another time with\n>> base := c:sub.\n>\n> When merging bc and cb there are two merge bases: b and c.  The recursive\n> merge strategy first performs a \"virtual\" merge between b and c and uses\n> the result as a fictional merge base between bc and cb.  Currently the\n> submodule merge search runs during the \"virtual\" merge and gives advice.\n> Then it later dies while trying to search during the \"real\" merge.\n\nBut what happens if this \"virtual\" merge of b and c does not succeed\nlike in our case?\n\n> After applying my patch, try this:\n>\n>  $ cd t && ./t7405-submodule-merge.sh --verbose\n>  ...\n>  Merging:\n>  8cbd0fb cb\n>  virtual top-bc\n>  found 2 common ancestor(s):\n>  f6b4d5a b\n>  4d9cfab c\n>    Merging:\n>    f6b4d5a b\n>    4d9cfab c\n>    found 1 common ancestor(s):\n>    a2ff72f a\n>  warning: Failed to merge submodule sub (multiple merges found)\n>   806049692f8921101f2e7223852e3bd74f7187c8: > Merge branch 'sub-c' into sub-bc\n>   db70dfacda48ce55365256a58eaf89b7da87cbe7: > Merge branch 'sub-b' into sub-cb\n>    Auto-merging sub\n>    CONFLICT (submodule): Merge conflict in sub\n>  fatal: --ancestry-path given but there are no bottom commits\n\nYeah I have seen this and it looked to me as if those two commits are\nbc:sub and cb:sub since the commit message talks about c:sub merged into\nbc:sub and vice versa.\n\n> One can see that the advice given talks about merging \"b:sub\" and \"c:sub\"\n> and the suggested commits are actually \"bc:sub\" and \"cb:sub\".  This advice\n> is not useful to someone mergeing bc and cb.\n\nIn this case we should only output the advice of the last merge of\ncourse. It will have the same result in this case though since the merge\nof bc:sub and cb:sub still has just these two candidates.\n\nCheers Heiko\n"},{"id":"174198","messageId":"7v39gqo2fn.fsf@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"20110824194618.GD45292@book.hvoigt.net","subject":"Re: [PATCH] submodule: Demonstrate known breakage during recursive merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-24T22:43:56Z","receivedAt":"2011-08-24T22:43:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n>> I have been suspecting that most of this should be done in a separate\n>> helper program that is run via run_command() interface, without\n>> contaminating the object pool the main merge process has with data from\n>> the submodule object store to begin with (i.e. add_submodule_odb() and\n>> everything below should go). Wouldn't it be a lot cleaner solution?\n>\n> Hmm, I would like to keep it in process. Since there are platforms where\n> spawning new processes is very slow.\n\nAdding submodule's odb into the main process _will_ also have performance\npenalties because it will make it more expensive to look up objects that\nbelong to the superproject when the superproject wants its own look up.\n\nIn case you haven't realized yet, walking revision graph multiple times\nwhile making sure that you do not affect other revision traversals in\neffect is hard to arrange right. But more importantly, correctness counts\nmore than performing quickly and giving a bogus result with premature\noptimization that makes it harder to implement things correctly (and\nharder to verify the change is correct).\n"},{"id":"174224","messageId":"074f22629c034dba738b7241c78229db7f9159ec.1314275112.git.brad.king@kitware.com","threadId":"28206","inReplyTo":"680d2679c3275c01152500760311b5f96a93ea62.1314193375.git.brad.king@kitware.com","subject":"[PATCH v2] submodule: Demonstrate known breakage during recursive merge","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2011-08-25T12:28:39Z","receivedAt":"2011-08-25T12:28:39Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"Since commit 68d03e4a (Implement automatic fast-forward merge for\nsubmodules, 2010-07-07) we try to suggest submodule commits that resolve\na conflict.  Consider a true recursive merge case\n\n    b---bc\n   / \\ /\n  o   X\n   \\ / \\\n    c---cb\n\nin which the two heads themselves (bc,cb) had resolved a submodule\nconflict (i.e. reference different commits than their parents).  The\nsubmodule merge search runs during the temporary merge of the two merge\nbases (b,c) and prints out a suggestion that is not meaningful to the\nuser.  Then during the main merge the submodule merge search runs again\nbut dies with the message\n\n  fatal: --ancestry-path given but there are no bottom commits\n\nwhile trying to enumerate candidates.  Demonstrate this known breakage\nwith a new test in t7405-submodule-merge covering the case.\n\nSigned-off-by: Brad King <brad.king@kitware.com>\n---\n\nThis fixes the first version of the patch by using \"merge --no-commit\"\ninstead of \"merge -n\" where the intention is to not commit the merge.\nBoth instances are followed by a submodule checkout, add, and then a\nreal commit.  Now the history in the test actually looks like what\nis in the commit message.\n\nBrad\n\n t/t7405-submodule-merge.sh |   51 ++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 51 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex a8fb30b..14da2e3 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -228,4 +228,55 @@ test_expect_success 'merging with a modify/modify conflict between merge bases'\n \tgit merge d\n '\n \n+# canonical criss-cross history in top and submodule\n+test_expect_success 'setup for recursive merge with submodule' '\n+\tmkdir merge-recursive &&\n+\t(cd merge-recursive &&\n+\t git init &&\n+\t mkdir sub &&\n+\t (cd sub &&\n+\t  git init &&\n+\t  test_commit a &&\n+\t  git checkout -b sub-b master &&\n+\t  test_commit b &&\n+\t  git checkout -b sub-c master &&\n+\t  test_commit c &&\n+\t  git checkout -b sub-bc sub-b &&\n+\t  git merge sub-c &&\n+\t  git checkout -b sub-cb sub-c &&\n+\t  git merge sub-b &&\n+\t  git checkout master) &&\n+\t git add sub &&\n+\t git commit -m a &&\n+\t git checkout -b top-b master &&\n+\t (cd sub && git checkout sub-b) &&\n+\t git add sub &&\n+\t git commit -m b &&\n+\t git checkout -b top-c master &&\n+\t (cd sub && git checkout sub-c) &&\n+\t git add sub &&\n+\t git commit -m c &&\n+\t git checkout -b top-bc top-b &&\n+\t git merge -s ours --no-commit top-c &&\n+\t (cd sub && git checkout sub-bc) &&\n+\t git add sub &&\n+\t git commit -m bc &&\n+\t git checkout -b top-cb top-c &&\n+\t git merge -s ours --no-commit top-b &&\n+\t (cd sub && git checkout sub-cb) &&\n+\t git add sub &&\n+\t git commit -m cb)\n+'\n+\n+# merge should leave submodule unmerged in index\n+test_expect_failure 'recursive merge with submodule' '\n+\t(cd merge-recursive &&\n+\t test_must_fail git merge top-bc &&\n+\t echo \"160000 $(git rev-parse top-cb:sub) 2\tsub\" > expect2 &&\n+\t echo \"160000 $(git rev-parse top-bc:sub) 3\tsub\" > expect3 &&\n+\t git ls-files -u > actual &&\n+\t grep \"$(cat expect2)\" actual > /dev/null &&\n+\t grep \"$(cat expect3)\" actual > /dev/null)\n+'\n+\n test_done\n-- \n1.7.4.4\n"},{"id":"174239","messageId":"438ea0b254ccafb3fc9f3431f8f86007cc03132b.1314290439.git.brad.king@kitware.com","threadId":"28206","inReplyTo":"20110824213205.GI45292@book.hvoigt.net","subject":"[PATCH] rev-list: Demonstrate breakage with --ancestry-path --all","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2011-08-25T16:49:13Z","receivedAt":"2011-08-25T16:49:13Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"The option added by commit ebdc94f3 (revision: --ancestry-path,\n2010-04-20) does not work properly in combination with --all, at least\nin the case of a criss-cross merge:\n\n    b---bc\n   / \\ /\n  a   X\n   \\ / \\\n    c---cb\n\nThere are no descendants of 'cb' in the history.  The command\n\n  git rev-list --ancestry-path cb..bc\n\ncorrectly reports no commits.  However, the command\n\n  git rev-list --ancestry-path --all ^cb\n\nreports 'bc'.  Add a test case to t6019-rev-list-ancestry-path\ndemonstrating this breakage.\n\nSigned-off-by: Brad King <brad.king@kitware.com>\n---\n\nI tried to fix the submodule merge search during a recursive merge by\nonly doing it when o->call_depth is zero.  While testing the fix I\nnoticed that the merge search was reporting an incorrect commit as a\nsuggested submodule merge resolution.  Internally the merge search uses\n\"rev-list --merges --ancestry-path --all ^a\" to find all merges that\ncontain 'a'.  The rev-list incorrectly reports the other side of the\ncriss-cross history.  I narrowed the problem down to this test case.\n\nBrad\n\n t/t6019-rev-list-ancestry-path.sh |   35 +++++++++++++++++++++++++++++++++++\n 1 files changed, 35 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex 7641029..aa4674f 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -70,4 +70,39 @@ test_expect_success 'rev-list --ancestry-patch D..M -- M.t' '\n \ttest_cmp expect actual\n '\n \n+#   b---bc\n+#  / \\ /\n+# a   X\n+#  \\ / \\\n+#   c---cb\n+test_expect_success 'setup criss-cross' '\n+\tmkdir criss-cross &&\n+\t(cd criss-cross &&\n+\t git init &&\n+\t test_commit A &&\n+\t git checkout -b b master &&\n+\t test_commit B &&\n+\t git checkout -b c master &&\n+\t test_commit C &&\n+\t git checkout -b bc b -- &&\n+\t git merge c &&\n+\t git checkout -b cb c -- &&\n+\t git merge b &&\n+\t git checkout master)\n+'\n+\n+# no commits in bc descend from cb\n+test_expect_success 'criss-cross: rev-list --ancestry-path cb..bc' '\n+\t(cd criss-cross &&\n+\t git rev-list --ancestry-path cb..bc > actual &&\n+\t test -z \"$(cat actual)\")\n+'\n+\n+# no commits in repository descend from cb\n+test_expect_failure 'criss-cross: rev-list --ancestry-path --all ^cb' '\n+\t(cd criss-cross &&\n+\t git rev-list --ancestry-path --all ^cb > actual &&\n+\t test -z \"$(cat actual)\")\n+'\n+\n test_done\n-- \n1.7.4.4\n"},{"id":"174268","messageId":"20110825211144.GA67523@book.hvoigt.net","threadId":"28206","inReplyTo":"7v39gqo2fn.fsf@alter.siamese.dyndns.org","subject":"[PATCH] allow multiple calls to submodule merge search for the same path","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-25T21:11:44Z","receivedAt":"2011-08-25T21:11:44Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"When multiple merge-bases are found for two commits to be merged the\nmerge machinery will ask twice for a merge resolution. Currently its not\npossible to use the revision-walking api for walking the same commits\nmultiple times. Thats why we now run the revision walking in a seperate\ngit process.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\nHi,\n\nOn Wed, Aug 24, 2011 at 03:43:56PM -0700, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n> >> I have been suspecting that most of this should be done in a separate\n> >> helper program that is run via run_command() interface, without\n> >> contaminating the object pool the main merge process has with data from\n> >> the submodule object store to begin with (i.e. add_submodule_odb() and\n> >> everything below should go). Wouldn't it be a lot cleaner solution?\n> >\n> > Hmm, I would like to keep it in process. Since there are platforms where\n> > spawning new processes is very slow.\n> \n> Adding submodule's odb into the main process _will_ also have performance\n> penalties because it will make it more expensive to look up objects that\n> belong to the superproject when the superproject wants its own look up.\n> \n> In case you haven't realized yet, walking revision graph multiple times\n> while making sure that you do not affect other revision traversals in\n> effect is hard to arrange right. But more importantly, correctness counts\n> more than performing quickly and giving a bogus result with premature\n> optimization that makes it harder to implement things correctly (and\n> harder to verify the change is correct).\n\nYes of course correctness has priority one. And so here is a patch\nimplementing the rev-iteration using a seperate process. Once the\nrevision walking api is able to walk multiple times we get back to it\nagain.\n\nI noticed that the suggestion for the merge resolution presented is\nwrong which Brad also seems to have found as presented in his patch in\nmessage:\n\n<438ea0b254ccafb3fc9f3431f8f86007cc03132b.1314290439.git.brad.king@kitware.com>\n\nBut this breakage depends on the wrong output from rev-list with the\n--ancestry-path option. This patch should be correct on its own.\n\n submodule.c                |   31 ++++++++++++++++++++++---------\n t/t7405-submodule-merge.sh |    2 +-\n 2 files changed, 23 insertions(+), 10 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 1ba9646..9820df7 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -581,29 +581,42 @@ static int find_first_merges(struct object_array *result, const char *path,\n \tchar merged_revision[42];\n \tconst char *rev_args[] = { \"rev-list\", \"--merges\", \"--ancestry-path\",\n \t\t\t\t   \"--all\", merged_revision, NULL };\n-\tstruct rev_info revs;\n-\tstruct setup_revision_opt rev_opts;\n+\tstruct child_process cp;\n+\tstruct strbuf one_rev = STRBUF_INIT;\n \n \tmemset(&merges, 0, sizeof(merges));\n \tmemset(result, 0, sizeof(struct object_array));\n-\tmemset(&rev_opts, 0, sizeof(rev_opts));\n+\tmemset(&cp, 0, sizeof(cp));\n \n \t/* get all revisions that merge commit a */\n \tsnprintf(merged_revision, sizeof(merged_revision), \"^%s\",\n \t\t\tsha1_to_hex(a->object.sha1));\n-\tinit_revisions(&revs, NULL);\n-\trev_opts.submodule = path;\n-\tsetup_revisions(sizeof(rev_args)/sizeof(char *)-1, rev_args, &revs, &rev_opts);\n+\n+\tcp.argv = rev_args;\n+\tcp.env = local_repo_env;\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tcp.out = -1;\n+\tcp.dir = path;\n+\tif (start_command(&cp))\n+\t\tdie(\"Could not run 'git rev-list --merges --ancestry-path --all %s' \"\n+\t\t\t\t\"command in submodule %s\", merged_revision, path);\n+\tFILE *out = fdopen(cp.out, \"r\");\n+\tif (!out)\n+\t\tdie(\"Could not open pipe of rev-list command.\");\n \n \t/* save all revisions from the above list that contain b */\n-\tif (prepare_revision_walk(&revs))\n-\t\tdie(\"revision walk setup failed\");\n-\twhile ((commit = get_revision(&revs)) != NULL) {\n+\twhile (strbuf_getline(&one_rev, out, '\\n') != EOF) {\n+\t\tcommit = lookup_commit_reference_by_name(one_rev.buf);\n \t\tstruct object *o = &(commit->object);\n \t\tif (in_merge_bases(b, &commit, 1))\n \t\t\tadd_object_array(o, NULL, &merges);\n \t}\n \n+\tfclose(out);\n+\tfinish_command(&cp);\n+\tstrbuf_release(&one_rev);\n+\n \t/* Now we've got all merges that contain a and b. Prune all\n \t * merges that contain another found merge and save them in\n \t * result.\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 8f6f2d6..603fb72 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '\n '\n \n # merge should leave submodule unmerged in index\n-test_expect_failure 'recursive merge with submodule' '\n+test_expect_success 'recursive merge with submodule' '\n \t(cd merge-recursive &&\n \t test_must_fail git merge top-bc &&\n \t echo \"160000 $(git rev-parse top-cb:sub) 2\tsub\" > expect2 &&\n-- \n1.7.6.551.g0f51b.dirty\n"},{"id":"174284","messageId":"7v4o15hyxx.fsf@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"438ea0b254ccafb3fc9f3431f8f86007cc03132b.1314290439.git.brad.king@kitware.com","subject":"Re: [PATCH] rev-list: Demonstrate breakage with --ancestry-path --all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-25T23:08:10Z","receivedAt":"2011-08-25T23:08:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brad King <brad.king@kitware.com> writes:\n\n> The option added by commit ebdc94f3 (revision: --ancestry-path,\n> 2010-04-20) does not work properly in combination with --all, at least\n> in the case of a criss-cross merge:\n>\n>     b---bc\n>    / \\ /\n>   a   X\n>    \\ / \\\n>     c---cb\n\nHmm, what should --ancestry-path do given more than one positive commit to\nbegin with, let alone --all? I do not think the request itself does not\nmake sense.\n"},{"id":"174285","messageId":"7vwre1gjq1.fsf@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"20110825211144.GA67523@book.hvoigt.net","subject":"Re: [PATCH] allow multiple calls to submodule merge search for the same path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-25T23:22:14Z","receivedAt":"2011-08-25T23:22:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\n> index 8f6f2d6..603fb72 100755\n> --- a/t/t7405-submodule-merge.sh\n> +++ b/t/t7405-submodule-merge.sh\n> @@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '\n>  '\n>  \n>  # merge should leave submodule unmerged in index\n> -test_expect_failure 'recursive merge with submodule' '\n> +test_expect_success 'recursive merge with submodule' '\n>  \t(cd merge-recursive &&\n>  \t test_must_fail git merge top-bc &&\n>  \t echo \"160000 $(git rev-parse top-cb:sub) 2\tsub\" > expect2 &&\n\nWhat is this patch based on?\n"},{"id":"174286","messageId":"7vsjopgjfs.fsf@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"7vwre1gjq1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] allow multiple calls to submodule merge search for the same path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-25T23:28:23Z","receivedAt":"2011-08-25T23:28:23Z","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> Heiko Voigt <hvoigt@hvoigt.net> writes:\n>\n>> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\n>> index 8f6f2d6..603fb72 100755\n>> --- a/t/t7405-submodule-merge.sh\n>> +++ b/t/t7405-submodule-merge.sh\n>> @@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '\n>>  '\n>>  \n>>  # merge should leave submodule unmerged in index\n>> -test_expect_failure 'recursive merge with submodule' '\n>> +test_expect_success 'recursive merge with submodule' '\n>>  \t(cd merge-recursive &&\n>>  \t test_must_fail git merge top-bc &&\n>>  \t echo \"160000 $(git rev-parse top-cb:sub) 2\tsub\" > expect2 &&\n>\n> What is this patch based on?\n\nAh, nevermind. I figured it out. Thanks.\n"},{"id":"174287","messageId":"7vobzdgixd.fsf@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"7vsjopgjfs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] allow multiple calls to submodule merge search for the same path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-25T23:39:26Z","receivedAt":"2011-08-25T23:39:26Z","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> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Heiko Voigt <hvoigt@hvoigt.net> writes:\n>>\n>>> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\n>>> index 8f6f2d6..603fb72 100755\n>>> --- a/t/t7405-submodule-merge.sh\n>>> +++ b/t/t7405-submodule-merge.sh\n>>> @@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '\n>>>  '\n>>>  \n>>>  # merge should leave submodule unmerged in index\n>>> -test_expect_failure 'recursive merge with submodule' '\n>>> +test_expect_success 'recursive merge with submodule' '\n>>>  \t(cd merge-recursive &&\n>>>  \t test_must_fail git merge top-bc &&\n>>>  \t echo \"160000 $(git rev-parse top-cb:sub) 2\tsub\" > expect2 &&\n>>\n>> What is this patch based on?\n>\n> Ah, nevermind. I figured it out. Thanks.\n\nJust FYI; squashed this on top to fix compilation breakage.\nThanks.\n\n submodule.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 21a57d2..752cd8a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -738,6 +738,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n \tstruct object_array merges;\n \tstruct commit *commit;\n \tint contains_another;\n+\tFILE *out;\n \n \tchar merged_revision[42];\n \tconst char *rev_args[] = { \"rev-list\", \"--merges\", \"--ancestry-path\",\n@@ -762,14 +763,15 @@ static int find_first_merges(struct object_array *result, const char *path,\n \tif (start_command(&cp))\n \t\tdie(\"Could not run 'git rev-list --merges --ancestry-path --all %s' \"\n \t\t\t\t\"command in submodule %s\", merged_revision, path);\n-\tFILE *out = fdopen(cp.out, \"r\");\n+\tout = fdopen(cp.out, \"r\");\n \tif (!out)\n \t\tdie(\"Could not open pipe of rev-list command.\");\n \n \t/* save all revisions from the above list that contain b */\n \twhile (strbuf_getline(&one_rev, out, '\\n') != EOF) {\n+\t\tstruct object *o;\n \t\tcommit = lookup_commit_reference_by_name(one_rev.buf);\n-\t\tstruct object *o = &(commit->object);\n+\t\to = &(commit->object);\n \t\tif (in_merge_bases(b, &commit, 1))\n \t\t\tadd_object_array(o, NULL, &merges);\n \t}\n"},{"id":"174289","messageId":"7vhb55gig1.fsf@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"7v4o15hyxx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rev-list: Demonstrate breakage with --ancestry-path --all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-25T23:49:50Z","receivedAt":"2011-08-25T23:49:50Z","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> Brad King <brad.king@kitware.com> writes:\n>\n>> The option added by commit ebdc94f3 (revision: --ancestry-path,\n>> 2010-04-20) does not work properly in combination with --all, at least\n>> in the case of a criss-cross merge:\n>>\n>>     b---bc\n>>    / \\ /\n>>   a   X\n>>    \\ / \\\n>>     c---cb\n>\n> Hmm, what should --ancestry-path do given more than one positive commit to\n> begin with, let alone --all?\n\nI actually think that this does not have much to do with \"criss-cross\"-ness. \nInstead of computing those that can be reached from cb, we are computing\nthose that can be reached from either b, c or cb.\n\nThis needs fixing, but it takes a bit more than a quick hack. Stay tuned\n;-)\n\nThanks.\n"},{"id":"174290","messageId":"7vbovdgf73.fsf_-_@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"438ea0b254ccafb3fc9f3431f8f86007cc03132b.1314290439.git.brad.king@kitware.com","subject":"[PATCH 1/2] revision: keep track of the end-user input from the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-26T01:00:00Z","receivedAt":"2011-08-26T01:00:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Given a complex set of revision specifiers on the command line, it is too\nlate to look at the flags of the objects in the initial traversal list at\nthe beginning of limit_list() in order to determine what the objects the\nend-user explicitly listed on the command line were. The process to move\nobjects from the pending array to the traversal list may have marked\nobjects that are not mentioned as UNINTERESTING, when handle_commit()\nmarked the parents of UNINTERESTING commits mentioned on the command line\nby calling mark_parents_uninteresting().\n\nThis made \"rev-list --ancestry-path ^A ...\" to mistakenly list commits\nthat are descendants of A's parents but that are not descendants of A\nitself, as ^A from the command line causes A and its parents marked as\nUNINTERESTING before coming to limit_list(), and we try to enumerate the\ncommits that are descendants of these commits that are UNINTERESTING\nbefore we start walking the history.\n\nIt actually is too late even if we inspected the pending object array\nbefore calling prepare_revision_walk(), as some of the same objects might\nhave been mentioned twice, once as positive and another time as negative.\nThe \"rev-list --some-option A --not --all\" command may want to notice,\neven if the resulting set is empty, that the user showed some interest in\n\"A\" and do something special about it.\n\nPrepare a separate array to keep track of what syntactic element was used\nto cause each object to appear in the pending array from the command line,\nand populate it as setup_revisions() parses the command line.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * Should apply cleanly on top of your test case.\n\n revision.c |   37 +++++++++++++++++++++++++++++++++----\n revision.h |   20 ++++++++++++++++++++\n 2 files changed, 53 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 96d7fa7..3e87c86 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -797,6 +797,23 @@ static int limit_list(struct rev_info *revs)\n \treturn 0;\n }\n \n+static void add_rev_cmdline(struct rev_info *revs,\n+\t\t\t    struct object *item,\n+\t\t\t    const char *name,\n+\t\t\t    int whence,\n+\t\t\t    unsigned flags)\n+{\n+\tstruct rev_cmdline_info *info = &revs->cmdline;\n+\tint nr = info->nr;\n+\n+\tALLOC_GROW(info->rev, nr + 1, info->alloc);\n+\tinfo->rev[nr].item = item;\n+\tinfo->rev[nr].name = name;\n+\tinfo->rev[nr].whence = whence;\n+\tinfo->rev[nr].flags = flags;\n+\tinfo->nr++;\n+}\n+\n struct all_refs_cb {\n \tint all_flags;\n \tint warned_bad_reflog;\n@@ -809,6 +826,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1, int flag,\n \tstruct all_refs_cb *cb = cb_data;\n \tstruct object *object = get_reference(cb->all_revs, path, sha1,\n \t\t\t\t\t      cb->all_flags);\n+\tadd_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);\n \tadd_pending_object(cb->all_revs, object, path);\n \treturn 0;\n }\n@@ -835,6 +853,7 @@ static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)\n \t\tstruct object *o = parse_object(sha1);\n \t\tif (o) {\n \t\t\to->flags |= cb->all_flags;\n+\t\t\t/* ??? CMDLINEFLAGS ??? */\n \t\t\tadd_pending_object(cb->all_revs, o, \"\");\n \t\t}\n \t\telse if (!cb->warned_bad_reflog) {\n@@ -871,12 +890,13 @@ static void handle_reflog(struct rev_info *revs, unsigned flags)\n \tfor_each_reflog(handle_one_reflog, &cb);\n }\n \n-static int add_parents_only(struct rev_info *revs, const char *arg, int flags)\n+static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)\n {\n \tunsigned char sha1[20];\n \tstruct object *it;\n \tstruct commit *commit;\n \tstruct commit_list *parents;\n+\tconst char *arg = arg_;\n \n \tif (*arg == '^') {\n \t\tflags ^= UNINTERESTING;\n@@ -898,6 +918,7 @@ static int add_parents_only(struct rev_info *revs, const char *arg, int flags)\n \tfor (parents = commit->parents; parents; parents = parents->next) {\n \t\tit = &parents->item->object;\n \t\tit->flags |= flags;\n+\t\tadd_rev_cmdline(revs, it, arg_, REV_CMD_PARENTS_ONLY, flags);\n \t\tadd_pending_object(revs, it, arg);\n \t}\n \treturn 1;\n@@ -987,7 +1008,7 @@ static void prepare_show_merge(struct rev_info *revs)\n \trevs->limited = 1;\n }\n \n-int handle_revision_arg(const char *arg, struct rev_info *revs,\n+int handle_revision_arg(const char *arg_, struct rev_info *revs,\n \t\t\tint flags,\n \t\t\tint cant_be_filename)\n {\n@@ -996,6 +1017,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \tstruct object *object;\n \tunsigned char sha1[20];\n \tint local_flags;\n+\tconst char *arg = arg_;\n \n \tdotdot = strstr(arg, \"..\");\n \tif (dotdot) {\n@@ -1004,6 +1026,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\tconst char *this = arg;\n \t\tint symmetric = *next == '.';\n \t\tunsigned int flags_exclude = flags ^ UNINTERESTING;\n+\t\tunsigned int a_flags;\n \n \t\t*dotdot = 0;\n \t\tnext += symmetric;\n@@ -1036,10 +1059,15 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\t\t\tadd_pending_commit_list(revs, exclude,\n \t\t\t\t\t\t\tflags_exclude);\n \t\t\t\tfree_commit_list(exclude);\n-\t\t\t\ta->object.flags |= flags | SYMMETRIC_LEFT;\n+\t\t\t\ta_flags = flags | SYMMETRIC_LEFT;\n \t\t\t} else\n-\t\t\t\ta->object.flags |= flags_exclude;\n+\t\t\t\ta_flags = flags_exclude;\n+\t\t\ta->object.flags |= a_flags;\n \t\t\tb->object.flags |= flags;\n+\t\t\tadd_rev_cmdline(revs, &a->object, this,\n+\t\t\t\t\tREV_CMD_LEFT, a_flags);\n+\t\t\tadd_rev_cmdline(revs, &b->object, next,\n+\t\t\t\t\tREV_CMD_RIGHT, flags);\n \t\t\tadd_pending_object(revs, &a->object, this);\n \t\t\tadd_pending_object(revs, &b->object, next);\n \t\t\treturn 0;\n@@ -1070,6 +1098,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \tif (!cant_be_filename)\n \t\tverify_non_filename(revs->prefix, arg);\n \tobject = get_reference(revs, arg, sha1, flags ^ local_flags);\n+\tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n \tadd_pending_object_with_mode(revs, object, arg, mode);\n \treturn 0;\n }\ndiff --git a/revision.h b/revision.h\nindex 855464f..031cc7c 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -23,6 +23,23 @@ struct rev_info;\n struct log_info;\n struct string_list;\n \n+struct rev_cmdline_info {\n+\tunsigned int nr;\n+\tunsigned int alloc;\n+\tstruct rev_cmdline_entry {\n+\t\tstruct object *item;\n+\t\tconst char *name;\n+\t\tenum {\n+\t\t\tREV_CMD_REF,\n+\t\t\tREV_CMD_PARENTS_ONLY,\n+\t\t\tREV_CMD_LEFT,\n+\t\t\tREV_CMD_RIGHT,\n+\t\t\tREV_CMD_REV\n+\t\t} whence;\n+\t\tunsigned flags;\n+\t} *rev;\n+};\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -31,6 +48,9 @@ struct rev_info {\n \t/* Parents of shown commits */\n \tstruct object_array boundary_commits;\n \n+\t/* The end-points specified by the end user */\n+\tstruct rev_cmdline_info cmdline;\n+\n \t/* Basic information */\n \tconst char *prefix;\n \tconst char *def;\n-- \n1.7.6.1.385.gb7fcd0\n"},{"id":"174291","messageId":"7v7h61gf5i.fsf_-_@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"438ea0b254ccafb3fc9f3431f8f86007cc03132b.1314290439.git.brad.king@kitware.com","subject":"[PATCH 2/2] revision: do not include sibling history in --ancestry-path output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-26T01:00:57Z","receivedAt":"2011-08-26T01:00:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If the commit specified as the bottom of the commit range has a direct\nparent that has another child commit that contributed to the resulting\nhistory, \"rev-list --ancestry-path\" was confused and listed that side\nhistory as well.\n\n             D---E\n            /     \\\n        ---X---A---B---C\n\nIn this history, \"rev-list --ancestry-path A..C\" should list among what\nthe corresponding command without --ancestry-path option would produce,\nnamely, D, E, B and C, but limiting the result to those that are\ndescendant of A (i.e. B and C). Due to the command line parser subtlety\ncorrected by the previous commit, it also listed those that are descendant\nof X as well.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * And this should fix the breakage you demonstrated.\n\n revision.c |   16 ++++++++++------\n 1 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 3e87c86..48a2db4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -724,12 +724,16 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n  * to filter the result of \"A..B\" further to the ones that can actually\n  * reach A.\n  */\n-static struct commit_list *collect_bottom_commits(struct commit_list *list)\n+static struct commit_list *collect_bottom_commits(struct rev_info *revs)\n {\n-\tstruct commit_list *elem, *bottom = NULL;\n-\tfor (elem = list; elem; elem = elem->next)\n-\t\tif (elem->item->object.flags & UNINTERESTING)\n-\t\t\tcommit_list_insert(elem->item, &bottom);\n+\tstruct commit_list *bottom = NULL;\n+\tint i;\n+\tfor (i = 0; i < revs->cmdline.nr; i++) {\n+\t\tstruct rev_cmdline_entry *elem = &revs->cmdline.rev[i];\n+\t\tif ((elem->flags & UNINTERESTING) &&\n+\t\t    elem->item->type == OBJ_COMMIT)\n+\t\t\tcommit_list_insert((struct commit *)elem->item, &bottom);\n+\t}\n \treturn bottom;\n }\n \n@@ -743,7 +747,7 @@ static int limit_list(struct rev_info *revs)\n \tstruct commit_list *bottom = NULL;\n \n \tif (revs->ancestry_path) {\n-\t\tbottom = collect_bottom_commits(list);\n+\t\tbottom = collect_bottom_commits(revs);\n \t\tif (!bottom)\n \t\t\tdie(\"--ancestry-path given but there are no bottom commits\");\n \t}\n-- \n1.7.6.1.385.gb7fcd0\n"},{"id":"174292","messageId":"CAGdFq_he-skXpEy903RSY2ravDXo1eVCN19RzJeHXQCT__HTmA@mail.gmail.com","threadId":"28206","inReplyTo":"7vbovdgf73.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] revision: keep track of the end-user input from the command line","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-26T01:08:31Z","receivedAt":"2011-08-26T01:08:31Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Thu, Aug 25, 2011 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:\n> Prepare a separate array to keep track of what syntactic element was used\n> to cause each object to appear in the pending array from the command line,\n> and populate it as setup_revisions() parses the command line.\n\nThank you! I was really dreading looking into this myself, so I'm very\nglad that you could find the time to look into it yourself.\n\n> @@ -835,6 +853,7 @@ static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)\n>                struct object *o = parse_object(sha1);\n>                if (o) {\n>                        o->flags |= cb->all_flags;\n> +                       /* ??? CMDLINEFLAGS ??? */\n>                        add_pending_object(cb->all_revs, o, \"\");\n>                }\n>                else if (!cb->warned_bad_reflog) {\n\nWhat is happening here?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"174294","messageId":"7vy5yggbfv.fsf@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"CAGdFq_he-skXpEy903RSY2ravDXo1eVCN19RzJeHXQCT__HTmA@mail.gmail.com","subject":"Re: [PATCH 1/2] revision: keep track of the end-user input from the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-26T02:21:08Z","receivedAt":"2011-08-26T02:21:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier <srabbelier@gmail.com> writes:\n\n>> Prepare a separate array to keep track of what syntactic element was used\n>> to cause each object to appear in the pending array from the command line,\n>> and populate it as setup_revisions() parses the command line.\n>\n> Thank you! I was really dreading looking into this myself, so I'm very\n> glad that you could find the time to look into it yourself.\n\nI debated long and hard if I should instead fatten object array entry and\nshove this information there without adding a new structure, which would\nhave resulted in something very similar to what you had, so you should\ntake some credit for the code, and also credit for a large part of the\nmotivation (the second paragraph in the log is entirely your use case).\n\nWe might end up unifying this command line information array and the\npending object array after reviewing what other future callers would want\nfrom this new information, but at least by doing it this way I can rest\nassured that no existing code that is unaware of the mechanism would get\nany unintended side effects in the earlier rounds.\n\n>> @@ -835,6 +853,7 @@ static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)\n>>                struct object *o = parse_object(sha1);\n>>                if (o) {\n>>                        o->flags |= cb->all_flags;\n>> +                       /* ??? CMDLINEFLAGS ??? */\n>>                        add_pending_object(cb->all_revs, o, \"\");\n>>                }\n>>                else if (!cb->warned_bad_reflog) {\n>\n> What is happening here?\n\nWe could have add_rev_cmdline() call there if we really wanted to, but I\ndecided not to do so for two reasons:\n\n(1) with \"rev-list -g HEAD\", the user is not explicitly mentioning all\n    objects in the reflog of the HEAD---it might still make sense to mark\n    HEAD as explicitly mentioned as positive, but the code to do so will\n    not sit here anyway; and more importantly,\n\n(2) I was appalled by how broken the design and the implementation of\n    walking the reflog was (it shoves _all_ the objects in the entire\n    history recorded in the reflog to the pending queue before letting the\n    caller do an iota of work). It is against the general design of Git,\n    and the design of the revision walk machinery in particular that tries\n    very hard to be incremental. It goes against a good software design\n    taste.\n\nFor the latter reason, I think in the longer term we should correct the\nimplementation to walk the reflog to keep an iterator (a structure that\nholds an open file descriptor to a reflog file, and perhaps a little bit\nof buffer) in struct rev_info, and teach get_revisions() to lazily read\nfrom there, reading the file backward. Once that happens, this callback\nfrom for_each_reflog_ent() will go away, so I didn't bother.\n"},{"id":"174313","messageId":"4E5796D8.3060407@kitware.com","threadId":"28206","inReplyTo":"7v7h61gf5i.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] revision: do not include sibling history in --ancestry-path output","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2011-08-26T12:51:36Z","receivedAt":"2011-08-26T12:51:36Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"On 8/25/2011 9:00 PM, Junio C Hamano wrote:\n> If the commit specified as the bottom of the commit range has a direct\n> parent that has another child commit that contributed to the resulting\n> history, \"rev-list --ancestry-path\" was confused and listed that side\n> history as well.\n>\n>               D---E\n>              /     \\\n>          ---X---A---B---C\n>\n> In this history, \"rev-list --ancestry-path A..C\" should list among what\n> the corresponding command without --ancestry-path option would produce,\n> namely, D, E, B and C, but limiting the result to those that are\n> descendant of A (i.e. B and C). Due to the command line parser subtlety\n> corrected by the previous commit, it also listed those that are descendant\n> of X as well.\n>\n> Signed-off-by: Junio C Hamano<gitster@pobox.com>\n> ---\n>   * And this should fix the breakage you demonstrated.\n\nYes it does, thanks.  It also makes the submodule search during recursive\nmerge work as I expect after the fix I tried (only run the search if\no->call_depth == 0).  I'll submit that patch separately when I find time.\n\nTested-by: Brad King <brad.king@kitware.com>\n\nThanks,\n-Brad\n"},{"id":"174321","messageId":"03300e44101092641810b34086738a151e01d8ee.1314368109.git.brad.king@kitware.com","threadId":"28206","inReplyTo":"074f22629c034dba738b7241c78229db7f9159ec.1314275112.git.brad.king@kitware.com","subject":"[PATCH/RFC] submodule: Search for merges only at end of recursive merge","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2011-08-26T14:18:28Z","receivedAt":"2011-08-26T14:18:28Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"The submodule merge search is not useful during virtual merges because\nthe results cannot be used automatically.  Furthermore any suggesions\nmade by the search may apply to commits different than HEAD:sub and\nMERGE_HEAD:sub, thus confusing the user.  Skip searching for submodule\nmerges during a virtual merge such as that between B and C while merging\nthe heads of:\n\n    B---BC\n   / \\ /\n  A   X\n   \\ / \\\n    C---CB\n\nRun the search only when the recursion level is zero (!o->call_depth).\nThis fixes known breakage tested in t7405-submodule-merge.\n\nSigned-off-by: Brad King <brad.king@kitware.com>\n---\n\nThis addresses the submodule search problem reported in the message\nto which this replies.  If you think it is correct I can resubmit\nboth the test and this patch as a single series.\n\n merge-recursive.c          |    3 ++-\n submodule.c                |   11 ++++++++---\n submodule.h                |    4 +++-\n t/t7405-submodule-merge.sh |    2 +-\n 4 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 0cc1e6f..2118ee9 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -830,7 +830,8 @@ static struct merge_file_info merge_file(struct merge_options *o,\n \t\t\tfree(result_buf.ptr);\n \t\t\tresult.clean = (merge_status == 0);\n \t\t} else if (S_ISGITLINK(a->mode)) {\n-\t\t\tresult.clean = merge_submodule(result.sha, one->path, one->sha1,\n+\t\t\tresult.clean = merge_submodule(o, result.sha,\n+\t\t\t\t\t\t       one->path, one->sha1,\n \t\t\t\t\t\t       a->sha1, b->sha1);\n \t\t} else if (S_ISLNK(a->mode)) {\n \t\t\thashcpy(result.sha, a->sha1);\ndiff --git a/submodule.c b/submodule.c\nindex 1ba9646..c7fdafa 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -8,6 +8,7 @@\n #include \"diffcore.h\"\n #include \"refs.h\"\n #include \"string-list.h\"\n+#include \"merge-recursive.h\"\n \n static struct string_list config_name_for_path;\n static struct string_list config_fetch_recurse_submodules_for_name;\n@@ -642,9 +643,9 @@ static void print_commit(struct commit *commit)\n #define MERGE_WARNING(path, msg) \\\n \twarning(\"Failed to merge submodule %s (%s)\", path, msg);\n \n-int merge_submodule(unsigned char result[20], const char *path,\n-\t\t    const unsigned char base[20], const unsigned char a[20],\n-\t\t    const unsigned char b[20])\n+int merge_submodule(struct merge_options *o, unsigned char result[20],\n+\t\t    const char *path, const unsigned char base[20],\n+\t\t    const unsigned char a[20], const unsigned char b[20])\n {\n \tstruct commit *commit_base, *commit_a, *commit_b;\n \tint parent_count;\n@@ -699,6 +700,10 @@ int merge_submodule(unsigned char result[20], const char *path,\n \t * user needs to confirm the resolution.\n \t */\n \n+\t/* This case makes sense only at the main depth 0 merge.  */\n+\tif (o->call_depth)\n+\t\treturn 0;\n+\n \t/* find commit which merges them */\n \tparent_count = find_first_merges(&merges, path, commit_a, commit_b);\n \tswitch (parent_count) {\ndiff --git a/submodule.h b/submodule.h\nindex 5350b0d..f22172c 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -2,6 +2,7 @@\n #define SUBMODULE_H\n \n struct diff_options;\n+struct merge_options;\n \n enum {\n \tRECURSE_SUBMODULES_ON_DEMAND = -1,\n@@ -27,7 +28,8 @@ int fetch_populated_submodules(int num_options, const char **options,\n \t\t\t       const char *prefix, int command_line_option,\n \t\t\t       int quiet);\n unsigned is_submodule_modified(const char *path, int ignore_untracked);\n-int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],\n+int merge_submodule(struct merge_options *o, unsigned char result[20],\n+\t\t    const char *path, const unsigned char base[20],\n \t\t    const unsigned char a[20], const unsigned char b[20]);\n \n #endif\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 14da2e3..0d5b42a 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '\n '\n \n # merge should leave submodule unmerged in index\n-test_expect_failure 'recursive merge with submodule' '\n+test_expect_success 'recursive merge with submodule' '\n \t(cd merge-recursive &&\n \t test_must_fail git merge top-bc &&\n \t echo \"160000 $(git rev-parse top-cb:sub) 2\tsub\" > expect2 &&\n-- \n1.7.4.4\n"},{"id":"174340","messageId":"7vr548c7un.fsf@alter.siamese.dyndns.org","threadId":"28206","inReplyTo":"03300e44101092641810b34086738a151e01d8ee.1314368109.git.brad.king@kitware.com","subject":"Re: [PATCH/RFC] submodule: Search for merges only at end of recursive merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-26T19:04:32Z","receivedAt":"2011-08-26T19:04:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is a knee-jerk reaction without thinking things thoroughly through,\nbut wouldn't it make more sense to do this by conditionally calling\nmerge_submodule() when !o->call_depth, leaving the callee oblivious to\nwhat is in the \"merge_options\" structure? That way, you do not have to\ntouch submodule.c at all, I would think.\n\nAfter all, merge_submodule() should be usable in a future merge strategy\nthat is different from recursive and has no notion of call_depth.\n"},{"id":"174341","messageId":"8e4e80f75d281c8bdd07b0a29eee9ad407661102.1314386859.git.brad.king@kitware.com","threadId":"28206","inReplyTo":"7vr548c7un.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2/RFC] submodule: Search for merges only at end of recursive merge","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2011-08-26T19:30:56Z","receivedAt":"2011-08-26T19:30:56Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"The submodule merge search is not useful during virtual merges because\nthe results cannot be used automatically.  Furthermore any suggestions\nmade by the search may apply to commits different than HEAD:sub and\nMERGE_HEAD:sub, thus confusing the user.  Skip searching for submodule\nmerges during a virtual merge such as that between B and C while merging\nthe heads of:\n\n    B---BC\n   / \\ /\n  A   X\n   \\ / \\\n    C---CB\n\nRun the search only when the recursion level is zero (!o->call_depth).\nThis fixes known breakage tested in t7405-submodule-merge.\n\nSigned-off-by: Brad King <brad.king@kitware.com>\n---\nOn 8/26/2011 3:04 PM, Junio C Hamano wrote:\n> This is a knee-jerk reaction without thinking things thoroughly through,\n> but wouldn't it make more sense to do this by conditionally calling\n> merge_submodule() when !o->call_depth, leaving the callee oblivious to\n> what is in the \"merge_options\" structure? That way, you do not have to\n> touch submodule.c at all, I would think.\n\nI originally considered that but I think merge_submodule is still useful\nduring virtual merges in the fast forward case.  I haven't thought that\nthrough in detail though.\n\n> After all, merge_submodule() should be usable in a future merge strategy\n> that is different from recursive and has no notion of call_depth.\n\nThat's a worthwhile goal.  Perhaps instead we should pass in a parameter\nto tell merge_submodule whether or not to do the search after the fast\nforward case fails.\n\nBrad\n\n merge-recursive.c          |    6 ++++--\n submodule.c                |    6 +++++-\n submodule.h                |    2 +-\n t/t7405-submodule-merge.sh |    2 +-\n 4 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 0cc1e6f..390811e 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -830,8 +830,10 @@ static struct merge_file_info merge_file(struct merge_options *o,\n \t\t\tfree(result_buf.ptr);\n \t\t\tresult.clean = (merge_status == 0);\n \t\t} else if (S_ISGITLINK(a->mode)) {\n-\t\t\tresult.clean = merge_submodule(result.sha, one->path, one->sha1,\n-\t\t\t\t\t\t       a->sha1, b->sha1);\n+\t\t\tresult.clean = merge_submodule(result.sha,\n+\t\t\t\t\t\t       one->path, one->sha1,\n+\t\t\t\t\t\t       a->sha1, b->sha1,\n+\t\t\t\t\t\t       !o->call_depth);\n \t\t} else if (S_ISLNK(a->mode)) {\n \t\t\thashcpy(result.sha, a->sha1);\n \ndiff --git a/submodule.c b/submodule.c\nindex 1ba9646..bf4f693 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -644,7 +644,7 @@ static void print_commit(struct commit *commit)\n \n int merge_submodule(unsigned char result[20], const char *path,\n \t\t    const unsigned char base[20], const unsigned char a[20],\n-\t\t    const unsigned char b[20])\n+\t\t    const unsigned char b[20], int search)\n {\n \tstruct commit *commit_base, *commit_a, *commit_b;\n \tint parent_count;\n@@ -699,6 +699,10 @@ int merge_submodule(unsigned char result[20], const char *path,\n \t * user needs to confirm the resolution.\n \t */\n \n+\t/* Skip the search if makes no sense to the calling context.  */\n+\tif (!search)\n+\t\treturn 0;\n+\n \t/* find commit which merges them */\n \tparent_count = find_first_merges(&merges, path, commit_a, commit_b);\n \tswitch (parent_count) {\ndiff --git a/submodule.h b/submodule.h\nindex 5350b0d..d4344c8 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -28,6 +28,6 @@ int fetch_populated_submodules(int num_options, const char **options,\n \t\t\t       int quiet);\n unsigned is_submodule_modified(const char *path, int ignore_untracked);\n int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],\n-\t\t    const unsigned char a[20], const unsigned char b[20]);\n+\t\t    const unsigned char a[20], const unsigned char b[20], int search);\n \n #endif\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 14da2e3..0d5b42a 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '\n '\n \n # merge should leave submodule unmerged in index\n-test_expect_failure 'recursive merge with submodule' '\n+test_expect_success 'recursive merge with submodule' '\n \t(cd merge-recursive &&\n \t test_must_fail git merge top-bc &&\n \t echo \"160000 $(git rev-parse top-cb:sub) 2\tsub\" > expect2 &&\n-- \n1.7.4.4\n"}]}