Martin von Zweigbergk <martin.von.zweigbergk@xxxxxxxxx> writes:
> 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?
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
'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html