Re: [PATCH] merge-tree: sometimes, d/f conflict is not an issue

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 8, 2007, 20:02 UTC
Message-ID: <7v8x9q1x5t.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0707081353560.4248@racer.site>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 9 quoted lines

> Besides, IMHO there is a deeper issue. Since merge-recursive started out 
> as a Python script, and grew there until it was usable, and grew the 
> rename detection therein, too, until it was finally converted to C, it 
> accumulated a lot of features that would have been nice to have 
> independently.
> ...
> So there we are, with two really big and unwieldy chunks of code, each 
> deserving an own GSoC project to clean them up.  Or maybe not even a GSoC 
> project, but a longer project.

I would not disagree that tree merging part is a large piece of code with nontrivial development history. While I do agree that merge-tree approach is attractive in the longer run, I do not think the current "use read-tree for policy-neutral merge and then higher level to decide what to do with conflicts" is beyond repair.

I tried a variant of cherry-pick that does _not_ use merge-recursive on the reproduction recipe from Gerrit & Rémi. Essentially, a cherry-pick with a backend is:

	git-merge-$backend $commit^ -- HEAD $commit

It was made cumbersome to try different merge backend when cherry-pick has become built-in, but the above essentially was what we had in the shell script version.

Interestingly, git-merge-resolve fails, but for an entirely different reason; there is a bug in git-merge-one-file. I am kind of surprised that this has been left undetected for a long time (this shows how everybody uses git-merge-recursive these days and not git-merge-resolve). commit ed93b449 adds tests for only the "read-tree" part, even though it makes "matching" change to merge-one-file, which was the breakage. The bug is that merge-one-file did not "resolve" an unmerged index entry when it decided what the result should be --- it decided only in its comments but not with what it does! Embarrassing (a fix is attached).

With that fix, the above "cherry-pick" lookalike with $backend set to "resolve" and $commit set to "branch" in Gerrit's example reproduction recipe seems to do the right thing.

> As it is, both unpack_trees() and merge-recursive have a certain degree of 
> not-quite duplicated yet wants-to-do-largely-the-same functionality.  
> Which of course leads to much finger pointing: "it's unpack_trees() fault. 
> no. it's merge-recursive's fault. no, vice versa."

I do not think there is any pointing-fingers involved in this case. The division of labor between "read-tree -m" and its caller has been very clear from the beginning, and has not changed. The former does "uncontroversial parts of merge", and the latter uses its own policy decision to resolve conflicts.

The "uncontroversial parts of merge" explicitly exclude "one side removes (or adds), other side does not do anything" case. This is cumbersome for rename-unaware merge-resolve, because its policy is "we do not worry about the case that the apparent removal is in fact it moves it to somewhere else -- if one side removes, the result will not have it", and for that policy if "read-tree -m" did so it would have less work to do. But we don't, exactly because other policy like merge-recursive may want to look at the apparently removed path and figure out if there is a rename involved.

The last time I looked at merge-recursive.c, I think the problem I saw was the way it maintains and uses two lists that keeps track of the set of directories and files; get_files_dirs() is called for both head and merge at the beginning, and I did not see a code that says "Oh, we recorded the path 'link' as being potentially a directory at the beginning, but we have decided to resolve 'link/file' by removing it, and 'link' does not have to exist as a directory anymore. resolving 'link' as a symbolic link is perfectly fine" --- and the reason Gerrit's test case fails was something like that.

-- >8 -- [PATCH] Fix merge-one-file for our-side-added/our-side-removed cases

When commit ed93b449 changed the script so that it does not touch untracked working tree file, we forgot that we still needed to resolve the index entry (otherwise they are left unmerged).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index ebbb575..1e7727d 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -27,8 +27,9 @@ case "${1:-.}${2:-.}${3:-.}" in
 		# read-tree checked that index matches HEAD already,
 		# so we know we do not have this path tracked.
 		# there may be an unrelated working tree file here,
-		# which we should just leave unmolested.
-		exit 0
+		# which we should just leave unmolested.  Make sure
+		# we do not have it in the index, though.
+		exec git update-index --remove -- "$4"
 	fi
 	if test -f "$4"; then
 		rm -f -- "$4" &&
@@ -42,7 +43,8 @@ case "${1:-.}${2:-.}${3:-.}" in
 #
 ".$2.")
 	# the other side did not add and we added so there is nothing
-	# to be done.
+	# to be done, except making the path merged.
+	exec git update-index --add --cacheinfo "$6" "$2" "$4"
 	;;
 "..$3")
 	echo "Adding $4"
@@ -50,7 +52,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 		echo "ERROR: untracked $4 is overwritten by the merge."
 		exit 1
 	}
-	git update-index --add --cacheinfo "$6$7" "$2$3" "$4" &&
+	git update-index --add --cacheinfo "$7" "$3" "$4" &&
 		exec git checkout-index -u -f -- "$4"
 	;;