[PATCH 00/29] t: detect and fix broken &&-chains in subshells
The --chain-lint[1] option detects breakage in the top-level &&-chain of tests. This series undertakes the more complex task of teaching it to also detect &&-chain breakage within subshells. See patch 29/29 for the gory details of how that's done. The series is built atop 'master', however, it has a conflict with an in-flight topic. In particular, patch 1/29 rewrites a test in t7508 in 'master' to avoid &&-chain breakage. 'jc/clean-after-sanity-tests' in 'next' performs pretty much the same rewrite. If this series is queued atop 'master', then it needs patch 1/29; if it is queued somewhere above 'jc/clean-after-sanity-tests', then 1/29 can be dropped. Aside from identifying a rather significant number of &&-chain breaks, repairing those broken chains uncovered genuine bugs in several tests which were hidden by missing &&-chain links. Those bugs are also fixed by this series. I would appreciate if the following people would double-check my fixes: Stefan Bellar - 8/29 "t7400" and (especially) 13/29 "lib-submodule-update" Jonathan Tan - 10/29 "t9001" Elijah Newren - 6/29 "t6036" In order to keep the noise level down and ease review burden, please take into account that I largely avoided modernizations and cleanups, and limited changes to merely adding "&&" even when some other transformation would have made the fix nicer overall. (For example, I resisted the urge to replace a series of 'echo' statements in a subshell with a simple here-doc.) Finally, although all simple &&-chain fixes originally inhabited a single patch, they were split out into several patches to avoid hitting vger.kernel.org's message size limit. However, when queuing, all patches titled "t: fix broken &&-chains in subshells" could be squashed into a single patch titled "t: fix broken &&-chains in subshells". [1]: https://public-inbox.org/git/20150320100429.ga17...@peff.net/ Eric Sunshine (29): t7508: use test_when_finished() instead of managing exit code manually t0001: use "{...}" block around "||" expression rather than subshell t1300: use sane_unset() to avoid breaking &&-chain t3303: use standard here-doc tag "EOF" to avoid fooling --chain-lint t5505: modernize and simplify hard-to-digest test t6036: fix broken "merge fails but has appropriate contents" tests t7201: drop pointless "exit 0" at end of subshell t7400: fix broken "submodule add/reconfigure --force" test t7810: use test_expect_code() instead of hand-rolled comparison t9001: fix broken "invoke hook" test t9104: use "{...}" block around "||" expression rather than subshell t9401: drop unnecessary nested subshell t/lib-submodule-update: fix broken "replace submodule must-fail" test t: drop subshell with missing &&-chain in favor of simpler construct t: drop unnecessary terminating semicolons in subshell t: use test_might_fail() instead of manipulating exit code manually t: use test_must_fail() instead of checking exit code manually t-t0999: fix broken &&-chains in subshells t1000-t1999: fix broken &&-chains in subshells t2000-t2999: fix broken &&-chains in subshells t3000-t3999: fix broken &&-chains in subshells t3030: fix broken &&-chains in subshells t4000-t4999: fix broken &&-chains in subshells t5000-t5999: fix broken &&-chains in subshells t6000-t6999: fix broken &&-chains in subshells t7000-t7999: fix broken &&-chains in subshells t9000-t: fix broken &&-chains in subshells t9119: fix broken &&-chains in subshells t/test-lib: teach --chain-lint to detect broken &&-chains in subshells t/lib-submodule-update.sh | 10 +- t/t-basic.sh | 2 +- t/t0001-init.sh | 2 +- t/t0003-attributes.sh | 24 +- t/t0021-conversion.sh | 4 +- t/t0090-cache-tree.sh | 2 +- t/t1004-read-tree-m-u-wf.sh | 8 +- t/t1005-read-tree-reset.sh| 10 +- t/t1008-read-tree-overlay.sh | 2 +- t/t1020-subdirectory.sh | 2 +- t/t1050-large.sh | 6 +- t/t1300-config.sh | 2 +- t/t1411-reflog-show.sh| 6 +- t/t1507-rev-parse-upstream.sh | 6 +- t/t1512-rev-parse-disambiguation.sh | 6 +- t/t1700-split-index.sh| 2 +- t/t2016-checkout-patch.sh | 24 +- t/t2103-update-index-ignore-missing.sh| 2 +- t/t2202-add-addremove.sh | 14 +- t/t3000-ls-files-others.sh| 2 +- t/t3006-ls-files-long.sh | 2 +- t/t3008-ls-files-lazy-init-name-hash.sh | 8 +- t/t3030-merge-recursive.sh| 340 +- t/t3050-subprojects-fetch.sh | 8 +- t/t3102-ls-tree-wildcards.sh | 2 +- t/t3301-notes.sh
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine wrote: > The --chain-lint[1] option detects breakage in the top-level &&-chain of > tests. This series undertakes the more complex task of teaching it to > also detect &&-chain breakage within subshells. See patch 29/29 for the > gory details of how that's done. This is awesome. It'll replace a hacky script I had that attempted to remind me when I messed up, and fix its false positives to boot. Plus, I won't forget to run it. > Aside from identifying a rather significant number of &&-chain breaks, > repairing those broken chains uncovered genuine bugs in several tests > which were hidden by missing &&-chain links. Those bugs are also fixed > by this series. I would appreciate if the following people would > double-check my fixes: > > Stefan Bellar - 8/29 "t7400" and (especially) 13/29 "lib-submodule-update" > Jonathan Tan - 10/29 "t9001" > Elijah Newren - 6/29 "t6036" Commented on the patch in question; 6/29 looks good. I also looked over the rest of the series. Apart from the ones you specifically called out as needing review by others besides me, and the final patch which makes me feel like a sed neophyte, all but one patch looked good to me. I just have a small question for that remaining patch, which I posted there. Thanks, Elijah
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
On Tue, Jun 26, 2018 at 5:20 AM Elijah Newren wrote: > On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine > wrote: > > Aside from identifying a rather significant number of &&-chain breaks, > > repairing those broken chains uncovered genuine bugs in several tests > > which were hidden by missing &&-chain links. Those bugs are also fixed > > by this series. I would appreciate if the following people would > > double-check my fixes: > > > > Stefan Bellar - 8/29 "t7400" and (especially) 13/29 "lib-submodule-update" > > Jonathan Tan - 10/29 "t9001" > > Elijah Newren - 6/29 "t6036" > > Commented on the patch in question; 6/29 looks good. > > I also looked over the rest of the series. Apart from the ones you > specifically called out as needing review by others besides me, and > the final patch which makes me feel like a sed neophyte, all but one > patch looked good to me. I just have a small question for that > remaining patch, which I posted there. I guess you refer to your question[1] about whether test_must_fail() is the correct choice over test_expect_code(). I just responded[2] with a hopefully satisfactory answer. [1]: https://public-inbox.org/git/CABPp-BFmfN6=e+3bakt-nh5hmu-368shgdnrnkrnmrvknx0...@mail.gmail.com/ [2]: https://public-inbox.org/git/CAPig+cRTG625H3CF1Zw30vQt2W8uKf1xLxVaQni2YbJ=xai...@mail.gmail.com/
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
Hi Eric, On Tue, Jun 26, 2018, 2:31 AM Eric Sunshine wrote: > > On Tue, Jun 26, 2018 at 5:20 AM Elijah Newren wrote: > > On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine > > wrote: > > > Aside from identifying a rather significant number of &&-chain breaks, > > > repairing those broken chains uncovered genuine bugs in several tests > > > which were hidden by missing &&-chain links. Those bugs are also fixed > > > by this series. I would appreciate if the following people would > > > double-check my fixes: > > > > > > Stefan Bellar - 8/29 "t7400" and (especially) 13/29 "lib-submodule-update" > > > Jonathan Tan - 10/29 "t9001" > > > Elijah Newren - 6/29 "t6036" > > > > Commented on the patch in question; 6/29 looks good. > > > > I also looked over the rest of the series. Apart from the ones you > > specifically called out as needing review by others besides me, and > > the final patch which makes me feel like a sed neophyte, all but one > > patch looked good to me. I just have a small question for that > > remaining patch, which I posted there. > > I guess you refer to your question[1] about whether test_must_fail() > is the correct choice over test_expect_code(). I just responded[2] > with a hopefully satisfactory answer. Yes, it does. Thanks!
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
Eric Sunshine writes: > The --chain-lint[1] option detects breakage in the top-level &&-chain of > tests. This series undertakes the more complex task of teaching it to > also detect &&-chain breakage within subshells. See patch 29/29 for the > gory details of how that's done. I first looked at 29/29 and got heavily inclined to reject that step, and then continued reading from 1/29 to around 15/29. I like these earlier changes that fix existing breakage, of course. I also like many of the changes that simplify and/or modernise the test scripts very much, but they are unusable as-is as long as their justification is "chain-lint will start barfing on these constructs".
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
On Tue, Jun 26, 2018 at 3:38 PM Junio C Hamano wrote: > I first looked at 29/29 and got heavily inclined to reject that > step, and then continued reading from 1/29 to around 15/29. > > I like these earlier changes that fix existing breakage, of course. > I also like many of the changes that simplify and/or modernise the > test scripts very much, but they are unusable as-is as long as their > justification is "chain-lint will start barfing on these constructs". Sorry, I'm having difficulty understanding. Are you saying that you don't want patches which exist merely to pacify --chain-lint? (For instance, 2/29 "t0001: use "{...}" block around "||" expression rather than subshell".) Or are you saying that you don't like how the commit messages are worded, and that they should instead emphasize that the change is good for its own sake, without mentioning --chain-lint?
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
Eric Sunshine writes: > On Tue, Jun 26, 2018 at 3:38 PM Junio C Hamano wrote: >> I first looked at 29/29 and got heavily inclined to reject that >> step, and then continued reading from 1/29 to around 15/29. >> >> I like these earlier changes that fix existing breakage, of course. >> I also like many of the changes that simplify and/or modernise the >> test scripts very much, but they are unusable as-is as long as their >> justification is "chain-lint will start barfing on these constructs". > > Sorry, I'm having difficulty understanding. > > Are you saying that you don't want patches which exist merely to > pacify --chain-lint? (For instance, 2/29 "t0001: use "{...}" block > around "||" expression rather than subshell".) Yes. > Or are you saying that you don't like how the commit messages are > worded, and that they should instead emphasize that the change is good > for its own sake, without mentioning --chain-lint? Yes, too. For example, 03/29 is a good clean-up, and its value is not diminished even if we reject the subprocess munging --chain-lint in 29/29. As opposed to 02/29 which mostly is about appeasing the "shell parser" in 29/29 (or you could justify it saying "one less fork and process" if that gives us a measurable benefit).
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
Jun 26, 2018 at 03:31:11PM -0700, Junio C Hamano wrote: > Eric Sunshine writes: >> On Tue, Jun 26, 2018 at 3:38 PM Junio C Hamano wrote: >>> I like these earlier changes that fix existing breakage, of course. >>> I also like many of the changes that simplify and/or modernise the >>> test scripts very much, but they are unusable as-is as long as their >>> justification is "chain-lint will start barfing on these constructs". >> >> Sorry, I'm having difficulty understanding. >> >> Are you saying that you don't want patches which exist merely to >> pacify --chain-lint? (For instance, 2/29 "t0001: use "{...}" block >> around "||" expression rather than subshell".) > > Yes. > >> Or are you saying that you don't like how the commit messages are >> worded, and that they should instead emphasize that the change is good >> for its own sake, without mentioning --chain-lint? > > Yes, too. > > For example, 03/29 is a good clean-up, and its value is not > diminished even if we reject the subprocess munging --chain-lint in > 29/29. > > As opposed to 02/29 which mostly is about appeasing the "shell > parser" in 29/29 (or you could justify it saying "one less fork and > process" if that gives us a measurable benefit). This is a lighter-weight example of the practice described at https://lkml.kernel.org/r/alpine.LFD.2.00.1001251002430.3574@localhost.localdomain/. In my opinion it's good advice, often worth repeating. Thanks, Jonathan