Re: [PATCH v4 1/9] checkout tests: index should be clean after dwim checkout

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2018, 04:06 UTC
Message-ID: <xmqqwovj882f.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180531195252.29173-2-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 9 quoted lines

> Assert that whenever there's a DWIM checkout that the index should be
> clean afterwards, in addition to the correct branch being checked-out.
> ...
> So let's amend the tests (mostly added in) 399e4a1c56 ("t2024: Add
> tests verifying current DWIM behavior of 'git checkout <branch>'",
> 2013-04-21) to always assert that "status" is clean after we run
> "checkout", that's being done with "-uno" because there's going to be
> some untracked files related to the test itself which we don't care
> about.

It might not be absolutely necessary to state, but it would be helpful to say that you are assuming to start a checkout (DWIM or otherwise) from a clean state; without the assumption, the readers need to think for a few breaths why "the index should be clean" is true.

The intention and the implementation of the change both mostly look good to me from a quick read.

Show 20 quoted lines

>  test_expect_success 'setup' '
>  	test_commit my_master &&
>  	git init repo_a &&
> @@ -55,6 +61,7 @@ test_expect_success 'checkout of non-existing branch fails' '
>  	test_might_fail git branch -D xyzzy &&
>  
>  	test_must_fail git checkout xyzzy &&
> +	status_uno_is_clean &&
>  	test_must_fail git rev-parse --verify refs/heads/xyzzy &&
>  	test_branch master
>  '
> @@ -64,8 +71,10 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
>  	test_might_fail git branch -D foo &&
>  
>  	test_must_fail git checkout foo &&
> +	status_uno_is_clean &&
>  	test_must_fail git rev-parse --verify refs/heads/foo &&
> -	test_branch master
> +	test_branch master &&
> +	status_uno_is_clean

Hmm, what's the point of this second one?

>  '