On Mon, 21 Mar 2011, Junio C Hamano wrote:
> 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.
Thanks. I really appreciate it. My commit message did feel a little
lazy and I didn't know the history.
> 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.
Ah, of course. I did think this should go to maint, so it seems stupid
not to have split it. Will do.
> Also, please don't add a new test script 7510 that conflicts what is
> already in flight ('pu' has t/t7510-commit-notes.sh).
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.
> 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