[RFC] Add basic syntax check on shell scripts

[RFC] Add basic syntax check on shell scripts

[Torsten Bögershausen]

The test suite needs to be run on different platforms.
As it may be difficult for contributors to catch syntax
which work on GNU/linux, but is unportable, make a quick check
for the most common problems.
"sed -i", "echo -n" or "array in shell scripts"
This list is not complete, and may need to be extended

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
We add 1 second test execution time
Is this a useful idea at all?
 
 t/t99999-syntax-check.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100755 t/t99999-syntax-check.sh

diff --git a/t/t99999-syntax-check.sh b/t/t99999-syntax-check.sh
new file mode 100755
index 0000000..c4a9289
--- /dev/null
+++ b/t/t99999-syntax-check.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='Basic check if shell syntax is portable'
+
+. ./test-lib.sh
+
+
+test_expect_success 'No arrays in shell scripts' '
+	>expected &&
+	(grep -i -n "^[	 ]*declare[	 ][	 ]*" ../*.sh ../../git-* >actual 2>&1 || : ) &&
+	test_cmp expected actual &&
+	rm expected actual
+'
+
+test_expect_success 'No sed -i' '
+	>expected &&
+	(grep -n "^[	 ]*sed[	 ][	 ]*\-i" ../*.sh ../../git-* >actual 2>&1 || : ) &&
+	test_cmp expected actual &&
+	rm expected actual
+'
+
+test_expect_success 'No echo -n' '
+	>expected &&
+	(grep -n "^[	 ]*echo[	 ][	 ]*\-n" ../*.sh ../../git-* >actual 2>&1 || : ) &&
+	test_cmp expected actual &&
+	rm expected actual
+'
+test_done
-- 
1.8.0.197.g5a90748

Re: [RFC] Add basic syntax check on shell scripts

[Stefano Lattarini]

On 12/02/2012 02:17 PM, Torsten Bögershausen wrote:
> The test suite needs to be run on different platforms.
> As it may be difficult for contributors to catch syntax
> which work on GNU/linux, but is unportable, make a quick check
> for the most common problems.
> "sed -i", "echo -n" or "array in shell scripts"
> This list is not complete, and may need to be extended
> 
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> We add 1 second test execution time
> Is this a useful idea at all?
> 
FWIW, I think such an idea is useful (and also easy to implement,
so another +1 from me).

>  t/t99999-syntax-check.sh | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100755 t/t99999-syntax-check.sh
> 
> diff --git a/t/t99999-syntax-check.sh b/t/t99999-syntax-check.sh
> new file mode 100755
> index 0000000..c4a9289
> --- /dev/null
> +++ b/t/t99999-syntax-check.sh
> @@ -0,0 +1,28 @@
> +#!/bin/sh
> +
> +test_description='Basic check if shell syntax is portable'
> +
> +. ./test-lib.sh
> +
> +
> +test_expect_success 'No arrays in shell scripts' '
> +	>expected &&
> +	(grep -i -n "^[	 ]*declare[	 ][	 ]*" ../*.sh ../../git-* >actual 2>&1 || : ) &&
>
Here I'd simply use:

    ! grep -n "^declare[	 ][	 ]*" ../*.sh ../../*.sh

And similarly for the tests below.

In addition, the globs above still miss some files ('perf/perf-lib.sh'
and 'valgrind/analyze.sh', for example); so we might want to improve
it, using, say, "git ls-files" (or find(1), in case the test is to be
run also from distribution tarballs).

HTH,
  Stefano

Re: [RFC] Add basic syntax check on shell scripts

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> The test suite needs to be run on different platforms.
> As it may be difficult for contributors to catch syntax
> which work on GNU/linux, but is unportable, make a quick check
> for the most common problems.
> "sed -i", "echo -n" or "array in shell scripts"
> This list is not complete, and may need to be extended
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> We add 1 second test execution time
> Is this a useful idea at all?

Please do not name it after t/t[0-9]*.sh pattern, which are about
testing git.

This (once it gets cleaned up to reduce false positives) belongs to
"cd t && make test-lint".

>  
>  t/t99999-syntax-check.sh | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100755 t/t99999-syntax-check.sh
>
> diff --git a/t/t99999-syntax-check.sh b/t/t99999-syntax-check.sh
> new file mode 100755
> index 0000000..c4a9289
> --- /dev/null
> +++ b/t/t99999-syntax-check.sh
> @@ -0,0 +1,28 @@
> +#!/bin/sh
> +
> +test_description='Basic check if shell syntax is portable'
> +
> +. ./test-lib.sh
> +
> +
> +test_expect_success 'No arrays in shell scripts' '
> +	>expected &&
> +	(grep -i -n "^[	 ]*declare[	 ][	 ]*" ../*.sh ../../git-* >actual 2>&1 || : ) &&
> +	test_cmp expected actual &&
> +	rm expected actual
> +'
> +
> +test_expect_success 'No sed -i' '
> +	>expected &&
> +	(grep -n "^[	 ]*sed[	 ][	 ]*\-i" ../*.sh ../../git-* >actual 2>&1 || : ) &&
> +	test_cmp expected actual &&
> +	rm expected actual
> +'
> +
> +test_expect_success 'No echo -n' '
> +	>expected &&
> +	(grep -n "^[	 ]*echo[	 ][	 ]*\-n" ../*.sh ../../git-* >actual 2>&1 || : ) &&
> +	test_cmp expected actual &&
> +	rm expected actual
> +'
> +test_done

Re: [RFC] Add basic syntax check on shell scripts

[Nguyen Thai Ngoc Duy]

On Mon, Dec 3, 2012 at 11:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> The test suite needs to be run on different platforms.
>> As it may be difficult for contributors to catch syntax
>> which work on GNU/linux, but is unportable, make a quick check
>> for the most common problems.
>> "sed -i", "echo -n" or "array in shell scripts"
>> This list is not complete, and may need to be extended
>>
>> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>> ---
>> We add 1 second test execution time
>> Is this a useful idea at all?
>
> Please do not name it after t/t[0-9]*.sh pattern, which are about
> testing git.
>
> This (once it gets cleaned up to reduce false positives) belongs to
> "cd t && make test-lint".

Or a project commit hook? We can see how it goes and whether we can
improve anything for projects that rely on hooks.
-- 
Duy

Re: [RFC] Add basic syntax check on shell scripts

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>> This (once it gets cleaned up to reduce false positives) belongs to
>> "cd t && make test-lint".
>
> Or a project commit hook?

Surely.  It is OK to have "cd t && make test-lint" in your
pre-commit hook.

A few more things in addition to what Torsten's script attempts to
catch that we would want to catch are:

 * Do not spell string equality with "test $a == $b"; that is
   bash-ism and you only need "=" (which works in bash, too);

 * Do not capture output from "wc -l" in a variable and string
   compare with a constant, e.g. 

	lnum=$(wc -l <...) && test "$lnum" = 9

   as some wc implementations place extra SP in its output;

 * Do not use "test_must_fail" to run non-git command and require it
   to fail (instead, just write "! cmd").

 * Do not write ERE with backslashes and expect "grep" to grok them;
   that's GNUism.  e.g.

	grep "^\(author\|committer\) "

   is bad.  Use egrep (or "grep -E") if you want to use ERE.

Re: [RFC] Add basic syntax check on shell scripts

[Nguyen Thai Ngoc Duy]

On Wed, Dec 5, 2012 at 2:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Or a project commit hook?
>
> Surely.  It is OK to have "cd t && make test-lint" in your
> pre-commit hook.

No, what I meant is a shared pre-commit script that all git devs are
encouraged (or forced) to install so bugs are found locally rather
than after patches are sent to you. The hook content does not really
matter.
-- 
Duy

Re: [RFC] Add basic syntax check on shell scripts

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Wed, Dec 5, 2012 at 2:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Or a project commit hook?
>>
>> Surely.  It is OK to have "cd t && make test-lint" in your
>> pre-commit hook.
>
> No, what I meant is a shared pre-commit script that all git devs are
> encouraged (or forced) to install so bugs are found locally rather
> than after patches are sent to you. The hook content does not really
> matter.

Honestly, I do not really care (yet); what you are talkng about is
merely an addition to Documentation/SubmittingPatches, which is
trivial.

The content of the hook is much more important.

If it has too many false positives, it is not worth even encouraging
its use to less experienced ones, as they will have hard time to
figure out which errors matter and which erros can be ignored.  It
will make contributing to the project harder, not easier.

As I do not think (1) we would be able to do a good job reducing
false positives without writing a full POSIX shell parser, and (2)
we would want to be in the business of writing POSIX shell parser
[*1*], I am somewhat skeptical.

And if we cannot come up with a reliable hook in the first place,
there isn't much point in discussing a policy to encourage such a
hook, is it?


[Footnote]

*1* Is there a free one that is portable, perhaps written in either
Perl or Python, to which we can easily plug our own logic?  In order
to catch basic errors, I think it is sufficient to tokenize the
script into independent series of simple commands, even ignoring
variable substitutions and evals, and just have a bunch of "we do
not allow option X to command Y" rules (e.g. "no -i to sed", "the
first argument must be 'git' for "test_must_fail").

Re: [RFC] Add basic syntax check on shell scripts

[Jeff King]

On Wed, Dec 05, 2012 at 12:43:30PM +0700, Nguyen Thai Ngoc Duy wrote:

> On Wed, Dec 5, 2012 at 2:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Or a project commit hook?
> >
> > Surely.  It is OK to have "cd t && make test-lint" in your
> > pre-commit hook.
> 
> No, what I meant is a shared pre-commit script that all git devs are
> encouraged (or forced) to install so bugs are found locally rather
> than after patches are sent to you. The hook content does not really
> matter.

I think that is orthogonal. You would want to implement the guts of such
a hook outside the hook itself, so that it could be run at arbitrary
times. So even if we want such a hook, the development should probably
look like:

  1. Implement checks in t/Makefile, triggered by "make test-lint" or
     similar.

  2. Run "make test-lint" in a hook.

I do not use such a hook myself, but I do run "test-lint" as part of my
"make test", and I "make test" each series I send (and if the series has
non-trivial refactoring, each individual patch of the series to catch
breakages that come and go during refactoring). But I decide when to run
those checks, not a hook.

Anyway, I do think a "shell portability lint" would be a great addition
to "test-lint", but I am slightly skeptical that it will be easy to
write a good one that does not have false positives. Still, there may be
some low-hanging fruit. I have not looked carefully at Torsten's patch
yet.

-Peff

Re: [RFC] Add basic syntax check on shell scripts

[Jeff King]

On Wed, Dec 05, 2012 at 02:30:56AM -0500, Jeff King wrote:

> Anyway, I do think a "shell portability lint" would be a great addition
> to "test-lint", but I am slightly skeptical that it will be easy to
> write a good one that does not have false positives. Still, there may be
> some low-hanging fruit. I have not looked carefully at Torsten's patch
> yet.

Hrm. I had the impression initially that Torsten's patch was about
testing the test scripts themselves. But it is really about testing the
installed shell scripts. In that sense, test-lint is not the right
place.

You would want a "check shell script portability" script, and you would
probably want to run it:

  - on the regular built scripts; possibly during build time (I have done
    this before with "perl -c" for perl scripts and it is reasonably
    successful). Or in a test script, as added in his patch (though I
    note it does not seem to pass as posted, getting confused by trying
    to grep "git-gui").

  - on the test scripts themselves via test-lint

I think as long as such a script erred on the side of false negatives,
it would be OK (because false positives are a giant headache, and
ultimately the real test is people exercising the code itself on their
shells; this is just an early check to help contributors who do not have
such shells).

-Peff

PS Debian developers use a checkbashisms script to find some portability
   problems. It might be worth looking at, though I notice it generates
   a lot of bogus "unterminated string" results for our t/t*.sh scripts.

Re: [RFC] Add basic syntax check on shell scripts

[Sebastian Schuberth]

On 2012/12/04 20:39 , Junio C Hamano wrote:

> A few more things in addition to what Torsten's script attempts to
> catch that we would want to catch are:

[...]

 >   * Do not write ERE with backslashes and expect "grep" to grok them;
 >     that's GNUism.  e.g.
 >
 > 	grep "^\(author\|committer\) "
 >
 >     is bad.  Use egrep (or "grep -E") if you want to use ERE.

Yet more thing that is probably worth catching, although not related to 
bashism: Avoid the use of "which" in favor of e.g. "type".

In any case, having this check as a local pre-commit hook would be great!

-- 
Sebastian Schuberth

Re: [RFC] Add basic syntax check on shell scripts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You would want a "check shell script portability" script, and you would
> probably want to run it:
>
>   - on the regular built scripts; possibly during build time (I have done
>     this before with "perl -c" for perl scripts and it is reasonably
>     successful). Or in a test script, as added in his patch (though I
>     note it does not seem to pass as posted, getting confused by trying
>     to grep "git-gui").
>
>   - on the test scripts themselves via test-lint
>
> I think as long as such a script erred on the side of false negatives,
> it would be OK (because false positives are a giant headache, and
> ultimately the real test is people exercising the code itself on their
> shells; this is just an early check to help contributors who do not have
> such shells).

Yeah, you have a good point that we should cover the scripts outside tests
and test-lint is not a good match for them.