Re: [PATCH v3 1/3] t7501: add merge conflict tests for dry run

[Junio C Hamano <gitster@pobox.com>]

Samuel Lijin <sxlijin@gmail.com> writes:

> The behavior of git commit when doing a dry run changes if there are
> unfixed/fixed merge conflits, but the test suite currently only asserts
> that `git commit --dry-run` succeeds when all merge conflicts are fixed.
>
> Add tests to document the behavior of all flags which imply a dry run
> when (1) there is at least one unfixed merge conflict and (2) when all
> merge conflicts are all fixed.

s/conflits/conflicts/
s/fixed/resolved/g	(both above and in the patch text)
s/unfixed/unresolved/g  (both above and in the patch text)

> Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
> ---
>  t/t7501-commit.sh | 45 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> index fa61b1a4e..be087e73f 100755
> --- a/t/t7501-commit.sh
> +++ b/t/t7501-commit.sh
> @@ -652,7 +652,8 @@ test_expect_success '--only works on to-be-born branch' '
>  	test_cmp expected actual
>  '
>  
> -test_expect_success '--dry-run with conflicts fixed from a merge' '
> +# set up env for tests of --dry-run given fixed/unfixed merge conflicts
> +test_expect_success 'setup env with unfixed merge conflicts' '
>  	# setup two branches with conflicting information
>  	# in the same file, resolve the conflict,
>  	# call commit with --dry-run
> @@ -665,11 +666,45 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
>  	git checkout -b branch-2 HEAD^1 &&
>  	echo "commit-2-state" >test-file &&
>  	git commit -m "commit 2" -i test-file &&
> -	! $(git merge --no-commit commit-1) &&
> -	echo "commit-2-state" >test-file &&
> +	test_expect_code 1 git merge --no-commit commit-1

The original is bad and also embarrassing.  Whatever comes out of
the standard output of "git merge" is $IFS split and executed as a
shell command (which likely results in "no such command" failure)
and it tries to make sure that a failure happens.

The right way to write that line (without your enhancement in this
patch) would have been:

	test_must_fail git merge --no-commit commit-1 &&

I doubt it is a good idea to hardcode exit status of 1 by using
test_expect_code, though.  "git merge --help" does not say anything
about "1 means this failure, 2 means that failure, 3 means that
other failure".  And my quick forward scan of this series does not
tell me that you are trying to declare that from here on we _will_
make that promise to the end users by carving the exit status(es) in
stone.  The same about "git commit"'s exit code in the following
four tests.

> +'
> +
> +test_expect_success '--dry-run with unfixed merge conflicts' '
> +	test_expect_code 1 git commit --dry-run
> +'
> +
> +test_expect_success '--short with unfixed merge conflicts' '
> +	test_expect_code 1 git commit --short
> +'
> +
> +test_expect_success '--porcelain with unfixed merge conflicts' '
> +	test_expect_code 1 git commit --porcelain
> +'
> +
> +test_expect_success '--long with unfixed merge conflicts' '
> +	test_expect_code 1 git commit --long
> +'
> +
> +test_expect_success '--dry-run with conflicts fixed from a merge' '
> +	echo "merge-conflicts-fixed" >test-file &&

The original test pretended that we resolved favouring the current
state with "commit-2-state" in the file, as if we ran "-s ours".
Is there a reason why we now use a different contents, or is this
just a change based on subjective preference?  

    Not saying that the latter is necessrily bad; just trying to
    understand why we are making this change.

>  	git add test-file &&
> -	git commit --dry-run &&
> -	git commit -m "conflicts fixed from merge."
> +	git commit --dry-run

OK, the original tried --dry-run to ensure it exited with 0 status
(i.e. have something to commit) and then did a commit to record the
updated state with a message.  You are checking only the dry-run
part, leaving the check of the final commit's status to another
test.

> +'
> +
> +test_expect_failure '--short with conflicts fixed from a merge' '
> +	git commit --short
> +'

With "test_expect_failure", you are saying that "--short" _should_
exit with 0 but currently it does not.  An untold expectation is
that even with the breakage with the exit code, the command still
honors the (implicit) --dry-run correctly and does not create a
new commit.

That was actually tested in the original.  By &&-chaining like this

	git commit --dry-run &&
	git commit -m "conflicts fixed from merge."

we would have noticed if a newly introduced bug caused the first
step "commit --dry-run" to return non-zero status (because then the
step would fail), or if it stopped being dry-run and made a commit
(because then the next step would fail with "nothing to commit").

But by splitting these into separate tests, the patch makes such a
potential failure with "git commit --short" break the later steps.

Not very nice.

It may be a better change to just do in the original one

	git add test-file &&
	git commit --dry-run &&
+	git commit --short &&
+	git commit --long &&
+	git commit --porcelain &&
	git commit -m "conflicts fixed from merge."

without adding these new and separate tests, and then mark that one
to expect a failure (because it would pass up to the --dry-run
commit, but the --short commit would fail) at this step, perhaps?

> +test_expect_failure '--porcelain with conflicts fixed from a merge' '
> +	git commit --porcelain
> +'
> +
> +test_expect_success '--long with conflicts fixed from a merge' '
> +	git commit --long
> +'
> +
> +test_expect_success '--message with conflicts fixed from a merge' '
> +	git commit --message "conflicts fixed from merge."
>  '
>  
>  test_done