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

[Junio C Hamano <gitster@pobox.com>]

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

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