Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-07 Thread Torsten Bögershausen
On 07.01.13 19:07, Junio C Hamano wrote: > Torsten Bögershausen writes: > >> Sorry for late answer, but there is a problem (both linux and Mac OS X) :-( >> $ make test-lint does not do shel syntax check, neither >> $ make test-lint-shell-syntax > > In which directory? > > $ make -C t test-l

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-07 Thread Junio C Hamano
Torsten Bögershausen writes: > Sorry for late answer, but there is a problem (both linux and Mac OS X) :-( > $ make test-lint does not do shel syntax check, neither > $ make test-lint-shell-syntax In which directory? $ make -C t test-lint-shell-syntax ... passes silently ... $ ed t/

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-07 Thread Torsten Bögershausen
On 03.01.13 01:08, Junio C Hamano wrote: > Junio C Hamano writes: > >> I would actually not add this to TEST_LINT by default, especially >> when "duplicates" and "executable" that are much simpler and less >> likely to hit false positives are not on by default. >> >> At least, a change to add thi

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Junio C Hamano
Torsten Bögershausen writes: > When the dust has settled, we can either enable the check always, > or mention "make test-lint-shell-syntax" in the Documentation. In the longer term, I'm pretty much in favor of enabling all the checks that are cheap by default, as that would help people catch eas

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Torsten Bögershausen
On 03.01.13 01:16, Junio C Hamano wrote: > Torsten Bögershausen writes: > >> At least on my system the following combination works: >> >> git diff >> diff --git a/t/Makefile b/t/Makefile >> index f8f8c54..391a5ca 100644 >> --- a/t/Makefile >> +++ b/t/Makefile >> @@ -8,7 +8,7 @@ >> >> #GIT_TEST

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Junio C Hamano
Torsten Bögershausen writes: > At least on my system the following combination works: > > git diff > diff --git a/t/Makefile b/t/Makefile > index f8f8c54..391a5ca 100644 > --- a/t/Makefile > +++ b/t/Makefile > @@ -8,7 +8,7 @@ > > #GIT_TEST_OPTS = --verbose --debug > SHELL_PATH ?= $(SHELL) > -

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Junio C Hamano
Junio C Hamano writes: > I would actually not add this to TEST_LINT by default, especially > when "duplicates" and "executable" that are much simpler and less > likely to hit false positives are not on by default. > > At least, a change to add this to TEST_LINT by default must wait to > be merged

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Junio C Hamano
Jeff King writes: > On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote: > >> Add the perl script "check-non-portable-shell.pl" to detect non-portable >> shell syntax > > Cool. Thanks for adding more test-lint. But... > >> diff --git a/t/Makefile b/t/Makefile >> index 88e289f..7b

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Torsten Bögershausen
On 03.01.13 00:22, Jeff King wrote: > On Thu, Jan 03, 2013 at 12:14:32AM +0100, Torsten Bögershausen wrote: > >>> This is wrong if $(PERL_PATH) contains spaces, no? Doing "$(PERL_PATH)" >>> is also wrong, because the expansion happens in 'make', and a >>> $(PERL_PATH) with double-quotes would fool

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Jeff King
On Thu, Jan 03, 2013 at 12:14:32AM +0100, Torsten Bögershausen wrote: > > This is wrong if $(PERL_PATH) contains spaces, no? Doing "$(PERL_PATH)" > > is also wrong, because the expansion happens in 'make', and a > > $(PERL_PATH) with double-quotes would fool the shell. Since we export > > $PERL_PA

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Torsten Bögershausen
On 02.01.13 10:46, Jeff King wrote:> On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote: > >> Add the perl script "check-non-portable-shell.pl" to detect non-portable >> shell syntax > > Cool. Thanks for adding more test-lint. But... > >> diff --git a/t/Makefile b/t/Makefile >

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Junio C Hamano
Jeff King writes: > So taking all of that, a more idiomatic perl script would look something > like: > > my $exit_code; > sub err { > my $msg = shift; > print "$ARGV:$.: error: $msg: $_\n"; > $exit_code = 1; > } > > while (<>) { > chomp; > if (/^\s*sed\s+-i/) { >

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-02 Thread Jeff King
On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote: > Add the perl script "check-non-portable-shell.pl" to detect non-portable > shell syntax Cool. Thanks for adding more test-lint. But... > diff --git a/t/Makefile b/t/Makefile > index 88e289f..7b0c4dc 100644 > --- a/t/Makefile

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-01 Thread Junio C Hamano
Torsten Bögershausen writes: > The suggestion is to run it every time the test suite is run, at the begining. > And it seems to be fast enough: > > $ time ./check-non-portable-shell.pl ../../git.master/t/t[0-9]*.sh > real0m0.263s > user0m0.239s > sys 0m0.021s Hmph. OK. -- To unsubsc

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-01 Thread Torsten Bögershausen
On 01.01.13 23:07, Junio C Hamano wrote: [snip] > What it checks looks like a good start, but the indentation of it > (and the log message) seems very screwed up. OK > I also have to wonder what's the false positive rate of this. When > you are preparing a new test, you would ideally want a mode t

Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-01 Thread Junio C Hamano
Torsten Bögershausen writes: > Add the perl script "check-non-portable-shell.pl" to detect non-portable > shell syntax > Many systems use gnu tools which accept an extended syntax in shell scripts, > which is not portable on all systems and causes the test suite to fail. > > To prevent contributo

[PATCH 1/4] test: Add target test-lint-shell-syntax

2013-01-01 Thread Torsten Bögershausen
Add the perl script "check-non-portable-shell.pl" to detect non-portable shell syntax Many systems use gnu tools which accept an extended syntax in shell scripts, which is not portable on all systems and causes the test suite to fail. To prevent contributors using e.g. Linux to add non-portable te