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

[Junio C Hamano]

Junio C Hamano  writes:

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

Of course, if you want to be more thorough, anticipating that other
people in their future updates may break --short but not --long or
--porcelain, testing each option in separate test_expect_success is
a necessary way to do so, but then you'd need to actually be more
thorough, by not merely running each of them in separate
test_expect_success block but also arranging that each of them start
in an expected state to try the thing we want it to try.  That is

for opt in --dry-run --short --long --porcelain
do
test_expect_success "commit $opt" '
set up the conflicted state after merge &&
git commit $opt
'
done

where the "set up the state" part makes sure it can tolerate
potential mistakes of previous run of "git commit $opt" (e.g. it
by mistake made a commit, making the index identical to HEAD and
taking us out of "merge in progress" state).

But from your 1/3 I did not get the impression that you particularly
want to be more thorough, and from your 3/3 I did not get the
impression that you anticipate --short/--long/--porcelain may get
broken independently.  And if that is the case, then chaining all of
them together like the above is a more honest way to express that we
are only doing a minimum set of testing.

Thanks.

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

[Junio C Hamano]

Samuel Lijin  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 
> ---
>  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