Samuel Lijin <sxlijin@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
