Re: [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity

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

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

>  git-stash.sh     | 17 +++++++++++++++++
>  t/t3903-stash.sh |  2 +-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 94793c1a9..789ce2f41 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -55,6 +55,20 @@ untracked_files () {
>  	git ls-files -o $z $excl_opt -- "$@"
>  }
>  
> +prepare_fallback_ident () {
> +	if ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null 2>&1
> +	then
> +		GIT_AUTHOR_NAME="git stash"
> +		GIT_AUTHOR_EMAIL=git@stash
> +		GIT_COMMITTER_NAME="git stash"
> +		GIT_COMMITTER_EMAIL=git@stash
> +		export GIT_AUTHOR_NAME
> +		export GIT_AUTHOR_EMAIL
> +		export GIT_COMMITTER_NAME
> +		export GIT_COMMITTER_EMAIL
> +	fi
> +}
> +
>  clear_stash () {
>  	if test $# != 0
>  	then
> @@ -67,6 +81,9 @@ clear_stash () {
>  }
>  
>  create_stash () {
> +
> +	prepare_fallback_ident
> +
>  	stash_msg=
>  	untracked=
>  	while test $# != 0

That looks like a sensible implementation to me.

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index bab8bec67..0b0814421 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1096,7 +1096,7 @@ test_expect_success 'stash -- <subdir> works with binary files' '
>  	test_path_is_file subdir/untracked
>  '
>  
> -test_expect_failure 'stash works when user.name and user.email are not set' '
> +test_expect_success 'stash works when user.name and user.email are not set' '

This line claims to the readers of patch that the known breakage
this known test piece demonstrated has been corrected, but they need
to refresh their memory by going back to the previous patch to see
if this "failure-to-success" flipping is done to the right test
piece, and what exactly the test piece tested to see the existing
breakage, because all the interesting part of the test are chomped
outside the post-context of this hunk.

Unless the fix is fairly complex, adding ought-to-succeed tests that
expect success that break when the code change gets omitted from the
patch in the same patch as the fix itself (i.e. squash patch 1/2 and
patch 2/2 into a single patch) would be more helpful for the readers
(it also helps cherry-picking the fix later to earlier maintenance
tracks if it becomes necessary).

>  	git reset &&
>  	git var GIT_COMMITTER_IDENT >expected &&
>  	>1 &&