{"thread":{"id":"42263","subject":"t4151 missing quotes","startedAt":"2016-05-09T16:09:45Z","lastAt":"2016-05-10T13:48:36Z","messageCount":13,"participants":["Armin Kunaschik","Eric Sunshine","Junio C Hamano","Stefan Beller","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"285926","messageId":"CALR6jEiBsU+jQ8VoRsniMdztCpVDemQ3r00W-OXdRP6ZEt9CFg@mail.gmail.com","threadId":"42263","inReplyTo":null,"subject":"t4151 missing quotes","fromName":"Armin Kunaschik","fromEmail":"megabreit@googlemail.com","sentAt":"2016-05-09T16:09:45Z","receivedAt":"2016-05-09T16:09:45Z","isPatch":false,"sender":{"key":"megabreit@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/10429218?v=4"},"body":"Hi there,\n\nskipping through some failed tests I found more (smaller) problems\ninside the test... when test arguments are empty they need to be\nquoted (quite a lot test in this sentence).\n\nError is like\nt4151-am-abort.sh[5]: test: argument expected\n\nMy patch:\n\n*** t4151-am-abort.sh Mon May 9 17:51:44 2016\n--- t4151-am-abort.sh.orig Fri Apr 29 23:37:00 2016\n***************\n*** 67,73 ****\n test_expect_success 'am -3 --skip removes otherfile-4' '\n git reset --hard initial &&\n test_must_fail git am -3 0003-*.patch &&\n! test 3 -eq \"$(git ls-files -u | wc -l)\" &&\n test 4 = \"$(cat otherfile-4)\" &&\n git am --skip &&\n test_cmp_rev initial HEAD &&\n--- 67,73 ----\n test_expect_success 'am -3 --skip removes otherfile-4' '\n git reset --hard initial &&\n test_must_fail git am -3 0003-*.patch &&\n! test 3 -eq $(git ls-files -u | wc -l) &&\n test 4 = \"$(cat otherfile-4)\" &&\n git am --skip &&\n test_cmp_rev initial HEAD &&\n***************\n*** 78,88 ****\n test_expect_success 'am -3 --abort removes otherfile-4' '\n git reset --hard initial &&\n test_must_fail git am -3 0003-*.patch &&\n! test 3 -eq \"$(git ls-files -u | wc -l)\" &&\n test 4 = \"$(cat otherfile-4)\" &&\n git am --abort &&\n test_cmp_rev initial HEAD &&\n! test -z \"$(git ls-files -u)\" &&\n test_path_is_missing otherfile-4\n '\n\n--- 78,88 ----\n test_expect_success 'am -3 --abort removes otherfile-4' '\n git reset --hard initial &&\n test_must_fail git am -3 0003-*.patch &&\n! test 3 -eq $(git ls-files -u | wc -l) &&\n test 4 = \"$(cat otherfile-4)\" &&\n git am --abort &&\n test_cmp_rev initial HEAD &&\n! test -z $(git ls-files -u) &&\n test_path_is_missing otherfile-4\n '\n\n***************\n*** 146,152 ****\n git reset &&\n rm -f otherfile-4 otherfile-2 file-1 file-2 &&\n test_must_fail git am -3 initial.patch 0003-*.patch &&\n! test 3 -eq \"$(git ls-files -u | wc -l)\" &&\n test 4 = \"$(cat otherfile-4)\" &&\n git am --abort &&\n test -z \"$(git ls-files -u)\" &&\n--- 146,152 ----\n git reset &&\n rm -f otherfile-4 otherfile-2 file-1 file-2 &&\n test_must_fail git am -3 initial.patch 0003-*.patch &&\n! test 3 -eq $(git ls-files -u | wc -l) &&\n test 4 = \"$(cat otherfile-4)\" &&\n git am --abort &&\n test -z \"$(git ls-files -u)\" &&\n\nRegards,\nArmin\n"},{"id":"285930","messageId":"CAPig+cTbAA8xDWvCXbBF+HJpxONS38hcjAiNuocC+PUBro9ALg@mail.gmail.com","threadId":"42263","inReplyTo":"CALR6jEiBsU+jQ8VoRsniMdztCpVDemQ3r00W-OXdRP6ZEt9CFg@mail.gmail.com","subject":"Re: t4151 missing quotes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-09T16:22:14Z","receivedAt":"2016-05-09T16:22:14Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik\n wrote:\n> skipping through some failed tests I found more (smaller) problems\n> inside the test... when test arguments are empty they need to be\n> quoted (quite a lot test in this sentence).\n>\n> Error is like\n> t4151-am-abort.sh[5]: test: argument expected\n>\n> My patch:\n>\n> *** t4151-am-abort.sh Mon May 9 17:51:44 2016\n> --- t4151-am-abort.sh.orig Fri Apr 29 23:37:00 2016\n> ***************\n> *** 67,73 ****\n> test_expect_success 'am -3 --skip removes otherfile-4' '\n> git reset --hard initial &&\n> test_must_fail git am -3 0003-*.patch &&\n> ! test 3 -eq \"$(git ls-files -u | wc -l)\" &&\n> test 4 = \"$(cat otherfile-4)\" &&\n> git am --skip &&\n> test_cmp_rev initial HEAD &&\n> --- 67,73 ----\n> test_expect_success 'am -3 --skip removes otherfile-4' '\n> git reset --hard initial &&\n> test_must_fail git am -3 0003-*.patch &&\n> ! test 3 -eq $(git ls-files -u | wc -l) &&\n> test 4 = \"$(cat otherfile-4)\" &&\n> git am --skip &&\n> test_cmp_rev initial HEAD &&\n> ***************\n\nSome comments:\n\nQuoting the output of 'wc -l' will break the tests on Mac OS X and BSD\nsince the output contains leading whitespace which won't match the \"3\"\non the other side of the '='.\n\nYour diff is backward, comparing 'current' against 'original', which\nmakes it difficult to read. Reviewers on this list expect to see\n'original' compared against 'current'.\n\nUse a unified format to make the diff easier to read; or just use\ngit-diff or git-format patch, which is even simpler.\n\nIt's not clear how the output of 'wc -l' could ever be the empty\nstring. Perhaps git-ls-files is dying and causing the pipe to abort\nbefore 'wc -l' ever outputs anything? Without additional information\nabout the problem you're experiencing, it's difficult to judge if this\nchange is a good idea.\n"},{"id":"285933","messageId":"CAPig+cS9OeqGz9xM802pDR68tO7fLos3HKWezy8_y2gsyn_fAA@mail.gmail.com","threadId":"42263","inReplyTo":"CAPig+cTbAA8xDWvCXbBF+HJpxONS38hcjAiNuocC+PUBro9ALg@mail.gmail.com","subject":"Re: t4151 missing quotes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-09T16:26:45Z","receivedAt":"2016-05-09T16:26:45Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 9, 2016 at 12:22 PM, Eric Sunshine wrote:\n> On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik\n> wrote:\n>> *** t4151-am-abort.sh Mon May 9 17:51:44 2016\n>> --- t4151-am-abort.sh.orig Fri Apr 29 23:37:00 2016\n>> ! test 3 -eq \"$(git ls-files -u | wc -l)\" &&\n>> ! test 3 -eq $(git ls-files -u | wc -l) &&\n>\n> Some comments:\n>\n> Quoting the output of 'wc -l' will break the tests on Mac OS X and BSD\n> since the output contains leading whitespace which won't match the \"3\"\n> on the other side of the '='.\n\nThis isn't quite accurate. Since the test is using '-eq' rather than\n'=', the leading whitespace won't be a problem, but it can still be\nproblematic if you're somehow getting an empty result from the\npipeline:\n\n % test 3 -eq \"$(echo)\"\n -bash: test: : integer expression expected\n\n> Your diff is backward, comparing 'current' against 'original', which\n> makes it difficult to read. Reviewers on this list expect to see\n> 'original' compared against 'current'.\n>\n> Use a unified format to make the diff easier to read; or just use\n> git-diff or git-format patch, which is even simpler.\n>\n> It's not clear how the output of 'wc -l' could ever be the empty\n> string. Perhaps git-ls-files is dying and causing the pipe to abort\n> before 'wc -l' ever outputs anything? Without additional information\n> about the problem you're experiencing, it's difficult to judge if this\n> change is a good idea.\n"},{"id":"285935","messageId":"CALR6jEgaNSAQOpxSK46h71PMRhakDa=UCC5gbTyg77BcaOaoPg@mail.gmail.com","threadId":"42263","inReplyTo":"CAPig+cTbAA8xDWvCXbBF+HJpxONS38hcjAiNuocC+PUBro9ALg@mail.gmail.com","subject":"Re: t4151 missing quotes","fromName":"Armin Kunaschik","fromEmail":"megabreit@googlemail.com","sentAt":"2016-05-09T16:35:49Z","receivedAt":"2016-05-09T16:35:49Z","isPatch":false,"sender":{"key":"megabreit@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/10429218?v=4"},"body":"Sorry, this was my first patch to the list. I'll do better :-)\nYou are right about the \"wc -l\" parts. Maybe I was a bit over\npessimistic. Throw away my last mail.\nIn my case test 9 ran unsuccessful because of an empty \"git ls-files -u\"\n\nThis reduces the diff to this one (hopefully the right way now):\n*** ./t4151-am-abort.sh.orig Fri Apr 29 23:37:00 2016\n--- ./t4151-am-abort.sh Mon May 9 18:28:18 2016\n***************\n*** 82,88 ****\n test 4 = \"$(cat otherfile-4)\" &&\n git am --abort &&\n test_cmp_rev initial HEAD &&\n! test -z $(git ls-files -u) &&\n test_path_is_missing otherfile-4\n '\n\n--- 82,88 ----\n test 4 = \"$(cat otherfile-4)\" &&\n git am --abort &&\n test_cmp_rev initial HEAD &&\n! test -z \"$(git ls-files -u)\" &&\n test_path_is_missing otherfile-4\n '\n\nAll the other similar occurrences are correctly quoted.\n\nOn Mon, May 9, 2016 at 6:22 PM, Eric Sunshine wrote:\n> On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik\n> wrote:\n>> skipping through some failed tests I found more (smaller) problems\n>> inside the test... when test arguments are empty they need to be\n>> quoted (quite a lot test in this sentence).\n>>\n>> Error is like\n>> t4151-am-abort.sh[5]: test: argument expected\n>>\n>> My patch:\n>>\n>> *** t4151-am-abort.sh Mon May 9 17:51:44 2016\n>> --- t4151-am-abort.sh.orig Fri Apr 29 23:37:00 2016\n>> ***************\n>> *** 67,73 ****\n>> test_expect_success 'am -3 --skip removes otherfile-4' '\n>> git reset --hard initial &&\n>> test_must_fail git am -3 0003-*.patch &&\n>> ! test 3 -eq \"$(git ls-files -u | wc -l)\" &&\n>> test 4 = \"$(cat otherfile-4)\" &&\n>> git am --skip &&\n>> test_cmp_rev initial HEAD &&\n>> --- 67,73 ----\n>> test_expect_success 'am -3 --skip removes otherfile-4' '\n>> git reset --hard initial &&\n>> test_must_fail git am -3 0003-*.patch &&\n>> ! test 3 -eq $(git ls-files -u | wc -l) &&\n>> test 4 = \"$(cat otherfile-4)\" &&\n>> git am --skip &&\n>> test_cmp_rev initial HEAD &&\n>> ***************\n>\n> Some comments:\n>\n> Quoting the output of 'wc -l' will break the tests on Mac OS X and BSD\n> since the output contains leading whitespace which won't match the \"3\"\n> on the other side of the '='.\n>\n> Your diff is backward, comparing 'current' against 'original', which\n> makes it difficult to read. Reviewers on this list expect to see\n> 'original' compared against 'current'.\n>\n> Use a unified format to make the diff easier to read; or just use\n> git-diff or git-format patch, which is even simpler.\n>\n> It's not clear how the output of 'wc -l' could ever be the empty\n> string. Perhaps git-ls-files is dying and causing the pipe to abort\n> before 'wc -l' ever outputs anything? Without additional information\n> about the problem you're experiencing, it's difficult to judge if this\n> change is a good idea.\n"},{"id":"285954","messageId":"CAPig+cQS32O0jxJkWRB2r5vQw3LHVeD_vK+3O9Q3ominO38S2g@mail.gmail.com","threadId":"42263","inReplyTo":"CALR6jEgaNSAQOpxSK46h71PMRhakDa=UCC5gbTyg77BcaOaoPg@mail.gmail.com","subject":"Re: t4151 missing quotes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-09T17:57:35Z","receivedAt":"2016-05-09T17:57:35Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[please don't top-post on this list]\n\nOn Mon, May 9, 2016 at 12:35 PM, Armin Kunaschik\n wrote:\n> Sorry, this was my first patch to the list. I'll do better :-)\n> You are right about the \"wc -l\" parts. Maybe I was a bit over\n> pessimistic. Throw away my last mail.\n\nDone :-)\n\n> In my case test 9 ran unsuccessful because of an empty \"git ls-files -u\"\n\nOkay, that makes perfect sense and does indeed deserve to be fixed.\n\n> This reduces the diff to this one (hopefully the right way now):\n\nPerhaps you can turn this into a proper patch acceptable for inclusion\nin the project. If you're interested in attempting this, take a look\nat Documentation/Submitting.\n\n> *** ./t4151-am-abort.sh.orig Fri Apr 29 23:37:00 2016\n> --- ./t4151-am-abort.sh Mon May 9 18:28:18 2016\n> ***************\n> *** 82,88 ****\n> test 4 = \"$(cat otherfile-4)\" &&\n> git am --abort &&\n> test_cmp_rev initial HEAD &&\n> ! test -z $(git ls-files -u) &&\n> test_path_is_missing otherfile-4\n> '\n>\n> --- 82,88 ----\n> test 4 = \"$(cat otherfile-4)\" &&\n> git am --abort &&\n> test_cmp_rev initial HEAD &&\n> ! test -z \"$(git ls-files -u)\" &&\n> test_path_is_missing otherfile-4\n> '\n\nThis fix looks fine. Thanks.\n"},{"id":"285955","messageId":"CALR6jEhR7cAGTnv=VQACrSLh0CEW37oPb9gZ5W+dwz7H8irodA@mail.gmail.com","threadId":"42263","inReplyTo":"CAPig+cQS32O0jxJkWRB2r5vQw3LHVeD_vK+3O9Q3ominO38S2g@mail.gmail.com","subject":"Re: t4151 missing quotes","fromName":"Armin Kunaschik","fromEmail":"megabreit@googlemail.com","sentAt":"2016-05-09T18:11:13Z","receivedAt":"2016-05-09T18:11:13Z","isPatch":false,"sender":{"key":"megabreit@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/10429218?v=4"},"body":"I'm currently concentrating on finding problems with my setup... this\nis already a tough job :-)\nI'm a git beginner, and Documentation/SubmittingPatches would keep me\nbusy for a week.\nSo anybody feel free to submit this thingy.\n\nArmin\n"},{"id":"285961","messageId":"xmqq37pruklb.fsf@gitster.mtv.corp.google.com","threadId":"42263","inReplyTo":"CALR6jEgaNSAQOpxSK46h71PMRhakDa=UCC5gbTyg77BcaOaoPg@mail.gmail.com","subject":"Re: t4151 missing quotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-09T18:56:00Z","receivedAt":"2016-05-09T18:56:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Something like this follows Documentation/SubmittingPatches, except\nthat it further needs your Sign-off before mine, which I can forge\nif you say it is OK.\n\nThanks for a report and an analysis of the issue.\n\n-- >8 --\nFrom: Armin Kunaschik \nSubject: t4151: make sure argument to 'test -z' is given\n\n88d50724 (am --skip: revert changes introduced by failed 3way merge,\n2015-06-06), unlike all the other patches in the series, forgot to\nquote the output from \"$(git ls-files -u)\" when using it as the\nargument to \"test -z\", leading to a syntax error.\n\nNote that $(git ls-files -u | wc -l) are deliberately left unquoted,\nas some implementations of \"wc -l\" includes extra blank characters\nin its output and cannot be compared as string, i.e. \"test 0 = $(...)\".\n\nSigned-off-by: \nSigned-off-by: Junio C Hamano \n---\n t/t4151-am-abort.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex 833e7b2..b878c21 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -82,7 +82,7 @@ test_expect_success 'am -3 --abort removes otherfile-4' '\n \ttest 4 = \"$(cat otherfile-4)\" &&\n \tgit am --abort &&\n \ttest_cmp_rev initial HEAD &&\n-\ttest -z $(git ls-files -u) &&\n+\ttest -z \"$(git ls-files -u)\" &&\n \ttest_path_is_missing otherfile-4\n '\n \n"},{"id":"285963","messageId":"CAGZ79ka6SXUqQNZJJGMsYYnjbX1qRqmYToxrDE0Zc8YOmss1OA@mail.gmail.com","threadId":"42263","inReplyTo":"xmqq37pruklb.fsf@gitster.mtv.corp.google.com","subject":"Re: t4151 missing quotes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-09T19:19:24Z","receivedAt":"2016-05-09T19:19:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 9, 2016 at 11:56 AM, Junio C Hamano wrote:\n> Something like this follows Documentation/SubmittingPatches, except\n> that it further needs your Sign-off before mine, which I can forge\n> if you say it is OK.\n\nThe sign-off is a simple line at the end of the explanation for\nthe patch, which certifies that you wrote it or otherwise have\nthe right to pass it on as a open-source patch. The rules are\npretty simple: if you can certify the below:\n\n Developer's Certificate of Origin 1.1\n\n By making a contribution to this project, I certify that:\n\n (a) The contribution was created in whole or in part by me and I\n have the right to submit it under the open source license\n indicated in the file; or\n\n (b) The contribution is based upon previous work that, to the best\n of my knowledge, is covered under an appropriate open source\n license and I have the right under that license to submit that\n work with modifications, whether created in whole or in part\n by me, under the same open source license (unless I am\n permitted to submit under a different license), as indicated\n in the file; or\n\n (c) The contribution was provided directly to me by some other\n person who certified (a), (b) or (c) and I have not modified\n it.\n\n (d) I understand and agree that this project and the contribution\n are public and that a record of the contribution (including all\n personal information I submit with it, including my sign-off) is\n maintained indefinitely and may be redistributed consistent with\n this project or the open source license(s) involved.\n\n\n>\n> Thanks for a report and an analysis of the issue.\n>\n> -- >8 --\n> From: Armin Kunaschik \n> Subject: t4151: make sure argument to 'test -z' is given\n>\n> 88d50724 (am --skip: revert changes introduced by failed 3way merge,\n> 2015-06-06), unlike all the other patches in the series, forgot to\n> quote the output from \"$(git ls-files -u)\" when using it as the\n> argument to \"test -z\", leading to a syntax error.\n>\n> Note that $(git ls-files -u | wc -l) are deliberately left unquoted,\n> as some implementations of \"wc -l\" includes extra blank characters\n> in its output and cannot be compared as string, i.e. \"test 0 = $(...)\".\n>\n> Signed-off-by:\n> Signed-off-by: Junio C Hamano \n> ---\n> t/t4151-am-abort.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\n> index 833e7b2..b878c21 100755\n> --- a/t/t4151-am-abort.sh\n> +++ b/t/t4151-am-abort.sh\n> @@ -82,7 +82,7 @@ test_expect_success 'am -3 --abort removes otherfile-4' '\n> test 4 = \"$(cat otherfile-4)\" &&\n> git am --abort &&\n> test_cmp_rev initial HEAD &&\n> - test -z $(git ls-files -u) &&\n> + test -z \"$(git ls-files -u)\" &&\n> test_path_is_missing otherfile-4\n> '\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"285969","messageId":"CAPig+cS4Bj4N8d1a29z8=f30owOec1pB=yF32ZUPmDH2Tu2kXA@mail.gmail.com","threadId":"42263","inReplyTo":"xmqq37pruklb.fsf@gitster.mtv.corp.google.com","subject":"Re: t4151 missing quotes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-09T20:16:03Z","receivedAt":"2016-05-09T20:16:03Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 9, 2016 at 2:56 PM, Junio C Hamano wrote:\n> Something like this follows Documentation/SubmittingPatches [...]\n>\n> -- >8 --\n> From: Armin Kunaschik \n> Subject: t4151: make sure argument to 'test -z' is given\n>\n> 88d50724 (am --skip: revert changes introduced by failed 3way merge,\n> 2015-06-06), unlike all the other patches in the series, forgot to\n> quote the output from \"$(git ls-files -u)\" when using it as the\n> argument to \"test -z\", leading to a syntax error.\n\nTo make it clear that this was not a syntax error in the typical case,\nit might make sense to say:\n\n ...potentially leading to a syntax error if some earlier tests failed.\n\n> Note that $(git ls-files -u | wc -l) are deliberately left unquoted,\n> as some implementations of \"wc -l\" includes extra blank characters\n> in its output and cannot be compared as string, i.e. \"test 0 = $(...)\".\n\nAside from the above nit, this all looks good and (for what it's worth) is:\n\n Reviewed-by: Eric Sunshine \n\n> Signed-off-by:\n> Signed-off-by: Junio C Hamano \n> ---\n> t/t4151-am-abort.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\n> index 833e7b2..b878c21 100755\n> --- a/t/t4151-am-abort.sh\n> +++ b/t/t4151-am-abort.sh\n> @@ -82,7 +82,7 @@ test_expect_success 'am -3 --abort removes otherfile-4' '\n> test 4 = \"$(cat otherfile-4)\" &&\n> git am --abort &&\n> test_cmp_rev initial HEAD &&\n> - test -z $(git ls-files -u) &&\n> + test -z \"$(git ls-files -u)\" &&\n> test_path_is_missing otherfile-4\n> '\n"},{"id":"285971","messageId":"xmqqmvnzt0yx.fsf@gitster.mtv.corp.google.com","threadId":"42263","inReplyTo":"CAPig+cS4Bj4N8d1a29z8=f30owOec1pB=yF32ZUPmDH2Tu2kXA@mail.gmail.com","subject":"Re: t4151 missing quotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-09T20:45:10Z","receivedAt":"2016-05-09T20:45:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, May 9, 2016 at 2:56 PM, Junio C Hamano wrote:\n>> Something like this follows Documentation/SubmittingPatches [...]\n>>\n>> -- >8 --\n>> From: Armin Kunaschik \n>> Subject: t4151: make sure argument to 'test -z' is given\n>>\n>> 88d50724 (am --skip: revert changes introduced by failed 3way merge,\n>> 2015-06-06), unlike all the other patches in the series, forgot to\n>> quote the output from \"$(git ls-files -u)\" when using it as the\n>> argument to \"test -z\", leading to a syntax error.\n>\n> To make it clear that this was not a syntax error in the typical case,\n> it might make sense to say:\n>\n> ...potentially leading to a syntax error if some earlier tests failed.\n\nHmph, do we have a broken &&-chain?\n\nIf an earlier test fails and leaves an unmerged path, \"ls-files -u\"\nwould give some output, so \"test -z\" would get one or more non-empty\nstrings; if we feed multiple, this would fail. But we would not have\neven run \"test -z\" as long as we properly &&-chain these tests.\n\nI think the real issue is when the earlier step succeeds and does\nnot leave any unmerged path. In that case, we would run \"test -z\"\nwithout anything else on the command line, which would lead to an\nsyntax error.\n\n Side Note: /usr/bin/test and test (built into bash and dash)\n seem not to care about the lack of string in \"test -z \"\n and \"test -n \". It appears to me that they just take\n \"-z\" and \"-n\" without \"\" as a special case of \"test\n \" that is fed \"-z\" or \"-n\" as . Apparently, the\n platform Armin is working on doesn't.\n\nPerhaps\n\n ... leading to a syntax error on some platforms whose \"test\"\n does not interpret \"test -z\" (no other arguments) as testing if\n a string \"-z\" is the null string (which GNU test and test that\n is built into bash and dash seem to do).\n\nwould be an improvement?\n"},{"id":"285995","messageId":"CAPig+cSRUqSQZ1G73X6szfXjJEwopaO20H_k2vrmmy1qpEftLQ@mail.gmail.com","threadId":"42263","inReplyTo":"xmqqmvnzt0yx.fsf@gitster.mtv.corp.google.com","subject":"Re: t4151 missing quotes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-09T21:35:11Z","receivedAt":"2016-05-09T21:35:11Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 9, 2016 at 4:45 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> On Mon, May 9, 2016 at 2:56 PM, Junio C Hamano wrote:\n>>> Something like this follows Documentation/SubmittingPatches [...]\n>>>\n>>> -- >8 --\n>>> From: Armin Kunaschik \n>>> Subject: t4151: make sure argument to 'test -z' is given\n>>>\n>>> 88d50724 (am --skip: revert changes introduced by failed 3way merge,\n>>> 2015-06-06), unlike all the other patches in the series, forgot to\n>>> quote the output from \"$(git ls-files -u)\" when using it as the\n>>> argument to \"test -z\", leading to a syntax error.\n>>\n>> To make it clear that this was not a syntax error in the typical case,\n>> it might make sense to say:\n>>\n>> ...potentially leading to a syntax error if some earlier tests failed.\n>\n> Hmph, do we have a broken &&-chain?\n\nI don't know. Unfortunately, Armin didn't provide much information in\nhis initial email, saying only \"skipping through some failed tests\",\nwhich doesn't necessarily indicate if those tests failed or if he\nsomehow manually skipped them.\n\n> If an earlier test fails and leaves an unmerged path, \"ls-files -u\"\n> would give some output, so \"test -z\" would get one or more non-empty\n> strings; if we feed multiple, this would fail. But we would not have\n> even run \"test -z\" as long as we properly &&-chain these tests.\n>\n> I think the real issue is when the earlier step succeeds and does\n> not leave any unmerged path. In that case, we would run \"test -z\"\n> without anything else on the command line, which would lead to an\n> syntax error.\n>\n> Side Note: /usr/bin/test and test (built into bash and dash)\n> seem not to care about the lack of string in \"test -z \"\n> and \"test -n \". It appears to me that they just take\n> \"-z\" and \"-n\" without \"\" as a special case of \"test\n> \" that is fed \"-z\" or \"-n\" as . Apparently, the\n> platform Armin is working on doesn't.\n\nI also tested on Mac OS X and BSD, and they happily accept bare \"test\n-n\", as well (though, I don't doubt that there are old shells which\ncomplain).\n\n> Perhaps\n>\n> ... leading to a syntax error on some platforms whose \"test\"\n> does not interpret \"test -z\" (no other arguments) as testing if\n> a string \"-z\" is the null string (which GNU test and test that\n> is built into bash and dash seem to do).\n>\n> would be an improvement?\n\nYes, that sounds good.\n"},{"id":"286052","messageId":"CALR6jEiFq6kjB0pQgvvd0Pc3i1ZNbGSG2a=i7Hij0HmgUNjgEA@mail.gmail.com","threadId":"42263","inReplyTo":"CAPig+cSRUqSQZ1G73X6szfXjJEwopaO20H_k2vrmmy1qpEftLQ@mail.gmail.com","subject":"Re: t4151 missing quotes","fromName":"Armin Kunaschik","fromEmail":"megabreit@googlemail.com","sentAt":"2016-05-10T13:44:32Z","receivedAt":"2016-05-10T13:44:32Z","isPatch":false,"sender":{"key":"megabreit@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/10429218?v=4"},"body":"Sorry for any duplicate mails, the list blocked my html mail.\nNote to self: Don't use GMail on a tablet.\n\nOn Mon, May 9, 2016 at 11:35 PM, Eric Sunshine wrote:\n>>\n>> Hmph, do we have a broken &&-chain?\n>\n> I don't know. Unfortunately, Armin didn't provide much information in\n> his initial email, saying only \"skipping through some failed tests\",\n> which doesn't necessarily indicate if those tests failed or if he\n> somehow manually skipped them.\n\nIn t4151 there was only a problem with this test. All other tests\ninside t4151 were ok.\nSkipping through the tests referred to all git tests, not just t4151.\n\n>> If an earlier test fails and leaves an unmerged path, \"ls-files -u\"\n>> would give some output, so \"test -z\" would get one or more non-empty\n>> strings; if we feed multiple, this would fail. But we would not have\n>> even run \"test -z\" as long as we properly &&-chain these tests.\n>>\n>> I think the real issue is when the earlier step succeeds and does\n>> not leave any unmerged path. In that case, we would run \"test -z\"\n>> without anything else on the command line, which would lead to an\n>> syntax error.\n\nYes. While debugging the test, I saw a syntax error. I did not try to find out\nwhy the test argument is empty. It seems not necessary.. the test logic\nis still the same.\n\n>> Side Note: /usr/bin/test and test (built into bash and dash)\n>> seem not to care about the lack of string in \"test -z \"\n>> and \"test -n \". It appears to me that they just take\n>> \"-z\" and \"-n\" without \"\" as a special case of \"test\n>> \" that is fed \"-z\" or \"-n\" as . Apparently, the\n>> platform Armin is working on doesn't.\n>\n> I also tested on Mac OS X and BSD, and they happily accept bare \"test\n> -n\", as well (though, I don't doubt that there are old shells which\n> complain).\n\nI'm building on a quite current AIX 6.1 where /bin/sh defaults to /bin/ksh\nwhich is a posix shell (ksh88).\nUsing /bin/bash doesn't work because SHELL_PATH is only used in\ngit scripts but not in any t* test scripts.\n"},{"id":"286053","messageId":"20160510134836.GA8177@sigill.intra.peff.net","threadId":"42263","inReplyTo":"CALR6jEiFq6kjB0pQgvvd0Pc3i1ZNbGSG2a=i7Hij0HmgUNjgEA@mail.gmail.com","subject":"Re: t4151 missing quotes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-10T13:48:36Z","receivedAt":"2016-05-10T13:48:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 10, 2016 at 03:44:32PM +0200, Armin Kunaschik wrote:\n\n> I'm building on a quite current AIX 6.1 where /bin/sh defaults to /bin/ksh\n> which is a posix shell (ksh88).\n> Using /bin/bash doesn't work because SHELL_PATH is only used in\n> git scripts but not in any t* test scripts.\n\nIf you run \"make test\" (or just \"make\" inside \"t/\") the test scripts\nwill be executed with SHELL_PATH. If you run:\n\n ./t1234-whatever.sh\n\nthen obviously no, they will not be. Don't do that. Either use:\n\n make t1234-whatever.sh\n\nor:\n\n $YOUR_SHELL_PATH t1234-whatever.sh\n\n-Peff\n"}]}