* [PATCH] t3404: fix use of "VAR=VAL cmd" with a shell function
@ 2018-07-12 20:07 Junio C Hamano
2018-07-12 20:14 ` Jeff King
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2018-07-12 20:07 UTC (permalink / raw)
To: git, Johannes Schindelin
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(-)
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 &&
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] t3404: fix use of "VAR=VAL cmd" with a shell function
2018-07-12 20:07 [PATCH] t3404: fix use of "VAR=VAL cmd" with a shell function Junio C Hamano
@ 2018-07-12 20:14 ` Jeff King
2018-07-12 20:22 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2018-07-12 20:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin
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)?
It's too bad test_commit does not take arbitrary options, as you could
just use --author then, side-stepping the whole thing.
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] t3404: fix use of "VAR=VAL cmd" with a shell function
2018-07-12 20:14 ` Jeff King
@ 2018-07-12 20:22 ` Junio C Hamano
2018-07-12 20:31 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2018-07-12 20:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, Johannes Schindelin
Jeff King <peff@peff.net> writes:
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] t3404: fix use of "VAR=VAL cmd" with a shell function
2018-07-12 20:22 ` Junio C Hamano
@ 2018-07-12 20:31 ` Junio C Hamano
2018-07-12 23:51 ` Jeff King
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2018-07-12 20:31 UTC (permalink / raw)
To: Jeff King; +Cc: git, Johannes Schindelin
Junio C Hamano <gitster@pobox.com> writes:
>>> 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(-)
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 &&
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] t3404: fix use of "VAR=VAL cmd" with a shell function
2018-07-12 20:31 ` Junio C Hamano
@ 2018-07-12 23:51 ` Jeff King
2018-07-13 5:55 ` Eric Sunshine
0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2018-07-12 23:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin
On Thu, Jul 12, 2018 at 01:31:49PM -0700, Junio C Hamano wrote:
> >> ...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.
> 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-13 5:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-12 20:07 [PATCH] t3404: fix use of "VAR=VAL cmd" with a shell function Junio C Hamano
2018-07-12 20:14 ` Jeff King
2018-07-12 20:22 ` Junio C Hamano
2018-07-12 20:31 ` Junio C Hamano
2018-07-12 23:51 ` Jeff King
2018-07-13 5:55 ` Eric Sunshine
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).