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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 20, 2016, 23:59 UTC
Message-ID: <xmqqbn8f3iq5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1448376345-27339-1-git-send-email-szeder@ira.uka.de>

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

Show 14 quoted lines

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

Show 8 quoted lines

> +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 "$@"

Show 9 quoted lines

> +	)
> +}
> +
> +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.

Show 6 quoted lines

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

Show 8 quoted lines

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

Show 14 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

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