{"thread":{"id":"53960","subject":"[PATCH] t1450: fix quoting of NUL byte when corrupting pack","startedAt":"2020-08-01T22:06:56Z","lastAt":"2020-08-07T17:16:35Z","messageCount":16,"participants":["Martin Ågren","Junio C Hamano","Chris Torek","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"402627","messageId":"20200801220611.10453-1-martin.agren@gmail.com","threadId":"53960","inReplyTo":null,"subject":"[PATCH] t1450: fix quoting of NUL byte when corrupting pack","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-08-01T22:06:11Z","receivedAt":"2020-08-01T22:06:56Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We use\n\n  printf '\\0'\n\nto generate a NUL byte which we then `dd` into the packfile to ensure\nthat we modify the first byte of the first object, thereby\n(probabilistically) invalidating the checksum. Except the single quotes\nwe're using are interpreted to match with the ones we enclose the whole\ntest in. So we actually execute\n\n  printf \\0\n\nand end up injecting the ASCII code for \"0\", 0x30, instead.\n\nThe comment right above this `printf` invocation says that \"at least one\nof [the type bits] is not zero, so setting the first byte to 0 is\nsufficient\". Substituting \"0x30\" for \"0\" in that comment won't do: we'd\nneed to reason about which bits go where and just what the packfile\nlooks like that we're modifying in this test.\n\nLet's avoid all of that by actually executing\n\n  printf \"\\0\"\n\nto generate a NUL byte, as intended.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n If my reading is correct, when we substitute 0x30, the type will be 3\n (blob) and the size will be zero. So there might actually exist\n formally valid packfiles where this byte that we're modifying is\n already zero. What matters in the end is whether we might be using such\n a packfile in this exact test and from what I can tell, no, we won't be\n doing that.\n\n t/t1450-fsck.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 344a2aad82..af2a2c4682 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -714,7 +714,7 @@ test_expect_success 'fsck fails on corrupt packfile' '\n \t# at least one of which is not zero, so setting the first byte to 0 is\n \t# sufficient.)\n \tchmod a+w .git/objects/pack/pack-$pack.pack &&\n-\tprintf '\\0' | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&\n+\tprintf \"\\0\" | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&\n \n \ttest_when_finished \"rm -f .git/objects/pack/pack-$pack.*\" &&\n \tremove_object $hsh &&\n-- \n2.28.0.81.ge8ab941b67\n\n"},{"id":"402630","messageId":"xmqqmu3d27vs.fsf@gitster.c.googlers.com","threadId":"53960","inReplyTo":"20200801220611.10453-1-martin.agren@gmail.com","subject":"Re: [PATCH] t1450: fix quoting of NUL byte when corrupting pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-02T00:45:11Z","receivedAt":"2020-08-02T00:45:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> We use\n>\n>   printf '\\0'\n>\n> to generate a NUL byte which we then `dd` into the packfile to ensure\n> that we modify the first byte of the first object, thereby\n> (probabilistically) invalidating the checksum. Except the single quotes\n> we're using are interpreted to match with the ones we enclose the whole\n> test in. So we actually execute\n>\n>   printf \\0\n>\n> and end up injecting the ASCII code for \"0\", 0x30, instead.\n>\n> The comment right above this `printf` invocation says that \"at least one\n> of [the type bits] is not zero, so setting the first byte to 0 is\n> sufficient\". Substituting \"0x30\" for \"0\" in that comment won't do: we'd\n> need to reason about which bits go where and just what the packfile\n> looks like that we're modifying in this test.\n>\n> Let's avoid all of that by actually executing\n>\n>   printf \"\\0\"\n>\n> to generate a NUL byte, as intended.\n\nThanks.  Very well explained.\n\nI wonder if it is an easy way to find similar problems without too\nmuch hand-parsing of the test scripts.  Inside a modern test_expect_*\nthat begins and ends the test body with a single quote, any line\nthat has a single quote that is not quoted could be suspect, but\nthat would probably give us too many false positive.\n\n\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  If my reading is correct, when we substitute 0x30, the type will be 3\n>  (blob) and the size will be zero. So there might actually exist\n>  formally valid packfiles where this byte that we're modifying is\n>  already zero. What matters in the end is whether we might be using such\n>  a packfile in this exact test and from what I can tell, no, we won't be\n>  doing that.\n>\n>  t/t1450-fsck.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index 344a2aad82..af2a2c4682 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -714,7 +714,7 @@ test_expect_success 'fsck fails on corrupt packfile' '\n>  \t# at least one of which is not zero, so setting the first byte to 0 is\n>  \t# sufficient.)\n>  \tchmod a+w .git/objects/pack/pack-$pack.pack &&\n> -\tprintf '\\0' | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&\n> +\tprintf \"\\0\" | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&\n>  \n>  \ttest_when_finished \"rm -f .git/objects/pack/pack-$pack.*\" &&\n>  \tremove_object $hsh &&\n"},{"id":"402631","messageId":"CAPx1GvcZFVfmP8aTdXu_epdSn1EDy_cP6-=hi65c_5DbNEq+FQ@mail.gmail.com","threadId":"53960","inReplyTo":"20200801220611.10453-1-martin.agren@gmail.com","subject":"Re: [PATCH] t1450: fix quoting of NUL byte when corrupting pack","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-08-02T01:00:45Z","receivedAt":"2020-08-02T01:00:59Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Sat, Aug 1, 2020 at 3:08 PM Martin Ågren <martin.agren@gmail.com> wrote:\n>   printf '\\0'\n>\n> to generate a NUL byte\n[but]\n> end up injecting the ASCII code for \"0\", 0x30, instead.\n\n> -       printf '\\0' | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&\n> +       printf \"\\0\" | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&\n\nIn sh/bash, this should make no difference, and that's what I get here.\nAm I missing something?\n\n$ printf '\\0' | hexdump\n0000000 0000\n0000001\n$ printf \"\\0\" | hexdump\n0000000 0000\n0000001\n\nChris\n"},{"id":"402632","messageId":"CAPx1GvcD80MfjZLiKBrH8BgsvVToZ90C057gk7b4BUzbLMNu2A@mail.gmail.com","threadId":"53960","inReplyTo":"CAPx1GvcZFVfmP8aTdXu_epdSn1EDy_cP6-=hi65c_5DbNEq+FQ@mail.gmail.com","subject":"Re: [PATCH] t1450: fix quoting of NUL byte when corrupting pack","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-08-02T01:02:07Z","receivedAt":"2020-08-02T01:02:21Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"Oh, wait, I am indeed missing something -- the script itself is\nall in single quotes, so the single quotes on the line in question\nare wrong.  Never mind!\n\nChris\n"},{"id":"402646","messageId":"20200802143018.5501-1-martin.agren@gmail.com","threadId":"53960","inReplyTo":"xmqqmu3d27vs.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] t1450: fix quoting of NUL byte when corrupting pack","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-08-02T14:30:18Z","receivedAt":"2020-08-02T14:30:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 2 Aug 2020 at 02:45, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> I wonder if it is an easy way to find similar problems without too\n> much hand-parsing of the test scripts.  Inside a modern test_expect_*\n> that begins and ends the test body with a single quote, any line\n> that has a single quote that is not quoted could be suspect, but\n> that would probably give us too many false positive.\n\nI did some very crude hand-parsing and identified a few spots that\nlooked needlessly complicated and where I could change or drop the\nquoting so that the code we see is the code we also execute. Patch\nbelow.\n\nSome of those sites made me go \"what is actually going on here?\" which\nwould be an argument for simplifying them. Others are more \"maybe we\nshould avoid those quote marks, but the test is obviously correct\nanyway\".\n\nSo I didn't spot any other bugs (like the 0x30/NUL thing in my original\npatch), which is good, but also makes me not very motivated to invest in\nmore advanced parsing.\n\nMartin\n\n-- >8 --\nSubject: [PATCH] t: don't spuriously close & open quotes\n\nIf we write something like\n\ntest_expect_success 'sed a little' '\n  sed -e 's/hi/lo/' in >out\n'\n\nthe human readers do not see what the shell eventually sees:\n\n  sed -e s/hi/lo/ in >out\n\nIn this example, this makes no real difference. But this can be confusing\nand can make the reader wonder whether we actually rely on that whole\n\"close the quote, do some stuff, then start quoting again\" and what is\nactually going on. It could potentially also be a bug in the test.\n\nChange several such single quotes to use double quotes instead or, in a\nfew cases, drop them altogether. These were identified using some crude\ngrepping. We're not fixing any test bugs here, but we're hopefully\nmaking these tests slightly easier to grok and to maintain.\n\n(There are legitimate use cases for closing a quote and opening a new\none, e.g., both '\\'' and '\"'\"' can be used to produce a literal single\nquote. I'm not touching any of those here.)\n\nIn t9401, tuck the redirecting \">\" to the filename while we're touching\nthose lines.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t1400-update-ref.sh           |  2 +-\n t/t3501-revert-cherry-pick.sh   |  4 ++--\n t/t3507-cherry-pick-conflict.sh |  4 ++--\n t/t4005-diff-rename-2.sh        |  4 ++--\n t/t4034-diff-words.sh           |  2 +-\n t/t4104-apply-boundary.sh       | 24 ++++++++++++------------\n t/t4150-am.sh                   |  8 ++++----\n t/t4200-rerere.sh               |  2 +-\n t/t5302-pack-index.sh           |  2 +-\n t/t5510-fetch.sh                |  4 ++--\n t/t5553-set-upstream.sh         |  6 +++---\n t/t6026-merge-attr.sh           |  4 ++--\n t/t7001-mv.sh                   |  2 +-\n t/t7600-merge.sh                |  6 +++---\n t/t9001-send-email.sh           | 10 +++++-----\n t/t9100-git-svn-basic.sh        |  2 +-\n t/t9401-git-cvsserver-crlf.sh   |  8 ++++----\n t/t9402-git-cvsserver-refs.sh   |  2 +-\n 18 files changed, 48 insertions(+), 48 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 27171f8261..d0d36750bc 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -324,7 +324,7 @@ test_expect_success \"create $m (logged by config)\" '\n test_expect_success \"update $m (logged by config)\" '\n \ttest_config core.logAllRefUpdates true &&\n \tGIT_COMMITTER_DATE=\"2005-05-26 23:33\" \\\n-\tgit update-ref HEAD'\" $B $A \"'-m \"Switch\" &&\n+\tgit update-ref HEAD $B $A -m \"Switch\" &&\n \ttest $B = $(git show-ref -s --verify $m)\n '\n test_expect_success \"set $m (logged by config)\" '\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 7c1da21df1..3669dfb1be 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -47,7 +47,7 @@ test_expect_success 'cherry-pick --nonsense' '\n \tgit diff --exit-code HEAD &&\n \ttest_must_fail git cherry-pick --nonsense 2>msg &&\n \tgit diff --exit-code HEAD \"$pos\" &&\n-\ttest_i18ngrep '[Uu]sage:' msg\n+\ttest_i18ngrep \"[Uu]sage:\" msg\n '\n \n test_expect_success 'revert --nonsense' '\n@@ -56,7 +56,7 @@ test_expect_success 'revert --nonsense' '\n \tgit diff --exit-code HEAD &&\n \ttest_must_fail git revert --nonsense 2>msg &&\n \tgit diff --exit-code HEAD \"$pos\" &&\n-\ttest_i18ngrep '[Uu]sage:' msg\n+\ttest_i18ngrep \"[Uu]sage:\" msg\n '\n \n test_expect_success 'cherry-pick after renaming branch' '\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 752bc43487..f107622a9e 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -512,7 +512,7 @@ test_expect_success 'commit after failed cherry-pick adds -s at the right place'\n \tSigned-off-by: C O Mitter <committer@example.com>\n \t# Conflicts:\n \tEOF\n-\tgrep -e \"^# Conflicts:\" -e '^Signed-off-by' .git/COMMIT_EDITMSG >actual &&\n+\tgrep -e \"^# Conflicts:\" -e \"^Signed-off-by\" .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual &&\n \n \tcat <<-\\EOF >expected &&\n@@ -541,7 +541,7 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n \tSigned-off-by: C O Mitter <committer@example.com>\n \tConflicts:\n \tEOF\n-\tgrep -e \"^Conflicts:\" -e '^Signed-off-by' .git/COMMIT_EDITMSG >actual &&\n+\tgrep -e \"^Conflicts:\" -e \"^Signed-off-by\" .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t4005-diff-rename-2.sh b/t/t4005-diff-rename-2.sh\nindex f542d2929d..d18a80493c 100755\n--- a/t/t4005-diff-rename-2.sh\n+++ b/t/t4005-diff-rename-2.sh\n@@ -14,8 +14,8 @@ test_expect_success 'setup reference tree' '\n \tgit update-index --add COPYING rezrov &&\n \ttree=$(git write-tree) &&\n \techo $tree &&\n-\tsed -e 's/HOWEVER/However/' <COPYING >COPYING.1 &&\n-\tsed -e 's/GPL/G.P.L/g' <COPYING >COPYING.2 &&\n+\tsed -e \"s/HOWEVER/However/\" <COPYING >COPYING.1 &&\n+\tsed -e \"s/GPL/G.P.L/g\" <COPYING >COPYING.2 &&\n \torigoid=$(git hash-object COPYING) &&\n \toid1=$(git hash-object COPYING.1) &&\n \toid2=$(git hash-object COPYING.2)\ndiff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh\nindex fb145aa173..0c8fb39ced 100755\n--- a/t/t4034-diff-words.sh\n+++ b/t/t4034-diff-words.sh\n@@ -102,7 +102,7 @@ test_expect_success 'word diff with runs of whitespace' '\n '\n \n test_expect_success '--word-diff=porcelain' '\n-\tsed 's/#.*$//' >expect <<-EOF &&\n+\tsed \"s/#.*$//\" >expect <<-EOF &&\n \t\tdiff --git a/pre b/post\n \t\tindex $pre..$post 100644\n \t\t--- a/pre\ndiff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh\nindex 32e3b0ee0b..19f08d9ccd 100755\n--- a/t/t4104-apply-boundary.sh\n+++ b/t/t4104-apply-boundary.sh\n@@ -11,7 +11,7 @@ test_description='git apply boundary tests\n L=\"c d e f g h i j k l m n o p q r s t u v w x\"\n \n test_expect_success setup '\n-\tfor i in b '\"$L\"' y\n+\tfor i in b $L y\n \tdo\n \t\techo $i\n \tdone >victim &&\n@@ -19,7 +19,7 @@ test_expect_success setup '\n \tgit update-index --add victim &&\n \n \t# add to the head\n-\tfor i in a b '\"$L\"' y\n+\tfor i in a b $L y\n \tdo\n \t\techo $i\n \tdone >victim &&\n@@ -28,7 +28,7 @@ test_expect_success setup '\n \tgit diff --unified=0 >add-a-patch.without &&\n \n \t# insert at line two\n-\tfor i in b a '\"$L\"' y\n+\tfor i in b a $L y\n \tdo\n \t\techo $i\n \tdone >victim &&\n@@ -37,7 +37,7 @@ test_expect_success setup '\n \tgit diff --unified=0 >insert-a-patch.without &&\n \n \t# modify at the head\n-\tfor i in a '\"$L\"' y\n+\tfor i in a $L y\n \tdo\n \t\techo $i\n \tdone >victim &&\n@@ -46,7 +46,7 @@ test_expect_success setup '\n \tgit diff --unified=0 >mod-a-patch.without &&\n \n \t# remove from the head\n-\tfor i in '\"$L\"' y\n+\tfor i in $L y\n \tdo\n \t\techo $i\n \tdone >victim &&\n@@ -55,7 +55,7 @@ test_expect_success setup '\n \tgit diff --unified=0 >del-a-patch.without &&\n \n \t# add to the tail\n-\tfor i in b '\"$L\"' y z\n+\tfor i in b $L y z\n \tdo\n \t\techo $i\n \tdone >victim &&\n@@ -64,7 +64,7 @@ test_expect_success setup '\n \tgit diff --unified=0 >add-z-patch.without &&\n \n \t# modify at the tail\n-\tfor i in b '\"$L\"' z\n+\tfor i in b $L z\n \tdo\n \t\techo $i\n \tdone >victim &&\n@@ -73,7 +73,7 @@ test_expect_success setup '\n \tgit diff --unified=0 >mod-z-patch.without &&\n \n \t# remove from the tail\n-\tfor i in b '\"$L\"'\n+\tfor i in b $L\n \tdo\n \t\techo $i\n \tdone >victim &&\n@@ -95,8 +95,8 @@ do\n \t\ttest_expect_success \"apply $kind-patch $with context\" '\n \t\t\tcat original >victim &&\n \t\t\tgit update-index victim &&\n-\t\t\tgit apply --index '\"$u$kind-patch.$with\"' &&\n-\t\t\ttest_cmp '\"$kind\"'-expect victim\n+\t\t\tgit apply --index $u$kind-patch.$with &&\n+\t\t\ttest_cmp $kind-expect victim\n \t\t'\n \tdone\n done\n@@ -110,8 +110,8 @@ do\n \ttest_expect_success \"apply non-git $kind-patch without context\" '\n \t\tcat original >victim &&\n \t\tgit update-index victim &&\n-\t\tgit apply --unidiff-zero --index '\"$kind-ng.without\"' &&\n-\t\ttest_cmp '\"$kind\"'-expect victim\n+\t\tgit apply --unidiff-zero --index $kind-ng.without &&\n+\t\ttest_cmp $kind-expect victim\n \t'\n done\n \ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex bda4586a79..855ed11b32 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -989,7 +989,7 @@ test_expect_success 'am -s unexpected trailer block' '\n \tSigned-off-by: J C H <j@c.h>\n \tEOF\n \tgit commit -F msg &&\n-\tgit cat-file commit HEAD | sed -e '1,/^$/d' >original &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >original &&\n \tgit format-patch --stdout -1 >patch &&\n \n \tgit reset --hard HEAD^ &&\n@@ -998,7 +998,7 @@ test_expect_success 'am -s unexpected trailer block' '\n \t\tcat original &&\n \t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n \t) >expect &&\n-\tgit cat-file commit HEAD | sed -e '1,/^$/d' >actual &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >actual &&\n \ttest_cmp expect actual &&\n \n \tcat >msg <<-\\EOF &&\n@@ -1009,7 +1009,7 @@ test_expect_success 'am -s unexpected trailer block' '\n \tEOF\n \tgit reset HEAD^ &&\n \tgit commit -F msg file &&\n-\tgit cat-file commit HEAD | sed -e '1,/^$/d' >original &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >original &&\n \tgit format-patch --stdout -1 >patch &&\n \n \tgit reset --hard HEAD^ &&\n@@ -1020,7 +1020,7 @@ test_expect_success 'am -s unexpected trailer block' '\n \t\techo &&\n \t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n \t) >expect &&\n-\tgit cat-file commit HEAD | sed -e '1,/^$/d' >actual &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 831d424c47..f99385c191 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -364,7 +364,7 @@ test_expect_success 'set up an unresolved merge' '\n \tgit reset --hard &&\n \tgit checkout version2 &&\n \tfifth=$(git rev-parse fifth) &&\n-\techo \"$fifth\t\tbranch 'fifth' of .\" |\n+\techo \"$fifth\t\tbranch fifth of .\" |\n \tgit fmt-merge-msg >msg &&\n \tancestor=$(git merge-base version2 fifth) &&\n \ttest_must_fail git merge-recursive \"$ancestor\" -- HEAD fifth &&\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 8981c9b90e..d1a6a3c37a 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -15,7 +15,7 @@ test_expect_success 'setup' '\n \ti=1 &&\n \twhile test $i -le 100\n \tdo\n-\t\tiii=$(printf '%03i' $i)\n+\t\tiii=$(printf \"%03i\" $i)\n \t\ttest-tool genrandom \"bar\" 200 > wide_delta_$iii &&\n \t\ttest-tool genrandom \"baz $iii\" 50 >> wide_delta_$iii &&\n \t\ttest-tool genrandom \"foo\"$i 100 > deep_delta_$iii &&\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 7456c567cd..6a6a760f5f 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -213,7 +213,7 @@ test_expect_success 'fetch tags when there is no tags' '\n test_expect_success 'fetch following tags' '\n \n \tcd \"$D\" &&\n-\tgit tag -a -m 'annotated' anno HEAD &&\n+\tgit tag -a -m \"annotated\" anno HEAD &&\n \tgit tag light HEAD &&\n \n \tmkdir four &&\n@@ -331,7 +331,7 @@ test_expect_success 'bundle does not prerequisite objects' '\n test_expect_success 'bundle should be able to create a full history' '\n \n \tcd \"$D\" &&\n-\tgit tag -a -m '1.0' v1.0 master &&\n+\tgit tag -a -m \"1.0\" v1.0 master &&\n \tgit bundle create bundle4 v1.0\n \n '\ndiff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh\nindex 81975ad8f9..7622981cbf 100755\n--- a/t/t5553-set-upstream.sh\n+++ b/t/t5553-set-upstream.sh\n@@ -81,7 +81,7 @@ test_expect_success 'fetch --set-upstream http://nosuchdomain.example.com fails\n \n test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '\n \tclear_config other other2 &&\n-\turl=\"file://'\"$PWD\"'\" &&\n+\turl=\"file://$PWD\" &&\n \tgit fetch --set-upstream \"$url\" &&\n \tcheck_config master \"$url\" HEAD &&\n \tcheck_config_missing other &&\n@@ -158,7 +158,7 @@ test_expect_success 'pull --set-upstream upstream with more than one branch does\n test_expect_success 'pull --set-upstream with valid URL sets upstream to URL' '\n \tclear_config master other other2 &&\n \tgit checkout master &&\n-\turl=\"file://'\"$PWD\"'\" &&\n+\turl=\"file://$PWD\" &&\n \tgit pull --set-upstream \"$url\" &&\n \tcheck_config master \"$url\" HEAD &&\n \tcheck_config_missing other &&\n@@ -168,7 +168,7 @@ test_expect_success 'pull --set-upstream with valid URL sets upstream to URL' '\n test_expect_success 'pull --set-upstream with valid URL and branch sets branch' '\n \tclear_config master other other2 &&\n \tgit checkout master &&\n-\turl=\"file://'\"$PWD\"'\" &&\n+\turl=\"file://$PWD\" &&\n \tgit pull --set-upstream \"$url\" master &&\n \tcheck_config master \"$url\" refs/heads/master &&\n \tcheck_config_missing other &&\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex 5900358ce9..76a55f838c 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -122,7 +122,7 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file master^:text) &&\n \ta=$(git unpack-file side^:text) &&\n \tb=$(git unpack-file master:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 'text'\" &&\n+\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \trm -f $o $a $b\n@@ -149,7 +149,7 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file master^:text) &&\n \ta=$(git unpack-file anchor:text) &&\n \tb=$(git unpack-file master:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 'text'\" &&\n+\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \tsed -e 1,3d -e 4q $a >check-3 &&\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex c978b6dee4..63d5f41a12 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -177,7 +177,7 @@ test_expect_success \"Sergey Vlasov's test case\" '\n \tdate >ab.c &&\n \tdate >ab/d &&\n \tgit add ab.c ab &&\n-\tgit commit -m 'initial' &&\n+\tgit commit -m \"initial\" &&\n \tgit mv ab a\n '\n \ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 1d45f9a4ed..dffcf053db 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -246,7 +246,7 @@ test_expect_success 'merge --squash c3 with c7' '\n \t#\tfile\n \tEOF\n \tgit cat-file commit HEAD >raw &&\n-\tsed -e '1,/^$/d' raw >actual &&\n+\tsed -e \"1,/^$/d\" raw >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -268,7 +268,7 @@ test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' '\n \t#\tfile\n \tEOF\n \tgit cat-file commit HEAD >raw &&\n-\tsed -e '1,/^$/d' raw >actual &&\n+\tsed -e \"1,/^$/d\" raw >actual &&\n \ttest_i18ncmp expect actual\n '\n \n@@ -292,7 +292,7 @@ test_expect_success 'merge c3 with c7 with --squash commit.cleanup = scissors' '\n \t#\tfile\n \tEOF\n \tgit cat-file commit HEAD >raw &&\n-\tsed -e '1,/^$/d' raw >actual &&\n+\tsed -e \"1,/^$/d\" raw >actual &&\n \ttest_i18ncmp expect actual\n '\n \ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex ec261085ec..3d68570450 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -1551,7 +1551,7 @@ test_expect_success $PREREQ '8-bit and sendemail.transferencoding=quoted-printab\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-8bit \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1568,7 +1568,7 @@ test_expect_success $PREREQ '8-bit and sendemail.transferencoding=base64' '\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-8bit \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1594,7 +1594,7 @@ test_expect_success $PREREQ 'convert from quoted-printable to base64' '\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-qp \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1624,7 +1624,7 @@ test_expect_success $PREREQ 'CRLF and sendemail.transferencoding=quoted-printabl\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-crlf \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1641,7 +1641,7 @@ test_expect_success $PREREQ 'CRLF and sendemail.transferencoding=base64' '\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-crlf \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \ndiff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh\nindex 3055943a22..30172664ac 100755\n--- a/t/t9100-git-svn-basic.sh\n+++ b/t/t9100-git-svn-basic.sh\n@@ -69,7 +69,7 @@ test_expect_success \"$name\" \"\n \tmv dir/new_file dir/file &&\n \tgit update-index --remove dir/file &&\n \tgit update-index --add dir/file/file &&\n-\tgit commit -m '$name' &&\n+\tgit commit -m \"$name\" &&\n \ttest_must_fail git svn set-tree --find-copies-harder --rmdir \\\n \t\tremotes/git-svn..mybranch\n \"\ndiff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh\nindex 84787eee9a..c7a0dd84a4 100755\n--- a/t/t9401-git-cvsserver-crlf.sh\n+++ b/t/t9401-git-cvsserver-crlf.sh\n@@ -167,10 +167,10 @@ test_expect_success 'adding files' '\n \n test_expect_success 'updating' '\n     git pull gitcvs.git &&\n-    echo 'hi' > subdir/newfile.bin &&\n-    echo 'junk' > subdir/file.h &&\n-    echo 'hi' > subdir/newfile.c &&\n-    echo 'hello' >> binfile.bin &&\n+    echo \"hi\" >subdir/newfile.bin &&\n+    echo \"junk\" >subdir/file.h &&\n+    echo \"hi\" >subdir/newfile.c &&\n+    echo \"hello\" >>binfile.bin &&\n     git add subdir/newfile.bin subdir/file.h subdir/newfile.c binfile.bin &&\n     git commit -q -m \"Add and change some files\" &&\n     git push gitcvs.git >/dev/null &&\ndiff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh\nindex cf31ace667..6436c91a3c 100755\n--- a/t/t9402-git-cvsserver-refs.sh\n+++ b/t/t9402-git-cvsserver-refs.sh\n@@ -178,7 +178,7 @@ test_expect_success 'setup v1.2 on b1' '\n \tmkdir cdir &&\n \techo \"cdir/cfile\" >cdir/cfile &&\n \tgit add -A cdir adir t3 t2 &&\n-\tgit commit -q -m 'v1.2' &&\n+\tgit commit -q -m \"v1.2\" &&\n \tgit tag v1.2 &&\n \tgit push --tags gitcvs.git b1:b1\n '\n-- \n2.28.0.81.ge8ab941b67\n\n"},{"id":"402647","messageId":"CAN0heSoYAJkZVG7_TPTo3Osz+FuR__AY5Ykc8OSwzARTotfwcQ@mail.gmail.com","threadId":"53960","inReplyTo":"CAPx1GvcD80MfjZLiKBrH8BgsvVToZ90C057gk7b4BUzbLMNu2A@mail.gmail.com","subject":"Re: [PATCH] t1450: fix quoting of NUL byte when corrupting pack","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-08-02T14:35:10Z","receivedAt":"2020-08-02T14:35:24Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 2 Aug 2020 at 03:02, Chris Torek <chris.torek@gmail.com> wrote:\n>\n> Oh, wait, I am indeed missing something -- the script itself is\n> all in single quotes, so the single quotes on the line in question\n> are wrong.  Never mind!\n\nNo worries! Thanks for having a look at the patch. Is there anything\nthat could be done to make this clearer in the commit message? (I find it\nquite awkward to discuss quoting: will the reader understand which\nquoting is part of my own formatting of the message vs which is part of\nthe quoting issue I want to get across!?)\n\nMartin\n"},{"id":"402661","messageId":"CAPx1GvdZNeuQqmYm8G62Zr02k=B5GK69xPw84WnvMCeJU7_amQ@mail.gmail.com","threadId":"53960","inReplyTo":"CAN0heSoYAJkZVG7_TPTo3Osz+FuR__AY5Ykc8OSwzARTotfwcQ@mail.gmail.com","subject":"Re: [PATCH] t1450: fix quoting of NUL byte when corrupting pack","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-08-02T16:20:13Z","receivedAt":"2020-08-02T16:20:27Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Sun, Aug 2, 2020 at 7:35 AM Martin Ågren <martin.agren@gmail.com> wrote:\n> No worries! Thanks for having a look at the patch. Is there anything\n> that could be done to make this clearer in the commit message? (I find it\n> quite awkward to discuss quoting: will the reader understand which\n> quoting is part of my own formatting of the message vs which is part of\n> the quoting issue I want to get across!?)\n\nThis is indeed a problem...\n\nPerhaps something along these lines (generic boilerplate\nfor any single-quote fixes, that should be adjusted for the\nactual fix):\n\n    In the test scripts, the recommended style is, e.g.:\n\n        test_expect_success 'name' '\n            multi-line test\n            goes here\n        '\n\n    When using this style, any single quote in the multi-line\n    test section is actually closing the lone single quotes\n    that surround it.  To avoid confusion, minimize and/or\n    eliminate the use of single quotes here.\n\nChris\n"},{"id":"402663","messageId":"CAPig+cR=AdRbJgnVBJsbEtNmV41bRDkXE-tyWsztoYF7jZKeAQ@mail.gmail.com","threadId":"53960","inReplyTo":"20200802143018.5501-1-martin.agren@gmail.com","subject":"Re: [PATCH] t1450: fix quoting of NUL byte when corrupting pack","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-02T17:22:02Z","receivedAt":"2020-08-02T17:22:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 2, 2020 at 10:30 AM Martin Ågren <martin.agren@gmail.com> wrote:\n> Subject: [PATCH] t: don't spuriously close & open quotes\n> [...]\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\n> @@ -177,7 +177,7 @@ test_expect_success \"Sergey Vlasov's test case\" '\n>         git add ab.c ab &&\n> -       git commit -m 'initial' &&\n> +       git commit -m \"initial\" &&\n>         git mv ab a\n>  '\n\nThis was discovered[1] independently a couple weeks ago; glad to see\nit was picked up by your grep'ing, as well.\n\n[1]: https://lore.kernel.org/git/CAPig+cQpUu2UO-+jWn1nTaDykWnxwuEitzVB7PnW2SS_b7V8Hg@mail.gmail.com/\n"},{"id":"402665","messageId":"xmqqmu3drkvf.fsf@gitster.c.googlers.com","threadId":"53960","inReplyTo":"CAPx1GvdZNeuQqmYm8G62Zr02k=B5GK69xPw84WnvMCeJU7_amQ@mail.gmail.com","subject":"Re: [PATCH] t1450: fix quoting of NUL byte when corrupting pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-02T17:57:40Z","receivedAt":"2020-08-02T17:57:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Torek <chris.torek@gmail.com> writes:\n\n> On Sun, Aug 2, 2020 at 7:35 AM Martin Ågren <martin.agren@gmail.com> wrote:\n>> No worries! Thanks for having a look at the patch. Is there anything\n>> that could be done to make this clearer in the commit message? (I find it\n>> quite awkward to discuss quoting: will the reader understand which\n>> quoting is part of my own formatting of the message vs which is part of\n>> the quoting issue I want to get across!?)\n>\n> This is indeed a problem...\n>\n> Perhaps something along these lines (generic boilerplate\n> for any single-quote fixes, that should be adjusted for the\n> actual fix):\n>\n>     In the test scripts, the recommended style is, e.g.:\n>\n>         test_expect_success 'name' '\n>             multi-line test\n>             goes here\n>         '\n>\n>     When using this style, any single quote in the multi-line\n>     test section is actually closing the lone single quotes\n>     that surround it.  To avoid confusion, minimize and/or\n>     eliminate the use of single quotes here.\n\nAnother thing that falls into the same class and probably be a good\naddition to the above \"tip\" is how $variables are interpolated, i.e.\n\n\ttest_expect_success 'test name' '\n\t\ttest-that-references $variable &&\n\t\tanother-test-that-references \"$variable\"\n\t'\n\nare 99% of the time the right way to refer to variable that is\nassigned outside the test itself (e.g. the whole four lines shown\nabove may be in a loop, \"for variable in a b c; ... ;done\").\n\n\ttest_expect_success 'test name' '\n\t\ttest-that-references '$variable' &&\n\t\tanother-test-that-references '\"$variable\"'\n\t'\n\nis most likely a wrong way to write for the first one (i.e. what if\nthe value in $variable has $IFS whitespace) and \"not wrong per-se\nbut unnecessary\" for the second one.\n\nSame applies to $(command) substitution, but it is more important.\n\"Step out of the quote, evaluate and step back into the quote\"\npattern would mean the command is evaluated while formulating the\nbody of the test, not while running the test, which often is not\nwhat the author intended.\n\nThanks.\n"},{"id":"403090","messageId":"cover.1596742823.git.martin.agren@gmail.com","threadId":"53960","inReplyTo":"20200802143018.5501-1-martin.agren@gmail.com","subject":"[PATCH v2 0/2] t: don't spuriously close and reopen quotes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-08-06T20:08:52Z","receivedAt":"2020-08-06T20:09:09Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is an updated version of the patch I posted inline in [1]. I've\ntweaked the end result a bit and pulled out the modifications to t4104\nto their own patch. t4104 is also where most of the interdiff is spent.\n\nI've updated the commit message of the main patch based on the\ndiscussion in the earlier thread.\n\n[1] https://lore.kernel.org/git/20200802143018.5501-1-martin.agren@gmail.com/\n\nMartin Ågren (2):\n  t: don't spuriously close and reopen quotes\n  t4104: modernize and simplify quoting\n\n t/t1400-update-ref.sh           |  2 +-\n t/t3501-revert-cherry-pick.sh   |  4 +--\n t/t3507-cherry-pick-conflict.sh |  4 +--\n t/t4005-diff-rename-2.sh        |  4 +--\n t/t4034-diff-words.sh           |  2 +-\n t/t4104-apply-boundary.sh       | 57 +++++++++------------------------\n t/t4150-am.sh                   |  8 ++---\n t/t4200-rerere.sh               |  2 +-\n t/t5302-pack-index.sh           |  2 +-\n t/t5510-fetch.sh                |  4 +--\n t/t5553-set-upstream.sh         |  6 ++--\n t/t6026-merge-attr.sh           |  4 +--\n t/t7001-mv.sh                   |  2 +-\n t/t7600-merge.sh                |  6 ++--\n t/t9001-send-email.sh           | 10 +++---\n t/t9100-git-svn-basic.sh        |  6 ++--\n t/t9401-git-cvsserver-crlf.sh   |  8 ++---\n t/t9402-git-cvsserver-refs.sh   |  2 +-\n 18 files changed, 53 insertions(+), 80 deletions(-)\n\nInterdiff against v1:\ndiff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh\nindex 19f08d9ccd..71ef4132d1 100755\n--- a/t/t4104-apply-boundary.sh\n+++ b/t/t4104-apply-boundary.sh\n@@ -3,80 +3,55 @@\n # Copyright (c) 2005 Junio C Hamano\n #\n \n-test_description='git apply boundary tests\n+test_description='git apply boundary tests'\n \n-'\n . ./test-lib.sh\n \n L=\"c d e f g h i j k l m n o p q r s t u v w x\"\n \n test_expect_success setup '\n-\tfor i in b $L y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b $L y >victim &&\n \tcat victim >original &&\n \tgit update-index --add victim &&\n \n \t# add to the head\n-\tfor i in a b $L y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines a b $L y >victim &&\n \tcat victim >add-a-expect &&\n \tgit diff victim >add-a-patch.with &&\n \tgit diff --unified=0 >add-a-patch.without &&\n \n \t# insert at line two\n-\tfor i in b a $L y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b a $L y >victim &&\n \tcat victim >insert-a-expect &&\n \tgit diff victim >insert-a-patch.with &&\n \tgit diff --unified=0 >insert-a-patch.without &&\n \n \t# modify at the head\n-\tfor i in a $L y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines a $L y >victim &&\n \tcat victim >mod-a-expect &&\n \tgit diff victim >mod-a-patch.with &&\n \tgit diff --unified=0 >mod-a-patch.without &&\n \n \t# remove from the head\n-\tfor i in $L y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines $L y >victim &&\n \tcat victim >del-a-expect &&\n \tgit diff victim >del-a-patch.with &&\n \tgit diff --unified=0 >del-a-patch.without &&\n \n \t# add to the tail\n-\tfor i in b $L y z\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b $L y z >victim &&\n \tcat victim >add-z-expect &&\n \tgit diff victim >add-z-patch.with &&\n \tgit diff --unified=0 >add-z-patch.without &&\n \n \t# modify at the tail\n-\tfor i in b $L z\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b $L z >victim &&\n \tcat victim >mod-z-expect &&\n \tgit diff victim >mod-z-patch.with &&\n \tgit diff --unified=0 >mod-z-patch.without &&\n \n \t# remove from the tail\n-\tfor i in b $L\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b $L >victim &&\n \tcat victim >del-z-expect &&\n \tgit diff victim >del-z-patch.with &&\n \tgit diff --unified=0 >del-z-patch.without\n@@ -88,15 +63,15 @@ for with in with without\n do\n \tcase \"$with\" in\n \twith) u= ;;\n-\twithout) u='--unidiff-zero ' ;;\n+\twithout) u=--unidiff-zero ;;\n \tesac\n \tfor kind in add-a add-z insert-a mod-a mod-z del-a del-z\n \tdo\n \t\ttest_expect_success \"apply $kind-patch $with context\" '\n \t\t\tcat original >victim &&\n \t\t\tgit update-index victim &&\n-\t\t\tgit apply --index $u$kind-patch.$with &&\n-\t\t\ttest_cmp $kind-expect victim\n+\t\t\tgit apply --index $u \"$kind-patch.$with\" &&\n+\t\t\ttest_cmp \"$kind-expect\" victim\n \t\t'\n \tdone\n done\n@@ -110,13 +85,12 @@ do\n \ttest_expect_success \"apply non-git $kind-patch without context\" '\n \t\tcat original >victim &&\n \t\tgit update-index victim &&\n-\t\tgit apply --unidiff-zero --index $kind-ng.without &&\n-\t\ttest_cmp $kind-expect victim\n+\t\tgit apply --unidiff-zero --index \"$kind-ng.without\" &&\n+\t\ttest_cmp \"$kind-expect\" victim\n \t'\n done\n \n test_expect_success 'two lines' '\n-\n \t>file &&\n \tgit add file &&\n \techo aaa >file &&\n@@ -125,11 +99,10 @@ test_expect_success 'two lines' '\n \techo bbb >file &&\n \tgit add file &&\n \ttest_must_fail git apply --check patch\n-\n '\n \n test_expect_success 'apply patch with 3 context lines matching at end' '\n-\t{ echo a; echo b; echo c; echo d; } >file &&\n+\ttest_write_lines a b c d >file &&\n \tgit add file &&\n \techo e >>file &&\n \tgit diff >patch &&\ndiff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh\nindex 1f9d13819f..e9c575c359 100755\n--- a/t/t9100-git-svn-basic.sh\n+++ b/t/t9100-git-svn-basic.sh\n@@ -63,7 +63,7 @@ test_expect_success \"$name\" '\n \n \n name='detect node change from file to directory #1'\n-test_expect_success \"$name\" \"\n+test_expect_success \"$name\" '\n \tmkdir dir/new_file &&\n \tmv dir/file dir/new_file/file &&\n \tmv dir/new_file dir/file &&\n@@ -72,7 +72,7 @@ test_expect_success \"$name\" \"\n \tgit commit -m \"$name\" &&\n \ttest_must_fail git svn set-tree --find-copies-harder --rmdir \\\n \t\tremotes/git-svn..mybranch\n-\"\n+'\n \n \n name='detect node change from directory to file #1'\n-- \n2.28.0.199.g4234a9100e\n\n"},{"id":"403091","messageId":"c60416a111bada9624b8b64235ac5c4dd3dedac4.1596742823.git.martin.agren@gmail.com","threadId":"53960","inReplyTo":"cover.1596742823.git.martin.agren@gmail.com","subject":"[PATCH v2 1/2] t: don't spuriously close and reopen quotes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-08-06T20:08:53Z","receivedAt":"2020-08-06T20:09:14Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In the test scripts, the recommended style is, e.g.:\n\n    test_expect_success 'name' '\n        do-something somehow &&\n        do-some-more testing\n    '\n\nWhen using this style, any single quote in the multi-line test section\nis actually closing the lone single quotes that surround it.\n\nIt can be a non-issue in practice:\n\n    test_expect_success 'sed a little' '\n        sed -e 's/hi/lo/' in >out # \"ok\": no whitespace in s/hi/lo/\n    '\n\nOr it can be a bug in the test, e.g., because variable interpolation\nhappens before the test even begins executing:\n\n    v=abc\n\n    test_expect_success 'variable interpolation' '\n        v=def &&\n        echo '\"$v\"' # abc\n    '\n\nChange several such in-test single quotes to use double quotes instead\nor, in a few cases, drop them altogether. These were identified using\nsome crude grepping. We're not fixing any test bugs here, but we're\nhopefully making these tests slightly easier to grok and to maintain.\n\nThere are legitimate use cases for closing a quote and opening a new\none, e.g., both '\\'' and '\"'\"' can be used to produce a literal single\nquote. I'm not touching any of those here.\n\nIn t9401, tuck the redirecting \">\" to the filename while we're touching\nthose lines.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t1400-update-ref.sh           |  2 +-\n t/t3501-revert-cherry-pick.sh   |  4 ++--\n t/t3507-cherry-pick-conflict.sh |  4 ++--\n t/t4005-diff-rename-2.sh        |  4 ++--\n t/t4034-diff-words.sh           |  2 +-\n t/t4150-am.sh                   |  8 ++++----\n t/t4200-rerere.sh               |  2 +-\n t/t5302-pack-index.sh           |  2 +-\n t/t5510-fetch.sh                |  4 ++--\n t/t5553-set-upstream.sh         |  6 +++---\n t/t6026-merge-attr.sh           |  4 ++--\n t/t7001-mv.sh                   |  2 +-\n t/t7600-merge.sh                |  6 +++---\n t/t9001-send-email.sh           | 10 +++++-----\n t/t9100-git-svn-basic.sh        |  6 +++---\n t/t9401-git-cvsserver-crlf.sh   |  8 ++++----\n t/t9402-git-cvsserver-refs.sh   |  2 +-\n 17 files changed, 38 insertions(+), 38 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 27171f8261..d0d36750bc 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -324,7 +324,7 @@ test_expect_success \"create $m (logged by config)\" '\n test_expect_success \"update $m (logged by config)\" '\n \ttest_config core.logAllRefUpdates true &&\n \tGIT_COMMITTER_DATE=\"2005-05-26 23:33\" \\\n-\tgit update-ref HEAD'\" $B $A \"'-m \"Switch\" &&\n+\tgit update-ref HEAD $B $A -m \"Switch\" &&\n \ttest $B = $(git show-ref -s --verify $m)\n '\n test_expect_success \"set $m (logged by config)\" '\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 7c1da21df1..3669dfb1be 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -47,7 +47,7 @@ test_expect_success 'cherry-pick --nonsense' '\n \tgit diff --exit-code HEAD &&\n \ttest_must_fail git cherry-pick --nonsense 2>msg &&\n \tgit diff --exit-code HEAD \"$pos\" &&\n-\ttest_i18ngrep '[Uu]sage:' msg\n+\ttest_i18ngrep \"[Uu]sage:\" msg\n '\n \n test_expect_success 'revert --nonsense' '\n@@ -56,7 +56,7 @@ test_expect_success 'revert --nonsense' '\n \tgit diff --exit-code HEAD &&\n \ttest_must_fail git revert --nonsense 2>msg &&\n \tgit diff --exit-code HEAD \"$pos\" &&\n-\ttest_i18ngrep '[Uu]sage:' msg\n+\ttest_i18ngrep \"[Uu]sage:\" msg\n '\n \n test_expect_success 'cherry-pick after renaming branch' '\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 752bc43487..f107622a9e 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -512,7 +512,7 @@ test_expect_success 'commit after failed cherry-pick adds -s at the right place'\n \tSigned-off-by: C O Mitter <committer@example.com>\n \t# Conflicts:\n \tEOF\n-\tgrep -e \"^# Conflicts:\" -e '^Signed-off-by' .git/COMMIT_EDITMSG >actual &&\n+\tgrep -e \"^# Conflicts:\" -e \"^Signed-off-by\" .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual &&\n \n \tcat <<-\\EOF >expected &&\n@@ -541,7 +541,7 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n \tSigned-off-by: C O Mitter <committer@example.com>\n \tConflicts:\n \tEOF\n-\tgrep -e \"^Conflicts:\" -e '^Signed-off-by' .git/COMMIT_EDITMSG >actual &&\n+\tgrep -e \"^Conflicts:\" -e \"^Signed-off-by\" .git/COMMIT_EDITMSG >actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t4005-diff-rename-2.sh b/t/t4005-diff-rename-2.sh\nindex f542d2929d..d18a80493c 100755\n--- a/t/t4005-diff-rename-2.sh\n+++ b/t/t4005-diff-rename-2.sh\n@@ -14,8 +14,8 @@ test_expect_success 'setup reference tree' '\n \tgit update-index --add COPYING rezrov &&\n \ttree=$(git write-tree) &&\n \techo $tree &&\n-\tsed -e 's/HOWEVER/However/' <COPYING >COPYING.1 &&\n-\tsed -e 's/GPL/G.P.L/g' <COPYING >COPYING.2 &&\n+\tsed -e \"s/HOWEVER/However/\" <COPYING >COPYING.1 &&\n+\tsed -e \"s/GPL/G.P.L/g\" <COPYING >COPYING.2 &&\n \torigoid=$(git hash-object COPYING) &&\n \toid1=$(git hash-object COPYING.1) &&\n \toid2=$(git hash-object COPYING.2)\ndiff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh\nindex fb145aa173..0c8fb39ced 100755\n--- a/t/t4034-diff-words.sh\n+++ b/t/t4034-diff-words.sh\n@@ -102,7 +102,7 @@ test_expect_success 'word diff with runs of whitespace' '\n '\n \n test_expect_success '--word-diff=porcelain' '\n-\tsed 's/#.*$//' >expect <<-EOF &&\n+\tsed \"s/#.*$//\" >expect <<-EOF &&\n \t\tdiff --git a/pre b/post\n \t\tindex $pre..$post 100644\n \t\t--- a/pre\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex bda4586a79..855ed11b32 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -989,7 +989,7 @@ test_expect_success 'am -s unexpected trailer block' '\n \tSigned-off-by: J C H <j@c.h>\n \tEOF\n \tgit commit -F msg &&\n-\tgit cat-file commit HEAD | sed -e '1,/^$/d' >original &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >original &&\n \tgit format-patch --stdout -1 >patch &&\n \n \tgit reset --hard HEAD^ &&\n@@ -998,7 +998,7 @@ test_expect_success 'am -s unexpected trailer block' '\n \t\tcat original &&\n \t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n \t) >expect &&\n-\tgit cat-file commit HEAD | sed -e '1,/^$/d' >actual &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >actual &&\n \ttest_cmp expect actual &&\n \n \tcat >msg <<-\\EOF &&\n@@ -1009,7 +1009,7 @@ test_expect_success 'am -s unexpected trailer block' '\n \tEOF\n \tgit reset HEAD^ &&\n \tgit commit -F msg file &&\n-\tgit cat-file commit HEAD | sed -e '1,/^$/d' >original &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >original &&\n \tgit format-patch --stdout -1 >patch &&\n \n \tgit reset --hard HEAD^ &&\n@@ -1020,7 +1020,7 @@ test_expect_success 'am -s unexpected trailer block' '\n \t\techo &&\n \t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n \t) >expect &&\n-\tgit cat-file commit HEAD | sed -e '1,/^$/d' >actual &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 831d424c47..f99385c191 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -364,7 +364,7 @@ test_expect_success 'set up an unresolved merge' '\n \tgit reset --hard &&\n \tgit checkout version2 &&\n \tfifth=$(git rev-parse fifth) &&\n-\techo \"$fifth\t\tbranch 'fifth' of .\" |\n+\techo \"$fifth\t\tbranch fifth of .\" |\n \tgit fmt-merge-msg >msg &&\n \tancestor=$(git merge-base version2 fifth) &&\n \ttest_must_fail git merge-recursive \"$ancestor\" -- HEAD fifth &&\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 8981c9b90e..d1a6a3c37a 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -15,7 +15,7 @@ test_expect_success 'setup' '\n \ti=1 &&\n \twhile test $i -le 100\n \tdo\n-\t\tiii=$(printf '%03i' $i)\n+\t\tiii=$(printf \"%03i\" $i)\n \t\ttest-tool genrandom \"bar\" 200 > wide_delta_$iii &&\n \t\ttest-tool genrandom \"baz $iii\" 50 >> wide_delta_$iii &&\n \t\ttest-tool genrandom \"foo\"$i 100 > deep_delta_$iii &&\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex a66dbe0bde..6cb954e2f0 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -213,7 +213,7 @@ test_expect_success 'fetch tags when there is no tags' '\n test_expect_success 'fetch following tags' '\n \n \tcd \"$D\" &&\n-\tgit tag -a -m 'annotated' anno HEAD &&\n+\tgit tag -a -m \"annotated\" anno HEAD &&\n \tgit tag light HEAD &&\n \n \tmkdir four &&\n@@ -331,7 +331,7 @@ test_expect_success 'bundle does not prerequisite objects' '\n test_expect_success 'bundle should be able to create a full history' '\n \n \tcd \"$D\" &&\n-\tgit tag -a -m '1.0' v1.0 master &&\n+\tgit tag -a -m \"1.0\" v1.0 master &&\n \tgit bundle create bundle4 v1.0\n \n '\ndiff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh\nindex 81975ad8f9..7622981cbf 100755\n--- a/t/t5553-set-upstream.sh\n+++ b/t/t5553-set-upstream.sh\n@@ -81,7 +81,7 @@ test_expect_success 'fetch --set-upstream http://nosuchdomain.example.com fails\n \n test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '\n \tclear_config other other2 &&\n-\turl=\"file://'\"$PWD\"'\" &&\n+\turl=\"file://$PWD\" &&\n \tgit fetch --set-upstream \"$url\" &&\n \tcheck_config master \"$url\" HEAD &&\n \tcheck_config_missing other &&\n@@ -158,7 +158,7 @@ test_expect_success 'pull --set-upstream upstream with more than one branch does\n test_expect_success 'pull --set-upstream with valid URL sets upstream to URL' '\n \tclear_config master other other2 &&\n \tgit checkout master &&\n-\turl=\"file://'\"$PWD\"'\" &&\n+\turl=\"file://$PWD\" &&\n \tgit pull --set-upstream \"$url\" &&\n \tcheck_config master \"$url\" HEAD &&\n \tcheck_config_missing other &&\n@@ -168,7 +168,7 @@ test_expect_success 'pull --set-upstream with valid URL sets upstream to URL' '\n test_expect_success 'pull --set-upstream with valid URL and branch sets branch' '\n \tclear_config master other other2 &&\n \tgit checkout master &&\n-\turl=\"file://'\"$PWD\"'\" &&\n+\turl=\"file://$PWD\" &&\n \tgit pull --set-upstream \"$url\" master &&\n \tcheck_config master \"$url\" refs/heads/master &&\n \tcheck_config_missing other &&\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex 5900358ce9..76a55f838c 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -122,7 +122,7 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file master^:text) &&\n \ta=$(git unpack-file side^:text) &&\n \tb=$(git unpack-file master:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 'text'\" &&\n+\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \trm -f $o $a $b\n@@ -149,7 +149,7 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file master^:text) &&\n \ta=$(git unpack-file anchor:text) &&\n \tb=$(git unpack-file master:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 'text'\" &&\n+\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \tsed -e 1,3d -e 4q $a >check-3 &&\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 36b50d0b4c..b64b736d2f 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -177,7 +177,7 @@ test_expect_success \"Sergey Vlasov's test case\" '\n \tdate >ab.c &&\n \tdate >ab/d &&\n \tgit add ab.c ab &&\n-\tgit commit -m 'initial' &&\n+\tgit commit -m \"initial\" &&\n \tgit mv ab a\n '\n \ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 1d45f9a4ed..dffcf053db 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -246,7 +246,7 @@ test_expect_success 'merge --squash c3 with c7' '\n \t#\tfile\n \tEOF\n \tgit cat-file commit HEAD >raw &&\n-\tsed -e '1,/^$/d' raw >actual &&\n+\tsed -e \"1,/^$/d\" raw >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -268,7 +268,7 @@ test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' '\n \t#\tfile\n \tEOF\n \tgit cat-file commit HEAD >raw &&\n-\tsed -e '1,/^$/d' raw >actual &&\n+\tsed -e \"1,/^$/d\" raw >actual &&\n \ttest_i18ncmp expect actual\n '\n \n@@ -292,7 +292,7 @@ test_expect_success 'merge c3 with c7 with --squash commit.cleanup = scissors' '\n \t#\tfile\n \tEOF\n \tgit cat-file commit HEAD >raw &&\n-\tsed -e '1,/^$/d' raw >actual &&\n+\tsed -e \"1,/^$/d\" raw >actual &&\n \ttest_i18ncmp expect actual\n '\n \ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex ec261085ec..3d68570450 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -1551,7 +1551,7 @@ test_expect_success $PREREQ '8-bit and sendemail.transferencoding=quoted-printab\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-8bit \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1568,7 +1568,7 @@ test_expect_success $PREREQ '8-bit and sendemail.transferencoding=base64' '\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-8bit \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1594,7 +1594,7 @@ test_expect_success $PREREQ 'convert from quoted-printable to base64' '\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-qp \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1624,7 +1624,7 @@ test_expect_success $PREREQ 'CRLF and sendemail.transferencoding=quoted-printabl\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-crlf \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1641,7 +1641,7 @@ test_expect_success $PREREQ 'CRLF and sendemail.transferencoding=base64' '\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\temail-using-crlf \\\n \t\t2>errors >out &&\n-\tsed '1,/^$/d' msgtxt1 >actual &&\n+\tsed \"1,/^$/d\" msgtxt1 >actual &&\n \ttest_cmp expected actual\n '\n \ndiff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh\nindex 9f2d19ecc4..e9c575c359 100755\n--- a/t/t9100-git-svn-basic.sh\n+++ b/t/t9100-git-svn-basic.sh\n@@ -63,16 +63,16 @@ test_expect_success \"$name\" '\n \n \n name='detect node change from file to directory #1'\n-test_expect_success \"$name\" \"\n+test_expect_success \"$name\" '\n \tmkdir dir/new_file &&\n \tmv dir/file dir/new_file/file &&\n \tmv dir/new_file dir/file &&\n \tgit update-index --remove dir/file &&\n \tgit update-index --add dir/file/file &&\n-\tgit commit -m '$name' &&\n+\tgit commit -m \"$name\" &&\n \ttest_must_fail git svn set-tree --find-copies-harder --rmdir \\\n \t\tremotes/git-svn..mybranch\n-\"\n+'\n \n \n name='detect node change from directory to file #1'\ndiff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh\nindex 84787eee9a..c7a0dd84a4 100755\n--- a/t/t9401-git-cvsserver-crlf.sh\n+++ b/t/t9401-git-cvsserver-crlf.sh\n@@ -167,10 +167,10 @@ test_expect_success 'adding files' '\n \n test_expect_success 'updating' '\n     git pull gitcvs.git &&\n-    echo 'hi' > subdir/newfile.bin &&\n-    echo 'junk' > subdir/file.h &&\n-    echo 'hi' > subdir/newfile.c &&\n-    echo 'hello' >> binfile.bin &&\n+    echo \"hi\" >subdir/newfile.bin &&\n+    echo \"junk\" >subdir/file.h &&\n+    echo \"hi\" >subdir/newfile.c &&\n+    echo \"hello\" >>binfile.bin &&\n     git add subdir/newfile.bin subdir/file.h subdir/newfile.c binfile.bin &&\n     git commit -q -m \"Add and change some files\" &&\n     git push gitcvs.git >/dev/null &&\ndiff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh\nindex cf31ace667..6436c91a3c 100755\n--- a/t/t9402-git-cvsserver-refs.sh\n+++ b/t/t9402-git-cvsserver-refs.sh\n@@ -178,7 +178,7 @@ test_expect_success 'setup v1.2 on b1' '\n \tmkdir cdir &&\n \techo \"cdir/cfile\" >cdir/cfile &&\n \tgit add -A cdir adir t3 t2 &&\n-\tgit commit -q -m 'v1.2' &&\n+\tgit commit -q -m \"v1.2\" &&\n \tgit tag v1.2 &&\n \tgit push --tags gitcvs.git b1:b1\n '\n-- \n2.28.0.199.g4234a9100e\n\n"},{"id":"403092","messageId":"3be4695a2778c9c72a943e438e61a18d4de468a5.1596742823.git.martin.agren@gmail.com","threadId":"53960","inReplyTo":"cover.1596742823.git.martin.agren@gmail.com","subject":"[PATCH v2 2/2] t4104: modernize and simplify quoting","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-08-06T20:08:54Z","receivedAt":"2020-08-06T20:09:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Drop whitespace in the value of `$test_description` and in a test body\nand use `test_write_lines`.\n\nStop defining `$u` with a trailing space just so that we can tuck it in\nlike `git foo $u$more...` and get minimal whitespace in the command:\n`git foo $u $more...` is more readable at the \"cost\" of an empty `$u`\nyielding `git foo  something...`.\n\nFinally, avoid using single quotes within the test scripts to repeatedly\nclose and reopen the quotes that wrap the test scripts (see the previous\ncommit). This \"unnecessary\" quoting does mean that the verbose test\noutput shows the interpolated values, i.e., the shell code we're\nrunning. But the downside is that the source of the script does *not*\nshow the shell code we're eventually executing, leaving the reader to\nreason about what we really do and whether there are any quoting issues.\n(There aren't.)\n\nWhere we run through loops to generate several \"identical but different\"\ntests, the test message contains the interpolated variables we're\nlooping on, meaning one can always identify exactly which instance has\nfailed, even if the verbose test output shows the exact same test body\nseveral times.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t4104-apply-boundary.sh | 57 +++++++++++----------------------------\n 1 file changed, 15 insertions(+), 42 deletions(-)\n\ndiff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh\nindex 32e3b0ee0b..71ef4132d1 100755\n--- a/t/t4104-apply-boundary.sh\n+++ b/t/t4104-apply-boundary.sh\n@@ -3,80 +3,55 @@\n # Copyright (c) 2005 Junio C Hamano\n #\n \n-test_description='git apply boundary tests\n+test_description='git apply boundary tests'\n \n-'\n . ./test-lib.sh\n \n L=\"c d e f g h i j k l m n o p q r s t u v w x\"\n \n test_expect_success setup '\n-\tfor i in b '\"$L\"' y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b $L y >victim &&\n \tcat victim >original &&\n \tgit update-index --add victim &&\n \n \t# add to the head\n-\tfor i in a b '\"$L\"' y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines a b $L y >victim &&\n \tcat victim >add-a-expect &&\n \tgit diff victim >add-a-patch.with &&\n \tgit diff --unified=0 >add-a-patch.without &&\n \n \t# insert at line two\n-\tfor i in b a '\"$L\"' y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b a $L y >victim &&\n \tcat victim >insert-a-expect &&\n \tgit diff victim >insert-a-patch.with &&\n \tgit diff --unified=0 >insert-a-patch.without &&\n \n \t# modify at the head\n-\tfor i in a '\"$L\"' y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines a $L y >victim &&\n \tcat victim >mod-a-expect &&\n \tgit diff victim >mod-a-patch.with &&\n \tgit diff --unified=0 >mod-a-patch.without &&\n \n \t# remove from the head\n-\tfor i in '\"$L\"' y\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines $L y >victim &&\n \tcat victim >del-a-expect &&\n \tgit diff victim >del-a-patch.with &&\n \tgit diff --unified=0 >del-a-patch.without &&\n \n \t# add to the tail\n-\tfor i in b '\"$L\"' y z\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b $L y z >victim &&\n \tcat victim >add-z-expect &&\n \tgit diff victim >add-z-patch.with &&\n \tgit diff --unified=0 >add-z-patch.without &&\n \n \t# modify at the tail\n-\tfor i in b '\"$L\"' z\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b $L z >victim &&\n \tcat victim >mod-z-expect &&\n \tgit diff victim >mod-z-patch.with &&\n \tgit diff --unified=0 >mod-z-patch.without &&\n \n \t# remove from the tail\n-\tfor i in b '\"$L\"'\n-\tdo\n-\t\techo $i\n-\tdone >victim &&\n+\ttest_write_lines b $L >victim &&\n \tcat victim >del-z-expect &&\n \tgit diff victim >del-z-patch.with &&\n \tgit diff --unified=0 >del-z-patch.without\n@@ -88,15 +63,15 @@ for with in with without\n do\n \tcase \"$with\" in\n \twith) u= ;;\n-\twithout) u='--unidiff-zero ' ;;\n+\twithout) u=--unidiff-zero ;;\n \tesac\n \tfor kind in add-a add-z insert-a mod-a mod-z del-a del-z\n \tdo\n \t\ttest_expect_success \"apply $kind-patch $with context\" '\n \t\t\tcat original >victim &&\n \t\t\tgit update-index victim &&\n-\t\t\tgit apply --index '\"$u$kind-patch.$with\"' &&\n-\t\t\ttest_cmp '\"$kind\"'-expect victim\n+\t\t\tgit apply --index $u \"$kind-patch.$with\" &&\n+\t\t\ttest_cmp \"$kind-expect\" victim\n \t\t'\n \tdone\n done\n@@ -110,13 +85,12 @@ do\n \ttest_expect_success \"apply non-git $kind-patch without context\" '\n \t\tcat original >victim &&\n \t\tgit update-index victim &&\n-\t\tgit apply --unidiff-zero --index '\"$kind-ng.without\"' &&\n-\t\ttest_cmp '\"$kind\"'-expect victim\n+\t\tgit apply --unidiff-zero --index \"$kind-ng.without\" &&\n+\t\ttest_cmp \"$kind-expect\" victim\n \t'\n done\n \n test_expect_success 'two lines' '\n-\n \t>file &&\n \tgit add file &&\n \techo aaa >file &&\n@@ -125,11 +99,10 @@ test_expect_success 'two lines' '\n \techo bbb >file &&\n \tgit add file &&\n \ttest_must_fail git apply --check patch\n-\n '\n \n test_expect_success 'apply patch with 3 context lines matching at end' '\n-\t{ echo a; echo b; echo c; echo d; } >file &&\n+\ttest_write_lines a b c d >file &&\n \tgit add file &&\n \techo e >>file &&\n \tgit diff >patch &&\n-- \n2.28.0.199.g4234a9100e\n\n"},{"id":"403095","messageId":"CAPig+cSjHg2-WYqdkZAS0ye1goj_=5RN3mdjt0-4kSBqNm6WLg@mail.gmail.com","threadId":"53960","inReplyTo":"c60416a111bada9624b8b64235ac5c4dd3dedac4.1596742823.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 1/2] t: don't spuriously close and reopen quotes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-06T20:26:15Z","receivedAt":"2020-08-06T20:26:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 6, 2020 at 4:09 PM Martin Ågren <martin.agren@gmail.com> wrote:\n> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\n> @@ -364,7 +364,7 @@ test_expect_success 'set up an unresolved merge' '\n>         fifth=$(git rev-parse fifth) &&\n> -       echo \"$fifth            branch 'fifth' of .\" |\n> +       echo \"$fifth            branch fifth of .\" |\n>         git fmt-merge-msg >msg &&\n\nThis one is a bit weird. It really seems as if the intent was to quote\nthe word \"fifth\" in the merge message, so dropping the quotes\naltogether seems wrong. However, the file 'msg' is never even\nconsulted in this test (or any other test), so is this just \"dead\ncode\" (including the leading 'fifth=' assignment which also is\notherwise unused outside the 'echo')?\n\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> @@ -213,7 +213,7 @@ test_expect_success 'fetch tags when there is no tags' '\n>  test_expect_success 'fetch following tags' '\n>         cd \"$D\" &&\n> -       git tag -a -m 'annotated' anno HEAD &&\n> +       git tag -a -m \"annotated\" anno HEAD &&\n\nThere are a fair number of these quoted single-token arguments\ncontaining no special characters which don't actually need to be\nquoted at all, so an alternative would be simply to drop the quotes\naltogether, making the commands less syntactically noisy. However,\nthat might be outside the intended scope of this change.\n"},{"id":"403124","messageId":"CAN0heSpjgc0GUHnebeBdtr6Yny3Y_jsjz5hTfJqw15bZiPc8HQ@mail.gmail.com","threadId":"53960","inReplyTo":"CAPig+cSjHg2-WYqdkZAS0ye1goj_=5RN3mdjt0-4kSBqNm6WLg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] t: don't spuriously close and reopen quotes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-08-07T08:45:29Z","receivedAt":"2020-08-07T08:45:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Eric,\n\nThanks for having a look.\n\nOn Thu, 6 Aug 2020 at 22:26, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Thu, Aug 6, 2020 at 4:09 PM Martin Ågren <martin.agren@gmail.com> wrote:\n> > diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\n> > @@ -364,7 +364,7 @@ test_expect_success 'set up an unresolved merge' '\n> >         fifth=$(git rev-parse fifth) &&\n> > -       echo \"$fifth            branch 'fifth' of .\" |\n> > +       echo \"$fifth            branch fifth of .\" |\n> >         git fmt-merge-msg >msg &&\n>\n> This one is a bit weird. It really seems as if the intent was to quote\n> the word \"fifth\" in the merge message, so dropping the quotes\n> altogether seems wrong. However, the file 'msg' is never even\n> consulted in this test (or any other test), so is this just \"dead\n> code\" (including the leading 'fifth=' assignment which also is\n> otherwise unused outside the 'echo')?\n\nHuh, good catch. The tests showed up in v2 of the patch [1] and there's\nnot enough context in the archives to tell whether something was partly\nremoved from an earlier \"v1.5\" or partly added but not all the way.\n\nWe expect to fail the merge -- and we do, and not because of this msg\nthing -- and then we look around a little, but we don't resolve the\nmerge and make a commit. And from what I can tell, there wouldn't be\nmuch point in making a commit. So I should be able to safely drop this\n\"dead code\" entirely.\n\n[1] https://lore.kernel.org/git/20100805112837.GL13779@burratino/>\n\n> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> > @@ -213,7 +213,7 @@ test_expect_success 'fetch tags when there is no tags' '\n> >  test_expect_success 'fetch following tags' '\n> >         cd \"$D\" &&\n> > -       git tag -a -m 'annotated' anno HEAD &&\n> > +       git tag -a -m \"annotated\" anno HEAD &&\n>\n> There are a fair number of these quoted single-token arguments\n> containing no special characters which don't actually need to be\n> quoted at all, so an alternative would be simply to drop the quotes\n> altogether, making the commands less syntactically noisy. However,\n> that might be outside the intended scope of this change.\n\nFWIW, I find something like `git foo -m \"message\"` a lot more\nintuitive/reasonable than, e.g., `git foo \"HEAD\"` to signal that here's\na message that could in principle have whitespace, even if this one\ndoesn't. For all of these, I tried to follow the surrounding style. For\nexample in t9401, where we do `echo \"hi\"`. We certainly don't need the\nquotes there, but t9401 is very consistent on this and I didn't want to\nmuddy that unnecessarily.\n\nIf we say that \"don't use quotes if you don't need to\" is a reasonable\nthing to strive for, I can drop these in a reroll. I think I'll be\ninjecting a patch anyway for the \"msg\" you pointed out in t4200, so I\ncan certainly tweak this patch to be a bit more aggressive in dropping\nunnecessary quotes.\n\nThanks\nMartin\n"},{"id":"403132","messageId":"CAPig+cR+JmQsj9qAALq6oxYQb9E94TAEqXHx+dAt=E0FZH6WwA@mail.gmail.com","threadId":"53960","inReplyTo":"CAN0heSpjgc0GUHnebeBdtr6Yny3Y_jsjz5hTfJqw15bZiPc8HQ@mail.gmail.com","subject":"Re: [PATCH v2 1/2] t: don't spuriously close and reopen quotes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-07T16:17:22Z","receivedAt":"2020-08-07T16:17:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 7, 2020 at 4:45 AM Martin Ågren <martin.agren@gmail.com> wrote:\n> On Thu, 6 Aug 2020 at 22:26, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Thu, Aug 6, 2020 at 4:09 PM Martin Ågren <martin.agren@gmail.com> wrote:\n> > > -    echo \"$fifth      branch 'fifth' of .\" |\n> > > +    echo \"$fifth      branch fifth of .\" |\n> >\n> > This one is a bit weird. It really seems as if the intent was to quote\n> > the word \"fifth\" in the merge message, so dropping the quotes\n> > altogether seems wrong. However, the file 'msg' is never even\n> > consulted in this test (or any other test), so is this just \"dead\n> > code\" (including the leading 'fifth=' assignment which also is\n> > otherwise unused outside the 'echo')?\n>\n> Huh, good catch. [...] So I should be able to safely drop this\n> \"dead code\" entirely.\n\nThat could be done atop this series if there is no other reason to\nre-roll.\n\n> > > -    git tag -a -m 'annotated' anno HEAD &&\n> > > +    git tag -a -m \"annotated\" anno HEAD &&\n> >\n> > There are a fair number of these quoted single-token arguments\n> > containing no special characters which don't actually need to be\n> > quoted at all, so an alternative would be simply to drop the quotes\n> > altogether, making the commands less syntactically noisy. However,\n> > that might be outside the intended scope of this change.\n>\n> If we say that \"don't use quotes if you don't need to\" is a reasonable\n> thing to strive for, I can drop these in a reroll. I think I'll be\n> injecting a patch anyway for the \"msg\" you pointed out in t4200, so I\n> can certainly tweak this patch to be a bit more aggressive in dropping\n> unnecessary quotes.\n\nNo need. Matching the local convention makes sense, and I don't insist\nupon such a change at all. Mine was a non-actionable observation, and\nit's entirely subjective anyhow.\n\nI don't feel strongly about whether the series should be re-rolled.\nIt's true that dropping that dead code (mentioned above) would make more\nsense coming before the patch which fixes up the quoting, but it\nwouldn't bother me if the dead-code removal was done as a follow-on\npatch.\n"},{"id":"403135","messageId":"xmqq4kpeidg5.fsf@gitster.c.googlers.com","threadId":"53960","inReplyTo":"CAPig+cR+JmQsj9qAALq6oxYQb9E94TAEqXHx+dAt=E0FZH6WwA@mail.gmail.com","subject":"Re: [PATCH v2 1/2] t: don't spuriously close and reopen quotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T17:16:26Z","receivedAt":"2020-08-07T17:16:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> I don't feel strongly about whether the series should be re-rolled.\n> It's true that dropping that dead code (mentioned above) would make more\n> sense coming before the patch which fixes up the quoting, but it\n> wouldn't bother me if the dead-code removal was done as a follow-on\n> patch.\n\nYeah, I agree that a preparatory clean-up followed by the main\nchange would logically make more sense, but if it is swapped it is\nnot the end of the world ;-)\n\nThanks, both.\n"}]}