Re: [PATCH] diffcore-rename: don't consider unmerged path as source

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

I re-read the patch and found that the core of the patch, i.e. the change to diffcore-rename.c, is basically sound.

I'd like to see the commit log message to describe the cause of the breakage better. Perhaps something like this:

  Since e9c8409 (diff-index --cached --raw: show tree entry on the LHS for
  unmerged entries., 2007-01-05), an unmerged entry should be detected by
  using DIFF_PAIR_UNMERGED(p), not by noticing both one and two sides of
  the filepair records mode=0 entries. However, it forgot to update some
  parts of the rename detection logic.

  This only makes difference in the "diff --cached" codepath where an
  unmerged filepair carries information on the entries that came from the
  tree.  It probably hasn't been noticed for a long time because nobody
  would run "diff -M" during a conflict resolution, but "git status" uses
  rename detection when it internally runs "diff-index" and "diff-files"
  and gives nonsense results.

  In an unmerged pair, "one" side can have a valid filespec to record the
  tree entry (e.g. what's in HEAD) when running "diff --cached". This can
  be used as a rename source to other paths in the index that are not
  unmerged. The path that is unmerged by definition does not have the
  final content yet (i.e. "two" side cannot have a valid filespec), so it
  can never be a rename destination.

  Use the DIFF_PAIR_UNMERGED() to detect unmerged filepair correctly, and
  allow the valid "one" side of an unmerged filepair to be considered a
  potential rename source, but never to be considered a rename destination.

Please split changes to wt-status.c (indentation and symbolic constant) and builtin/commit.c (symbolic constant) to a single commit that is separate from this fix, as we would want to backport the fix to older maintenance tracks.

Also, please don't add a new test script 7510 that conflicts what is already in flight ('pu' has t/t7510-commit-notes.sh). Instead, tack something like the following (and your diff-index tests) at the end of t/t7060-wtstatus.sh, as you should be able to do this without actually running a merge.

Answering the last question in the comment part of your message, the paths are supposed to be shown in the name order, so your comparison (and the comparison below) should be the right thing to do.

Thanks.

test_expect_success 'rename & unmerged setup' '
	git rm -f -r . &&
	cat "$TEST_DIRECTORY/README" >ONE &&
	git add ONE &&
	test_tick &&
	git commit -m "One commit with ONE" &&

	echo Modified >TWO &&
	cat ONE >>TWO &&
	cat ONE >>THREE &&
	git add TWO THREE &&
	sha1=$(git rev-parse :ONE) &&
	git rm --cached ONE &&
	(
		echo "100644 $sha1 1	ONE" &&
		echo "100644 $sha1 2	ONE" &&
		echo "100644 $sha1 3	ONE"
	) | git update-index --index-info &&
	echo Further >>THREE
'

test_expect_success 'rename & unmerged status' '
	git status -suno >actual &&
	cat >expect <<-EOF &&
	UU ONE
	AM THREE
	A  TWO
	EOF
	test_cmp expect actual
'