{"thread":{"id":"32262","subject":"[RFC] Add basic syntax check on shell scripts","startedAt":"2012-12-02T13:17:24Z","lastAt":"2012-12-05T16:12:21Z","messageCount":11,"participants":["Torsten Bögershausen","Stefano Lattarini","Junio C Hamano","Nguyen Thai Ngoc Duy","Jeff King","Sebastian Schuberth"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"204425","messageId":"201212021417.25525.tboegi@web.de","threadId":"32262","inReplyTo":null,"subject":"[RFC] Add basic syntax check on shell scripts","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2012-12-02T13:17:24Z","receivedAt":"2012-12-02T13:17:24Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"The test suite needs to be run on different platforms.\nAs it may be difficult for contributors to catch syntax\nwhich work on GNU/linux, but is unportable, make a quick check\nfor the most common problems.\n\"sed -i\", \"echo -n\" or \"array in shell scripts\"\nThis list is not complete, and may need to be extended\n\nSigned-off-by: Torsten Bögershausen \n---\nWe add 1 second test execution time\nIs this a useful idea at all?\n \n t/t99999-syntax-check.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n create mode 100755 t/t99999-syntax-check.sh\n\ndiff --git a/t/t99999-syntax-check.sh b/t/t99999-syntax-check.sh\nnew file mode 100755\nindex 0000000..c4a9289\n--- /dev/null\n+++ b/t/t99999-syntax-check.sh\n@@ -0,0 +1,28 @@\n+#!/bin/sh\n+\n+test_description='Basic check if shell syntax is portable'\n+\n+. ./test-lib.sh\n+\n+\n+test_expect_success 'No arrays in shell scripts' '\n+\t>expected &&\n+\t(grep -i -n \"^[\t ]*declare[\t ][\t ]*\" ../*.sh ../../git-* >actual 2>&1 || : ) &&\n+\ttest_cmp expected actual &&\n+\trm expected actual\n+'\n+\n+test_expect_success 'No sed -i' '\n+\t>expected &&\n+\t(grep -n \"^[\t ]*sed[\t ][\t ]*\\-i\" ../*.sh ../../git-* >actual 2>&1 || : ) &&\n+\ttest_cmp expected actual &&\n+\trm expected actual\n+'\n+\n+test_expect_success 'No echo -n' '\n+\t>expected &&\n+\t(grep -n \"^[\t ]*echo[\t ][\t ]*\\-n\" ../*.sh ../../git-* >actual 2>&1 || : ) &&\n+\ttest_cmp expected actual &&\n+\trm expected actual\n+'\n+test_done\n-- \n1.8.0.197.g5a90748\n"},{"id":"204426","messageId":"50BB65EA.5080805@gmail.com","threadId":"32262","inReplyTo":"201212021417.25525.tboegi@web.de","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2012-12-02T14:30:02Z","receivedAt":"2012-12-02T14:30:02Z","isPatch":false,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"On 12/02/2012 02:17 PM, Torsten Bögershausen wrote:\n> The test suite needs to be run on different platforms.\n> As it may be difficult for contributors to catch syntax\n> which work on GNU/linux, but is unportable, make a quick check\n> for the most common problems.\n> \"sed -i\", \"echo -n\" or \"array in shell scripts\"\n> This list is not complete, and may need to be extended\n> \n> Signed-off-by: Torsten Bögershausen \n> ---\n> We add 1 second test execution time\n> Is this a useful idea at all?\n> \nFWIW, I think such an idea is useful (and also easy to implement,\nso another +1 from me).\n\n> t/t99999-syntax-check.sh | 28 ++++++++++++++++++++++++++++\n> 1 file changed, 28 insertions(+)\n> create mode 100755 t/t99999-syntax-check.sh\n> \n> diff --git a/t/t99999-syntax-check.sh b/t/t99999-syntax-check.sh\n> new file mode 100755\n> index 0000000..c4a9289\n> --- /dev/null\n> +++ b/t/t99999-syntax-check.sh\n> @@ -0,0 +1,28 @@\n> +#!/bin/sh\n> +\n> +test_description='Basic check if shell syntax is portable'\n> +\n> +. ./test-lib.sh\n> +\n> +\n> +test_expect_success 'No arrays in shell scripts' '\n> +\t>expected &&\n> +\t(grep -i -n \"^[\t ]*declare[\t ][\t ]*\" ../*.sh ../../git-* >actual 2>&1 || : ) &&\n>\nHere I'd simply use:\n\n ! grep -n \"^declare[\t ][\t ]*\" ../*.sh ../../*.sh\n\nAnd similarly for the tests below.\n\nIn addition, the globs above still miss some files ('perf/perf-lib.sh'\nand 'valgrind/analyze.sh', for example); so we might want to improve\nit, using, say, \"git ls-files\" (or find(1), in case the test is to be\nrun also from distribution tarballs).\n\nHTH,\n Stefano\n"},{"id":"204454","messageId":"7vzk1vrs63.fsf@alter.siamese.dyndns.org","threadId":"32262","inReplyTo":"201212021417.25525.tboegi@web.de","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-03T16:56:04Z","receivedAt":"2012-12-03T16:56:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> The test suite needs to be run on different platforms.\n> As it may be difficult for contributors to catch syntax\n> which work on GNU/linux, but is unportable, make a quick check\n> for the most common problems.\n> \"sed -i\", \"echo -n\" or \"array in shell scripts\"\n> This list is not complete, and may need to be extended\n>\n> Signed-off-by: Torsten Bögershausen \n> ---\n> We add 1 second test execution time\n> Is this a useful idea at all?\n\nPlease do not name it after t/t[0-9]*.sh pattern, which are about\ntesting git.\n\nThis (once it gets cleaned up to reduce false positives) belongs to\n\"cd t && make test-lint\".\n\n> \n> t/t99999-syntax-check.sh | 28 ++++++++++++++++++++++++++++\n> 1 file changed, 28 insertions(+)\n> create mode 100755 t/t99999-syntax-check.sh\n>\n> diff --git a/t/t99999-syntax-check.sh b/t/t99999-syntax-check.sh\n> new file mode 100755\n> index 0000000..c4a9289\n> --- /dev/null\n> +++ b/t/t99999-syntax-check.sh\n> @@ -0,0 +1,28 @@\n> +#!/bin/sh\n> +\n> +test_description='Basic check if shell syntax is portable'\n> +\n> +. ./test-lib.sh\n> +\n> +\n> +test_expect_success 'No arrays in shell scripts' '\n> +\t>expected &&\n> +\t(grep -i -n \"^[\t ]*declare[\t ][\t ]*\" ../*.sh ../../git-* >actual 2>&1 || : ) &&\n> +\ttest_cmp expected actual &&\n> +\trm expected actual\n> +'\n> +\n> +test_expect_success 'No sed -i' '\n> +\t>expected &&\n> +\t(grep -n \"^[\t ]*sed[\t ][\t ]*\\-i\" ../*.sh ../../git-* >actual 2>&1 || : ) &&\n> +\ttest_cmp expected actual &&\n> +\trm expected actual\n> +'\n> +\n> +test_expect_success 'No echo -n' '\n> +\t>expected &&\n> +\t(grep -n \"^[\t ]*echo[\t ][\t ]*\\-n\" ../*.sh ../../git-* >actual 2>&1 || : ) &&\n> +\ttest_cmp expected actual &&\n> +\trm expected actual\n> +'\n> +test_done\n"},{"id":"204476","messageId":"CACsJy8BxviWRHqGvptsJVmkFM6HQa9HnLWsh5V6Ec6Fqv52sGA@mail.gmail.com","threadId":"32262","inReplyTo":"7vzk1vrs63.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-04T07:20:20Z","receivedAt":"2012-12-04T07:20:20Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 3, 2012 at 11:56 PM, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n>> The test suite needs to be run on different platforms.\n>> As it may be difficult for contributors to catch syntax\n>> which work on GNU/linux, but is unportable, make a quick check\n>> for the most common problems.\n>> \"sed -i\", \"echo -n\" or \"array in shell scripts\"\n>> This list is not complete, and may need to be extended\n>>\n>> Signed-off-by: Torsten Bögershausen \n>> ---\n>> We add 1 second test execution time\n>> Is this a useful idea at all?\n>\n> Please do not name it after t/t[0-9]*.sh pattern, which are about\n> testing git.\n>\n> This (once it gets cleaned up to reduce false positives) belongs to\n> \"cd t && make test-lint\".\n\nOr a project commit hook? We can see how it goes and whether we can\nimprove anything for projects that rely on hooks.\n-- \nDuy\n"},{"id":"204498","messageId":"7vobi9mwt4.fsf@alter.siamese.dyndns.org","threadId":"32262","inReplyTo":"CACsJy8BxviWRHqGvptsJVmkFM6HQa9HnLWsh5V6Ec6Fqv52sGA@mail.gmail.com","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-04T19:39:19Z","receivedAt":"2012-12-04T19:39:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n>> This (once it gets cleaned up to reduce false positives) belongs to\n>> \"cd t && make test-lint\".\n>\n> Or a project commit hook?\n\nSurely. It is OK to have \"cd t && make test-lint\" in your\npre-commit hook.\n\nA few more things in addition to what Torsten's script attempts to\ncatch that we would want to catch are:\n\n * Do not spell string equality with \"test $a == $b\"; that is\n bash-ism and you only need \"=\" (which works in bash, too);\n\n * Do not capture output from \"wc -l\" in a variable and string\n compare with a constant, e.g. \n\n\tlnum=$(wc -l <...) && test \"$lnum\" = 9\n\n as some wc implementations place extra SP in its output;\n\n * Do not use \"test_must_fail\" to run non-git command and require it\n to fail (instead, just write \"! cmd\").\n\n * Do not write ERE with backslashes and expect \"grep\" to grok them;\n that's GNUism. e.g.\n\n\tgrep \"^\\(author\\|committer\\) \"\n\n is bad. Use egrep (or \"grep -E\") if you want to use ERE.\n"},{"id":"204508","messageId":"CACsJy8BtX9fMkGDoVGKzgz7SSinbt0561B1ZKHu6fs+n8ewKGg@mail.gmail.com","threadId":"32262","inReplyTo":"7vobi9mwt4.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-05T05:43:30Z","receivedAt":"2012-12-05T05:43:30Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 5, 2012 at 2:39 AM, Junio C Hamano wrote:\n>> Or a project commit hook?\n>\n> Surely. It is OK to have \"cd t && make test-lint\" in your\n> pre-commit hook.\n\nNo, what I meant is a shared pre-commit script that all git devs are\nencouraged (or forced) to install so bugs are found locally rather\nthan after patches are sent to you. The hook content does not really\nmatter.\n-- \nDuy\n"},{"id":"204510","messageId":"7vobi9kpda.fsf@alter.siamese.dyndns.org","threadId":"32262","inReplyTo":"CACsJy8BtX9fMkGDoVGKzgz7SSinbt0561B1ZKHu6fs+n8ewKGg@mail.gmail.com","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-05T06:02:57Z","receivedAt":"2012-12-05T06:02:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Wed, Dec 5, 2012 at 2:39 AM, Junio C Hamano wrote:\n>>> Or a project commit hook?\n>>\n>> Surely. It is OK to have \"cd t && make test-lint\" in your\n>> pre-commit hook.\n>\n> No, what I meant is a shared pre-commit script that all git devs are\n> encouraged (or forced) to install so bugs are found locally rather\n> than after patches are sent to you. The hook content does not really\n> matter.\n\nHonestly, I do not really care (yet); what you are talkng about is\nmerely an addition to Documentation/SubmittingPatches, which is\ntrivial.\n\nThe content of the hook is much more important.\n\nIf it has too many false positives, it is not worth even encouraging\nits use to less experienced ones, as they will have hard time to\nfigure out which errors matter and which erros can be ignored. It\nwill make contributing to the project harder, not easier.\n\nAs I do not think (1) we would be able to do a good job reducing\nfalse positives without writing a full POSIX shell parser, and (2)\nwe would want to be in the business of writing POSIX shell parser\n[*1*], I am somewhat skeptical.\n\nAnd if we cannot come up with a reliable hook in the first place,\nthere isn't much point in discussing a policy to encourage such a\nhook, is it?\n\n\n[Footnote]\n\n*1* Is there a free one that is portable, perhaps written in either\nPerl or Python, to which we can easily plug our own logic? In order\nto catch basic errors, I think it is sufficient to tokenize the\nscript into independent series of simple commands, even ignoring\nvariable substitutions and evals, and just have a bunch of \"we do\nnot allow option X to command Y\" rules (e.g. \"no -i to sed\", \"the\nfirst argument must be 'git' for \"test_must_fail\").\n"},{"id":"204512","messageId":"20121205073055.GA5776@sigill.intra.peff.net","threadId":"32262","inReplyTo":"CACsJy8BtX9fMkGDoVGKzgz7SSinbt0561B1ZKHu6fs+n8ewKGg@mail.gmail.com","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-05T07:30:56Z","receivedAt":"2012-12-05T07:30:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 05, 2012 at 12:43:30PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Wed, Dec 5, 2012 at 2:39 AM, Junio C Hamano wrote:\n> >> Or a project commit hook?\n> >\n> > Surely. It is OK to have \"cd t && make test-lint\" in your\n> > pre-commit hook.\n> \n> No, what I meant is a shared pre-commit script that all git devs are\n> encouraged (or forced) to install so bugs are found locally rather\n> than after patches are sent to you. The hook content does not really\n> matter.\n\nI think that is orthogonal. You would want to implement the guts of such\na hook outside the hook itself, so that it could be run at arbitrary\ntimes. So even if we want such a hook, the development should probably\nlook like:\n\n 1. Implement checks in t/Makefile, triggered by \"make test-lint\" or\n similar.\n\n 2. Run \"make test-lint\" in a hook.\n\nI do not use such a hook myself, but I do run \"test-lint\" as part of my\n\"make test\", and I \"make test\" each series I send (and if the series has\nnon-trivial refactoring, each individual patch of the series to catch\nbreakages that come and go during refactoring). But I decide when to run\nthose checks, not a hook.\n\nAnyway, I do think a \"shell portability lint\" would be a great addition\nto \"test-lint\", but I am slightly skeptical that it will be easy to\nwrite a good one that does not have false positives. Still, there may be\nsome low-hanging fruit. I have not looked carefully at Torsten's patch\nyet.\n\n-Peff\n"},{"id":"204513","messageId":"20121205075401.GB5776@sigill.intra.peff.net","threadId":"32262","inReplyTo":"20121205073055.GA5776@sigill.intra.peff.net","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-05T07:54:02Z","receivedAt":"2012-12-05T07:54:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 05, 2012 at 02:30:56AM -0500, Jeff King wrote:\n\n> Anyway, I do think a \"shell portability lint\" would be a great addition\n> to \"test-lint\", but I am slightly skeptical that it will be easy to\n> write a good one that does not have false positives. Still, there may be\n> some low-hanging fruit. I have not looked carefully at Torsten's patch\n> yet.\n\nHrm. I had the impression initially that Torsten's patch was about\ntesting the test scripts themselves. But it is really about testing the\ninstalled shell scripts. In that sense, test-lint is not the right\nplace.\n\nYou would want a \"check shell script portability\" script, and you would\nprobably want to run it:\n\n - on the regular built scripts; possibly during build time (I have done\n this before with \"perl -c\" for perl scripts and it is reasonably\n successful). Or in a test script, as added in his patch (though I\n note it does not seem to pass as posted, getting confused by trying\n to grep \"git-gui\").\n\n - on the test scripts themselves via test-lint\n\nI think as long as such a script erred on the side of false negatives,\nit would be OK (because false positives are a giant headache, and\nultimately the real test is people exercising the code itself on their\nshells; this is just an early check to help contributors who do not have\nsuch shells).\n\n-Peff\n\nPS Debian developers use a checkbashisms script to find some portability\n problems. It might be worth looking at, though I notice it generates\n a lot of bogus \"unterminated string\" results for our t/t*.sh scripts.\n"},{"id":"204514","messageId":"50BF0FCD.6080700@gmail.com","threadId":"32262","inReplyTo":"7vobi9mwt4.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-12-05T09:11:41Z","receivedAt":"2012-12-05T09:11:41Z","isPatch":false,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 2012/12/04 20:39 , Junio C Hamano wrote:\n\n> A few more things in addition to what Torsten's script attempts to\n> catch that we would want to catch are:\n\n[...]\n\n > * Do not write ERE with backslashes and expect \"grep\" to grok them;\n > that's GNUism. e.g.\n >\n > \tgrep \"^\\(author\\|committer\\) \"\n >\n > is bad. Use egrep (or \"grep -E\") if you want to use ERE.\n\nYet more thing that is probably worth catching, although not related to \nbashism: Avoid the use of \"which\" in favor of e.g. \"type\".\n\nIn any case, having this check as a local pre-commit hook would be great!\n\n-- \nSebastian Schuberth\n"},{"id":"204528","messageId":"7vehj4lbq2.fsf@alter.siamese.dyndns.org","threadId":"32262","inReplyTo":"20121205075401.GB5776@sigill.intra.peff.net","subject":"Re: [RFC] Add basic syntax check on shell scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-05T16:12:21Z","receivedAt":"2012-12-05T16:12:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You would want a \"check shell script portability\" script, and you would\n> probably want to run it:\n>\n> - on the regular built scripts; possibly during build time (I have done\n> this before with \"perl -c\" for perl scripts and it is reasonably\n> successful). Or in a test script, as added in his patch (though I\n> note it does not seem to pass as posted, getting confused by trying\n> to grep \"git-gui\").\n>\n> - on the test scripts themselves via test-lint\n>\n> I think as long as such a script erred on the side of false negatives,\n> it would be OK (because false positives are a giant headache, and\n> ultimately the real test is people exercising the code itself on their\n> shells; this is just an early check to help contributors who do not have\n> such shells).\n\nYeah, you have a good point that we should cover the scripts outside tests\nand test-lint is not a good match for them.\n"}]}