[PATCH] submodule: Demonstrate known breakage during recursive merge

[PATCH] submodule: Demonstrate known breakage during recursive merge

[Brad King]

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(-)

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

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

[Heiko Voigt]

Hi,

thanks for finding this subtle bug!

On Wed, Aug 24, 2011 at 09:59:50AM -0400, Brad King wrote:
> 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(-)

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

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

[Junio C Hamano]

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?

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

[Heiko Voigt]

On Wed, Aug 24, 2011 at 12:24:22PM -0700, Junio C Hamano wrote:
> 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

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

[Brad King]

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

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

[Heiko Voigt]

On Wed, Aug 24, 2011 at 04:02:03PM -0400, Brad King wrote:
> 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

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

[Brad King]

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.

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

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

[Heiko Voigt]

Hi,

On Wed, Aug 24, 2011 at 04:40:50PM -0400, Brad King wrote:
> 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?

> 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

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

[Brad King]

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(-)

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

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

[Junio C Hamano]

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 do not think the request itself does not
make sense.

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

[Junio C Hamano]

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

> 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.

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

[Junio C Hamano]

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(-)

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

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

[Sverre Rabbelier]

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.

> @@ -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

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

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

>> 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.

>> @@ -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.

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

[Junio C Hamano]

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(-)

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

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

[Brad King]

On 8/25/2011 9:00 PM, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

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).

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

[Heiko Voigt]

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:
> 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(-)

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

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

[Junio C Hamano]

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?

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

> 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(-)

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);
 	}

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

[Brad King]

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>
---

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(-)

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

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

[Brad King]

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(-)

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

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

[Junio C Hamano]

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.

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

[Brad King]

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:
> 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(-)

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