threads / patch / 28206

patchsubmodule: Demonstrate known breakage during recursive merge

Subject: [PATCH] submodule: Demonstrate known breakage during recursive merge

## tl;dr

25 messages between Aug 24, 2011 and Aug 26, 2011. Diffs are folded; open one to read it.

replies: 24people: 4as markdown or json

Brad King· Aug 24, 2011, 13:59 UTC · lore

Since commit 68d03e4a (Implement automatic fast-forward merge for submodules, 2010-07-07) we try to suggest submodule commits that resolve a conflict. Consider a true recursive merge case

    b---bc
   / \ /
  o   X
   \ / \
    c---cb

in which the two heads themselves (bc,cb) had resolved a submodule conflict (i.e. reference different commits than their parents). The submodule merge search runs during the temporary merge of the two merge bases (b,c) and prints out a suggestion that is not meaningful to the user. Then during the main merge the submodule merge search runs again but dies with the message

  fatal: --ancestry-path given but there are no bottom commits

while trying to enumerate candidates. Demonstrate this known breakage with a new test in t7405-submodule-merge covering the case.

Signed-off-by: Brad King <brad.king@kitware.com>
---
Hi folks,

A co-worker encountered this problem last week in a real project. The merge dies and does not leave the submodule conflict in the index. We were able to work around it by moving the submodule out of the way and resolving the conflict by hand. Then I ran the merge again to debug the problem and narrowed it down to this case.

BTW, if one adds the line

  rm -rf sub && mkdir sub &&

just above the line

  test_must_fail git merge top-bc

then the test passes because no submodule merge search runs. Therefore this test can be switched to expect success when the problem is fixed.

Brad

 t/t7405-submodule-merge.sh |   51 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

Show changes to t/t7405-submodule-merge.sh +51 −0

diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index a8fb30b..8f6f2d6 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -228,4 +228,55 @@ test_expect_success 'merging with a modify/modify conflict between merge bases'
 	git merge d
 '
 
+# canonical criss-cross history in top and submodule
+test_expect_success 'setup for recursive merge with submodule' '
+	mkdir merge-recursive &&
+	(cd merge-recursive &&
+	 git init &&
+	 mkdir sub &&
+	 (cd sub &&
+	  git init &&
+	  test_commit a &&
+	  git checkout -b sub-b master &&
+	  test_commit b &&
+	  git checkout -b sub-c master &&
+	  test_commit c &&
+	  git checkout -b sub-bc sub-b &&
+	  git merge sub-c &&
+	  git checkout -b sub-cb sub-c &&
+	  git merge sub-b &&
+	  git checkout master) &&
+	 git add sub &&
+	 git commit -m a &&
+	 git checkout -b top-b master &&
+	 (cd sub && git checkout sub-b) &&
+	 git add sub &&
+	 git commit -m b &&
+	 git checkout -b top-c master &&
+	 (cd sub && git checkout sub-c) &&
+	 git add sub &&
+	 git commit -m c &&
+	 git checkout -b top-bc top-b &&
+	 git merge -s ours -n top-c &&
+	 (cd sub && git checkout sub-bc) &&
+	 git add sub &&
+	 git commit -m bc &&
+	 git checkout -b top-cb top-c &&
+	 git merge -s ours -n top-b &&
+	 (cd sub && git checkout sub-cb) &&
+	 git add sub &&
+	 git commit -m cb)
+'
+
+# merge should leave submodule unmerged in index
+test_expect_failure 'recursive merge with submodule' '
+	(cd merge-recursive &&
+	 test_must_fail git merge top-bc &&
+	 echo "160000 $(git rev-parse top-cb:sub) 2	sub" > expect2 &&
+	 echo "160000 $(git rev-parse top-bc:sub) 3	sub" > expect3 &&
+	 git ls-files -u > actual &&
+	 grep "$(cat expect2)" actual > /dev/null &&
+	 grep "$(cat expect3)" actual > /dev/null)
+'
+
 test_done

-- 
1.7.4.4

Heiko Voigt· Aug 24, 2011, 19:14 UTC · re: Brad King · lore

Re: [PATCH] submodule: Demonstrate known breakage during recursive merge

Hi,

thanks for finding this subtle bug!

On Wed, Aug 24, 2011 at 09:59:50AM -0400, Brad King wrote:

Show 9 quoted lines

> Since commit 68d03e4a (Implement automatic fast-forward merge for
> submodules, 2010-07-07) we try to suggest submodule commits that resolve
> a conflict.  Consider a true recursive merge case
> 
>     b---bc
>    / \ /
>   o   X
>    \ / \
>     c---cb

And here is a patch[1] that you can apply on top of yours which should fix this. An extra pair of merge machinery knowing eyes appreciated. Its a little bit workaroundish so if anymore has an idea how to fix this in nicer way, please tell me.

[1]--8<----
From: Heiko Voigt <hvoigt@hvoigt.net>
Subject: [PATCH] protect submodule merge search against multiple calls for
 the same path

When multiple merge-bases are found for two commits to be merged the merge machinery will ask twice for a merge resolution. Currently its not possible to use the revision-walking api for walking the same commits multiple times. Since the result will not change we can simply fail here if we are asked for a resolution of the same path again.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 submodule.c                |    9 +++++++++
 t/t7405-submodule-merge.sh |    2 +-
 2 files changed, 10 insertions(+), 1 deletions(-)

Show changes to 2 files +10 −1

submodule.c, t/t7405-submodule-merge.sh

diff --git a/submodule.c b/submodule.c
index 1ba9646..a4af08e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -646,6 +646,7 @@ int merge_submodule(unsigned char result[20], const char *path,
 		    const unsigned char base[20], const unsigned char a[20],
 		    const unsigned char b[20])
 {
+	static char last_path[PATH_MAX] = {'\0'};
 	struct commit *commit_base, *commit_a, *commit_b;
 	int parent_count;
 	struct object_array merges;
@@ -699,6 +700,13 @@ int merge_submodule(unsigned char result[20], const char *path,
 	 * user needs to confirm the resolution.
 	 */
 
+	/* in case of multiple merge-bases the merge algorithm will ask
+	 * again for a resolution. We should not search twice for the
+	 * same path.
+	 */
+	if (!strcmp(path, last_path))
+		return 0;
+
 	/* find commit which merges them */
 	parent_count = find_first_merges(&merges, path, commit_a, commit_b);
 	switch (parent_count) {
@@ -726,6 +734,7 @@ int merge_submodule(unsigned char result[20], const char *path,
 			print_commit((struct commit *) merges.objects[i].item);
 	}
 
+	memcpy(last_path, path, strlen(path) + 1);
 	free(merges.objects);
 	return 0;
 }
diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 8f6f2d6..603fb72 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '
 '
 
 # merge should leave submodule unmerged in index
-test_expect_failure 'recursive merge with submodule' '
+test_expect_success 'recursive merge with submodule' '
 	(cd merge-recursive &&
 	 test_must_fail git merge top-bc &&
 	 echo "160000 $(git rev-parse top-cb:sub) 2	sub" > expect2 &&

-- 
1.7.6.551.g4266ca

Junio C Hamano· Aug 24, 2011, 19:24 UTC · re: Heiko Voigt · lore

Re: [PATCH] submodule: Demonstrate known breakage during recursive merge

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 13 quoted lines

> ... Its a
> little bit workaroundish so if anymore has an idea how to fix this in
> nicer way, please tell me.
>
> [1]--8<----
> From: Heiko Voigt <hvoigt@hvoigt.net>
> Subject: [PATCH] protect submodule merge search against multiple calls for
>  the same path
>
> When multiple merge-bases are found for two commits to be merged the
> merge machinery will ask twice for a merge resolution. Currently its not
> possible to use the revision-walking api for walking the same commits
> multiple times.

I have been suspecting that most of this should be done in a separate helper program that is run via run_command() interface, without contaminating the object pool the main merge process has with data from the submodule object store to begin with (i.e. add_submodule_odb() and everything below should go). Wouldn't it be a lot cleaner solution?

Heiko Voigt· Aug 24, 2011, 19:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodule: Demonstrate known breakage during recursive merge

On Wed, Aug 24, 2011 at 12:24:22PM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > ... Its a
> > little bit workaroundish so if anymore has an idea how to fix this in
> > nicer way, please tell me.
> >
> > [1]--8<----
> > From: Heiko Voigt <hvoigt@hvoigt.net>
> > Subject: [PATCH] protect submodule merge search against multiple calls for
> >  the same path
> >
> > When multiple merge-bases are found for two commits to be merged the
> > merge machinery will ask twice for a merge resolution. Currently its not
> > possible to use the revision-walking api for walking the same commits
> > multiple times.
> 
> I have been suspecting that most of this should be done in a separate
> helper program that is run via run_command() interface, without
> contaminating the object pool the main merge process has with data from
> the submodule object store to begin with (i.e. add_submodule_odb() and
> everything below should go). Wouldn't it be a lot cleaner solution?

Hmm, I would like to keep it in process. Since there are platforms where spawning new processes is very slow. If just the revision-walking is an issue: I am currently working on extending it to be able to walk again, because we also need that for the recursive push series.

But even if we are able to search twice or more (in or out process) we would give the advice that there are multiple possible resolutions for each merge base. For the merge search we do not take the bases into account so the outcome will not change. So what I think would be cleaner (also UI wise) is to remember whether we already gave advice and not do it a second time.

Cheers Heiko

Brad King· Aug 24, 2011, 20:02 UTC · re: Heiko Voigt · lore

Re: [PATCH] submodule: Demonstrate known breakage during recursive merge

On 8/24/2011 3:14 PM, Heiko Voigt wrote:
 > thanks for finding this subtle bug!

Thanks for looking at it!

On 8/24/2011 3:46 PM, Heiko Voigt wrote:

> For the merge search we do not take the bases into
> account so the outcome will not change.

The test case creates history like this:

 >     b---bc
 >    / \ /
 >   o   X
 >    \ / \
 >     c---cb

where b, c, bc, and cb all reference different submodule commits.

Isn't the merge search asked to search for a descendant of "b:sub" and "c:sub" during the recursive part of the merge and then "bc:sub" and "cb:sub" during the primary merge? Might those results be different?

As for the UI part, I think the user would be interested only in the search results for the primary merge between HEAD and MERGE_HEAD. Results from the intermediate merges might not make sense.

Thanks, -Brad

Heiko Voigt· Aug 24, 2011, 20:27 UTC · re: Brad King · lore

Re: [PATCH] submodule: Demonstrate known breakage during recursive merge

On Wed, Aug 24, 2011 at 04:02:03PM -0400, Brad King wrote:

Show 22 quoted lines

> On 8/24/2011 3:14 PM, Heiko Voigt wrote:
> > thanks for finding this subtle bug!
>
> Thanks for looking at it!
>
> On 8/24/2011 3:46 PM, Heiko Voigt wrote:
>> For the merge search we do not take the bases into
>> account so the outcome will not change.
>
> The test case creates history like this:
>
> >     b---bc
> >    / \ /
> >   o   X
> >    \ / \
> >     c---cb
>
> where b, c, bc, and cb all reference different submodule commits.
>
> Isn't the merge search asked to search for a descendant of "b:sub" and "c:sub"
> during the recursive part of the merge and then "bc:sub" and "cb:sub" during
> the primary merge?  Might those results be different?

The merge is quite simple. All it does is check whether both changes base->a or base->b point forward in the submodule. Then it checks whether a is contained in b or the other way around. This is the only case in which it will succeed automatically.

Supposing you merge bc into cb: If I understand the situation correctly, the above is done first with a := cb:sub, b := bc:sub, base := b:sub and then another time with base := c:sub.

For the suggestion part only bc and cb are taken into account. That is we search for the first commit in the submodule refs which contains both bc:sub and cb:sub.

>
> As for the UI part, I think the user would be interested only in the search
> results for the primary merge between HEAD and MERGE_HEAD.  Results from the
> intermediate merges might not make sense.

As stated above since bc:sub and cb:sub will not change in between two searches the result for the suggestion will be the same. What I meant was that the same result would be output twice (or more).

Cheers Heiko

Brad King· Aug 24, 2011, 20:40 UTC · re: Heiko Voigt · lore

Re: [PATCH] submodule: Demonstrate known breakage during recursive merge

On 8/24/2011 4:27 PM, Heiko Voigt wrote:

Show 5 quoted lines

>>>      b---bc
>>>     / \ /
>>>    o   X
>>>     \ / \
>>>      c---cb

[snip]

> Supposing you merge bc into cb:
> If I understand the situation correctly, the above is done first with
> a := cb:sub, b := bc:sub, base := b:sub and then another time with
> base := c:sub.

When merging bc and cb there are two merge bases: b and c. The recursive merge strategy first performs a "virtual" merge between b and c and uses the result as a fictional merge base between bc and cb. Currently the submodule merge search runs during the "virtual" merge and gives advice. Then it later dies while trying to search during the "real" merge.

After applying my patch, try this:

  $ cd t && ./t7405-submodule-merge.sh --verbose
  ...
  Merging:
  8cbd0fb cb
  virtual top-bc
  found 2 common ancestor(s):
  f6b4d5a b
  4d9cfab c
    Merging:
    f6b4d5a b
    4d9cfab c
    found 1 common ancestor(s):
    a2ff72f a
  warning: Failed to merge submodule sub (multiple merges found)
   806049692f8921101f2e7223852e3bd74f7187c8: > Merge branch 'sub-c' into sub-bc
   db70dfacda48ce55365256a58eaf89b7da87cbe7: > Merge branch 'sub-b' into sub-cb
    Auto-merging sub
    CONFLICT (submodule): Merge conflict in sub
  fatal: --ancestry-path given but there are no bottom commits

One can see that the advice given talks about merging "b:sub" and "c:sub" and the suggested commits are actually "bc:sub" and "cb:sub". This advice is not useful to someone mergeing bc and cb.

-Brad

Heiko Voigt· Aug 24, 2011, 21:32 UTC · re: Brad King · lore

Re: [PATCH] submodule: Demonstrate known breakage during recursive merge

Hi,

On Wed, Aug 24, 2011 at 04:40:50PM -0400, Brad King wrote:

Show 17 quoted lines

> On 8/24/2011 4:27 PM, Heiko Voigt wrote:
>>>>      b---bc
>>>>     / \ /
>>>>    o   X
>>>>     \ / \
>>>>      c---cb
> [snip]
>> Supposing you merge bc into cb:
>> If I understand the situation correctly, the above is done first with
>> a := cb:sub, b := bc:sub, base := b:sub and then another time with
>> base := c:sub.
>
> When merging bc and cb there are two merge bases: b and c.  The recursive
> merge strategy first performs a "virtual" merge between b and c and uses
> the result as a fictional merge base between bc and cb.  Currently the
> submodule merge search runs during the "virtual" merge and gives advice.
> Then it later dies while trying to search during the "real" merge.

But what happens if this "virtual" merge of b and c does not succeed like in our case?

Show 21 quoted lines

> After applying my patch, try this:
>
>  $ cd t && ./t7405-submodule-merge.sh --verbose
>  ...
>  Merging:
>  8cbd0fb cb
>  virtual top-bc
>  found 2 common ancestor(s):
>  f6b4d5a b
>  4d9cfab c
>    Merging:
>    f6b4d5a b
>    4d9cfab c
>    found 1 common ancestor(s):
>    a2ff72f a
>  warning: Failed to merge submodule sub (multiple merges found)
>   806049692f8921101f2e7223852e3bd74f7187c8: > Merge branch 'sub-c' into sub-bc
>   db70dfacda48ce55365256a58eaf89b7da87cbe7: > Merge branch 'sub-b' into sub-cb
>    Auto-merging sub
>    CONFLICT (submodule): Merge conflict in sub
>  fatal: --ancestry-path given but there are no bottom commits

Yeah I have seen this and it looked to me as if those two commits are bc:sub and cb:sub since the commit message talks about c:sub merged into bc:sub and vice versa.

> One can see that the advice given talks about merging "b:sub" and "c:sub"
> and the suggested commits are actually "bc:sub" and "cb:sub".  This advice
> is not useful to someone mergeing bc and cb.

In this case we should only output the advice of the last merge of course. It will have the same result in this case though since the merge of bc:sub and cb:sub still has just these two candidates.

Cheers Heiko

Brad King· Aug 25, 2011, 16:49 UTC · re: Heiko Voigt · lore

[PATCH] rev-list: Demonstrate breakage with --ancestry-path --all

The option added by commit ebdc94f3 (revision: --ancestry-path, 2010-04-20) does not work properly in combination with --all, at least in the case of a criss-cross merge:

    b---bc
   / \ /
  a   X
   \ / \
    c---cb

There are no descendants of 'cb' in the history.  The command

  git rev-list --ancestry-path cb..bc

correctly reports no commits.  However, the command

  git rev-list --ancestry-path --all ^cb

reports 'bc'. Add a test case to t6019-rev-list-ancestry-path demonstrating this breakage.

Signed-off-by: Brad King <brad.king@kitware.com>
---

I tried to fix the submodule merge search during a recursive merge by only doing it when o->call_depth is zero. While testing the fix I noticed that the merge search was reporting an incorrect commit as a suggested submodule merge resolution. Internally the merge search uses "rev-list --merges --ancestry-path --all ^a" to find all merges that contain 'a'. The rev-list incorrectly reports the other side of the criss-cross history. I narrowed the problem down to this test case.

Brad

 t/t6019-rev-list-ancestry-path.sh |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

Show changes to t/t6019-rev-list-ancestry-path.sh +35 −0

diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh
index 7641029..aa4674f 100755
--- a/t/t6019-rev-list-ancestry-path.sh
+++ b/t/t6019-rev-list-ancestry-path.sh
@@ -70,4 +70,39 @@ test_expect_success 'rev-list --ancestry-patch D..M -- M.t' '
 	test_cmp expect actual
 '
 
+#   b---bc
+#  / \ /
+# a   X
+#  \ / \
+#   c---cb
+test_expect_success 'setup criss-cross' '
+	mkdir criss-cross &&
+	(cd criss-cross &&
+	 git init &&
+	 test_commit A &&
+	 git checkout -b b master &&
+	 test_commit B &&
+	 git checkout -b c master &&
+	 test_commit C &&
+	 git checkout -b bc b -- &&
+	 git merge c &&
+	 git checkout -b cb c -- &&
+	 git merge b &&
+	 git checkout master)
+'
+
+# no commits in bc descend from cb
+test_expect_success 'criss-cross: rev-list --ancestry-path cb..bc' '
+	(cd criss-cross &&
+	 git rev-list --ancestry-path cb..bc > actual &&
+	 test -z "$(cat actual)")
+'
+
+# no commits in repository descend from cb
+test_expect_failure 'criss-cross: rev-list --ancestry-path --all ^cb' '
+	(cd criss-cross &&
+	 git rev-list --ancestry-path --all ^cb > actual &&
+	 test -z "$(cat actual)")
+'
+
 test_done

-- 
1.7.4.4

Junio C Hamano· Aug 25, 2011, 23:08 UTC · re: Brad King · lore

Re: [PATCH] rev-list: Demonstrate breakage with --ancestry-path --all

Brad King <brad.king@kitware.com> writes:

Show 9 quoted lines

> The option added by commit ebdc94f3 (revision: --ancestry-path,
> 2010-04-20) does not work properly in combination with --all, at least
> in the case of a criss-cross merge:
>
>     b---bc
>    / \ /
>   a   X
>    \ / \
>     c---cb

Hmm, what should --ancestry-path do given more than one positive commit to begin with, let alone --all? I do not think the request itself does not make sense.

Junio C Hamano· Aug 25, 2011, 23:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-list: Demonstrate breakage with --ancestry-path --all

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

Show 14 quoted lines

> Brad King <brad.king@kitware.com> writes:
>
>> The option added by commit ebdc94f3 (revision: --ancestry-path,
>> 2010-04-20) does not work properly in combination with --all, at least
>> in the case of a criss-cross merge:
>>
>>     b---bc
>>    / \ /
>>   a   X
>>    \ / \
>>     c---cb
>
> Hmm, what should --ancestry-path do given more than one positive commit to
> begin with, let alone --all?

I actually think that this does not have much to do with "criss-cross"-ness. Instead of computing those that can be reached from cb, we are computing those that can be reached from either b, c or cb.

This needs fixing, but it takes a bit more than a quick hack. Stay tuned ;-)

Thanks.

Junio C Hamano· Aug 26, 2011, 01:00 UTC · re: Brad King · lore

[PATCH 1/2] revision: keep track of the end-user input from the command line

Given a complex set of revision specifiers on the command line, it is too late to look at the flags of the objects in the initial traversal list at the beginning of limit_list() in order to determine what the objects the end-user explicitly listed on the command line were. The process to move objects from the pending array to the traversal list may have marked objects that are not mentioned as UNINTERESTING, when handle_commit() marked the parents of UNINTERESTING commits mentioned on the command line by calling mark_parents_uninteresting().

This made "rev-list --ancestry-path ^A ..." to mistakenly list commits that are descendants of A's parents but that are not descendants of A itself, as ^A from the command line causes A and its parents marked as UNINTERESTING before coming to limit_list(), and we try to enumerate the commits that are descendants of these commits that are UNINTERESTING before we start walking the history.

It actually is too late even if we inspected the pending object array before calling prepare_revision_walk(), as some of the same objects might have been mentioned twice, once as positive and another time as negative. The "rev-list --some-option A --not --all" command may want to notice, even if the resulting set is empty, that the user showed some interest in "A" and do something special about it.

Prepare a separate array to keep track of what syntactic element was used to cause each object to appear in the pending array from the command line, and populate it as setup_revisions() parses the command line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Should apply cleanly on top of your test case.

 revision.c |   37 +++++++++++++++++++++++++++++++++----
 revision.h |   20 ++++++++++++++++++++
 2 files changed, 53 insertions(+), 4 deletions(-)

Show changes to 2 files +53 −4

revision.c, revision.h

diff --git a/revision.c b/revision.c
index 96d7fa7..3e87c86 100644
--- a/revision.c
+++ b/revision.c
@@ -797,6 +797,23 @@ static int limit_list(struct rev_info *revs)
 	return 0;
 }
 
+static void add_rev_cmdline(struct rev_info *revs,
+			    struct object *item,
+			    const char *name,
+			    int whence,
+			    unsigned flags)
+{
+	struct rev_cmdline_info *info = &revs->cmdline;
+	int nr = info->nr;
+
+	ALLOC_GROW(info->rev, nr + 1, info->alloc);
+	info->rev[nr].item = item;
+	info->rev[nr].name = name;
+	info->rev[nr].whence = whence;
+	info->rev[nr].flags = flags;
+	info->nr++;
+}
+
 struct all_refs_cb {
 	int all_flags;
 	int warned_bad_reflog;
@@ -809,6 +826,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1, int flag,
 	struct all_refs_cb *cb = cb_data;
 	struct object *object = get_reference(cb->all_revs, path, sha1,
 					      cb->all_flags);
+	add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);
 	add_pending_object(cb->all_revs, object, path);
 	return 0;
 }
@@ -835,6 +853,7 @@ static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)
 		struct object *o = parse_object(sha1);
 		if (o) {
 			o->flags |= cb->all_flags;
+			/* ??? CMDLINEFLAGS ??? */
 			add_pending_object(cb->all_revs, o, "");
 		}
 		else if (!cb->warned_bad_reflog) {
@@ -871,12 +890,13 @@ static void handle_reflog(struct rev_info *revs, unsigned flags)
 	for_each_reflog(handle_one_reflog, &cb);
 }
 
-static int add_parents_only(struct rev_info *revs, const char *arg, int flags)
+static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)
 {
 	unsigned char sha1[20];
 	struct object *it;
 	struct commit *commit;
 	struct commit_list *parents;
+	const char *arg = arg_;
 
 	if (*arg == '^') {
 		flags ^= UNINTERESTING;
@@ -898,6 +918,7 @@ static int add_parents_only(struct rev_info *revs, const char *arg, int flags)
 	for (parents = commit->parents; parents; parents = parents->next) {
 		it = &parents->item->object;
 		it->flags |= flags;
+		add_rev_cmdline(revs, it, arg_, REV_CMD_PARENTS_ONLY, flags);
 		add_pending_object(revs, it, arg);
 	}
 	return 1;
@@ -987,7 +1008,7 @@ static void prepare_show_merge(struct rev_info *revs)
 	revs->limited = 1;
 }
 
-int handle_revision_arg(const char *arg, struct rev_info *revs,
+int handle_revision_arg(const char *arg_, struct rev_info *revs,
 			int flags,
 			int cant_be_filename)
 {
@@ -996,6 +1017,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 	struct object *object;
 	unsigned char sha1[20];
 	int local_flags;
+	const char *arg = arg_;
 
 	dotdot = strstr(arg, "..");
 	if (dotdot) {
@@ -1004,6 +1026,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 		const char *this = arg;
 		int symmetric = *next == '.';
 		unsigned int flags_exclude = flags ^ UNINTERESTING;
+		unsigned int a_flags;
 
 		*dotdot = 0;
 		next += symmetric;
@@ -1036,10 +1059,15 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 				add_pending_commit_list(revs, exclude,
 							flags_exclude);
 				free_commit_list(exclude);
-				a->object.flags |= flags | SYMMETRIC_LEFT;
+				a_flags = flags | SYMMETRIC_LEFT;
 			} else
-				a->object.flags |= flags_exclude;
+				a_flags = flags_exclude;
+			a->object.flags |= a_flags;
 			b->object.flags |= flags;
+			add_rev_cmdline(revs, &a->object, this,
+					REV_CMD_LEFT, a_flags);
+			add_rev_cmdline(revs, &b->object, next,
+					REV_CMD_RIGHT, flags);
 			add_pending_object(revs, &a->object, this);
 			add_pending_object(revs, &b->object, next);
 			return 0;
@@ -1070,6 +1098,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 	if (!cant_be_filename)
 		verify_non_filename(revs->prefix, arg);
 	object = get_reference(revs, arg, sha1, flags ^ local_flags);
+	add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
 	add_pending_object_with_mode(revs, object, arg, mode);
 	return 0;
 }
diff --git a/revision.h b/revision.h
index 855464f..031cc7c 100644
--- a/revision.h
+++ b/revision.h
@@ -23,6 +23,23 @@ struct rev_info;
 struct log_info;
 struct string_list;
 
+struct rev_cmdline_info {
+	unsigned int nr;
+	unsigned int alloc;
+	struct rev_cmdline_entry {
+		struct object *item;
+		const char *name;
+		enum {
+			REV_CMD_REF,
+			REV_CMD_PARENTS_ONLY,
+			REV_CMD_LEFT,
+			REV_CMD_RIGHT,
+			REV_CMD_REV
+		} whence;
+		unsigned flags;
+	} *rev;
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -31,6 +48,9 @@ struct rev_info {
 	/* Parents of shown commits */
 	struct object_array boundary_commits;
 
+	/* The end-points specified by the end user */
+	struct rev_cmdline_info cmdline;
+
 	/* Basic information */
 	const char *prefix;
 	const char *def;

-- 
1.7.6.1.385.gb7fcd0

Sverre Rabbelier· Aug 26, 2011, 01:08 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] revision: keep track of the end-user input from the command line

Heya,

On Thu, Aug 25, 2011 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:

> Prepare a separate array to keep track of what syntactic element was used
> to cause each object to appear in the pending array from the command line,
> and populate it as setup_revisions() parses the command line.

Thank you! I was really dreading looking into this myself, so I'm very glad that you could find the time to look into it yourself.

Show 8 quoted lines

> @@ -835,6 +853,7 @@ static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)
>                struct object *o = parse_object(sha1);
>                if (o) {
>                        o->flags |= cb->all_flags;
> +                       /* ??? CMDLINEFLAGS ??? */
>                        add_pending_object(cb->all_revs, o, "");
>                }
>                else if (!cb->warned_bad_reflog) {

What is happening here?

-- 
Cheers,

Sverre Rabbelier

Junio C Hamano· Aug 26, 2011, 02:21 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 1/2] revision: keep track of the end-user input from the command line

Sverre Rabbelier <srabbelier@gmail.com> writes:

Show 6 quoted lines

>> Prepare a separate array to keep track of what syntactic element was used
>> to cause each object to appear in the pending array from the command line,
>> and populate it as setup_revisions() parses the command line.
>
> Thank you! I was really dreading looking into this myself, so I'm very
> glad that you could find the time to look into it yourself.

I debated long and hard if I should instead fatten object array entry and shove this information there without adding a new structure, which would have resulted in something very similar to what you had, so you should take some credit for the code, and also credit for a large part of the motivation (the second paragraph in the log is entirely your use case).

We might end up unifying this command line information array and the pending object array after reviewing what other future callers would want from this new information, but at least by doing it this way I can rest assured that no existing code that is unaware of the mechanism would get any unintended side effects in the earlier rounds.

Show 10 quoted lines

>> @@ -835,6 +853,7 @@ static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)
>>                struct object *o = parse_object(sha1);
>>                if (o) {
>>                        o->flags |= cb->all_flags;
>> +                       /* ??? CMDLINEFLAGS ??? */
>>                        add_pending_object(cb->all_revs, o, "");
>>                }
>>                else if (!cb->warned_bad_reflog) {
>
> What is happening here?

We could have add_rev_cmdline() call there if we really wanted to, but I decided not to do so for two reasons:

(1) with "rev-list -g HEAD", the user is not explicitly mentioning all
    objects in the reflog of the HEAD---it might still make sense to mark
    HEAD as explicitly mentioned as positive, but the code to do so will
    not sit here anyway; and more importantly,

(2) I was appalled by how broken the design and the implementation of
    walking the reflog was (it shoves _all_ the objects in the entire
    history recorded in the reflog to the pending queue before letting the
    caller do an iota of work). It is against the general design of Git,
    and the design of the revision walk machinery in particular that tries
    very hard to be incremental. It goes against a good software design
    taste.

For the latter reason, I think in the longer term we should correct the implementation to walk the reflog to keep an iterator (a structure that holds an open file descriptor to a reflog file, and perhaps a little bit of buffer) in struct rev_info, and teach get_revisions() to lazily read from there, reading the file backward. Once that happens, this callback from for_each_reflog_ent() will go away, so I didn't bother.

Junio C Hamano· Aug 26, 2011, 01:00 UTC · re: Brad King · lore

[PATCH 2/2] revision: do not include sibling history in --ancestry-path output

If the commit specified as the bottom of the commit range has a direct parent that has another child commit that contributed to the resulting history, "rev-list --ancestry-path" was confused and listed that side history as well.

             D---E
            /     \
        ---X---A---B---C

In this history, "rev-list --ancestry-path A..C" should list among what the corresponding command without --ancestry-path option would produce, namely, D, E, B and C, but limiting the result to those that are descendant of A (i.e. B and C). Due to the command line parser subtlety corrected by the previous commit, it also listed those that are descendant of X as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * And this should fix the breakage you demonstrated.

 revision.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

Show changes to revision.c +10 −6

diff --git a/revision.c b/revision.c
index 3e87c86..48a2db4 100644
--- a/revision.c
+++ b/revision.c
@@ -724,12 +724,16 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li
  * to filter the result of "A..B" further to the ones that can actually
  * reach A.
  */
-static struct commit_list *collect_bottom_commits(struct commit_list *list)
+static struct commit_list *collect_bottom_commits(struct rev_info *revs)
 {
-	struct commit_list *elem, *bottom = NULL;
-	for (elem = list; elem; elem = elem->next)
-		if (elem->item->object.flags & UNINTERESTING)
-			commit_list_insert(elem->item, &bottom);
+	struct commit_list *bottom = NULL;
+	int i;
+	for (i = 0; i < revs->cmdline.nr; i++) {
+		struct rev_cmdline_entry *elem = &revs->cmdline.rev[i];
+		if ((elem->flags & UNINTERESTING) &&
+		    elem->item->type == OBJ_COMMIT)
+			commit_list_insert((struct commit *)elem->item, &bottom);
+	}
 	return bottom;
 }
 
@@ -743,7 +747,7 @@ static int limit_list(struct rev_info *revs)
 	struct commit_list *bottom = NULL;
 
 	if (revs->ancestry_path) {
-		bottom = collect_bottom_commits(list);
+		bottom = collect_bottom_commits(revs);
 		if (!bottom)
 			die("--ancestry-path given but there are no bottom commits");
 	}

-- 
1.7.6.1.385.gb7fcd0

Brad King· Aug 26, 2011, 12:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] revision: do not include sibling history in --ancestry-path output

On 8/25/2011 9:00 PM, Junio C Hamano wrote:

Show 19 quoted lines

> If the commit specified as the bottom of the commit range has a direct
> parent that has another child commit that contributed to the resulting
> history, "rev-list --ancestry-path" was confused and listed that side
> history as well.
>
>               D---E
>              /     \
>          ---X---A---B---C
>
> In this history, "rev-list --ancestry-path A..C" should list among what
> the corresponding command without --ancestry-path option would produce,
> namely, D, E, B and C, but limiting the result to those that are
> descendant of A (i.e. B and C). Due to the command line parser subtlety
> corrected by the previous commit, it also listed those that are descendant
> of X as well.
>
> Signed-off-by: Junio C Hamano<gitster@pobox.com>
> ---
>   * And this should fix the breakage you demonstrated.

Yes it does, thanks. It also makes the submodule search during recursive merge work as I expect after the fix I tried (only run the search if o->call_depth == 0). I'll submit that patch separately when I find time.

Tested-by: Brad King <brad.king@kitware.com>

Thanks, -Brad

Junio C Hamano· Aug 24, 2011, 22:43 UTC · re: Heiko Voigt · lore

Re: [PATCH] submodule: Demonstrate known breakage during recursive merge

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 8 quoted lines

>> I have been suspecting that most of this should be done in a separate
>> helper program that is run via run_command() interface, without
>> contaminating the object pool the main merge process has with data from
>> the submodule object store to begin with (i.e. add_submodule_odb() and
>> everything below should go). Wouldn't it be a lot cleaner solution?
>
> Hmm, I would like to keep it in process. Since there are platforms where
> spawning new processes is very slow.

Adding submodule's odb into the main process _will_ also have performance penalties because it will make it more expensive to look up objects that belong to the superproject when the superproject wants its own look up.

In case you haven't realized yet, walking revision graph multiple times while making sure that you do not affect other revision traversals in effect is hard to arrange right. But more importantly, correctness counts more than performing quickly and giving a bogus result with premature optimization that makes it harder to implement things correctly (and harder to verify the change is correct).

Heiko Voigt· Aug 25, 2011, 21:11 UTC · re: Junio C Hamano · lore

[PATCH] allow multiple calls to submodule merge search for the same path

When multiple merge-bases are found for two commits to be merged the merge machinery will ask twice for a merge resolution. Currently its not possible to use the revision-walking api for walking the same commits multiple times. Thats why we now run the revision walking in a seperate git process.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
Hi,

On Wed, Aug 24, 2011 at 03:43:56PM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> >> I have been suspecting that most of this should be done in a separate
> >> helper program that is run via run_command() interface, without
> >> contaminating the object pool the main merge process has with data from
> >> the submodule object store to begin with (i.e. add_submodule_odb() and
> >> everything below should go). Wouldn't it be a lot cleaner solution?
> >
> > Hmm, I would like to keep it in process. Since there are platforms where
> > spawning new processes is very slow.
> 
> Adding submodule's odb into the main process _will_ also have performance
> penalties because it will make it more expensive to look up objects that
> belong to the superproject when the superproject wants its own look up.
> 
> In case you haven't realized yet, walking revision graph multiple times
> while making sure that you do not affect other revision traversals in
> effect is hard to arrange right. But more importantly, correctness counts
> more than performing quickly and giving a bogus result with premature
> optimization that makes it harder to implement things correctly (and
> harder to verify the change is correct).

Yes of course correctness has priority one. And so here is a patch implementing the rev-iteration using a seperate process. Once the revision walking api is able to walk multiple times we get back to it again.

I noticed that the suggestion for the merge resolution presented is wrong which Brad also seems to have found as presented in his patch in message:

<438ea0b254ccafb3fc9f3431f8f86007cc03132b.1314290439.git.brad.king@kitware.com>

But this breakage depends on the wrong output from rev-list with the --ancestry-path option. This patch should be correct on its own.

 submodule.c                |   31 ++++++++++++++++++++++---------
 t/t7405-submodule-merge.sh |    2 +-
 2 files changed, 23 insertions(+), 10 deletions(-)

Show changes to 2 files +23 −10

submodule.c, t/t7405-submodule-merge.sh

diff --git a/submodule.c b/submodule.c
index 1ba9646..9820df7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -581,29 +581,42 @@ static int find_first_merges(struct object_array *result, const char *path,
 	char merged_revision[42];
 	const char *rev_args[] = { "rev-list", "--merges", "--ancestry-path",
 				   "--all", merged_revision, NULL };
-	struct rev_info revs;
-	struct setup_revision_opt rev_opts;
+	struct child_process cp;
+	struct strbuf one_rev = STRBUF_INIT;
 
 	memset(&merges, 0, sizeof(merges));
 	memset(result, 0, sizeof(struct object_array));
-	memset(&rev_opts, 0, sizeof(rev_opts));
+	memset(&cp, 0, sizeof(cp));
 
 	/* get all revisions that merge commit a */
 	snprintf(merged_revision, sizeof(merged_revision), "^%s",
 			sha1_to_hex(a->object.sha1));
-	init_revisions(&revs, NULL);
-	rev_opts.submodule = path;
-	setup_revisions(sizeof(rev_args)/sizeof(char *)-1, rev_args, &revs, &rev_opts);
+
+	cp.argv = rev_args;
+	cp.env = local_repo_env;
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	cp.out = -1;
+	cp.dir = path;
+	if (start_command(&cp))
+		die("Could not run 'git rev-list --merges --ancestry-path --all %s' "
+				"command in submodule %s", merged_revision, path);
+	FILE *out = fdopen(cp.out, "r");
+	if (!out)
+		die("Could not open pipe of rev-list command.");
 
 	/* save all revisions from the above list that contain b */
-	if (prepare_revision_walk(&revs))
-		die("revision walk setup failed");
-	while ((commit = get_revision(&revs)) != NULL) {
+	while (strbuf_getline(&one_rev, out, '\n') != EOF) {
+		commit = lookup_commit_reference_by_name(one_rev.buf);
 		struct object *o = &(commit->object);
 		if (in_merge_bases(b, &commit, 1))
 			add_object_array(o, NULL, &merges);
 	}
 
+	fclose(out);
+	finish_command(&cp);
+	strbuf_release(&one_rev);
+
 	/* Now we've got all merges that contain a and b. Prune all
 	 * merges that contain another found merge and save them in
 	 * result.
diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 8f6f2d6..603fb72 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '
 '
 
 # merge should leave submodule unmerged in index
-test_expect_failure 'recursive merge with submodule' '
+test_expect_success 'recursive merge with submodule' '
 	(cd merge-recursive &&
 	 test_must_fail git merge top-bc &&
 	 echo "160000 $(git rev-parse top-cb:sub) 2	sub" > expect2 &&

-- 
1.7.6.551.g0f51b.dirty

Junio C Hamano· Aug 25, 2011, 23:22 UTC · re: Heiko Voigt · lore

Re: [PATCH] allow multiple calls to submodule merge search for the same path

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 13 quoted lines

> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
> index 8f6f2d6..603fb72 100755
> --- a/t/t7405-submodule-merge.sh
> +++ b/t/t7405-submodule-merge.sh
> @@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '
>  '
>  
>  # merge should leave submodule unmerged in index
> -test_expect_failure 'recursive merge with submodule' '
> +test_expect_success 'recursive merge with submodule' '
>  	(cd merge-recursive &&
>  	 test_must_fail git merge top-bc &&
>  	 echo "160000 $(git rev-parse top-cb:sub) 2	sub" > expect2 &&

What is this patch based on?

Junio C Hamano· Aug 25, 2011, 23:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] allow multiple calls to submodule merge search for the same path

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

Show 17 quoted lines

> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
>> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
>> index 8f6f2d6..603fb72 100755
>> --- a/t/t7405-submodule-merge.sh
>> +++ b/t/t7405-submodule-merge.sh
>> @@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '
>>  '
>>  
>>  # merge should leave submodule unmerged in index
>> -test_expect_failure 'recursive merge with submodule' '
>> +test_expect_success 'recursive merge with submodule' '
>>  	(cd merge-recursive &&
>>  	 test_must_fail git merge top-bc &&
>>  	 echo "160000 $(git rev-parse top-cb:sub) 2	sub" > expect2 &&
>
> What is this patch based on?

Ah, nevermind. I figured it out. Thanks.

Junio C Hamano· Aug 25, 2011, 23:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] allow multiple calls to submodule merge search for the same path

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

Show 21 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Heiko Voigt <hvoigt@hvoigt.net> writes:
>>
>>> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
>>> index 8f6f2d6..603fb72 100755
>>> --- a/t/t7405-submodule-merge.sh
>>> +++ b/t/t7405-submodule-merge.sh
>>> @@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '
>>>  '
>>>  
>>>  # merge should leave submodule unmerged in index
>>> -test_expect_failure 'recursive merge with submodule' '
>>> +test_expect_success 'recursive merge with submodule' '
>>>  	(cd merge-recursive &&
>>>  	 test_must_fail git merge top-bc &&
>>>  	 echo "160000 $(git rev-parse top-cb:sub) 2	sub" > expect2 &&
>>
>> What is this patch based on?
>
> Ah, nevermind. I figured it out. Thanks.

Just FYI; squashed this on top to fix compilation breakage. Thanks.

 submodule.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Show changes to submodule.c +4 −2

diff --git a/submodule.c b/submodule.c
index 21a57d2..752cd8a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -738,6 +738,7 @@ static int find_first_merges(struct object_array *result, const char *path,
 	struct object_array merges;
 	struct commit *commit;
 	int contains_another;
+	FILE *out;
 
 	char merged_revision[42];
 	const char *rev_args[] = { "rev-list", "--merges", "--ancestry-path",
@@ -762,14 +763,15 @@ static int find_first_merges(struct object_array *result, const char *path,
 	if (start_command(&cp))
 		die("Could not run 'git rev-list --merges --ancestry-path --all %s' "
 				"command in submodule %s", merged_revision, path);
-	FILE *out = fdopen(cp.out, "r");
+	out = fdopen(cp.out, "r");
 	if (!out)
 		die("Could not open pipe of rev-list command.");
 
 	/* save all revisions from the above list that contain b */
 	while (strbuf_getline(&one_rev, out, '\n') != EOF) {
+		struct object *o;
 		commit = lookup_commit_reference_by_name(one_rev.buf);
-		struct object *o = &(commit->object);
+		o = &(commit->object);
 		if (in_merge_bases(b, &commit, 1))
 			add_object_array(o, NULL, &merges);
 	}

Brad King· Aug 25, 2011, 12:28 UTC · re: Brad King · lore

[PATCH v2] submodule: Demonstrate known breakage during recursive merge

Since commit 68d03e4a (Implement automatic fast-forward merge for submodules, 2010-07-07) we try to suggest submodule commits that resolve a conflict. Consider a true recursive merge case

    b---bc
   / \ /
  o   X
   \ / \
    c---cb

  fatal: --ancestry-path given but there are no bottom commits

while trying to enumerate candidates. Demonstrate this known breakage with a new test in t7405-submodule-merge covering the case.

Signed-off-by: Brad King <brad.king@kitware.com>
---

This fixes the first version of the patch by using "merge --no-commit" instead of "merge -n" where the intention is to not commit the merge. Both instances are followed by a submodule checkout, add, and then a real commit. Now the history in the test actually looks like what is in the commit message.

Brad

 t/t7405-submodule-merge.sh |   51 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

Show changes to t/t7405-submodule-merge.sh +51 −0

diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index a8fb30b..14da2e3 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -228,4 +228,55 @@ test_expect_success 'merging with a modify/modify conflict between merge bases'
 	git merge d
 '
 
+# canonical criss-cross history in top and submodule
+test_expect_success 'setup for recursive merge with submodule' '
+	mkdir merge-recursive &&
+	(cd merge-recursive &&
+	 git init &&
+	 mkdir sub &&
+	 (cd sub &&
+	  git init &&
+	  test_commit a &&
+	  git checkout -b sub-b master &&
+	  test_commit b &&
+	  git checkout -b sub-c master &&
+	  test_commit c &&
+	  git checkout -b sub-bc sub-b &&
+	  git merge sub-c &&
+	  git checkout -b sub-cb sub-c &&
+	  git merge sub-b &&
+	  git checkout master) &&
+	 git add sub &&
+	 git commit -m a &&
+	 git checkout -b top-b master &&
+	 (cd sub && git checkout sub-b) &&
+	 git add sub &&
+	 git commit -m b &&
+	 git checkout -b top-c master &&
+	 (cd sub && git checkout sub-c) &&
+	 git add sub &&
+	 git commit -m c &&
+	 git checkout -b top-bc top-b &&
+	 git merge -s ours --no-commit top-c &&
+	 (cd sub && git checkout sub-bc) &&
+	 git add sub &&
+	 git commit -m bc &&
+	 git checkout -b top-cb top-c &&
+	 git merge -s ours --no-commit top-b &&
+	 (cd sub && git checkout sub-cb) &&
+	 git add sub &&
+	 git commit -m cb)
+'
+
+# merge should leave submodule unmerged in index
+test_expect_failure 'recursive merge with submodule' '
+	(cd merge-recursive &&
+	 test_must_fail git merge top-bc &&
+	 echo "160000 $(git rev-parse top-cb:sub) 2	sub" > expect2 &&
+	 echo "160000 $(git rev-parse top-bc:sub) 3	sub" > expect3 &&
+	 git ls-files -u > actual &&
+	 grep "$(cat expect2)" actual > /dev/null &&
+	 grep "$(cat expect3)" actual > /dev/null)
+'
+
 test_done

-- 
1.7.4.4

Brad King· Aug 26, 2011, 14:18 UTC · re: Brad King · lore

[PATCH/RFC] submodule: Search for merges only at end of recursive merge

The submodule merge search is not useful during virtual merges because the results cannot be used automatically. Furthermore any suggesions made by the search may apply to commits different than HEAD:sub and MERGE_HEAD:sub, thus confusing the user. Skip searching for submodule merges during a virtual merge such as that between B and C while merging the heads of:

    B---BC
   / \ /
  A   X
   \ / \
    C---CB

Run the search only when the recursion level is zero (!o->call_depth). This fixes known breakage tested in t7405-submodule-merge.

Signed-off-by: Brad King <brad.king@kitware.com>
---

This addresses the submodule search problem reported in the message to which this replies. If you think it is correct I can resubmit both the test and this patch as a single series.

 merge-recursive.c          |    3 ++-
 submodule.c                |   11 ++++++++---
 submodule.h                |    4 +++-
 t/t7405-submodule-merge.sh |    2 +-
 4 files changed, 14 insertions(+), 6 deletions(-)

Show changes to 4 files +14 −6

merge-recursive.c, submodule.c, submodule.h, t/t7405-submodule-merge.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index 0cc1e6f..2118ee9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -830,7 +830,8 @@ static struct merge_file_info merge_file(struct merge_options *o,
 			free(result_buf.ptr);
 			result.clean = (merge_status == 0);
 		} else if (S_ISGITLINK(a->mode)) {
-			result.clean = merge_submodule(result.sha, one->path, one->sha1,
+			result.clean = merge_submodule(o, result.sha,
+						       one->path, one->sha1,
 						       a->sha1, b->sha1);
 		} else if (S_ISLNK(a->mode)) {
 			hashcpy(result.sha, a->sha1);
diff --git a/submodule.c b/submodule.c
index 1ba9646..c7fdafa 100644
--- a/submodule.c
+++ b/submodule.c
@@ -8,6 +8,7 @@
 #include "diffcore.h"
 #include "refs.h"
 #include "string-list.h"
+#include "merge-recursive.h"
 
 static struct string_list config_name_for_path;
 static struct string_list config_fetch_recurse_submodules_for_name;
@@ -642,9 +643,9 @@ static void print_commit(struct commit *commit)
 #define MERGE_WARNING(path, msg) \
 	warning("Failed to merge submodule %s (%s)", path, msg);
 
-int merge_submodule(unsigned char result[20], const char *path,
-		    const unsigned char base[20], const unsigned char a[20],
-		    const unsigned char b[20])
+int merge_submodule(struct merge_options *o, unsigned char result[20],
+		    const char *path, const unsigned char base[20],
+		    const unsigned char a[20], const unsigned char b[20])
 {
 	struct commit *commit_base, *commit_a, *commit_b;
 	int parent_count;
@@ -699,6 +700,10 @@ int merge_submodule(unsigned char result[20], const char *path,
 	 * user needs to confirm the resolution.
 	 */
 
+	/* This case makes sense only at the main depth 0 merge.  */
+	if (o->call_depth)
+		return 0;
+
 	/* find commit which merges them */
 	parent_count = find_first_merges(&merges, path, commit_a, commit_b);
 	switch (parent_count) {
diff --git a/submodule.h b/submodule.h
index 5350b0d..f22172c 100644
--- a/submodule.h
+++ b/submodule.h
@@ -2,6 +2,7 @@
 #define SUBMODULE_H
 
 struct diff_options;
+struct merge_options;
 
 enum {
 	RECURSE_SUBMODULES_ON_DEMAND = -1,
@@ -27,7 +28,8 @@ int fetch_populated_submodules(int num_options, const char **options,
 			       const char *prefix, int command_line_option,
 			       int quiet);
 unsigned is_submodule_modified(const char *path, int ignore_untracked);
-int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
+int merge_submodule(struct merge_options *o, unsigned char result[20],
+		    const char *path, const unsigned char base[20],
 		    const unsigned char a[20], const unsigned char b[20]);
 
 #endif
diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 14da2e3..0d5b42a 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '
 '
 
 # merge should leave submodule unmerged in index
-test_expect_failure 'recursive merge with submodule' '
+test_expect_success 'recursive merge with submodule' '
 	(cd merge-recursive &&
 	 test_must_fail git merge top-bc &&
 	 echo "160000 $(git rev-parse top-cb:sub) 2	sub" > expect2 &&

-- 
1.7.4.4

Junio C Hamano· Aug 26, 2011, 19:04 UTC · re: Brad King · lore

Re: [PATCH/RFC] submodule: Search for merges only at end of recursive merge

This is a knee-jerk reaction without thinking things thoroughly through, but wouldn't it make more sense to do this by conditionally calling merge_submodule() when !o->call_depth, leaving the callee oblivious to what is in the "merge_options" structure? That way, you do not have to touch submodule.c at all, I would think.

After all, merge_submodule() should be usable in a future merge strategy that is different from recursive and has no notion of call_depth.

Brad King· Aug 26, 2011, 19:30 UTC · re: Junio C Hamano · lore

[PATCH v2/RFC] submodule: Search for merges only at end of recursive merge

The submodule merge search is not useful during virtual merges because the results cannot be used automatically. Furthermore any suggestions made by the search may apply to commits different than HEAD:sub and MERGE_HEAD:sub, thus confusing the user. Skip searching for submodule merges during a virtual merge such as that between B and C while merging the heads of:

    B---BC
   / \ /
  A   X
   \ / \
    C---CB

Run the search only when the recursion level is zero (!o->call_depth). This fixes known breakage tested in t7405-submodule-merge.

Signed-off-by: Brad King <brad.king@kitware.com>
---
On 8/26/2011 3:04 PM, Junio C Hamano wrote:

Show 5 quoted lines

> This is a knee-jerk reaction without thinking things thoroughly through,
> but wouldn't it make more sense to do this by conditionally calling
> merge_submodule() when !o->call_depth, leaving the callee oblivious to
> what is in the "merge_options" structure? That way, you do not have to
> touch submodule.c at all, I would think.

I originally considered that but I think merge_submodule is still useful during virtual merges in the fast forward case. I haven't thought that through in detail though.

> After all, merge_submodule() should be usable in a future merge strategy
> that is different from recursive and has no notion of call_depth.

That's a worthwhile goal. Perhaps instead we should pass in a parameter to tell merge_submodule whether or not to do the search after the fast forward case fails.

Brad

 merge-recursive.c          |    6 ++++--
 submodule.c                |    6 +++++-
 submodule.h                |    2 +-
 t/t7405-submodule-merge.sh |    2 +-
 4 files changed, 11 insertions(+), 5 deletions(-)

Show changes to 4 files +11 −5

merge-recursive.c, submodule.c, submodule.h, t/t7405-submodule-merge.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index 0cc1e6f..390811e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -830,8 +830,10 @@ static struct merge_file_info merge_file(struct merge_options *o,
 			free(result_buf.ptr);
 			result.clean = (merge_status == 0);
 		} else if (S_ISGITLINK(a->mode)) {
-			result.clean = merge_submodule(result.sha, one->path, one->sha1,
-						       a->sha1, b->sha1);
+			result.clean = merge_submodule(result.sha,
+						       one->path, one->sha1,
+						       a->sha1, b->sha1,
+						       !o->call_depth);
 		} else if (S_ISLNK(a->mode)) {
 			hashcpy(result.sha, a->sha1);
 
diff --git a/submodule.c b/submodule.c
index 1ba9646..bf4f693 100644
--- a/submodule.c
+++ b/submodule.c
@@ -644,7 +644,7 @@ static void print_commit(struct commit *commit)
 
 int merge_submodule(unsigned char result[20], const char *path,
 		    const unsigned char base[20], const unsigned char a[20],
-		    const unsigned char b[20])
+		    const unsigned char b[20], int search)
 {
 	struct commit *commit_base, *commit_a, *commit_b;
 	int parent_count;
@@ -699,6 +699,10 @@ int merge_submodule(unsigned char result[20], const char *path,
 	 * user needs to confirm the resolution.
 	 */
 
+	/* Skip the search if makes no sense to the calling context.  */
+	if (!search)
+		return 0;
+
 	/* find commit which merges them */
 	parent_count = find_first_merges(&merges, path, commit_a, commit_b);
 	switch (parent_count) {
diff --git a/submodule.h b/submodule.h
index 5350b0d..d4344c8 100644
--- a/submodule.h
+++ b/submodule.h
@@ -28,6 +28,6 @@ int fetch_populated_submodules(int num_options, const char **options,
 			       int quiet);
 unsigned is_submodule_modified(const char *path, int ignore_untracked);
 int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
-		    const unsigned char a[20], const unsigned char b[20]);
+		    const unsigned char a[20], const unsigned char b[20], int search);
 
 #endif
diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 14da2e3..0d5b42a 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '
 '
 
 # merge should leave submodule unmerged in index
-test_expect_failure 'recursive merge with submodule' '
+test_expect_success 'recursive merge with submodule' '
 	(cd merge-recursive &&
 	 test_must_fail git merge top-bc &&
 	 echo "160000 $(git rev-parse top-cb:sub) 2	sub" > expect2 &&

-- 
1.7.4.4