[PATCH 0/2] always run all lint targets when running the test suite

[PATCH 0/2] always run all lint targets when running the test suite

[Jens Lehmann]

I recently accidentally added a non-portable "echo -n" to a test
suite helper which "make test" didn't show. This series attempts
to detect such problems early when running the test suite.

The first patch includes the helper scripts to be tested too when
running "make test-lint" (and thus the test-lint-shell-syntax
target) in the test directory.

The second patch then uses all lint tests in the test run.

Jens Lehmann (2):
  t/Makefile: check helper scripts for non-portable shell commands too
  t/Makefile: always test all lint targets when running tests

 t/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.0.1.474.g5b85b58

[PATCH 1/2] t/Makefile: check helper scripts for non-portable shell commands too

[Jens Lehmann]

Currently only the "t[0-9][0-9][0-9][0-9]-*.sh" scripts are tested for
shell incompatibilities using the check-non-portable-shell.pl script. This
makes it easy to miss non-POSIX constructs added to one of the t/*lib*.sh
helper scripts, as they aren't automatically detected.

Fix that by adding a THELPERS variable containing all shell scripts that
aren't tests and add these to the "test-lint-shell-syntax" target too.
---
 t/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 8fd1a72..7fa6692 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -29,6 +29,7 @@ TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
 T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
 TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))
 TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
+THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))

 all: $(DEFAULT_TEST_TARGET)

@@ -65,7 +66,7 @@ test-lint-executable:
 		echo >&2 "non-executable tests:" $$bad; exit 1; }

 test-lint-shell-syntax:
-	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)
+	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)

 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
-- 
2.0.1.474.g5b85b58

[PATCH 2/2] t/Makefile: always test all lint targets when running tests

[Jens Lehmann]

Only the two targets "test-lint-duplicates" and "test-lint-executable" are
currently executed when running the test target. This was done on purpose
when the TEST_LINT variable was added in 81127d74. But as this does not
include the "test-lint-shell-syntax" target added the same day in commit
c7ce70ac, it is easy to accidentally add non portable shell constructs
without noticing that when running the test suite.

Fix that by always running all lint tests unless the TEST_LINT variable is
overridden. If we add less accurate or slow tests later we could still
fall back to exclude them like 81127d74 proposed. But for now it is better
to include all lint tests until proven otherwise.
---
 t/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 7fa6692..43b15e3 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -13,7 +13,7 @@ TAR ?= $(TAR)
 RM ?= rm -f
 PROVE ?= prove
 DEFAULT_TEST_TARGET ?= test
-TEST_LINT ?= test-lint-duplicates test-lint-executable
+TEST_LINT ?= test-lint

 ifdef TEST_OUTPUT_DIRECTORY
 TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
-- 
2.0.1.474.g5b85b58

Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Only the two targets "test-lint-duplicates" and "test-lint-executable" are
> currently executed when running the test target. This was done on purpose
> when the TEST_LINT variable was added in 81127d74. But as this does not
> include the "test-lint-shell-syntax" target added the same day in commit
> c7ce70ac, it is easy to accidentally add non portable shell constructs
> without noticing that when running the test suite.

I not running the lint-shell-syntax that is fundamentally flaky to
avoid false positives is very much on purpose.  The flakiness is not
the fault of the implementor of the lint-shell-syntax, but comes
from the approach taken to pretend that simple pattern matching can
parse shell scripts.  It may not complain on the current set of
scripts, but that is not really by design but by accident.

So I am not very enthusiastic to see this change myself.

Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests

[Jens Lehmann]

Am 07.07.2014 20:13, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> Only the two targets "test-lint-duplicates" and "test-lint-executable" are
>> currently executed when running the test target. This was done on purpose
>> when the TEST_LINT variable was added in 81127d74. But as this does not
>> include the "test-lint-shell-syntax" target added the same day in commit
>> c7ce70ac, it is easy to accidentally add non portable shell constructs
>> without noticing that when running the test suite.
> 
> I not running the lint-shell-syntax that is fundamentally flaky to
> avoid false positives is very much on purpose.  The flakiness is not
> the fault of the implementor of the lint-shell-syntax, but comes
> from the approach taken to pretend that simple pattern matching can
> parse shell scripts.  It may not complain on the current set of
> scripts, but that is not really by design but by accident.
> 
> So I am not very enthusiastic to see this change myself.

Ok, I understand we do not want to lightly risk false positives. I
just noticed that I accidentally forgot to sign off this series, so
I'd resend just the first patch with a proper SOB, ok?

Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests

[Junio C Hamano]

On Tue, Jul 8, 2014 at 12:24 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>
> Am 07.07.2014 20:13, schrieb Junio C Hamano:
> >
> > So I am not very enthusiastic to see this change myself.
>
> Ok, I understand we do not want to lightly risk false positives. I
> just noticed that I accidentally forgot to sign off this series, so
> I'd resend just the first patch with a proper SOB, ok?


Nah, let's do both and how it plays out. My not being very enthusiastic
myself does not necessarily mean that it is bad for the project. Maybe
most people like it and if I cannot bear with it I can always turn it off
myself for my environment.

I just have a strange feeling that we may be seeing some twisted shell
script updates and when the author gets asked why it was written in
such a strange way, the answer might turn out to be "just to work around
the false positive from the test-lint", which I would not want to see.

[PATCH v2 0/2] always run all lint targets when running the test suite

[Jens Lehmann]

Am 09.07.2014 07:42, schrieb Junio C Hamano:
> On Tue, Jul 8, 2014 at 12:24 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>
>> Am 07.07.2014 20:13, schrieb Junio C Hamano:
>>>
>>> So I am not very enthusiastic to see this change myself.
>>
>> Ok, I understand we do not want to lightly risk false positives. I
>> just noticed that I accidentally forgot to sign off this series, so
>> I'd resend just the first patch with a proper SOB, ok?
> 
> 
> Nah, let's do both and how it plays out. My not being very enthusiastic
> myself does not necessarily mean that it is bad for the project. Maybe
> most people like it and if I cannot bear with it I can always turn it off
> myself for my environment.
> 
> I just have a strange feeling that we may be seeing some twisted shell
> script updates and when the author gets asked why it was written in
> such a strange way, the answer might turn out to be "just to work around
> the false positive from the test-lint", which I would not want to see.

Me neither. But until then it might well be that the benefit of having
this test on by default outweighs this potential problem. It would have
surely detected my fingers typing "echo -n" without my brain being alert
enough to catch this portability issue ;-)

This is the updated version with proper SOBs and an updated commit message
for 2/2 which is trying to sum up the considerations raised in this thread.


Jens Lehmann (2):
  t/Makefile: check helper scripts for non-portable shell commands too
  t/Makefile: always test all lint targets when running tests

 t/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.0.1.476.gf051ede

[PATCH v2 1/2] t/Makefile: check helper scripts for non-portable shell commands too

[Jens Lehmann]

Currently only the "t[0-9][0-9][0-9][0-9]-*.sh" scripts are tested for
shell incompatibilities using the check-non-portable-shell.pl script. This
makes it easy to miss non-POSIX constructs added to one of the t/*lib*.sh
helper scripts, as they aren't automatically detected.

Fix that by adding a THELPERS variable containing all shell scripts that
aren't tests and add these to the "test-lint-shell-syntax" target too.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 t/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 8fd1a72..7fa6692 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -29,6 +29,7 @@ TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
 T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
 TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))
 TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
+THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))

 all: $(DEFAULT_TEST_TARGET)

@@ -65,7 +66,7 @@ test-lint-executable:
 		echo >&2 "non-executable tests:" $$bad; exit 1; }

 test-lint-shell-syntax:
-	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)
+	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)

 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
-- 
2.0.1.476.gf051ede

[PATCH v2 2/2] t/Makefile: always test all lint targets when running tests

[Jens Lehmann]

Only the two targets "test-lint-duplicates" and "test-lint-executable" are
currently executed when running the test target. This was done on purpose
when the TEST_LINT variable was added in 81127d74 to avoid twisted shell
scripting by developers only to avoid false positives that might result
from the rather simple minded tests, e.g. test-lint-shell-syntax. But it
looks like it might be better to include all lint tests to help developers
to detect non portable shell constructs before the patch is sent to the
list and reviewed there.

Change the TEST_LINT variable to run all lint test unless the TEST_LINT
variable is overridden. If we hit false positives more often than helping
developers to avoid non-portable code (or add less accurate or slow tests
later) we could still fall back to exclude them like 81127d74 proposed.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 t/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 7fa6692..43b15e3 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -13,7 +13,7 @@ TAR ?= $(TAR)
 RM ?= rm -f
 PROVE ?= prove
 DEFAULT_TEST_TARGET ?= test
-TEST_LINT ?= test-lint-duplicates test-lint-executable
+TEST_LINT ?= test-lint

 ifdef TEST_OUTPUT_DIRECTORY
 TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
-- 
2.0.1.476.gf051ede

Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests

[Jeff King]

On Mon, Jul 07, 2014 at 11:13:11AM -0700, Junio C Hamano wrote:

> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
> > Only the two targets "test-lint-duplicates" and "test-lint-executable" are
> > currently executed when running the test target. This was done on purpose
> > when the TEST_LINT variable was added in 81127d74. But as this does not
> > include the "test-lint-shell-syntax" target added the same day in commit
> > c7ce70ac, it is easy to accidentally add non portable shell constructs
> > without noticing that when running the test suite.
> 
> I not running the lint-shell-syntax that is fundamentally flaky to
> avoid false positives is very much on purpose.  The flakiness is not
> the fault of the implementor of the lint-shell-syntax, but comes
> from the approach taken to pretend that simple pattern matching can
> parse shell scripts.  It may not complain on the current set of
> scripts, but that is not really by design but by accident.
> 
> So I am not very enthusiastic to see this change myself.

Let me play devil's advocate for a moment.

Is lint-shell-syntax in fact flaky? I know we discussed false positives
when it was originally added, but I think the current implementation
tries hard to avoid them. Given that it provides no false positives on
the current code base (without many people running it), it seems likely
to stay that way. And the cost if we are wrong is either fixing the tool
or disabling it (so worst case we are back where we started, modulo a
little effort to enable it and then revert).

What do we gain? We have an extra line of defense that helps newer shell
script writers fix their bugs before they make it to the list. That
catches more bugs, and reduces effort for reviewers. And it is exactly
these newer shell script writers that need the default flipped; they do
not know about portability and the lint target in the first place.

I dunno. I am not that enthusiastic about the change, either, but I tend
to think it will probably not hurt, and may help.

-Peff