[RFC] Add basic syntax check on shell scripts

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

On 12/02/2012 02:17 PM, Torsten Bögershausen wrote:

FWIW, I think such an idea is useful (and also easy to implement, so another +1 from me).

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

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

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".

On Mon, Dec 3, 2012 at 11:56 PM, Junio C Hamano <gitster@pobox.com> wrote:

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

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

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.

On Wed, Dec 5, 2012 at 2:39 AM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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

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").

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

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

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

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.

Jeff King <peff@peff.net> writes:

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

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

[...]

 >   * 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!