Re: [PATCH] t3404: fix use of "VAR=VAL cmd" with a shell function

From: Jeff King <peff@peff.net>
Date: Jul 12, 2018, 23:51 UTC
Message-ID: <20180712235156.GA10590@sigill.intra.peff.net>
In-Reply-To: <xmqqzhywurey.fsf@gitster-ct.c.googlers.com>

On Thu, Jul 12, 2018 at 01:31:49PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> >> ...would you want to use test_when_finished here (both for robustness,
> >> but also to make it more clear to a reader what's going on)?
> >
> > Perhaps.
> 
> Yes, but this one ends up to be overly ugly.
> 
> The restoreing of the AUTHOR_NAME must be done not just before this
> test_expect_success finishes and control goes on to the next test,
> but also before "git rebase -i" in the middle of this test that is
> expected to fail with conflict, as we want it to see the original
> author name (not the modified AttributeMe name).

OK, that is ugly.

Show 14 quoted lines

>  test_expect_success 'retain authorship w/ conflicts' '
> +	oGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&
> +	test_when_finished "GIT_AUTHOR_NAME=\$oGIT_AUTHOR_NAME" &&
> +
>  	git reset --hard twerp &&
>  	test_commit a conflict a conflict-a &&
>  	git reset --hard twerp &&
> -	GIT_AUTHOR_NAME=AttributeMe \
> +
> +	GIT_AUTHOR_NAME=AttributeMe &&
> +	export GIT_AUTHOR_NAME &&
>  	test_commit b conflict b conflict-b &&
> +	GIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&
> +

I'd actually go so far as to say that it is less ugly without the helper entirely, like:

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 352a52e59d..10d50650ae 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -267,8 +267,9 @@ test_expect_success 'retain authorship w/ conflicts' '
 	git reset --hard twerp &&
 	test_commit a conflict a conflict-a &&
 	git reset --hard twerp &&
-	GIT_AUTHOR_NAME=AttributeMe \
-	test_commit b conflict b conflict-b &&
+	echo b >conflict &&
+	git add conflict &&
+	git commit --author="AttributeMe <attr@example.com>" -m b &&
 	set_fake_editor &&
 	test_must_fail git rebase -i conflict-a &&
 	echo resolved >conflict &&

but it is probably not worth spending more brain cycles on this. Any of
the solutions is fine by me.

(I do agree that being able to automatically catch these with a linter
would be worth brain cycles, but I cannot immediately think a of a way
to do so).

-Peff