[PATCH] Better errors when trying to merge a submodule

[PATCH] Better errors when trying to merge a submodule

[Finn Arne Gangstad]

Instead of dying with weird errors when trying to merge submodules from a
supermodule, emit errors that show what the problem is.

Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>
---

If you try to merge a submodule from a supermodule, you get some very
strange error messages. With this patch you get a nice clean error
message indicating that this isn't supported instead.


 git-merge-one-file.sh |    7 +++++++
 merge-recursive.c     |    2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 1e7727d..7aee342 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -82,6 +82,13 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
+	case ",$6,$7," in
+	*,160000,*)
+		echo "ERROR: $4: Not merging submodule."
+		exit 1
+		;;
+	esac
+
 	src2=`git-unpack-file $3`
 	case "$1" in
 	'')
diff --git a/merge-recursive.c b/merge-recursive.c
index 9a1e2f2..ecae8ea 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1046,6 +1046,8 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
 
 			free(result_buf.ptr);
 			result.clean = (merge_status == 0);
+                } else if (S_ISGITLINK(a->mode) || S_ISGITLINK(b->mode)) {
+                        die("cannot merge submodules!");
 		} else {
 			if (!(S_ISLNK(a->mode) || S_ISLNK(b->mode)))
 				die("cannot merge modes?");
-- 
1.5.3.7.1149.g591a-dirty

Re: [PATCH] Better errors when trying to merge a submodule

[Junio C Hamano]

Finn Arne Gangstad <finnag@pvv.org> writes:

> Instead of dying with weird errors when trying to merge submodules from a
> supermodule, emit errors that show what the problem is.

Thanks.

Your change to merge-one-file.sh is Ok, although I'd reword the message
a bit, and fold it as a new case arm to the existing case statement
immediately above.

Your change to merge-recursive is not quite right, although you spotted
correctly what codepath needs to be fixed.  merge_file() should not die
in such a case but set result.clean to 0 to signal that the result has
conflicts and cannot be merged, pick the sha1 from the current tree
(i.e. side A), and let the caller deal with the conflict.  If you die
there, the user cannot resolve a merge if this happens while building a
virtual ancestor commit during a recursive merge of two crisscrossing
histories.

Perhaps something like this...

-- >8 --
Support a merge with conflicting gitlink change

merge-recursive did not support merging trees that have conflicting
changes in submodules they contain, and died.  Support it exactly the
same way as how it handles conflicting symbolic link changes --- mark it
as a conflict, take the tentative result from the current side, and
letting the caller resolve the conflict, without dying in merge_file()
function.

Also reword the error message issued when merge_file() has to die
because it sees a tree entry of type it does not support yet.

---

 git-merge-one-file.sh |    4 ++++
 merge-recursive.c     |   10 ++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 1e7727d..9ee3f80 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -80,6 +80,10 @@ case "${1:-.}${2:-.}${3:-.}" in
 		echo "ERROR: $4: Not merging symbolic link changes."
 		exit 1
 		;;
+	*,160000,*)
+		echo "ERROR: $4: Not merging conflicting submodule changes."
+		exit 1
+		;;
 	esac
 
 	src2=`git-unpack-file $3`
diff --git a/merge-recursive.c b/merge-recursive.c
index 9a1e2f2..2a58dad 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1046,14 +1046,16 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
 
 			free(result_buf.ptr);
 			result.clean = (merge_status == 0);
-		} else {
-			if (!(S_ISLNK(a->mode) || S_ISLNK(b->mode)))
-				die("cannot merge modes?");
-
+		} else if (S_ISGITLINK(a->mode)) {
+			result.clean = 0;
+			hashcpy(result.sha, a->sha1);
+		} else if (S_ISLNK(a->mode)) {
 			hashcpy(result.sha, a->sha1);
 
 			if (!sha_eq(a->sha1, b->sha1))
 				result.clean = 0;
+		} else {
+			die("unsupported object type in the tree");
 		}
 	}
 

Re: [PATCH] Better errors when trying to merge a submodule

[Finn Arne Gangstad]

On Mon, Dec 10, 2007 at 11:22:05AM -0800, Junio C Hamano wrote:
> Finn Arne Gangstad <finnag@pvv.org> writes:
> 
> > Instead of dying with weird errors when trying to merge submodules from a
> > supermodule, emit errors that show what the problem is.
> 
> Thanks.
> 
> Your change to merge-one-file.sh is Ok, although I'd reword the message
> a bit, and fold it as a new case arm to the existing case statement
> immediately above.
> [...]
> merge-recursive did not support merging trees that have conflicting
> changes in submodules they contain, and died.  Support it exactly the
> same way as how it handles conflicting symbolic link changes --- mark it
> as a conflict, take the tentative result from the current side, and
> letting the caller resolve the conflict, without dying in merge_file()
> function.
> [...]

Your patch is obviously much nicer than the one I sent in, so please
put it in if/when convenient! 

On another note, has there been any though to get merge to support
sub-module merging properly? It seems like it should be possible (and
it would make submodules a lot more useful)

- Finn Arne