{"thread":{"id":"46312","subject":"[PATCH] t5534: fix misleading grep invocation","startedAt":"2017-07-05T11:38:03Z","lastAt":"2017-07-07T16:41:56Z","messageCount":6,"participants":["Johannes Schindelin","Junio C Hamano","Michael J Gruber"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"323835","messageId":"936f43ce2c993e545deaee5e196c018933375214.1499254650.git.johannes.schindelin@gmx.de","threadId":"46312","inReplyTo":null,"subject":"[PATCH] t5534: fix misleading grep invocation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-07-05T11:37:49Z","receivedAt":"2017-07-05T11:38:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It seems to be a little-known feature of `grep` (and it certainly came\nas a surprise to this here developer who believed to know the Unix tools\npretty well) that multiple patterns can be passed in the same\ncommand-line argument simply by separating them by newlines. Watch, and\nlearn:\n\n\t$ printf '1\\n2\\n3\\n' | grep \"$(printf '1\\n3\\n')\"\n\t1\n\t3\n\nThat behavior also extends to patterns passed via `-e`, and it is not\nmodified by passing the option `-E` (but trying this with -P issues the\nerror \"grep: the -P option only supports a single pattern\").\n\nIt seems that there are more old Unix hands who are surprised by this\nbehavior, as grep invocations of the form\n\n\tgrep \"$(git rev-parse A B) C\" file\n\nwere introduced in a85b377d041 (push: the beginning of \"git push\n--signed\", 2014-09-12), and later faithfully copy-edited in b9459019bbb\n(push: heed user.signingkey for signed pushes, 2014-10-22).\n\nPlease note that the output of `git rev-parse A B` separates the object\nIDs via *newlines*, not via spaces, and those newlines are preserved\nbecause the interpolation is enclosed in double quotes.\n\nAs a consequence, these tests try to validate that the file contains\neither A's object ID, or B's object ID followed by C, or both. Clearly,\nhowever, what the test wanted to see is that there is a line that\ncontains all of them.\n\nThis is clearly unintended, and the grep invocations in question really\nmatch too many lines.\n\nFix the test by avoiding the newlines in the patterns.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t5534-push-signed.sh | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex 5bcb288f5c4..464ffdd147a 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -119,8 +119,11 @@ test_expect_success GPG 'signed push sends push certificate' '\n \t\tsed -n -e \"s/^nonce /NONCE=/p\" -e \"/^$/q\" dst/push-cert\n \t) >expect &&\n \n-\tgrep \"$(git rev-parse noop ff) refs/heads/ff\" dst/push-cert &&\n-\tgrep \"$(git rev-parse noop noff) refs/heads/noff\" dst/push-cert &&\n+\tnoop=$(git rev-parse noop) &&\n+\tff=$(git rev-parse ff) &&\n+\tnoff=$(git rev-parse noff) &&\n+\tgrep \"$noop $ff refs/heads/ff\" dst/push-cert &&\n+\tgrep \"$noop $noff refs/heads/noff\" dst/push-cert &&\n \ttest_cmp expect dst/push-cert-status\n '\n \n@@ -200,8 +203,11 @@ test_expect_success GPG 'fail without key and heed user.signingkey' '\n \t\tsed -n -e \"s/^nonce /NONCE=/p\" -e \"/^$/q\" dst/push-cert\n \t) >expect &&\n \n-\tgrep \"$(git rev-parse noop ff) refs/heads/ff\" dst/push-cert &&\n-\tgrep \"$(git rev-parse noop noff) refs/heads/noff\" dst/push-cert &&\n+\tnoop=$(git rev-parse noop) &&\n+\tff=$(git rev-parse ff) &&\n+\tnoff=$(git rev-parse noff) &&\n+\tgrep \"$noop $ff refs/heads/ff\" dst/push-cert &&\n+\tgrep \"$noop $noff refs/heads/noff\" dst/push-cert &&\n \ttest_cmp expect dst/push-cert-status\n '\n \n\nbase-commit: 5116f791c12dda6b6c22fa85b600a8e30dfa168a\n-- \n2.13.2.windows.1\n\nPublished-As: https://github.com/dscho/git/releases/tag/t5534-fix-grep-pattern-v1\nFetch-It-Via: git fetch https://github.com/dscho/git t5534-fix-grep-pattern-v1\n"},{"id":"323839","messageId":"xmqq1spukfyw.fsf@gitster.mtv.corp.google.com","threadId":"46312","inReplyTo":"936f43ce2c993e545deaee5e196c018933375214.1499254650.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] t5534: fix misleading grep invocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-05T16:26:15Z","receivedAt":"2017-07-05T16:26:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> It seems to be a little-known feature of `grep` (and it certainly came\n> as a surprise to this here developer who believed to know the Unix tools\n> pretty well) that multiple patterns can be passed in the same\n> command-line argument simply by separating them by newlines. Watch, and\n> learn:\n>\n> \t$ printf '1\\n2\\n3\\n' | grep \"$(printf '1\\n3\\n')\"\n> \t1\n> \t3\n>\n> That behavior also extends to patterns passed via `-e`, and it is not\n> modified by passing the option `-E` (but trying this with -P issues the\n> error \"grep: the -P option only supports a single pattern\").\n>\n> It seems that there are more old Unix hands who are surprised by this\n> behavior, as grep invocations of the form\n>\n> \tgrep \"$(git rev-parse A B) C\" file\n>\n> were introduced in a85b377d041 (push: the beginning of \"git push\n> --signed\", 2014-09-12), and later faithfully copy-edited in b9459019bbb\n> (push: heed user.signingkey for signed pushes, 2014-10-22).\n>\n> Please note that the output of `git rev-parse A B` separates the object\n> IDs via *newlines*, not via spaces, and those newlines are preserved\n> because the interpolation is enclosed in double quotes.\n>\n> As a consequence, these tests try to validate that the file contains\n> either A's object ID, or B's object ID followed by C, or both. Clearly,\n> however, what the test wanted to see is that there is a line that\n> contains all of them.\n>\n> This is clearly unintended, and the grep invocations in question really\n> match too many lines.\n>\n> Fix the test by avoiding the newlines in the patterns.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n\nThe invocation this fixes is not just misleading but simply wrong.\nNicely spotted.\n\nThanks, will queue.\n\n>  t/t5534-push-signed.sh | 14 ++++++++++----\n>  1 file changed, 10 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\n> index 5bcb288f5c4..464ffdd147a 100755\n> --- a/t/t5534-push-signed.sh\n> +++ b/t/t5534-push-signed.sh\n> @@ -119,8 +119,11 @@ test_expect_success GPG 'signed push sends push certificate' '\n>  \t\tsed -n -e \"s/^nonce /NONCE=/p\" -e \"/^$/q\" dst/push-cert\n>  \t) >expect &&\n>  \n> -\tgrep \"$(git rev-parse noop ff) refs/heads/ff\" dst/push-cert &&\n> -\tgrep \"$(git rev-parse noop noff) refs/heads/noff\" dst/push-cert &&\n> +\tnoop=$(git rev-parse noop) &&\n> +\tff=$(git rev-parse ff) &&\n> +\tnoff=$(git rev-parse noff) &&\n> +\tgrep \"$noop $ff refs/heads/ff\" dst/push-cert &&\n> +\tgrep \"$noop $noff refs/heads/noff\" dst/push-cert &&\n>  \ttest_cmp expect dst/push-cert-status\n>  '\n>  \n> @@ -200,8 +203,11 @@ test_expect_success GPG 'fail without key and heed user.signingkey' '\n>  \t\tsed -n -e \"s/^nonce /NONCE=/p\" -e \"/^$/q\" dst/push-cert\n>  \t) >expect &&\n>  \n> -\tgrep \"$(git rev-parse noop ff) refs/heads/ff\" dst/push-cert &&\n> -\tgrep \"$(git rev-parse noop noff) refs/heads/noff\" dst/push-cert &&\n> +\tnoop=$(git rev-parse noop) &&\n> +\tff=$(git rev-parse ff) &&\n> +\tnoff=$(git rev-parse noff) &&\n> +\tgrep \"$noop $ff refs/heads/ff\" dst/push-cert &&\n> +\tgrep \"$noop $noff refs/heads/noff\" dst/push-cert &&\n>  \ttest_cmp expect dst/push-cert-status\n>  '\n>  \n>\n> base-commit: 5116f791c12dda6b6c22fa85b600a8e30dfa168a\n"},{"id":"323889","messageId":"22feab0a-ca75-2aea-1ec9-2f71fe40c9d0@grubix.eu","threadId":"46312","inReplyTo":"xmqq1spukfyw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t5534: fix misleading grep invocation","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2017-07-06T09:20:06Z","receivedAt":"2017-07-06T09:20:24Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 05.07.2017 18:26:\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n>> It seems to be a little-known feature of `grep` (and it certainly came\n>> as a surprise to this here developer who believed to know the Unix tools\n>> pretty well) that multiple patterns can be passed in the same\n>> command-line argument simply by separating them by newlines. Watch, and\n>> learn:\n>>\n>> \t$ printf '1\\n2\\n3\\n' | grep \"$(printf '1\\n3\\n')\"\n>> \t1\n>> \t3\n>>\n>> That behavior also extends to patterns passed via `-e`, and it is not\n>> modified by passing the option `-E` (but trying this with -P issues the\n>> error \"grep: the -P option only supports a single pattern\").\n>>\n>> It seems that there are more old Unix hands who are surprised by this\n>> behavior, as grep invocations of the form\n>>\n>> \tgrep \"$(git rev-parse A B) C\" file\n>>\n>> were introduced in a85b377d041 (push: the beginning of \"git push\n>> --signed\", 2014-09-12), and later faithfully copy-edited in b9459019bbb\n>> (push: heed user.signingkey for signed pushes, 2014-10-22).\n>>\n>> Please note that the output of `git rev-parse A B` separates the object\n>> IDs via *newlines*, not via spaces, and those newlines are preserved\n>> because the interpolation is enclosed in double quotes.\n>>\n>> As a consequence, these tests try to validate that the file contains\n>> either A's object ID, or B's object ID followed by C, or both. Clearly,\n>> however, what the test wanted to see is that there is a line that\n>> contains all of them.\n>>\n>> This is clearly unintended, and the grep invocations in question really\n>> match too many lines.\n>>\n>> Fix the test by avoiding the newlines in the patterns.\n>>\n>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>> ---\n> \n> The invocation this fixes is not just misleading but simply wrong.\n> Nicely spotted.\n\nIn addition, the patch makes sure to catch any rev-parse failures which\nthe original invocation shove under the rug.\n\n> Thanks, will queue.\n\nThanks from the faithful copy-editor ;)\n\nHow did you spot this? Are there grep versions that behave differently?\n\n>>  t/t5534-push-signed.sh | 14 ++++++++++----\n>>  1 file changed, 10 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\n>> index 5bcb288f5c4..464ffdd147a 100755\n>> --- a/t/t5534-push-signed.sh\n>> +++ b/t/t5534-push-signed.sh\n>> @@ -119,8 +119,11 @@ test_expect_success GPG 'signed push sends push certificate' '\n>>  \t\tsed -n -e \"s/^nonce /NONCE=/p\" -e \"/^$/q\" dst/push-cert\n>>  \t) >expect &&\n>>  \n>> -\tgrep \"$(git rev-parse noop ff) refs/heads/ff\" dst/push-cert &&\n>> -\tgrep \"$(git rev-parse noop noff) refs/heads/noff\" dst/push-cert &&\n>> +\tnoop=$(git rev-parse noop) &&\n>> +\tff=$(git rev-parse ff) &&\n>> +\tnoff=$(git rev-parse noff) &&\n>> +\tgrep \"$noop $ff refs/heads/ff\" dst/push-cert &&\n>> +\tgrep \"$noop $noff refs/heads/noff\" dst/push-cert &&\n>>  \ttest_cmp expect dst/push-cert-status\n>>  '\n>>  \n>> @@ -200,8 +203,11 @@ test_expect_success GPG 'fail without key and heed user.signingkey' '\n>>  \t\tsed -n -e \"s/^nonce /NONCE=/p\" -e \"/^$/q\" dst/push-cert\n>>  \t) >expect &&\n>>  \n>> -\tgrep \"$(git rev-parse noop ff) refs/heads/ff\" dst/push-cert &&\n>> -\tgrep \"$(git rev-parse noop noff) refs/heads/noff\" dst/push-cert &&\n>> +\tnoop=$(git rev-parse noop) &&\n>> +\tff=$(git rev-parse ff) &&\n>> +\tnoff=$(git rev-parse noff) &&\n>> +\tgrep \"$noop $ff refs/heads/ff\" dst/push-cert &&\n>> +\tgrep \"$noop $noff refs/heads/noff\" dst/push-cert &&\n>>  \ttest_cmp expect dst/push-cert-status\n>>  '\n>>  \n>>\n>> base-commit: 5116f791c12dda6b6c22fa85b600a8e30dfa168a\n"},{"id":"323900","messageId":"xmqqk23lfsbd.fsf@gitster.mtv.corp.google.com","threadId":"46312","inReplyTo":"22feab0a-ca75-2aea-1ec9-2f71fe40c9d0@grubix.eu","subject":"Re: [PATCH] t5534: fix misleading grep invocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-06T16:23:02Z","receivedAt":"2017-07-06T16:23:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@grubix.eu> writes:\n\n> Junio C Hamano venit, vidit, dixit 05.07.2017 18:26:\n>\n>> The invocation this fixes is not just misleading but simply wrong.\n>> Nicely spotted.\n>\n> In addition, the patch makes sure to catch any rev-parse failures which\n> the original invocation shove under the rug.\n\nYeah, good thing that this got fixed ;-)\n\n"},{"id":"323947","messageId":"alpine.DEB.2.21.1.1707071259550.84669@virtualbox","threadId":"46312","inReplyTo":"22feab0a-ca75-2aea-1ec9-2f71fe40c9d0@grubix.eu","subject":"Re: [PATCH] t5534: fix misleading grep invocation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-07-07T11:13:06Z","receivedAt":"2017-07-07T11:13:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Thu, 6 Jul 2017, Michael J Gruber wrote:\n\n> Junio C Hamano venit, vidit, dixit 05.07.2017 18:26:\n> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> > \n> >> It seems to be a little-known feature of `grep` (and it certainly came\n> >> as a surprise to this here developer who believed to know the Unix tools\n> >> pretty well) that multiple patterns can be passed in the same\n> >> command-line argument simply by separating them by newlines. Watch, and\n> >> learn:\n> >>\n> >> \t$ printf '1\\n2\\n3\\n' | grep \"$(printf '1\\n3\\n')\"\n> >> \t1\n> >> \t3\n> >>\n> >> That behavior also extends to patterns passed via `-e`, and it is not\n> >> modified by passing the option `-E` (but trying this with -P issues the\n> >> error \"grep: the -P option only supports a single pattern\").\n> >>\n> >> It seems that there are more old Unix hands who are surprised by this\n> >> behavior, as grep invocations of the form\n> >>\n> >> \tgrep \"$(git rev-parse A B) C\" file\n> >>\n> >> were introduced in a85b377d041 (push: the beginning of \"git push\n> >> --signed\", 2014-09-12), and later faithfully copy-edited in b9459019bbb\n> >> (push: heed user.signingkey for signed pushes, 2014-10-22).\n> >>\n> >> Please note that the output of `git rev-parse A B` separates the object\n> >> IDs via *newlines*, not via spaces, and those newlines are preserved\n> >> because the interpolation is enclosed in double quotes.\n> >>\n> >> As a consequence, these tests try to validate that the file contains\n> >> either A's object ID, or B's object ID followed by C, or both. Clearly,\n> >> however, what the test wanted to see is that there is a line that\n> >> contains all of them.\n> >>\n> >> This is clearly unintended, and the grep invocations in question really\n> >> match too many lines.\n>\n> [...]\n>\n> How did you spot this? Are there grep versions that behave differently?\n\nYes, there are grep versions that behave differently... how did you guess?\n\nI am in the middle of an extended investigation trying to assess how\nfeasible it would be to use a native Win32 port of BusyBox (started by\nlong-time Git contributor Nguyễn Thái Ngọc Duy) in Git for Windows to\nexecute the many, many remaining Unix shell scripts that are a core part\nof Git (including crucial functionality such as bisect, rebase, stash and\nsubmodule, for which we suffer portability and performance problems).\n\nAnd it is BusyBox' grep that does not handle newlines in the pattern\nargument to split it into two alternative patterns.\n\nI first considered patching BusyBox to adhere to the expected behavior,\nbut then I looked closer and saw that the test's grep invocations actually\nmatched two lines instead of what I expected. An even closer look made me\nsuspect that the original intention was different from what the script\nactually does, and for once I tried to be nice in my commit message.\n\nCiao,\nDscho"},{"id":"323962","messageId":"xmqq8tk0ci7q.fsf@gitster.mtv.corp.google.com","threadId":"46312","inReplyTo":"alpine.DEB.2.21.1.1707071259550.84669@virtualbox","subject":"Re: [PATCH] t5534: fix misleading grep invocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-07T16:41:45Z","receivedAt":"2017-07-07T16:41:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Yes, there are grep versions that behave differently... how did you guess?\n>\n> I am in the middle of an extended investigation trying to assess how\n> feasible it would be to use a native Win32 port of BusyBox (started by\n> long-time Git contributor Nguyễn Thái Ngọc Duy) in Git for Windows to\n> execute the many, many remaining Unix shell scripts that are a core part\n> of Git (including crucial functionality such as bisect, rebase, stash and\n> submodule, for which we suffer portability and performance problems).\n\nI've long thought that BusyBox was primarily about size and not\nabout performance, but I can imagine that it would be a big win to\nbe able to run things like \"mkdir\" and \"rm\" without fork/exec, as it\nis likely to be extermely more expensive than preparing to call and\nactually making system calls mkdir(2), unlink(2), etc.\n\nInteresting.  I learned a new thing today, but apparently that\nFEATURE_SH_NOFORK was not a very new development.  I do not think\nanybody is crazy enough to attempt making Git a nofork applet,\nthough ;-)\n\n\n\n"}]}