[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
