{"thread":{"id":"37055","subject":"[PATCH 0/2] always run all lint targets when running the test suite","startedAt":"2014-07-03T22:19:42Z","lastAt":"2014-07-09T19:34:42Z","messageCount":10,"participants":["Jens Lehmann","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"245395","messageId":"53B5D6FE.2090700@web.de","threadId":"37055","inReplyTo":null,"subject":"[PATCH 0/2] always run all lint targets when running the test suite","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-07-03T22:19:42Z","receivedAt":"2014-07-03T22:19:42Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"I recently accidentally added a non-portable \"echo -n\" to a test\nsuite helper which \"make test\" didn't show. This series attempts\nto detect such problems early when running the test suite.\n\nThe first patch includes the helper scripts to be tested too when\nrunning \"make test-lint\" (and thus the test-lint-shell-syntax\ntarget) in the test directory.\n\nThe second patch then uses all lint tests in the test run.\n\nJens Lehmann (2):\n t/Makefile: check helper scripts for non-portable shell commands too\n t/Makefile: always test all lint targets when running tests\n\n t/Makefile | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\n-- \n2.0.1.474.g5b85b58\n"},{"id":"245396","messageId":"53B5D736.6090809@web.de","threadId":"37055","inReplyTo":"53B5D6FE.2090700@web.de","subject":"[PATCH 1/2] t/Makefile: check helper scripts for non-portable shell commands too","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-07-03T22:20:38Z","receivedAt":"2014-07-03T22:20:38Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Currently only the \"t[0-9][0-9][0-9][0-9]-*.sh\" scripts are tested for\nshell incompatibilities using the check-non-portable-shell.pl script. This\nmakes it easy to miss non-POSIX constructs added to one of the t/*lib*.sh\nhelper scripts, as they aren't automatically detected.\n\nFix that by adding a THELPERS variable containing all shell scripts that\naren't tests and add these to the \"test-lint-shell-syntax\" target too.\n---\n t/Makefile | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 8fd1a72..7fa6692 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -29,6 +29,7 @@ TEST_RESULTS_DIRECTORY_SQ = $(subst ','\\'',$(TEST_RESULTS_DIRECTORY))\n T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))\n TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n+THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))\n\n all: $(DEFAULT_TEST_TARGET)\n\n@@ -65,7 +66,7 @@ test-lint-executable:\n \t\techo >&2 \"non-executable tests:\" $$bad; exit 1; }\n\n test-lint-shell-syntax:\n-\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)\n+\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)\n\n aggregate-results-and-cleanup: $(T)\n \t$(MAKE) aggregate-results\n-- \n2.0.1.474.g5b85b58\n"},{"id":"245397","messageId":"53B5D76D.1090509@web.de","threadId":"37055","inReplyTo":"53B5D6FE.2090700@web.de","subject":"[PATCH 2/2] t/Makefile: always test all lint targets when running tests","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-07-03T22:21:33Z","receivedAt":"2014-07-03T22:21:33Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Only the two targets \"test-lint-duplicates\" and \"test-lint-executable\" are\ncurrently executed when running the test target. This was done on purpose\nwhen the TEST_LINT variable was added in 81127d74. But as this does not\ninclude the \"test-lint-shell-syntax\" target added the same day in commit\nc7ce70ac, it is easy to accidentally add non portable shell constructs\nwithout noticing that when running the test suite.\n\nFix that by always running all lint tests unless the TEST_LINT variable is\noverridden. If we add less accurate or slow tests later we could still\nfall back to exclude them like 81127d74 proposed. But for now it is better\nto include all lint tests until proven otherwise.\n---\n t/Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 7fa6692..43b15e3 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -13,7 +13,7 @@ TAR ?= $(TAR)\n RM ?= rm -f\n PROVE ?= prove\n DEFAULT_TEST_TARGET ?= test\n-TEST_LINT ?= test-lint-duplicates test-lint-executable\n+TEST_LINT ?= test-lint\n\n ifdef TEST_OUTPUT_DIRECTORY\n TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results\n-- \n2.0.1.474.g5b85b58\n"},{"id":"245484","messageId":"xmqq38eddolk.fsf@gitster.dls.corp.google.com","threadId":"37055","inReplyTo":"53B5D76D.1090509@web.de","subject":"Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-07T18:13:11Z","receivedAt":"2014-07-07T18:13:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann writes:\n\n> Only the two targets \"test-lint-duplicates\" and \"test-lint-executable\" are\n> currently executed when running the test target. This was done on purpose\n> when the TEST_LINT variable was added in 81127d74. But as this does not\n> include the \"test-lint-shell-syntax\" target added the same day in commit\n> c7ce70ac, it is easy to accidentally add non portable shell constructs\n> without noticing that when running the test suite.\n\nI not running the lint-shell-syntax that is fundamentally flaky to\navoid false positives is very much on purpose. The flakiness is not\nthe fault of the implementor of the lint-shell-syntax, but comes\nfrom the approach taken to pretend that simple pattern matching can\nparse shell scripts. It may not complain on the current set of\nscripts, but that is not really by design but by accident.\n\nSo I am not very enthusiastic to see this change myself.\n"},{"id":"245546","messageId":"53BC4569.3020907@web.de","threadId":"37055","inReplyTo":"xmqq38eddolk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-07-08T19:24:25Z","receivedAt":"2014-07-08T19:24:25Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 07.07.2014 20:13, schrieb Junio C Hamano:\n> Jens Lehmann writes:\n> \n>> Only the two targets \"test-lint-duplicates\" and \"test-lint-executable\" are\n>> currently executed when running the test target. This was done on purpose\n>> when the TEST_LINT variable was added in 81127d74. But as this does not\n>> include the \"test-lint-shell-syntax\" target added the same day in commit\n>> c7ce70ac, it is easy to accidentally add non portable shell constructs\n>> without noticing that when running the test suite.\n> \n> I not running the lint-shell-syntax that is fundamentally flaky to\n> avoid false positives is very much on purpose. The flakiness is not\n> the fault of the implementor of the lint-shell-syntax, but comes\n> from the approach taken to pretend that simple pattern matching can\n> parse shell scripts. It may not complain on the current set of\n> scripts, but that is not really by design but by accident.\n> \n> So I am not very enthusiastic to see this change myself.\n\nOk, I understand we do not want to lightly risk false positives. I\njust noticed that I accidentally forgot to sign off this series, so\nI'd resend just the first patch with a proper SOB, ok?\n"},{"id":"245569","messageId":"20140709053005.GD2318@sigill.intra.peff.net","threadId":"37055","inReplyTo":"xmqq38eddolk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-09T05:30:05Z","receivedAt":"2014-07-09T05:30:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 07, 2014 at 11:13:11AM -0700, Junio C Hamano wrote:\n\n> Jens Lehmann writes:\n> \n> > Only the two targets \"test-lint-duplicates\" and \"test-lint-executable\" are\n> > currently executed when running the test target. This was done on purpose\n> > when the TEST_LINT variable was added in 81127d74. But as this does not\n> > include the \"test-lint-shell-syntax\" target added the same day in commit\n> > c7ce70ac, it is easy to accidentally add non portable shell constructs\n> > without noticing that when running the test suite.\n> \n> I not running the lint-shell-syntax that is fundamentally flaky to\n> avoid false positives is very much on purpose. The flakiness is not\n> the fault of the implementor of the lint-shell-syntax, but comes\n> from the approach taken to pretend that simple pattern matching can\n> parse shell scripts. It may not complain on the current set of\n> scripts, but that is not really by design but by accident.\n> \n> So I am not very enthusiastic to see this change myself.\n\nLet me play devil's advocate for a moment.\n\nIs lint-shell-syntax in fact flaky? I know we discussed false positives\nwhen it was originally added, but I think the current implementation\ntries hard to avoid them. Given that it provides no false positives on\nthe current code base (without many people running it), it seems likely\nto stay that way. And the cost if we are wrong is either fixing the tool\nor disabling it (so worst case we are back where we started, modulo a\nlittle effort to enable it and then revert).\n\nWhat do we gain? We have an extra line of defense that helps newer shell\nscript writers fix their bugs before they make it to the list. That\ncatches more bugs, and reduces effort for reviewers. And it is exactly\nthese newer shell script writers that need the default flipped; they do\nnot know about portability and the lint target in the first place.\n\nI dunno. I am not that enthusiastic about the change, either, but I tend\nto think it will probably not hurt, and may help.\n\n-Peff\n"},{"id":"245571","messageId":"CAPc5daUXZNB=2X8zsrhs9=Z-nV1o1v7KWGydAj6UmBk23UBEEw@mail.gmail.com","threadId":"37055","inReplyTo":"53BC4569.3020907@web.de","subject":"Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-09T05:42:57Z","receivedAt":"2014-07-09T05:42:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Jul 8, 2014 at 12:24 PM, Jens Lehmann wrote:\n>\n> Am 07.07.2014 20:13, schrieb Junio C Hamano:\n> >\n> > So I am not very enthusiastic to see this change myself.\n>\n> Ok, I understand we do not want to lightly risk false positives. I\n> just noticed that I accidentally forgot to sign off this series, so\n> I'd resend just the first patch with a proper SOB, ok?\n\n\nNah, let's do both and how it plays out. My not being very enthusiastic\nmyself does not necessarily mean that it is bad for the project. Maybe\nmost people like it and if I cannot bear with it I can always turn it off\nmyself for my environment.\n\nI just have a strange feeling that we may be seeing some twisted shell\nscript updates and when the author gets asked why it was written in\nsuch a strange way, the answer might turn out to be \"just to work around\nthe false positive from the test-lint\", which I would not want to see.\n"},{"id":"245646","messageId":"53BD9908.1060807@web.de","threadId":"37055","inReplyTo":"CAPc5daUXZNB=2X8zsrhs9=Z-nV1o1v7KWGydAj6UmBk23UBEEw@mail.gmail.com","subject":"[PATCH v2 0/2] always run all lint targets when running the test suite","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-07-09T19:33:28Z","receivedAt":"2014-07-09T19:33:28Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 09.07.2014 07:42, schrieb Junio C Hamano:\n> On Tue, Jul 8, 2014 at 12:24 PM, Jens Lehmann wrote:\n>>\n>> Am 07.07.2014 20:13, schrieb Junio C Hamano:\n>>>\n>>> So I am not very enthusiastic to see this change myself.\n>>\n>> Ok, I understand we do not want to lightly risk false positives. I\n>> just noticed that I accidentally forgot to sign off this series, so\n>> I'd resend just the first patch with a proper SOB, ok?\n> \n> \n> Nah, let's do both and how it plays out. My not being very enthusiastic\n> myself does not necessarily mean that it is bad for the project. Maybe\n> most people like it and if I cannot bear with it I can always turn it off\n> myself for my environment.\n> \n> I just have a strange feeling that we may be seeing some twisted shell\n> script updates and when the author gets asked why it was written in\n> such a strange way, the answer might turn out to be \"just to work around\n> the false positive from the test-lint\", which I would not want to see.\n\nMe neither. But until then it might well be that the benefit of having\nthis test on by default outweighs this potential problem. It would have\nsurely detected my fingers typing \"echo -n\" without my brain being alert\nenough to catch this portability issue ;-)\n\nThis is the updated version with proper SOBs and an updated commit message\nfor 2/2 which is trying to sum up the considerations raised in this thread.\n\n\nJens Lehmann (2):\n t/Makefile: check helper scripts for non-portable shell commands too\n t/Makefile: always test all lint targets when running tests\n\n t/Makefile | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\n-- \n2.0.1.476.gf051ede\n"},{"id":"245647","messageId":"53BD9934.9050708@web.de","threadId":"37055","inReplyTo":"53BD9908.1060807@web.de","subject":"[PATCH v2 1/2] t/Makefile: check helper scripts for non-portable shell commands too","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-07-09T19:34:12Z","receivedAt":"2014-07-09T19:34:12Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Currently only the \"t[0-9][0-9][0-9][0-9]-*.sh\" scripts are tested for\nshell incompatibilities using the check-non-portable-shell.pl script. This\nmakes it easy to miss non-POSIX constructs added to one of the t/*lib*.sh\nhelper scripts, as they aren't automatically detected.\n\nFix that by adding a THELPERS variable containing all shell scripts that\naren't tests and add these to the \"test-lint-shell-syntax\" target too.\n\nSigned-off-by: Jens Lehmann \n---\n t/Makefile | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 8fd1a72..7fa6692 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -29,6 +29,7 @@ TEST_RESULTS_DIRECTORY_SQ = $(subst ','\\'',$(TEST_RESULTS_DIRECTORY))\n T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))\n TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n+THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))\n\n all: $(DEFAULT_TEST_TARGET)\n\n@@ -65,7 +66,7 @@ test-lint-executable:\n \t\techo >&2 \"non-executable tests:\" $$bad; exit 1; }\n\n test-lint-shell-syntax:\n-\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)\n+\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)\n\n aggregate-results-and-cleanup: $(T)\n \t$(MAKE) aggregate-results\n-- \n2.0.1.476.gf051ede\n"},{"id":"245648","messageId":"53BD9952.8060109@web.de","threadId":"37055","inReplyTo":"53BD9908.1060807@web.de","subject":"[PATCH v2 2/2] t/Makefile: always test all lint targets when running tests","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-07-09T19:34:42Z","receivedAt":"2014-07-09T19:34:42Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Only the two targets \"test-lint-duplicates\" and \"test-lint-executable\" are\ncurrently executed when running the test target. This was done on purpose\nwhen the TEST_LINT variable was added in 81127d74 to avoid twisted shell\nscripting by developers only to avoid false positives that might result\nfrom the rather simple minded tests, e.g. test-lint-shell-syntax. But it\nlooks like it might be better to include all lint tests to help developers\nto detect non portable shell constructs before the patch is sent to the\nlist and reviewed there.\n\nChange the TEST_LINT variable to run all lint test unless the TEST_LINT\nvariable is overridden. If we hit false positives more often than helping\ndevelopers to avoid non-portable code (or add less accurate or slow tests\nlater) we could still fall back to exclude them like 81127d74 proposed.\n\nSigned-off-by: Jens Lehmann \n---\n t/Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 7fa6692..43b15e3 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -13,7 +13,7 @@ TAR ?= $(TAR)\n RM ?= rm -f\n PROVE ?= prove\n DEFAULT_TEST_TARGET ?= test\n-TEST_LINT ?= test-lint-duplicates test-lint-executable\n+TEST_LINT ?= test-lint\n\n ifdef TEST_OUTPUT_DIRECTORY\n TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results\n-- \n2.0.1.476.gf051ede\n"}]}