threads / patch / 48873

patcht3404: fix use of "VAR=VAL cmd" with a shell function

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

## tl;dr

6 messages between Jul 12, 2018 and Jul 13, 2018. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Junio C Hamano· Jul 12, 2018, 20:07 UTC · lore
Bash may take it happily but running test with dash reveals a breakage.

This was not discovered for a long time as no tests after this test depended on GIT_AUTHOR_NAME to be reverted correctly back to the original value after this step is done.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * We could enclose the setting and exporting inside a subshell and
   do without the oGIT_AUTHOR_NAME temporary variable, but that
   would interfere with the timestamp increments done by
   test_commit, so I think doing it this way may be preferrable.
 t/t3404-rebase-interactive.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Show changes to t/t3404-rebase-interactive.sh +4 −1
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 7e9f375a24..fd43443ff5 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -280,8 +280,11 @@ 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 \
+	oGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&
+	GIT_AUTHOR_NAME=AttributeMe &&
+	export GIT_AUTHOR_NAME &&
 	test_commit b conflict b conflict-b &&
+	GIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&
 	set_fake_editor &&
 	test_must_fail git rebase -i conflict-a &&
 	echo resolved >conflict &&
Jeff King· Jul 12, 2018, 20:14 UTC · re: Junio C Hamano · lore

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

On Thu, Jul 12, 2018 at 01:07:51PM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> Bash may take it happily but running test with dash reveals a breakage.
> 
> This was not discovered for a long time as no tests after this test
> depended on GIT_AUTHOR_NAME to be reverted correctly back to the
> original value after this step is done.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * We could enclose the setting and exporting inside a subshell and
>    do without the oGIT_AUTHOR_NAME temporary variable, but that
>    would interfere with the timestamp increments done by
>    test_commit, so I think doing it this way may be preferrable.

Yeah, I agree that setting/unsetting is probably more sane for this case. Though...

Show 14 quoted lines
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 7e9f375a24..fd43443ff5 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -280,8 +280,11 @@ 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 \
> +	oGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&
> +	GIT_AUTHOR_NAME=AttributeMe &&
> +	export GIT_AUTHOR_NAME &&
>  	test_commit b conflict b conflict-b &&
> +	GIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&

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

It's too bad test_commit does not take arbitrary options, as you could just use --author then, side-stepping the whole thing.

-Peff
Junio C Hamano· Jul 12, 2018, 20:22 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 36 quoted lines
> On Thu, Jul 12, 2018 at 01:07:51PM -0700, Junio C Hamano wrote:
>
>> Bash may take it happily but running test with dash reveals a breakage.
>> 
>> This was not discovered for a long time as no tests after this test
>> depended on GIT_AUTHOR_NAME to be reverted correctly back to the
>> original value after this step is done.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> 
>>  * We could enclose the setting and exporting inside a subshell and
>>    do without the oGIT_AUTHOR_NAME temporary variable, but that
>>    would interfere with the timestamp increments done by
>>    test_commit, so I think doing it this way may be preferrable.
>
> Yeah, I agree that setting/unsetting is probably more sane for this
> case. Though...
>
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 7e9f375a24..fd43443ff5 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -280,8 +280,11 @@ 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 \
>> +	oGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&
>> +	GIT_AUTHOR_NAME=AttributeMe &&
>> +	export GIT_AUTHOR_NAME &&
>>  	test_commit b conflict b conflict-b &&
>> +	GIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&
>
> ...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.

I wish our test-lint caught "VAR=VAL shellfunc", but it is rather hard to do so, I would imagine.

Junio C Hamano· Jul 12, 2018, 20:31 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
>>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>>> index 7e9f375a24..fd43443ff5 100755
>>> --- a/t/t3404-rebase-interactive.sh
>>> +++ b/t/t3404-rebase-interactive.sh
>>> @@ -280,8 +280,11 @@ 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 \
>>> +	oGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&
>>> +	GIT_AUTHOR_NAME=AttributeMe &&
>>> +	export GIT_AUTHOR_NAME &&
>>>  	test_commit b conflict b conflict-b &&
>>> +	GIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&
>>
>> ...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).

 t/t3404-rebase-interactive.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
Show changes to t/t3404-rebase-interactive.sh +8 −1
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 481a350090..489b6196e0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -256,11 +256,18 @@ test_expect_success 'retain authorship' '
 '
 
 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 &&
+
 	set_fake_editor &&
 	test_must_fail git rebase -i conflict-a &&
 	echo resolved >conflict &&
Jeff King· Jul 12, 2018, 23:51 UTC · re: Junio C Hamano · lore

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

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:

Show changes to t/t3404-rebase-interactive.sh +3 −3
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
Eric Sunshine· Jul 13, 2018, 05:55 UTC · re: Jeff King · lore

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

On Thu, Jul 12, 2018 at 7:52 PM Jeff King <peff@peff.net> wrote:
> (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).
Perhaps something like this[1]?
[1]: https://public-inbox.org/git/20180713055205.32351-1-sunshine@sunshineco.com/T/

← back to recent threads