Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2018, 05:55 UTC
Message-ID: <xmqqsh01k1mr.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181114222524.2624-1-slawica92@hotmail.com>

Slavica Djukic <slavicadj.ip2018@gmail.com> writes:

> +test_expect_failure 'stash works when user.name and user.email are not set' '
> +	git reset &&
> +	git var GIT_COMMITTER_IDENT >expected &&

All the other existing test pieces in this file calls the expected result "expect"; is there a reason why this patch needs to be different (e.g. 'expect' file left by the earlier step needs to be kept unmodified for later use, or something like that)? If not, please avoid making a difference in irrelevant details, as that would waste time of readers by forcing them to guess if there is such a reason that readers cannot immediately see.

Anyway, we grab the committer ident we use by default during the test with this command. OK.

> +	>1 &&
> +	git add 1 &&
> +	git stash &&

And we make sure we can create stash.

> +	git var GIT_COMMITTER_IDENT >actual &&
> +	test_cmp expected actual &&

I am not sure what you are testing with this step. There is nothing that changed environment variables or configuration since we ran "git var" above. Why does this test suspect that somebody in the future may break the expectation that after running 'git add' and/or 'git stash', our committer identity may have been changed, and how would such a breakage happen?

> +	>2 &&
> +	git add 2 &&
> +	test_config user.useconfigonly true &&
> +	test_config stash.usebuiltin true &&

Now we start using use-config-only, so unsetting environment variables will cause trouble when Git insists on having an explicitly configured identities. Makes sense.

Show 7 quoted lines

> +	(
> +		sane_unset GIT_AUTHOR_NAME &&
> +		sane_unset GIT_AUTHOR_EMAIL &&
> +		sane_unset GIT_COMMITTER_NAME &&
> +		sane_unset GIT_COMMITTER_EMAIL &&
> +		test_unconfig user.email &&
> +		test_unconfig user.name &&

And then we try the same test, but without environment or config. Since we are unsetting the environment, in order to be nice for future test writers, we do this in a subshell, so that we do not have to restore the original values of environment variables.

Don't we need to be nice the same way for configuration variables, though? We _know_ that nobody sets user.{email,name} config up to this point in the test sequence, so that is why we do not do a "save before test and then restore to the original" dance on them. Even though we are relying on the fact that these two variables are left unset in the configuration file, we unconfig them here anyway, and I do think it is a good idea for documentation purposes (i.e. we are not documenting what we assume the config before running this test would be; we are documenting what state we want these two variables are in when running this "git stash"---that is, they are both unset).

So these later part of this test piece makes sense. I still do not know what you wanted to check in the earlier part of the test, though.

Show 5 quoted lines

> +		git stash
> +	)
> +'
> +
>  test_done