Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Thu, Aug 23, 2018 at 2:02 PM Ævar Arnfjörð Bjarmason
wrote:
> Found in some 2.19 testing on AIX:
Thanks for reporting these issues.
> > + /^[ ]*EOF[ ]*$/!bhereslurp
>
> AIX sed doesn't like this, and will yell:
> :hereslurp is greater than eight characters
> This on top fixes it:
Fix make sense, and checking POSIX[1] , I see that it says that
behavior is unspecified for labels longer than 8 bytes.
[1]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
> > +# bare "(" line?
> > +/^[ ]*([]*$/ {
> > + # stash for later printing
>
> AIX sed doesn't like this either, and prints:
> sed:# stash for later printing is not a recognized function.
> I have no idea what the fix is for that one.
That's the only indented comment in the entire script, so it's almost
certainly that. How about we move the indented comment up to the
comment for the enclosing block, so it reads:
# bare "(" line? -- stash for later printing
/^[ ]*([]*$/ {
h
bnextline
}
I could prepare such a patch, although perhaps it would be better for
you to do so since you are in a position to test it (and because you
discovered the problems)?
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Wed, Jul 11 2018, Eric Sunshine wrote:
Found in some 2.19 testing on AIX:
> +# here-doc -- swallow it to avoid false hits within its body (but keep the
> +# command to which it was attached)
> +/<<[ ]*[-\\]*EOF[]*/ {
> + s/[ ]*<<[ ]*[-\\]*EOF//
> + h
> + :hereslurp
> + N
> + s/.*\n//
> + /^[ ]*EOF[ ]*$/!bhereslurp
> + x
> +}
AIX sed doesn't like this, and will yell:
:hereslurp is greater than eight characters
This on top fixes it:
diff --git a/t/chainlint.sed b/t/chainlint.sed
index 8544df38df..2333705b27 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -100 +100 @@
- :hereslurp
+ :hered
@@ -104 +104 @@
- bhereslurp
+ bhered
@@ -286 +286 @@ s/[ ]*< +:subshell
> +# bare "(" line?
> +/^[ ]*([]*$/ {
> + # stash for later printing
> + h
> + bnextline
> +}
> +# "(..." line -- split off and stash "(", then process "..." as its own line
AIX sed doesn't like this either, and prints:
sed:# stash for later printing is not a recognized function.
I have no idea what the fix is for that one.
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jul 31, 2018 at 02:55:51PM -0400, Eric Sunshine wrote: > > I hesitate to make any suggestion here, as I think we may have passed > > a point of useful cost/benefit in sinking more time into this script. > > But...is switching to awk or perl an option? Our test suite already > > depends on having a vanilla perl, so I don't think it would be a new > > dependency. And it would give you actual data structures. > > It would, and I did consider it, however, I was very concerned about > startup cost (launch time) with heavyweight perl considering that it > would have to be run for _every_ test. With 13000+ tests, that cost > was a very real concern, especially for Windows users, but even for > MacOS users (such as myself, for which the full test suite already > takes probably close to 30 minutes to run, even on a ram drive). So, I > wanted something very lightweight (and deliberately used that word in > the commit message), and 'sed' seemed the lightest-weight of the > bunch. Both perl and sed seem about the same on my system (sometimes one is faster than the other, and sometimes vice versa). However, I expect for Windows the problem is not how big the child executable is, but running a child process at all. I might be wrong, though. > 'awk' might be about as lightweight as 'sed', and it may even be > possible to coerce it into handling the task (since the linter's job > is primarily just a bunch of regex matching with very little > "manipulating"). v1 of the linter was somewhat simpler and didn't deal > with these more complex cases, such as nested here-docs. v1 also did > rather more "manipulating" of the script since the result was meant to > be run by the shell. When it came time to implement v2, which detects > broken &&-chains itself by textual inspection, most of the > functionality (coming from v1) was already implemented in 'sed', so > 'awk' never really came up as a candidate since rewriting the script > from scratch in 'awk' didn't seem like a good idea. (And, at the time > v2 was started, I didn't know that these more complex cases would > arise.) So, 'awk' might be a viable alternative, and perhaps I'll take > a stab at it for fun at some point (or not), but I don't think there's > a pressing need right now. Yeah, I agree with that. -Peff
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jul 31, 2018 at 8:50 AM Jeff King wrote: > On Mon, Jul 30, 2018 at 05:38:06PM -0400, Eric Sunshine wrote: > > I considered that, but it doesn't handle nested here-docs, which we > > actually have in the test suite. For instance, from t9300-fast-import: > > [...] > > Nesting could be handled easily enough either by stashing away the > > opening tag and matching against it later _or_ by doing recursive > > here-doc folding, however, 'sed' isn't a proper programming language > > and can't be coerced into doing either of those. (And, it was tricky > > enough just getting it to handle the nested case with a limited set of > > recognized tag names, without having to explicitly handle every > > combination of those names nested inside one another.) > > I hesitate to make any suggestion here, as I think we may have passed > a point of useful cost/benefit in sinking more time into this script. > But...is switching to awk or perl an option? Our test suite already > depends on having a vanilla perl, so I don't think it would be a new > dependency. And it would give you actual data structures. It would, and I did consider it, however, I was very concerned about startup cost (launch time) with heavyweight perl considering that it would have to be run for _every_ test. With 13000+ tests, that cost was a very real concern, especially for Windows users, but even for MacOS users (such as myself, for which the full test suite already takes probably close to 30 minutes to run, even on a ram drive). So, I wanted something very lightweight (and deliberately used that word in the commit message), and 'sed' seemed the lightest-weight of the bunch. 'awk' might be about as lightweight as 'sed', and it may even be possible to coerce it into handling the task (since the linter's job is primarily just a bunch of regex matching with very little "manipulating"). v1 of the linter was somewhat simpler and didn't deal with these more complex cases, such as nested here-docs. v1 also did rather more "manipulating" of the script since the result was meant to be run by the shell. When it came time to implement v2, which detects broken &&-chains itself by textual inspection, most of the functionality (coming from v1) was already implemented in 'sed', so 'awk' never really came up as a candidate since rewriting the script from scratch in 'awk' didn't seem like a good idea. (And, at the time v2 was started, I didn't know that these more complex cases would arise.) So, 'awk' might be a viable alternative, and perhaps I'll take a stab at it for fun at some point (or not), but I don't think there's a pressing need right now.
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Mon, Jul 30, 2018 at 05:38:06PM -0400, Eric Sunshine wrote: > > I wonder if it should look for something like [A-Z][A-Z_]* to catch > > all of these. > > I considered that, but it doesn't handle nested here-docs, which we > actually have in the test suite. For instance, from t9300-fast-import: > > cat >input <<-INPUT_END && > mark :2 > data < $file2_data > EOF > ... > INPUT_END > > Nesting could be handled easily enough either by stashing away the > opening tag and matching against it later _or_ by doing recursive > here-doc folding, however, 'sed' isn't a proper programming language > and can't be coerced into doing either of those. (And, it was tricky > enough just getting it to handle the nested case with a limited set of > recognized tag names, without having to explicitly handle every > combination of those names nested inside one another.) I hesitate to make any suggestion here, as I think we may have passed a point of useful cost/benefit in sinking more time into this script. But...is switching to awk or perl an option? Our test suite already depends on having a vanilla perl, so I don't think it would be a new dependency. And it would give you actual data structures. But like I said, it may not be worth it. I'd be OK just adjusting the false positive and moving on. > I am, for a couple reasons, somewhat hesitant to tweak the heuristic. > > First, each tweak has the potential of causing more false-positives or > (perhaps worse) false-negatives. The linter's own test-suite is > supposed to protect against that, but test suite coverage is never > perfect. > > Second, ideally, the linter should protect against new broken > &&-chains from entering the codebase, so poorly coded historic tests > such as these aren't necessarily good motivation for tweaking, _and_ > it is (hopefully) unlikely that we would allow this sort of ugly shell > code to enter the codebase going forward. (The counterargument is that > this false-positive doesn't help someone coding up a new test who > hasn't yet submitted the patch to the mailing list where more seasoned > eyes would suggest better coding style.) Right, I think the real cost is somebody who adds "<
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Mon, Jul 30, 2018 at 4:59 PM Jonathan Nieder wrote: > Eric Sunshine wrote: > > The subshell linter would normally fold out the here-doc content, but > > 'sed' isn't a proper programming language, so the linter can't > > recognize arbitrary here-doc tags. Instead it has hard-coded knowledge > > of the tags commonly used in the Git tests, specifically EOF, EOT, and > > INPUT_END. > > Oh, hmm. I also see some others (outside subshells, though): > > EXPECT_END > [...] Correct. The linter does fold-out top-level EOF here-docs to hedge against the body of a here-doc containing something that might look like the start of a subshell (which would activate the more strict/expensive &&-chain validation). It special-cases top-level EOF because it's such a common tag name, thus an easy way to avoid false-positives, in general. I didn't bother trying to recognize _every_ possible tag since those other here-docs don't trigger any problems. (It's heuristic-based, after all.) > I wonder if it should look for something like [A-Z][A-Z_]* to catch > all of these. I considered that, but it doesn't handle nested here-docs, which we actually have in the test suite. For instance, from t9300-fast-import: cat >input <<-INPUT_END && mark :2 data < > The linter also deals with multi-line $(...) expressions, however, it > > currently only recognizes them when the $( is on its own line. > > That's reasonable, especially if "on its own line" means "at end of > line". It does; sorry for not being clear. > What would help most is if the error message could explain what is > going on, but I understand that that can be hard to do in a sed > script. Right, unfortunately, it can't be too helpful, but, when fixing all those broken chains, I did find it useful to dump the result after 'sed' processing in order to identify what it was actually complaining about. I did so by changing the $1 in this line from test-lib.sh: error "bug in the test script: broken &&-chain or run-away HERE-DOC: $1" to print the 'sed' output instead. The linter prepends "??AMP??" and "??SEMI??" to suspect lines. It also prepends lines with ">" to indicate where it thinks a subshell ends. This information was quite helpful when figuring out what was broken in the test (or, for false positives, where a heuristic had gone wrong). I considered enabling this output by default instead of $1 but decided against it since it is only helpful for broken &&-chains in subshells, thus doesn't aid in the more common case of top-level &&-breakage, thus might be confusing. > > I could try to update the linter to not trip over this sort of input, > > however, this test code is indeed ugly and difficult to understand, > > and your rewrite[1] of it makes it far easier to grok, so I'm not sure > > the effort would be worthwhile. What do you think? > > I'd be happy to look over a change that handles more here-doc > delimiters or produces a clearer message for tests in poor style, but > I agree with you that it's not too important. I am, for a couple reasons, somewhat hesitant to tweak the heuristic. First, each tweak has the potential of causing more false-positives or (perhaps worse) false-negatives. The linter's own test-suite is supposed to protect against that, but test suite coverage is never perfect. Second, ideally, the linter should protect against new broken &&-chains from entering the codebase, so poorly coded historic tests such as these aren't necessarily good motivation for tweaking, _and_ it is (hopefully) unlikely that we would allow this sort of ugly shell code to enter the codebase going forward. (The counterargument is that this false-positive doesn't help someone coding up a new test who hasn't yet submitted the patch to the mailing list where more seasoned eyes would suggest better coding style.)
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Eric Sunshine wrote: > On Mon, Jul 30, 2018 at 2:14 PM Jonathan Nieder wrote: >> ( >> chks_sub=$(cat <> $chks >> TXT >> ) && >> >> Ugly quoting, useless use of "cat", etc, aside, I don't think it's >> missing any &&. Hints? > > Yes, it's a false positive. > > The subshell linter would normally fold out the here-doc content, but > 'sed' isn't a proper programming language, so the linter can't > recognize arbitrary here-doc tags. Instead it has hard-coded knowledge > of the tags commonly used in the Git tests, specifically EOF, EOT, and > INPUT_END. Oh, hmm. I also see some others (outside subshells, though): EXPECT_END FRONTEND_END END_PART1 SETUP_END EOF2 EXPECTED END_OF_LOG INPUT_END END_EXPECT I wonder if it should look for something like [A-Z][A-Z_]* to catch all of these. > The linter also deals with multi-line $(...) expressions, however, it > currently only recognizes them when the $( is on its own line. That's reasonable, especially if "on its own line" means "at end of line". What would help most is if the error message could explain what is going on, but I understand that that can be hard to do in a sed script. [...] > I could try to update the linter to not trip over this sort of input, > however, this test code is indeed ugly and difficult to understand, > and your rewrite[1] of it makes it far easier to grok, so I'm not sure > the effort would be worthwhile. What do you think? I'd be happy to look over a change that handles more here-doc delimiters or produces a clearer message for tests in poor style, but I agree with you that it's not too important. Thanks for looking it over. Sincerely, Jonathan > [1]: > https://public-inbox.org/git/20180730190738.gd156...@aiede.svl.corp.google.com/
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Mon, Jul 30, 2018 at 2:14 PM Jonathan Nieder wrote: > Eric Sunshine wrote: > > Address this shortcoming by feeding the body of the test to a > > lightweight "linter" which can peer inside subshells and identify broken > > &&-chains by pure textual inspection. > > This is causing contrib/subtree tests to fail for me: running "make -C > contrib/subtree test" produces Thanks, I forgot that some of 'contrib' had bundled tests. (In fact, I just checked the other 'contrib' tests and found that a MediaWiki test has a broken top-level &&-chain.) > The problematic test code looks like this: > > ( > chks_sub=$(cat < $chks > TXT > ) && > > Ugly quoting, useless use of "cat", etc, aside, I don't think it's > missing any &&. Hints? Yes, it's a false positive. The subshell linter would normally fold out the here-doc content, but 'sed' isn't a proper programming language, so the linter can't recognize arbitrary here-doc tags. Instead it has hard-coded knowledge of the tags commonly used in the Git tests, specifically EOF, EOT, and INPUT_END. The linter also deals with multi-line $(...) expressions, however, it currently only recognizes them when the $( is on its own line. Had this test used one of the common here-doc tags _or_ had it formatted the $(...) as described, then it wouldn't have misfired. I could try to update the linter to not trip over this sort of input, however, this test code is indeed ugly and difficult to understand, and your rewrite[1] of it makes it far easier to grok, so I'm not sure the effort would be worthwhile. What do you think? [1]: https://public-inbox.org/git/20180730190738.gd156...@aiede.svl.corp.google.com/
[PATCH 0/2] subtree: fix &&-chain and simplify tests (Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells)
(resetting cc list) Jonathan Nieder wrote: > This is causing contrib/subtree tests to fail for me: running "make -C > contrib/subtree test" produces [...] > error: bug in the test script: broken &&-chain or run-away HERE-DOC: [...] > Ugly quoting, useless use of "cat", etc, aside, I don't think it's > missing any &&. Hints? Turns out it was missing a && too. :) These patches are against "master". Ideally this would have come before es/chain-lint-in-subshell. Since this is contrib/, I'm okay with losing bisectability and having it come after, though. Thoughts of all kinds welcome. Jonathan Nieder (2): subtree: add missing && to &&-chain subtree: simplify preparation of expected results contrib/subtree/t/t7900-subtree.sh | 121 - 1 file changed, 31 insertions(+), 90 deletions(-)
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Hi, Eric Sunshine wrote: > The --chain-lint option detects broken &&-chains by forcing the test to > exit early (as the very first step) with a sentinel value. If that > sentinel is the test's overall exit code, then the &&-chain is intact; > if not, then the chain is broken. Unfortunately, this detection does not > extend to &&-chains within subshells even when the subshell itself is > properly linked into the outer &&-chain. > > Address this shortcoming by feeding the body of the test to a > lightweight "linter" which can peer inside subshells and identify broken > &&-chains by pure textual inspection. Interesting. >Although the linter does not > actually parse shell scripts, it has enough knowledge of shell syntax to > reliably deal with formatting style variations (as evolved over the > years) and to avoid being fooled by non-shell content (such as inside > here-docs and multi-line strings). This is causing contrib/subtree tests to fail for me: running "make -C contrib/subtree test" produces [...] *** t7900-subtree.sh *** ok 1 - no merge from non-existent subtree ok 2 - no pull from non-existent subtree ok 3 - add subproj as subtree into sub dir/ with --prefix ok 4 - add subproj as subtree into sub dir/ with --prefix and --message ok 5 - add subproj as subtree into sub dir/ with --prefix as -P and --message as -m ok 6 - add subproj as subtree into sub dir/ with --squash and --prefix and --message ok 7 - merge new subproj history into sub dir/ with --prefix ok 8 - merge new subproj history into sub dir/ with --prefix and --message ok 9 - merge new subproj history into sub dir/ with --squash and --prefix and --message ok 10 - merge the added subproj again, should do nothing ok 11 - merge new subproj history into subdir/ with a slash appended to the argument of --prefix ok 12 - split requires option --prefix ok 13 - split requires path given by option --prefix must exist ok 14 - split sub dir/ with --rejoin ok 15 - split sub dir/ with --rejoin from scratch ok 16 - split sub dir/ with --rejoin and --message ok 17 - split "sub dir"/ with --branch ok 18 - check hash of split ok 19 - split "sub dir"/ with --branch for an existing branch ok 20 - split "sub dir"/ with --branch for an incompatible branch error: bug in the test script: broken &&-chain or run-away HERE-DOC: subtree_test_create_repo "$subtree_test_count" && [...] ) Makefile:44: recipe for target 't7900-subtree.sh' failed The problematic test code looks like this: ( cd "$subtree_test_count/sub proj" && git fetch .. subproj-br && git merge FETCH_HEAD && chks="sub1 sub2 sub3 sub4" && chks_sub=$(cat <
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Eric Sunshine writes: > However, existing tests aside, the more important goal is detecting > problems in new or updated tests hiding genuine bugs in changes to Git > itself, so it may have some value. Yes, indeed.
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Thu, Jul 12, 2018 at 12:56 PM Jeff King wrote: > Like Junio, I'm a little nervous that this is going to end up being a > maintenance burden. People may hit false positives and then be > confronted with this horrible mass of sed to try to figure out what went > wrong [...] A very valid concern. > But I came around to thinking: > - this found and fixed real problems in the test suite, with minimal > false positives across the existing code The counterargument (and arguing against my own case) is that, while it found 3 or 4 genuine test bugs hidden by &&-breakage, they were just that: bugs in the tests; they weren't hiding any bugs in Git itself, which is pretty measly return for the effort invested in the linter. However, existing tests aside, the more important goal is detecting problems in new or updated tests hiding genuine bugs in changes to Git itself, so it may have some value. > - worst case is that relief is only a "git revert" away Right. It's just a developer aid, not a user-facing feature which has to be maintained in perpetuity, so retiring it is easy.
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Thu, Jul 12, 2018 at 06:50:20AM -0400, Eric Sunshine wrote: > And, perhaps most important: We're not tied indefinitely to the > "subset" implemented by the current linter. If it is indeed found to > be too strict or limiting, it can always be loosened or retired > altogether. Yeah, I agree this is the key point. Like Junio, I'm a little nervous that this is going to end up being a maintenance burden. People may hit false positives and then be confronted with this horrible mass of sed to try to figure out what went wrong (which isn't to bust on your sed in particular; I think you made a heroic effort in commenting). But I came around to thinking: - this found and fixed real problems in the test suite, with minimal false positives across the existing code - it's being done by a long-time contributor, not somebody who is going to dump sed on us and leave - worst case is that relief is only a "git revert" away So I'm OK with merging it, and even running it by default. -Peff
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Wed, Jul 11, 2018 at 5:37 PM Junio C Hamano wrote:
> As with the previous "transform the script and feed the result to
> shell" approach, this risks to force us into writing our tests in a
> subset of valid shell language, which is the primary reason why I
> was not enthused when I saw the previous round. The worst part of
> it is that the subset is not strictly defined based on the shell
> language syntax or features (e.g. we allow this and that feature but
> not that other feature) but "whatever that does not cause the linter
> script to trigger false positives".
Some observations perhaps worth considering:
The linter is happy (no false positives) with the 13000+ existing
tests (though, of course, not all of them use subshells). Those tests,
written over many years, vary quite wildly in style and implementation
approach, so the "subset" of shell language accepted by the linter is
quite broad.
The original --chain-lint series (jk/test-chain-lint) had to make some
changes, such as wrapping code in a {...} block[1], merely to pacify
the linter. v2 of the subshell linter required no such changes.
The subshell linter was crafted to be on par with the existing
--chain-lint in terms of strictness (and looseness), so the subshell
linter is not more strict than the existing implementation. (For
instance, one can escape the strict &&-chain requirement in the
existing --chain-lint by wrapping code in a {...} block. The subshell
linter intentionally allows that escape, as well.)
And, perhaps most important: We're not tied indefinitely to the
"subset" implemented by the current linter. If it is indeed found to
be too strict or limiting, it can always be loosened or retired
altogether.
Thanks for the feedback.
[1]: bfe998fc9b (t0050: appease --chain-lint, 2015-03-20)
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Eric Sunshine writes:
> The --chain-lint option detects broken &&-chains by forcing the test to
> exit early (as the very first step) with a sentinel value. If that
> sentinel is the test's overall exit code, then the &&-chain is intact;
> if not, then the chain is broken. Unfortunately, this detection does not
> extend to &&-chains within subshells even when the subshell itself is
> properly linked into the outer &&-chain.
>
> Address this shortcoming by feeding the body of the test to a
> lightweight "linter" which can peer inside subshells and identify broken
> &&-chains by pure textual inspection. Although the linter does not
> ...
> Heuristics are employed to properly identify the extent of a subshell
> formatted in the old-style since a number of legitimate constructs may
> superficially appear to close the subshell even though they don't. For
> example, it understands that neither "x=$(command)" nor "case $x in *)"
> end a subshell, despite the ")" at the end of line.
>
> Due to limitations of the tool used ('sed') and its inherent
> line-by-line processing, only subshells one level deep are handled, as
> well as one-liner subshells one level below that. Subshells deeper than
> that or multi-line subshells at level two are passed through as-is, thus
> &&-chains in their bodies are not checked.
>
> Signed-off-by: Eric Sunshine
> ---
As with the previous "transform the script and feed the result to
shell" approach, this risks to force us into writing our tests in a
subset of valid shell language, which is the primary reason why I
was not enthused when I saw the previous round. The worst part of
it is that the subset is not strictly defined based on the shell
language syntax or features (e.g. we allow this and that feature but
not that other feature) but "whatever that does not cause the linter
script to trigger false positives".
So I dunno. I haven't spent enough time to carefully look at the
actual scripts to access how serious the "problem" I perceive
actually is with this series to form a firm opinion yet. Let me
come back to the topic after doing so.
