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

From: Brad King <brad.king@kitware.com>
Date: Aug 26, 2011, 19:30 UTC
Message-ID: <8e4e80f75d281c8bdd07b0a29eee9ad407661102.1314386859.git.brad.king@kitware.com>
In-Reply-To: <7vr548c7un.fsf@alter.siamese.dyndns.org>

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

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