{"thread":{"id":"61171","subject":"[PATCH 0/3] t7800: fix quoting of looped test bodies","startedAt":"2024-03-21T13:47:28Z","lastAt":"2024-03-22T14:37:46Z","messageCount":13,"participants":["Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"491141","messageId":"cover.1711028473.git.ps@pks.im","threadId":"61171","inReplyTo":null,"subject":"[PATCH 0/3] t7800: fix quoting of looped test bodies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T13:47:20Z","receivedAt":"2024-03-21T13:47:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nSZEDER made me aware [1] of a quoting issue with some test refactorings\nin t7800 I recently introduced when refactoring the tests to run in a\nloop. This patch series fixes those issues and also adds a paragraph to\n\"t/README\" to document how this is supposed to be done properly.\n\nThanks!\n\nPatrick\n\n[1]: https://lore.kernel.org/git/20240308221229.GA1908@szeder.dev/\n\nPatrick Steinhardt (3):\n  t7800: improve test descriptions with empty arguments\n  t7800: use single quotes for test bodies\n  t/README: document how to loop around test cases\n\n t/README            | 19 +++++++++++++++++++\n t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------\n 2 files changed, 39 insertions(+), 20 deletions(-)\n\n-- \n2.44.0\n\n"},{"id":"491142","messageId":"fd37c2931904bc64fb6ea14331aea60278f495eb.1711028473.git.ps@pks.im","threadId":"61171","inReplyTo":"cover.1711028473.git.ps@pks.im","subject":"[PATCH 1/3] t7800: improve test descriptions with empty arguments","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T13:47:24Z","receivedAt":"2024-03-21T13:47:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Some of the tests in t7800 are executed repeatedly in a loop with\ndifferent arguments. To distinguish these tests, the value of that\nvariable is rendered into the test title. But given that one of the\nvalues is the empty string, it results in a somewhat awkward test name:\n\n    difftool  ignores exit code\n\nImprove this by printing \"without options\" in case the value is empty.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t7800-difftool.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 96ae5d5880..80bf108f54 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -93,40 +93,40 @@ test_expect_success 'difftool forwards arguments to diff' '\n \n for opt in '' '--dir-diff'\n do\n-\ttest_expect_success \"difftool ${opt} ignores exit code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} ignores exit code\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\tgit difftool ${opt} -y -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code for built-ins\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins\" \"\n \t\ttest_config difftool.vimdiff.path false &&\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = true\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = true\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode true &&\n \t\ttest_must_fail git difftool ${opt} -y -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = false\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = false\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode false &&\n \t\tgit difftool ${opt} -y -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} ignores exit code with --no-trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} ignores exit code with --no-trust-exit-code\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode true &&\n \t\tgit difftool ${opt} -y --no-trust-exit-code -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} stops on error with --trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} stops on error with --trust-exit-code\" \"\n \t\ttest_when_finished 'rm -f for-diff .git/fail-right-file' &&\n \t\ttest_when_finished 'git reset -- for-diff' &&\n \t\twrite_script .git/fail-right-file <<-\\EOF &&\n@@ -140,7 +140,7 @@ do\n \t\ttest_line_count = 1 actual\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} honors exit status if command not found\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors exit status if command not found\" \"\n \t\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n \t\ttest_config difftool.trustExitCode false &&\n \t\tif test \"${opt}\" = '--dir-diff'\n-- \n2.44.0\n\n"},{"id":"491143","messageId":"a4ca974397aadef251c2317efa88fd9c03e8f195.1711028473.git.ps@pks.im","threadId":"61171","inReplyTo":"cover.1711028473.git.ps@pks.im","subject":"[PATCH 2/3] t7800: use single quotes for test bodies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T13:47:28Z","receivedAt":"2024-03-21T13:47:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In eb84c8b6ce (git-difftool--helper: honor `--trust-exit-code` with\n`--dir-diff`, 2024-02-20) we have started to loop around some of the\ntests in t7800 so that they are reexecuted with slightly different\narguments. As part of that refactoring the quoting of test bodies was\nchanged from single quotes (') to double quotes (\") so that the value of\nthe loop variable is accessible to the body.\n\nAs the test body is later on passed to eval this change was not required\nthough. Let's revert it back to use single quotes as usual in our tests.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 80bf108f54..cc917b257e 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -93,42 +93,42 @@ test_expect_success 'difftool forwards arguments to diff' '\n \n for opt in '' '--dir-diff'\n do\n-\ttest_expect_success \"difftool ${opt:-without options} ignores exit code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} ignores exit code\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\tgit difftool ${opt} -y -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins\" '\n \t\ttest_config difftool.vimdiff.path false &&\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = true\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = true\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode true &&\n \t\ttest_must_fail git difftool ${opt} -y -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = false\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = false\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode false &&\n \t\tgit difftool ${opt} -y -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} ignores exit code with --no-trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} ignores exit code with --no-trust-exit-code\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode true &&\n \t\tgit difftool ${opt} -y --no-trust-exit-code -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} stops on error with --trust-exit-code\" \"\n-\t\ttest_when_finished 'rm -f for-diff .git/fail-right-file' &&\n-\t\ttest_when_finished 'git reset -- for-diff' &&\n+\ttest_expect_success \"difftool ${opt:-without options} stops on error with --trust-exit-code\" '\n+\t\ttest_when_finished \"rm -f for-diff .git/fail-right-file\" &&\n+\t\ttest_when_finished \"git reset -- for-diff\" &&\n \t\twrite_script .git/fail-right-file <<-\\EOF &&\n \t\techo failed\n \t\texit 1\n@@ -138,19 +138,19 @@ do\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code \\\n \t\t\t--extcmd .git/fail-right-file branch >actual &&\n \t\ttest_line_count = 1 actual\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} honors exit status if command not found\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors exit status if command not found\" '\n \t\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n \t\ttest_config difftool.trustExitCode false &&\n-\t\tif test \"${opt}\" = '--dir-diff'\n+\t\tif test \"${opt}\" = --dir-diff\n \t\tthen\n \t\t\texpected_code=127\n \t\telse\n \t\t\texpected_code=128\n \t\tfi &&\n-\t\ttest_expect_code \\${expected_code} git difftool ${opt} -y -t nonexistent branch\n-\t\"\n+\t\ttest_expect_code ${expected_code} git difftool ${opt} -y -t nonexistent branch\n+\t'\n done\n \n test_expect_success 'difftool honors --gui' '\n-- \n2.44.0\n\n"},{"id":"491144","messageId":"326fb7965036fccc1c23ad02f200251f6388e6dd.1711028473.git.ps@pks.im","threadId":"61171","inReplyTo":"cover.1711028473.git.ps@pks.im","subject":"[PATCH 3/3] t/README: document how to loop around test cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T13:47:33Z","receivedAt":"2024-03-21T13:47:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In some cases it makes sense to loop around test cases so that we can\nexecute the same test with slightly different arguments. There are some\ngotchas around quoting here though that are easy to miss and that may\nlead to easy-to-miss errors and portability issues.\n\nDocument the proper way to do this in \"t/README\".\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/README | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 36463d0742..d56401a254 100644\n--- a/t/README\n+++ b/t/README\n@@ -721,6 +721,25 @@ The \"do's:\"\n    Note that we still &&-chain the loop to propagate failures from\n    earlier commands.\n \n+ - Repeat tests with slightly different arguments in a loop.\n+\n+   In some cases it may make sense to re-run the same set of tests with\n+   different options or commands to ensure that the command behaves\n+   despite the different parameters. This can be achieved by looping\n+   around a specific parameter:\n+\n+\tfor arg in '' \"--foo\"\n+\tdo\n+\t\ttest_expect_success \"test command ${arg:-without arguments}\" '\n+\t\t\tcommand $arg\n+\t\t'\n+\tdone\n+\n+  Note that while the test title uses double quotes (\"), the test body\n+  should continue to use single quotes ('). The loop variable will be\n+  accessible regardless of the single quotes as the test body is passed\n+  to `eval`.\n+\n \n And here are the \"don'ts:\"\n \n-- \n2.44.0\n\n"},{"id":"491145","messageId":"Zfw6tX-79WisvYsL@tanuki","threadId":"61171","inReplyTo":"cover.1711028473.git.ps@pks.im","subject":"Re: [PATCH 0/3] t7800: fix quoting of looped test bodies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T13:48:37Z","receivedAt":"2024-03-21T13:48:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 21, 2024 at 02:47:20PM +0100, Patrick Steinhardt wrote:\n> Hi,\n> \n> SZEDER made me aware [1] of a quoting issue with some test refactorings\n> in t7800 I recently introduced when refactoring the tests to run in a\n> loop. This patch series fixes those issues and also adds a paragraph to\n> \"t/README\" to document how this is supposed to be done properly.\n> \n> Thanks!\n> \n> Patrick\n> \n> [1]: https://lore.kernel.org/git/20240308221229.GA1908@szeder.dev/\n\nI forgot to mention: this is built on top of\nps/difftool-dir-diff-exit-code at eb84c8b6ce (git-difftool--helper:\nhonor `--trust-exit-code` with `--dir-diff`, 2024-02-20).\n\nPatrick\n"},{"id":"491162","messageId":"xmqqzfur8q5z.fsf@gitster.g","threadId":"61171","inReplyTo":"326fb7965036fccc1c23ad02f200251f6388e6dd.1711028473.git.ps@pks.im","subject":"Re: [PATCH 3/3] t/README: document how to loop around test cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-21T17:12:40Z","receivedAt":"2024-03-21T17:12:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> In some cases it makes sense to loop around test cases so that we can\n> execute the same test with slightly different arguments. There are some\n> gotchas around quoting here though that are easy to miss and that may\n> lead to easy-to-miss errors and portability issues.\n>\n> Document the proper way to do this in \"t/README\".\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  t/README | 19 +++++++++++++++++++\n>  1 file changed, 19 insertions(+)\n>\n> diff --git a/t/README b/t/README\n> index 36463d0742..d56401a254 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -721,6 +721,25 @@ The \"do's:\"\n>     Note that we still &&-chain the loop to propagate failures from\n>     earlier commands.\n>  \n> + - Repeat tests with slightly different arguments in a loop.\n> +\n> +   In some cases it may make sense to re-run the same set of tests with\n> +   different options or commands to ensure that the command behaves\n> +   despite the different parameters. This can be achieved by looping\n> +   around a specific parameter:\n> +\n> +\tfor arg in '' \"--foo\"\n> +\tdo\n> +\t\ttest_expect_success \"test command ${arg:-without arguments}\" '\n> +\t\t\tcommand $arg\n> +\t\t'\n> +\tdone\n> +\n> +  Note that while the test title uses double quotes (\"), the test body\n> +  should continue to use single quotes ('). The loop variable will be\n> +  accessible regardless of the single quotes as the test body is passed\n> +  to `eval`.\n\nWe also want to say that they are not equivalent, don't we?\n\n        for var in '' a 'b\"c'\n        do\n                test_expect_success \"with dq <$var>\" \"\n                        echo \\\"$var\\\"\n                \"\n        done\n\nbreaks, but if we use\n\n                test_expect_success \"with sq <$var>\" '\n                        echo \"$var\"\n                '\n\nin the loop, it works as expected.\n\nOther than that, all three patches do make sense.\n\nThanks.\n"},{"id":"491204","messageId":"ZfzncGTNVwR8BrhR@tanuki","threadId":"61171","inReplyTo":"xmqqzfur8q5z.fsf@gitster.g","subject":"Re: [PATCH 3/3] t/README: document how to loop around test cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-22T02:05:36Z","receivedAt":"2024-03-22T02:05:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 21, 2024 at 10:12:40AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > In some cases it makes sense to loop around test cases so that we can\n> > execute the same test with slightly different arguments. There are some\n> > gotchas around quoting here though that are easy to miss and that may\n> > lead to easy-to-miss errors and portability issues.\n> >\n> > Document the proper way to do this in \"t/README\".\n> >\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  t/README | 19 +++++++++++++++++++\n> >  1 file changed, 19 insertions(+)\n> >\n> > diff --git a/t/README b/t/README\n> > index 36463d0742..d56401a254 100644\n> > --- a/t/README\n> > +++ b/t/README\n> > @@ -721,6 +721,25 @@ The \"do's:\"\n> >     Note that we still &&-chain the loop to propagate failures from\n> >     earlier commands.\n> >  \n> > + - Repeat tests with slightly different arguments in a loop.\n> > +\n> > +   In some cases it may make sense to re-run the same set of tests with\n> > +   different options or commands to ensure that the command behaves\n> > +   despite the different parameters. This can be achieved by looping\n> > +   around a specific parameter:\n> > +\n> > +\tfor arg in '' \"--foo\"\n> > +\tdo\n> > +\t\ttest_expect_success \"test command ${arg:-without arguments}\" '\n> > +\t\t\tcommand $arg\n> > +\t\t'\n> > +\tdone\n> > +\n> > +  Note that while the test title uses double quotes (\"), the test body\n> > +  should continue to use single quotes ('). The loop variable will be\n> > +  accessible regardless of the single quotes as the test body is passed\n> > +  to `eval`.\n> \n> We also want to say that they are not equivalent, don't we?\n> \n>         for var in '' a 'b\"c'\n>         do\n>                 test_expect_success \"with dq <$var>\" \"\n>                         echo \\\"$var\\\"\n>                 \"\n>         done\n> \n> breaks, but if we use\n> \n>                 test_expect_success \"with sq <$var>\" '\n>                         echo \"$var\"\n>                 '\n> \n> in the loop, it works as expected.\n\nHum, good point. How about the below diff? Will reroll the patch series\nif that looks good to you.\n\n--- a/t/README\n+++ b/t/README\n@@ -736,7 +736,8 @@ The \"do's:\"\n        done\n\n   Note that while the test title uses double quotes (\"), the test body\n-  should continue to use single quotes ('). The loop variable will be\n+  should continue to use single quotes (') to avoid breakage in case the\n+  values contain e.g. quoting characters. The loop variable will be\n   accessible regardless of the single quotes as the test body is passed\n   to `eval`.\n\nPatrick\n"},{"id":"491207","messageId":"xmqqv85f3tga.fsf@gitster.g","threadId":"61171","inReplyTo":"ZfzncGTNVwR8BrhR@tanuki","subject":"Re: [PATCH 3/3] t/README: document how to loop around test cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-22T02:12:53Z","receivedAt":"2024-03-22T02:12:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Hum, good point. How about the below diff? Will reroll the patch series\n> if that looks good to you.\n>\n> --- a/t/README\n> +++ b/t/README\n> @@ -736,7 +736,8 @@ The \"do's:\"\n>         done\n>\n>    Note that while the test title uses double quotes (\"), the test body\n> -  should continue to use single quotes ('). The loop variable will be\n> +  should continue to use single quotes (') to avoid breakage in case the\n> +  values contain e.g. quoting characters. The loop variable will be\n>    accessible regardless of the single quotes as the test body is passed\n>    to `eval`.\n\nWow, simple and effective.\n\n"},{"id":"491209","messageId":"cover.1711074118.git.ps@pks.im","threadId":"61171","inReplyTo":"cover.1711028473.git.ps@pks.im","subject":"[PATCH v2 0/3] t7800: fix quoting of looped test bodies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-22T02:23:32Z","receivedAt":"2024-03-22T02:23:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that aims to address some\nissues with looping around `test_expect_success` in t7800.\n\nChanges compared to v1:\n\n    - Fixed indentation of a paragraph in \"t/README\".\n\n    - Added a clarification why one wants to use single quotes to\n      \"t/README\".\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n  t7800: improve test descriptions with empty arguments\n  t7800: use single quotes for test bodies\n  t/README: document how to loop around test cases\n\n t/README            | 20 ++++++++++++++++++++\n t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------\n 2 files changed, 40 insertions(+), 20 deletions(-)\n\nRange-diff against v1:\n1:  fd37c29319 = 1:  fd37c29319 t7800: improve test descriptions with empty arguments\n2:  a4ca974397 = 2:  a4ca974397 t7800: use single quotes for test bodies\n3:  326fb79650 ! 3:  f83b710208 t/README: document how to loop around test cases\n    @@ t/README: The \"do's:\"\n     +\t\t'\n     +\tdone\n     +\n    -+  Note that while the test title uses double quotes (\"), the test body\n    -+  should continue to use single quotes ('). The loop variable will be\n    -+  accessible regardless of the single quotes as the test body is passed\n    -+  to `eval`.\n    ++   Note that while the test title uses double quotes (\"), the test body\n    ++   should continue to use single quotes (') to avoid breakage in case the\n    ++   values contain e.g. quoting characters. The loop variable will be\n    ++   accessible regardless of the single quotes as the test body is passed\n    ++   to `eval`.\n     +\n      \n      And here are the \"don'ts:\"\n-- \n2.44.0\n\n"},{"id":"491210","messageId":"fd37c2931904bc64fb6ea14331aea60278f495eb.1711074118.git.ps@pks.im","threadId":"61171","inReplyTo":"cover.1711074118.git.ps@pks.im","subject":"[PATCH v2 1/3] t7800: improve test descriptions with empty arguments","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-22T02:23:36Z","receivedAt":"2024-03-22T02:23:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Some of the tests in t7800 are executed repeatedly in a loop with\ndifferent arguments. To distinguish these tests, the value of that\nvariable is rendered into the test title. But given that one of the\nvalues is the empty string, it results in a somewhat awkward test name:\n\n    difftool  ignores exit code\n\nImprove this by printing \"without options\" in case the value is empty.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t7800-difftool.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 96ae5d5880..80bf108f54 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -93,40 +93,40 @@ test_expect_success 'difftool forwards arguments to diff' '\n \n for opt in '' '--dir-diff'\n do\n-\ttest_expect_success \"difftool ${opt} ignores exit code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} ignores exit code\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\tgit difftool ${opt} -y -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code for built-ins\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins\" \"\n \t\ttest_config difftool.vimdiff.path false &&\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = true\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = true\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode true &&\n \t\ttest_must_fail git difftool ${opt} -y -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = false\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = false\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode false &&\n \t\tgit difftool ${opt} -y -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} ignores exit code with --no-trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} ignores exit code with --no-trust-exit-code\" \"\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode true &&\n \t\tgit difftool ${opt} -y --no-trust-exit-code -t error branch\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} stops on error with --trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} stops on error with --trust-exit-code\" \"\n \t\ttest_when_finished 'rm -f for-diff .git/fail-right-file' &&\n \t\ttest_when_finished 'git reset -- for-diff' &&\n \t\twrite_script .git/fail-right-file <<-\\EOF &&\n@@ -140,7 +140,7 @@ do\n \t\ttest_line_count = 1 actual\n \t\"\n \n-\ttest_expect_success \"difftool ${opt} honors exit status if command not found\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors exit status if command not found\" \"\n \t\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n \t\ttest_config difftool.trustExitCode false &&\n \t\tif test \"${opt}\" = '--dir-diff'\n-- \n2.44.0\n\n"},{"id":"491211","messageId":"a4ca974397aadef251c2317efa88fd9c03e8f195.1711074118.git.ps@pks.im","threadId":"61171","inReplyTo":"cover.1711074118.git.ps@pks.im","subject":"[PATCH v2 2/3] t7800: use single quotes for test bodies","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-22T02:23:41Z","receivedAt":"2024-03-22T02:23:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In eb84c8b6ce (git-difftool--helper: honor `--trust-exit-code` with\n`--dir-diff`, 2024-02-20) we have started to loop around some of the\ntests in t7800 so that they are reexecuted with slightly different\narguments. As part of that refactoring the quoting of test bodies was\nchanged from single quotes (') to double quotes (\") so that the value of\nthe loop variable is accessible to the body.\n\nAs the test body is later on passed to eval this change was not required\nthough. Let's revert it back to use single quotes as usual in our tests.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 80bf108f54..cc917b257e 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -93,42 +93,42 @@ test_expect_success 'difftool forwards arguments to diff' '\n \n for opt in '' '--dir-diff'\n do\n-\ttest_expect_success \"difftool ${opt:-without options} ignores exit code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} ignores exit code\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\tgit difftool ${opt} -y -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins\" '\n \t\ttest_config difftool.vimdiff.path false &&\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = true\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = true\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode true &&\n \t\ttest_must_fail git difftool ${opt} -y -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = false\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors difftool.trustExitCode = false\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode false &&\n \t\tgit difftool ${opt} -y -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} ignores exit code with --no-trust-exit-code\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} ignores exit code with --no-trust-exit-code\" '\n \t\ttest_config difftool.error.cmd false &&\n \t\ttest_config difftool.trustExitCode true &&\n \t\tgit difftool ${opt} -y --no-trust-exit-code -t error branch\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} stops on error with --trust-exit-code\" \"\n-\t\ttest_when_finished 'rm -f for-diff .git/fail-right-file' &&\n-\t\ttest_when_finished 'git reset -- for-diff' &&\n+\ttest_expect_success \"difftool ${opt:-without options} stops on error with --trust-exit-code\" '\n+\t\ttest_when_finished \"rm -f for-diff .git/fail-right-file\" &&\n+\t\ttest_when_finished \"git reset -- for-diff\" &&\n \t\twrite_script .git/fail-right-file <<-\\EOF &&\n \t\techo failed\n \t\texit 1\n@@ -138,19 +138,19 @@ do\n \t\ttest_must_fail git difftool ${opt} -y --trust-exit-code \\\n \t\t\t--extcmd .git/fail-right-file branch >actual &&\n \t\ttest_line_count = 1 actual\n-\t\"\n+\t'\n \n-\ttest_expect_success \"difftool ${opt:-without options} honors exit status if command not found\" \"\n+\ttest_expect_success \"difftool ${opt:-without options} honors exit status if command not found\" '\n \t\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n \t\ttest_config difftool.trustExitCode false &&\n-\t\tif test \"${opt}\" = '--dir-diff'\n+\t\tif test \"${opt}\" = --dir-diff\n \t\tthen\n \t\t\texpected_code=127\n \t\telse\n \t\t\texpected_code=128\n \t\tfi &&\n-\t\ttest_expect_code \\${expected_code} git difftool ${opt} -y -t nonexistent branch\n-\t\"\n+\t\ttest_expect_code ${expected_code} git difftool ${opt} -y -t nonexistent branch\n+\t'\n done\n \n test_expect_success 'difftool honors --gui' '\n-- \n2.44.0\n\n"},{"id":"491212","messageId":"f83b710208c5b6d6bd6904230e5fd96aab0ef42e.1711074118.git.ps@pks.im","threadId":"61171","inReplyTo":"cover.1711074118.git.ps@pks.im","subject":"[PATCH v2 3/3] t/README: document how to loop around test cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-22T02:23:46Z","receivedAt":"2024-03-22T02:23:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In some cases it makes sense to loop around test cases so that we can\nexecute the same test with slightly different arguments. There are some\ngotchas around quoting here though that are easy to miss and that may\nlead to easy-to-miss errors and portability issues.\n\nDocument the proper way to do this in \"t/README\".\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/README | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 36463d0742..4422ab9b98 100644\n--- a/t/README\n+++ b/t/README\n@@ -721,6 +721,26 @@ The \"do's:\"\n    Note that we still &&-chain the loop to propagate failures from\n    earlier commands.\n \n+ - Repeat tests with slightly different arguments in a loop.\n+\n+   In some cases it may make sense to re-run the same set of tests with\n+   different options or commands to ensure that the command behaves\n+   despite the different parameters. This can be achieved by looping\n+   around a specific parameter:\n+\n+\tfor arg in '' \"--foo\"\n+\tdo\n+\t\ttest_expect_success \"test command ${arg:-without arguments}\" '\n+\t\t\tcommand $arg\n+\t\t'\n+\tdone\n+\n+   Note that while the test title uses double quotes (\"), the test body\n+   should continue to use single quotes (') to avoid breakage in case the\n+   values contain e.g. quoting characters. The loop variable will be\n+   accessible regardless of the single quotes as the test body is passed\n+   to `eval`.\n+\n \n And here are the \"don'ts:\"\n \n-- \n2.44.0\n\n"},{"id":"491244","messageId":"xmqqmsqq49jc.fsf@gitster.g","threadId":"61171","inReplyTo":"cover.1711074118.git.ps@pks.im","subject":"Re: [PATCH v2 0/3] t7800: fix quoting of looped test bodies","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-22T14:37:43Z","receivedAt":"2024-03-22T14:37:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Hi,\n>\n> this is the second version of my patch series that aims to address some\n> issues with looping around `test_expect_success` in t7800.\n\nLooking good.  Thanks.  Will queue.\n\nLet's mark it for 'next' already ;-)\n\n>\n> Changes compared to v1:\n>\n>     - Fixed indentation of a paragraph in \"t/README\".\n>\n>     - Added a clarification why one wants to use single quotes to\n>       \"t/README\".\n>\n> Thanks!\n>\n> Patrick\n>\n> Patrick Steinhardt (3):\n>   t7800: improve test descriptions with empty arguments\n>   t7800: use single quotes for test bodies\n>   t/README: document how to loop around test cases\n>\n>  t/README            | 20 ++++++++++++++++++++\n>  t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------\n>  2 files changed, 40 insertions(+), 20 deletions(-)\n>\n> Range-diff against v1:\n> 1:  fd37c29319 = 1:  fd37c29319 t7800: improve test descriptions with empty arguments\n> 2:  a4ca974397 = 2:  a4ca974397 t7800: use single quotes for test bodies\n> 3:  326fb79650 ! 3:  f83b710208 t/README: document how to loop around test cases\n>     @@ t/README: The \"do's:\"\n>      +\t\t'\n>      +\tdone\n>      +\n>     -+  Note that while the test title uses double quotes (\"), the test body\n>     -+  should continue to use single quotes ('). The loop variable will be\n>     -+  accessible regardless of the single quotes as the test body is passed\n>     -+  to `eval`.\n>     ++   Note that while the test title uses double quotes (\"), the test body\n>     ++   should continue to use single quotes (') to avoid breakage in case the\n>     ++   values contain e.g. quoting characters. The loop variable will be\n>     ++   accessible regardless of the single quotes as the test body is passed\n>     ++   to `eval`.\n>      +\n>       \n>       And here are the \"don'ts:\"\n"}]}