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

[Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>]

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

> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> 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
> '
>