Re: git-merge segfault in 1.6.6 and master

Junio C Hamano [off-list ref] writes:

When they are called with non-zero o->call_depth, they are supposed to
drop all the index entries that they handle down to stage #0 (even if the
path had contents level conflict).  For example, you see this bit in
process_entry():

	} else if (a_sha && b_sha) {
		/* Case C: Added in both (check for same permissions) and */
		/* case D: Modified in both, but differently. */
		const char *reason = "content";
		...
		mfi = merge_file(o, &one, &a, &b,
				 o->branch1, o->branch2);

		clean_merge = mfi.clean;
		if (!mfi.clean) {
			if (S_ISGITLINK(mfi.mode))
				reason = "submodule";
			output(o, 1, "CONFLICT (%s): Merge conflict in %s",
					reason, path);
		}
		update_file(o, mfi.clean, mfi.sha, mfi.mode, path);
	} ...

and update_file() eventually calls update_file_flags() to make sure that
the content in mfi.sha is at the stage #0 of path when o->call_depth is
non-zero (or mfi.clean is true).  process_renames() and process_entry()
are humongous functions that handle full of different cases, but all
codepaths must follow the rule not to leave non-stage #0 entries in the
index before merge_trees() function calls write_tree_from_memory().

We've fixed a similar bug in c94736a (merge-recursive: don't segfault
while handling rename clashes, 2009-07-30) and I think there were similar
breakages we fixed over time in the same area, but the two functions being
as huge as they are, I suspect you are hitting a codepath that hasn't been
fixed.

And there are.

For example, this (drop it as t9999-junk.sh in t/ directory, go there and
run "sh ./t-9999-junk.sh -v") shows one codepath that makes merge-recursive
fail to resolve the "common ancestor" merge.

-- -- store in t/t9999-junk.sh and run -- --
#!/bin/sh

test_description='common ancestor merge corner cases'

. ./test-lib.sh

test_expect_success 'setup' '
	mkdir D &&
	echo 1 >D/F1 &&
	echo 2 >D/F2 &&
	echo 3 >D/F3 &&
	echo 4 >D/F4 &&
	echo 5 >D/F5 &&
	echo 6 >D/F6 &&

	git add D &&
	test_tick &&
	git commit -m initial &&
	git branch side &&

	git checkout master &&
	git mv D/F1 D/M1 &&
	git rm D/F2 &&
	echo 7 >>D/F3 &&
	git mv D/F4 D/M4 &&
	git rm D/F5 &&
	mkdir D/F5 &&
	git mv D/F6 D/F5/M6 &&
	git add -u &&
	test_tick &&
	git commit -m master &&
	git tag A &&

	git checkout side &&
	git mv D/F1 D/S1 && # rename-rename conflict (dst)
	echo 8 >>D/F2 && # remove-modify conflict
	git mv D/F5 D/M4 && # rename-rename conflict (src)
	git add -u &&
	test_tick &&
	git commit -m side &&
	git tag B &&

	git checkout side &&
	test_tick &&
	git merge -s ours master &&
	git tag C &&

	git checkout master &&
	test_tick &&
	git merge -s ours B &&
	git tag D
'

test_expect_success 'criss-cross' '
	git checkout D &&
	test_must_fail git merge side
'

test_done
-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --

It dies after showing this (D/F5/M6 is left unresolved in the forced
"common ancestor" merge):

  Merging:
  f2f2f75 master
  7994826 side
  found 1 common ancestor(s):
  f561e36 initial
  CONFLICT (rename/rename): Rename "D/F1"->"D/M1" in branch "Temporary merge branch 1" rename "D/F1"->"D/S1" in "Temporary merge branch 2" (left unresolved)
  CONFLICT (rename/add): Rename D/F4->D/M4 in Temporary merge branch 1. D/M4 added in Temporary merge branch 2
  Adding merged D/M4
  Skipped D/M4 (merged same as existing)
  CONFLICT (rename/delete): Rename D/F5->D/M4 in Temporary merge branch 2 and deleted in Temporary merge branch 1
  Skipped D/F5/M6 (merged same as existing)
  CONFLICT (delete/modify): D/F2 deleted in Temporary merge branch 1 and modified in Temporary merge branch 2. Version Temporary merge branch 2 of D/F2 left in tree.
  There are unmerged index entries:
  2 D/F5/M6


The attached patch changes the behaviour to make this 9999-junk test pass,
but then it breaks t6036 (iow, the attached is _not_ a fix).

After I stared at the code for more than two hours, I gave up trying to
diagnose this by myself.  People more familiar with the merge-recursive
implementation might be able to help figuring this out and may prove my
suspicion wrong, but I have a feeling that without a fairly big rewrite
the code is unsalvageable.

-- >8 --
Not a fix

diff --git a/merge-recursive.c b/merge-recursive.c
index 1239647..132a6fc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -1052,8 +1052,8 @@ static int process_renames(struct merge_options *o,
 							update_stages(ren1_dst,
 								      one, a, b, 1);
 					}
-					update_file(o, mfi.clean, mfi.sha, mfi.mode, ren1_dst);
 				}
+				update_file(o, mfi.clean, mfi.sha, mfi.mode, ren1_dst);
 			}
 		}
 	}