Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests
Am 07.07.2014 20:13, schrieb Junio C Hamano: Jens Lehmann jens.lehm...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests
On Mon, Jul 07, 2014 at 11:13:11AM -0700, Junio C Hamano wrote: Jens Lehmann jens.lehm...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests
On Tue, Jul 8, 2014 at 12:24 PM, Jens Lehmann jens.lehm...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests
Jens Lehmann jens.lehm...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] t/Makefile: always test all lint targets when running tests
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
