Re: [PATCH 1/2] Add tests for git-sh-setup's require_clean_work_tree()

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

SZEDER Gábor <szeder@ira.uka.de> writes:

> Add tests that check require_clean_work_tree() in the common cases,
> i.e. on a branch with all combinations of clean and dirty index and
> worktree, and also add tests that exercise it on an orphan branch.
>
> require_clean_work_tree()'s behavior in the orphan branch cases is
> questionable, as it exits with error on an orphan branch independently
> from whether the index and worktree are clean or dirty (and it does so
> because of the invalid HEAD, meaning that even when it should exit
> with error, it does so for the wrong reason).  As scripts might rely
> on the current behavior, we err on the side of compatibility and
> safety, and expect the current behavior as success.
>
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> ---

I see this was queued while I was not around, but has been in "Needs
review" bucket in "What's cooking" report forever, without getting
reviews.

> +test_description='require_clean_work_tree'
> +
> +. ./test-lib.sh
> +
> +run_require_clean_work_tree () {
> +	(
> +		. "$(git --exec-path)"/git-sh-setup &&
> +		require_clean_work_tree "do-something"

"do-something"?  What are the typical thing to write here in real
scripts?  A name of the operation that wanted to ensure that the
working tree is clean, with an optional hint string?

Perhaps you want to take them as arguments to this helper function?
That way you do not have to decide what to pass here, i.e.

-		require_clean_work_tree "do-something"
+		require_clean_work_tree "$@"

> +	)
> +}
> +
> +test_expect_success 'setup' '
> +	test_commit initial file
> +'
> +
> +test_expect_success 'success on clean index and worktree' '
> +	run_require_clean_work_tree

... and if you want to, you can give the comment here, e.g.

	run_require_clean_work_tree "verify clean after init"

or something.

> +'
> +
> +test_expect_success 'error on dirty worktree' '
> +	test_when_finished "git reset --hard" &&
> +	echo dirty >file &&
> +	test_must_fail run_require_clean_work_tree

I think this should be

	! run_require_clean_work_tree

> +'
> +
> +test_expect_success 'error on dirty index' '
> +	test_when_finished "git reset --hard" &&
> +	echo dirty >file &&
> +	git add file &&
> +	test_must_fail run_require_clean_work_tree
> +'

Likewise.

> +test_expect_success 'error on dirty index and worktree' '
> +	test_when_finished "git reset --hard" &&
> +	echo dirty >file &&
> +	git add file &&
> +	echo dirtier >file &&
> +	test_must_fail run_require_clean_work_tree
> +'
> +
> +test_expect_success 'error on clean index and worktree while on orphan branch' '
> +	test_when_finished "git checkout master" &&
> +	git checkout --orphan orphan &&
> +	git reset --hard &&
> +	test_must_fail run_require_clean_work_tree
> +'

The title is wrong.  Immediately after creating and getting on an
orphan branch, you have stuff in the index that is not committed to
the branch, so your index cannot be clean by definition.  The
contents of the working tree may or may not be clean immediately
after getting on a new orphan branch, but you are doing "reset
--hard" to make the index and the working tree agree, so this is
testing the "clean working tree" case, I think.

> +test_expect_success 'error on dirty index while on orphan branch' '
> +	test_when_finished "git checkout master" &&
> +	git checkout --orphan orphan &&
> +	test_must_fail run_require_clean_work_tree
> +'

Assuming that the previous test succeeded and this test started with
a clean index and the working tree, checkout --orphan would give you
a dirty-index with clean working tree state.  So both the title and
the expectation of the test are correct, I think.

But it would be better not to rely on the effect of
test-when-finished of the previous test too much.  "git checkout
master && git reset --hard" at the beginning would be easier to read
the test and reason about it in isolation.

> +test_expect_success 'error on dirty index and work tree while on orphan branch' '
> +	test_when_finished "git checkout master" &&
> +	git checkout --orphan orphan &&
> +	echo dirty >file &&
> +	test_must_fail run_require_clean_work_tree
> +'

Likewise.

Thanks.