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

[Junio C Hamano <gitster@pobox.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.

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

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