{"thread":{"id":"48873","subject":"[PATCH] t3404: fix use of \"VAR=VAL cmd\" with a shell function","startedAt":"2018-07-12T20:07:55Z","lastAt":"2018-07-13T05:56:00Z","messageCount":6,"participants":["Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"352426","messageId":"xmqqefg8w73c.fsf@gitster-ct.c.googlers.com","threadId":"48873","inReplyTo":null,"subject":"[PATCH] t3404: fix use of \"VAR=VAL cmd\" with a shell function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T20:07:51Z","receivedAt":"2018-07-12T20:07:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bash may take it happily but running test with dash reveals a breakage.\n\nThis was not discovered for a long time as no tests after this test\ndepended on GIT_AUTHOR_NAME to be reverted correctly back to the\noriginal value after this step is done.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * We could enclose the setting and exporting inside a subshell and\n   do without the oGIT_AUTHOR_NAME temporary variable, but that\n   would interfere with the timestamp increments done by\n   test_commit, so I think doing it this way may be preferrable.\n\n t/t3404-rebase-interactive.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 7e9f375a24..fd43443ff5 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -280,8 +280,11 @@ test_expect_success 'retain authorship w/ conflicts' '\n \tgit reset --hard twerp &&\n \ttest_commit a conflict a conflict-a &&\n \tgit reset --hard twerp &&\n-\tGIT_AUTHOR_NAME=AttributeMe \\\n+\toGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&\n+\tGIT_AUTHOR_NAME=AttributeMe &&\n+\texport GIT_AUTHOR_NAME &&\n \ttest_commit b conflict b conflict-b &&\n+\tGIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&\n \tset_fake_editor &&\n \ttest_must_fail git rebase -i conflict-a &&\n \techo resolved >conflict &&\n"},{"id":"352429","messageId":"20180712201454.GA6281@sigill.intra.peff.net","threadId":"48873","inReplyTo":"xmqqefg8w73c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t3404: fix use of \"VAR=VAL cmd\" with a shell function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-12T20:14:54Z","receivedAt":"2018-07-12T20:14:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 12, 2018 at 01:07:51PM -0700, Junio C Hamano wrote:\n\n> Bash may take it happily but running test with dash reveals a breakage.\n> \n> This was not discovered for a long time as no tests after this test\n> depended on GIT_AUTHOR_NAME to be reverted correctly back to the\n> original value after this step is done.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> \n>  * We could enclose the setting and exporting inside a subshell and\n>    do without the oGIT_AUTHOR_NAME temporary variable, but that\n>    would interfere with the timestamp increments done by\n>    test_commit, so I think doing it this way may be preferrable.\n\nYeah, I agree that setting/unsetting is probably more sane for this\ncase. Though...\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 7e9f375a24..fd43443ff5 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -280,8 +280,11 @@ test_expect_success 'retain authorship w/ conflicts' '\n>  \tgit reset --hard twerp &&\n>  \ttest_commit a conflict a conflict-a &&\n>  \tgit reset --hard twerp &&\n> -\tGIT_AUTHOR_NAME=AttributeMe \\\n> +\toGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&\n> +\tGIT_AUTHOR_NAME=AttributeMe &&\n> +\texport GIT_AUTHOR_NAME &&\n>  \ttest_commit b conflict b conflict-b &&\n> +\tGIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&\n\n...would you want to use test_when_finished here (both for robustness,\nbut also to make it more clear to a reader what's going on)?\n\nIt's too bad test_commit does not take arbitrary options, as you could\njust use --author then, side-stepping the whole thing.\n\n-Peff\n"},{"id":"352431","messageId":"xmqq4lh4w6e8.fsf@gitster-ct.c.googlers.com","threadId":"48873","inReplyTo":"20180712201454.GA6281@sigill.intra.peff.net","subject":"Re: [PATCH] t3404: fix use of \"VAR=VAL cmd\" with a shell function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T20:22:55Z","receivedAt":"2018-07-12T20:22:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Jul 12, 2018 at 01:07:51PM -0700, Junio C Hamano wrote:\n>\n>> Bash may take it happily but running test with dash reveals a breakage.\n>> \n>> This was not discovered for a long time as no tests after this test\n>> depended on GIT_AUTHOR_NAME to be reverted correctly back to the\n>> original value after this step is done.\n>> \n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>> \n>>  * We could enclose the setting and exporting inside a subshell and\n>>    do without the oGIT_AUTHOR_NAME temporary variable, but that\n>>    would interfere with the timestamp increments done by\n>>    test_commit, so I think doing it this way may be preferrable.\n>\n> Yeah, I agree that setting/unsetting is probably more sane for this\n> case. Though...\n>\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 7e9f375a24..fd43443ff5 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -280,8 +280,11 @@ test_expect_success 'retain authorship w/ conflicts' '\n>>  \tgit reset --hard twerp &&\n>>  \ttest_commit a conflict a conflict-a &&\n>>  \tgit reset --hard twerp &&\n>> -\tGIT_AUTHOR_NAME=AttributeMe \\\n>> +\toGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&\n>> +\tGIT_AUTHOR_NAME=AttributeMe &&\n>> +\texport GIT_AUTHOR_NAME &&\n>>  \ttest_commit b conflict b conflict-b &&\n>> +\tGIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&\n>\n> ...would you want to use test_when_finished here (both for robustness,\n> but also to make it more clear to a reader what's going on)?\n\nPerhaps.\n\nI wish our test-lint caught \"VAR=VAL shellfunc\", but it is rather\nhard to do so, I would imagine.\n\n\n"},{"id":"352433","messageId":"xmqqzhywurey.fsf@gitster-ct.c.googlers.com","threadId":"48873","inReplyTo":"xmqq4lh4w6e8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t3404: fix use of \"VAR=VAL cmd\" with a shell function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T20:31:49Z","receivedAt":"2018-07-12T20:31:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>>> index 7e9f375a24..fd43443ff5 100755\n>>> --- a/t/t3404-rebase-interactive.sh\n>>> +++ b/t/t3404-rebase-interactive.sh\n>>> @@ -280,8 +280,11 @@ test_expect_success 'retain authorship w/ conflicts' '\n>>>  \tgit reset --hard twerp &&\n>>>  \ttest_commit a conflict a conflict-a &&\n>>>  \tgit reset --hard twerp &&\n>>> -\tGIT_AUTHOR_NAME=AttributeMe \\\n>>> +\toGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&\n>>> +\tGIT_AUTHOR_NAME=AttributeMe &&\n>>> +\texport GIT_AUTHOR_NAME &&\n>>>  \ttest_commit b conflict b conflict-b &&\n>>> +\tGIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&\n>>\n>> ...would you want to use test_when_finished here (both for robustness,\n>> but also to make it more clear to a reader what's going on)?\n>\n> Perhaps.\n\nYes, but this one ends up to be overly ugly.\n\nThe restoreing of the AUTHOR_NAME must be done not just before this\ntest_expect_success finishes and control goes on to the next test,\nbut also before \"git rebase -i\" in the middle of this test that is\nexpected to fail with conflict, as we want it to see the original\nauthor name (not the modified AttributeMe name).\n\n t/t3404-rebase-interactive.sh | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 481a350090..489b6196e0 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -256,11 +256,18 @@ test_expect_success 'retain authorship' '\n '\n \n test_expect_success 'retain authorship w/ conflicts' '\n+\toGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&\n+\ttest_when_finished \"GIT_AUTHOR_NAME=\\$oGIT_AUTHOR_NAME\" &&\n+\n \tgit reset --hard twerp &&\n \ttest_commit a conflict a conflict-a &&\n \tgit reset --hard twerp &&\n-\tGIT_AUTHOR_NAME=AttributeMe \\\n+\n+\tGIT_AUTHOR_NAME=AttributeMe &&\n+\texport GIT_AUTHOR_NAME &&\n \ttest_commit b conflict b conflict-b &&\n+\tGIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&\n+\n \tset_fake_editor &&\n \ttest_must_fail git rebase -i conflict-a &&\n \techo resolved >conflict &&\n"},{"id":"352442","messageId":"20180712235156.GA10590@sigill.intra.peff.net","threadId":"48873","inReplyTo":"xmqqzhywurey.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t3404: fix use of \"VAR=VAL cmd\" with a shell function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-12T23:51:56Z","receivedAt":"2018-07-12T23:52:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 12, 2018 at 01:31:49PM -0700, Junio C Hamano wrote:\n\n> >> ...would you want to use test_when_finished here (both for robustness,\n> >> but also to make it more clear to a reader what's going on)?\n> >\n> > Perhaps.\n> \n> Yes, but this one ends up to be overly ugly.\n> \n> The restoreing of the AUTHOR_NAME must be done not just before this\n> test_expect_success finishes and control goes on to the next test,\n> but also before \"git rebase -i\" in the middle of this test that is\n> expected to fail with conflict, as we want it to see the original\n> author name (not the modified AttributeMe name).\n\nOK, that is ugly.\n\n>  test_expect_success 'retain authorship w/ conflicts' '\n> +\toGIT_AUTHOR_NAME=$GIT_AUTHOR_NAME &&\n> +\ttest_when_finished \"GIT_AUTHOR_NAME=\\$oGIT_AUTHOR_NAME\" &&\n> +\n>  \tgit reset --hard twerp &&\n>  \ttest_commit a conflict a conflict-a &&\n>  \tgit reset --hard twerp &&\n> -\tGIT_AUTHOR_NAME=AttributeMe \\\n> +\n> +\tGIT_AUTHOR_NAME=AttributeMe &&\n> +\texport GIT_AUTHOR_NAME &&\n>  \ttest_commit b conflict b conflict-b &&\n> +\tGIT_AUTHOR_NAME=$oGIT_AUTHOR_NAME &&\n> +\n\nI'd actually go so far as to say that it is less ugly without the helper\nentirely, like:\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 352a52e59d..10d50650ae 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -267,8 +267,9 @@ test_expect_success 'retain authorship w/ conflicts' '\n \tgit reset --hard twerp &&\n \ttest_commit a conflict a conflict-a &&\n \tgit reset --hard twerp &&\n-\tGIT_AUTHOR_NAME=AttributeMe \\\n-\ttest_commit b conflict b conflict-b &&\n+\techo b >conflict &&\n+\tgit add conflict &&\n+\tgit commit --author=\"AttributeMe <attr@example.com>\" -m b &&\n \tset_fake_editor &&\n \ttest_must_fail git rebase -i conflict-a &&\n \techo resolved >conflict &&\n\nbut it is probably not worth spending more brain cycles on this. Any of\nthe solutions is fine by me.\n\n(I do agree that being able to automatically catch these with a linter\nwould be worth brain cycles, but I cannot immediately think a of a way\nto do so).\n\n-Peff\n"},{"id":"352452","messageId":"CAPig+cQXUP-5Rv+p2TuqavG=1HkO0qAZJRvLZcBkg=dB6gdZbw@mail.gmail.com","threadId":"48873","inReplyTo":"20180712235156.GA10590@sigill.intra.peff.net","subject":"Re: [PATCH] t3404: fix use of \"VAR=VAL cmd\" with a shell function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-13T05:55:47Z","receivedAt":"2018-07-13T05:56:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 12, 2018 at 7:52 PM Jeff King <peff@peff.net> wrote:\n> (I do agree that being able to automatically catch these with a linter\n> would be worth brain cycles, but I cannot immediately think a of a way\n> to do so).\n\nPerhaps something like this[1]?\n\n[1]: https://public-inbox.org/git/20180713055205.32351-1-sunshine@sunshineco.com/T/\n"}]}