threads / patch / 11225

patchBetter errors when trying to merge a submodule

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

## tl;dr

3 messages between Dec 10, 2007 and Dec 11, 2007. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Finn Arne Gangstad· Dec 10, 2007, 12:44 UTC · lore

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

Show changes to 2 files +9 −0

git-merge-one-file.sh, merge-recursive.c

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

Junio C Hamano· Dec 10, 2007, 19:22 UTC · re: Finn Arne Gangstad · lore

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

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

Show changes to 2 files +10 −4

git-merge-one-file.sh, merge-recursive.c

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

Finn Arne Gangstad· Dec 11, 2007, 18:11 UTC · re: Junio C Hamano · lore

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

On Mon, Dec 10, 2007 at 11:22:05AM -0800, Junio C Hamano wrote:

Show 18 quoted lines

> 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