Re: [PATCH 1/4] Add test for showing discarded changes with diff --cc

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 2, 2015, 20:55 UTC
Message-ID: <xmqqy4mai71d.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1428006853-21212-2-git-send-email-max@max630.net>

Max Kirillov <max@max630.net> writes:

Show 16 quoted lines

> If a hunk has been changed in both branches and conflict resolution
> fully takes one of branches, discarding all changes that are in the
> other, then the merge is not shown in 3-way diff which git uses by
> default.
>
> The advice is to use flag --cc and and explicitly add the mergebase was
> given in $gmane/191557. It was discovered though that it does not always
> work. If the whole file has not changed at all compared to one of
> branches then no difference is shown for this file.
>
> This looks inconsistent and for particular scenario of viewing discarded
> changes is undesirable.
>
> Add the test which demonstrates the issue.
>
> Signed-off-by: Max Kirillov <max@max630.net>

Thanks. I will not have time to review this properly at the moment while preparing 2.4-rc1, and I do not want to spend time figuring out if the parent culling you are chaning was done deliberately (in which case this patch may be breaking things) or not.

So I'll give a preliminary nitpicks first ;-)

>> Subject: Re: [PATCH 1/4] Add test for showing discarded changes with diff --cc

Subject: t4059: test 'diff --cc' with a change from only few parents

or something? "<area>: <what you did>" without capitalization of the beginning of what you did and the full-stop at the end.

Show 26 quoted lines

> ---
>  t/t4059-diff-merge-with-base.sh | 100 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
>  create mode 100755 t/t4059-diff-merge-with-base.sh
>
> diff --git a/t/t4059-diff-merge-with-base.sh b/t/t4059-diff-merge-with-base.sh
> new file mode 100755
> index 0000000..e650a33
> --- /dev/null
> +++ b/t/t4059-diff-merge-with-base.sh
> @@ -0,0 +1,100 @@
> +#!/bin/sh
> +
> +test_description='Diff aware of merge base'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	mkdir short &&
> +	mkdir long &&
> +	for fn in win1 win2 merge delete base only1 only2; do
> +		test_seq 3 >short/$fn
> +		git add short/$fn &&
> +		test_seq 11 >long/$fn &&
> +		git add long/$fn
> +	done &&

Two nits:

 - Style: have "do" on its own line.  A good rule of thumb is that
   you shouldn't have to type ';' when you are writing a shell
   script, unless you are terminating a case arm with ';;'.

	for fn in ...
        do
        	...
	done

 - Correctness: aside from missing && after "test_seq 3", if the
   last step "git add long/$fn" failed in an earlier iteration, you
   would not notice any breakage.  Either

	(
                for fn in ...
                do
                        ... &&
                        ... &&
                        ... || exit $?
                done
	)

or

	for fn in ...
        do
        	... &&
                ... &&
                ... || return $?
	done

    The latter is only valid in our test scripts, where the test
    framework gives you an artificial "caller" of the "subroutine"
    you are writing as the test body.

Show 20 quoted lines

> +	test_seq 11 | sed -e "s/^7/7change1/" -e "s/^11/11change2/" >long/base &&
> +	git add long/base &&
> + test_seq 11 | sed -e "s/^7/7change1/" -e "s/^11/11change2/" -e
> "s/^2/2change1/" >long/win1 &&
> +	git add long/win1 &&
> + test_seq 11 | sed -e "s/^7/7change1/" -e "s/^11/11change2/" -e
> "s/^2/2change2/" >long/win2 &&
> +	git add long/win2 &&
> + test_seq 11 | sed -e "s/^7/7change1/" -e "s/^11/11change2/" -e
> "s/^2/2merged/" >long/merge &&
> +	git add long/merge &&
> + test_seq 11 | sed -e "s/^7/7change1/" -e "s/^11/11change2/" -e
> "/^2/d" >long/delete &&
> +	git add long/delete &&
> + test_seq 11 | sed -e "s/^7/7change1/" -e "s/^11/11change2/" -e
> "s/^2/2change1/" >long/only1 &&
> +	git add long/only1 &&
> + test_seq 11 | sed -e "s/^7/7change1/" -e "s/^11/11change2/" -e
> "s/^2/2change2/" >long/only2 &&
> +	git add long/only2 &&

Patch is line-wrapped around here?