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

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

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

Show 39 quoted lines

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

Show 10 quoted lines

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