patchdiffcore-rename: don't consider unmerged path as source

The output from 'git status' and 'git diff-index --cached -M' is broken when there are unmerged files as well as new files similar to the unmerged file (in stage 1).

When two copies have been created the output is:

$ git status -s
C  original -> exact-copy
R  original -> modified-copy
 U original

There are several errors here: 1) "original" has invalid status " U",
2) "modified-copy" is listed as a rename, even though its source
("original") still exists, and 3) "exact-copy" is detected as a copy,
even though 'git status' is only supposed to show renames

In the verbose 'git status' output, the unmerged path is completely missing. 'git diff-index --cached -M' show similar output.

Fix these problems by making diffcore-rename not consider unmerged paths as source for rename detection. For simplicty, don't consider the path independent of the type of merge conflict, even if the path is deleted on at least one side. Still consider unmerged paths as source for copy detection.

While at it, update the users of diff_options to use DIFF_DETECT_RENAME instead of directly using the value 1 to enable rename detection.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---

This is the first time I look at the diff code, so please review carefully. I think the changes make sense, but I really don't know the code enough to be sure.

Also not sure about the "while at it" stuff...

The test cases assume that the paths will be printed in a certain order. Can I rely on that?

 builtin/commit.c                    |    2 +-
 diffcore-rename.c                   |    7 +++-
 t/t7510-status-merge-rename-copy.sh |   68 +++++++++++++++++++++++++++++++++++
 wt-status.c                         |    6 ++--
 4 files changed, 77 insertions(+), 6 deletions(-)
 create mode 100755 t/t7510-status-merge-rename-copy.sh

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

I think the above actually sort-of makes sense. The first column of status output is about comparing the HEAD and the index, and the second column is about comparing the index and the working tree, but I'll begin by explaining the latter.

When comparing an unmerged index with the working tree, the comparison itself does not make much sense. When producing textual diff, "diff-index" tends to give the difference between stage #2 and the working tree in order to show the difference from "our" version and the result of the incomplete merge, but when we need to show the result concisely in the "status -s" output, the fact that the index is unmerged is more important than the incomplete merge result in the working tree is different from the original, so we show "U".

So I think "U" is perfectly good there.

About the comparison between HEAD and index, "original" in HEAD is copied to "exact-copy" in the index, and "modified-copy" in the index has a very similar contents as "original" in HEAD. It may be a bug that the latter is shown as "R" and I suspect that is because the code mistook the unmerged entry in the index as missing. Turning that "R" to "C" may be worth doing. Change the code that says "ah, the index is unmerged at this path, so treat it as if it is not there" to "if the unmerged path does not have stage #2 entry, it is missing".

The prose is good but if you illustrated a bug with a command output, please follow it up with another command output that you think is the right output. It becomes easier to point out potential flaws in the design and the thought behind it, like I just did above about "U".

I don't think anybody said "only supposed to show renames", but I suspect that the recent lt/diff-rename topic may affect this part of the output.

I don't think the part after "Still..." is justified enough.

For that matter, everything after "For simplicity..." is not justified enough. What are you sacrificing in return for that simplicity? "Ideally we should show X but because we don't consider these we end up getting Y but that still is better than what we get today which is Z" is missing.

Because "while at it" is by definition stuff that is not essential, don't do "white at it" if you are not sure, if it adds unnecessary noise and burden on reviewers.

On Thu, 17 Mar 2011, Junio C Hamano wrote:

According to git-status's man page, that is true for paths without merge conflicts only. For paths with merge conflicts, the two letters represent each side of the merge. That is why I thought " U" (note the SP) was invalid. Reading the documentation again, I see that ' ' actually means 'unmodified'. I'm still not sure it is correct though. First, "original" was modified on both sides in this case, which is of course the reason it had a merge conflict. Second, the meaning of " U" is not described in the table in the man page. I have always assumed that the table is supposed to be complete.

Either way, the fact that "original" is completetly missing from the verbose 'git status' output must surely be incorrect.

For completeness, I should say that I think the output from 'git diff-files' is correct (and unchanged by this patch); only 'git status' and 'git diff-index' showed incorrect (as far as I can see) output.

$ git diff-files --name-status U original M original

Same output with '-M' or '-C'.

Exactly. That is what this patch tries to do (although not for 'git status', but for 'git diff-index --cached -C', see later answers).

Speaking of stage #2, I think my references to stage #1 in the commit message should have been referring to stage #2.

Makes sense. This is the output after this patch:

$ git status -s A exact-copy A modified-copy UU original

wt-status.c sets rev.diffopt.detect_rename = 1 (== DIFF_DETECT_RENAME). I think that means that it should only detect rename, not copies. With the patch applies, the files show up as added (as show in above). I tried changing it to DIFF_DETECT_COPY and then, not surprisingly, 'git status' showed them as copies instead (even with the patch applied).

While on the subject, I almost asked why 'git status' does not detect copies, but I never did. I kind of suspected that it might be too slow. Could there be some other reason?

I wasn't really sure how it should work, so I just punted and said "For simplicity" :-). What I was not sure about was what stage to compare to. For example, for "deleted by us" conflicts, the most likely outcome is that the file will be deleted, so it could make some sense to make it eligible for rename detection. This would mean that if a similar file was added by "them", it would be described as a rename in the git-status output. The same applies to "deleted by us", but if I understand correctly, we don't have the stage #3 content available to compare to at this point in the code, and even if we did, it would be inconsistent with how the diff is done with stage #2 everywhere else.

I don't have time to think more about this right now, but I hope the above explains what I simplfied from. I'll think a bit more about this later and might get back with another mail.

I changed it because I thought it is slightly more clear that DIFF_DETECT_RENAME, as opposed to 1, does not detect renames. I could either explain that reason in the commit message, do it in a separate patch, or just drop it completely. I don't have a strong opinion. Which would you prefer?

Thanks for reviewing. I will try to include much of the above in the commit message in a re-roll.

/Martin

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
'

On Mon, 21 Mar 2011, Junio C Hamano wrote:

Thanks. I really appreciate it. My commit message did feel a little lazy and I didn't know the history.

Ah, of course. I did think this should go to maint, so it seems stupid not to have split it. Will do.

Sorry, I missed that and thanks for the help below. I have not had much time lately, but I will do my best to send a re-roll in not too long.

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.

Commit message and first two test cases by Junio, the rest by Martin.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
I think I mean something slightly different when I say "don't consider
unmerged path as source" than you do when you say "allow the valid
"one" side of an unmerged filepair to be considered a potential rename
source". I mean that they shouldn't show up on the LHS of a "copied: A
-> B" line in git-status output. I think you mean that they should be
registered as rename sources, even though the destination will always
be the file itself, so other files will only end up having status
COPIED. I didn't know how phrase the subject line in a better way, but
feel free to change it as you like.

 diffcore-rename.c   |    7 ++++-
 t/t7060-wtstatus.sh |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 2 deletions(-)