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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 17, 2018, 17:05 UTC
Message-ID: <xmqq1sc1rdvz.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180715110807.25544-2-sxlijin@gmail.com>

Samuel Lijin <sxlijin@gmail.com> writes:

Show 7 quoted lines

> 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)

Show 26 quoted lines

> 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.

Show 20 quoted lines

> +'
> +
> +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.

Show 5 quoted lines

> +'
> +
> +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?

Show 13 quoted lines

> +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