Re: [PATCH 00/25] fix buggy tests, modernize tests, fix broken &&-chains

[Stefan Beller <sbeller@google.com>]

On Sun, Jul 1, 2018 at 5:24 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> This series fixes several buggy tests which went undetected due to
> broken &&-chains in subshells, modernizes some tests to take advantage
> of test functions (test_might_fail(), test_write_lines(), etc.), and
> fixes a lot of broken &&-chains in subshells. It applies atop
> 'master'. Happily, there are no broken &&-chains in subshells in any
> in-flight topic.
>
> It is split out from an earlier series[1] which additionally extended
> --chain-lint to work within subshells. I decided to make this an
> independent series because these (hopefully) non-controversial changes
> all stand on their own merit, and because I don't want to flood the list
> repeatedly with this lengthy series as I re-roll the "extend
> --chain-lint to work in subshells" functionality from [1].
>
> To ease review burden, I largely avoided noisy 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 with a simple
> here-doc.)
>
> Changes since [1]:
>
> * Found and fixed more &&-chain breakage, and converted a couple more
>   'unset' instances (which were hidden behind a MINGW prerequisite) to
>   sane_unset().
>
> * Rewrote commit messages to sell changes on their own merit rather than
>   leaning on --chain-lint as a crutch. (junio, jrnieder)
>
> * Changed a modernization/cleanup to use "! non-git-command" rather than
>   test_must_fail(), moving it to its own patch in the process. (j6t)
>
> * Changed "printf '%s\n'" idiom to test_write_lines(). (junio)
>
> * Incorporated Stefan's fix[2] for a broken 't/lib-submodule-update'
>   test since my interpretation of the problem was incorrect.
>
> * Converted a subshell 'echo' sequence to write_script().
>
> * Dropped patches which existed primarily to pacify --chain-lint; they
>   are no longer needed since I re-wrote the "linter" to detect &&-chain
>   breakage itself (by pure textual inspection) rather than by
>   incorporating subshell bodies into the main &&-chain:
>
>   t0001: use "{...}" block around "||" expression rather than subshell
>   t3303: use standard here-doc tag "EOF" to avoid fooling --chain-lint
>   t9104: use "{...}" block around "||" expression rather than subshell
>   t9401: drop unnecessary nested subshell
>
> * Dropped a patch which pretty much duplicated Junio's 037714252f
>   (tests: clean after SANITY tests, 2018-06-15), which graduated to
>   'master':
>
>   t7508: use test_when_finished() instead of managing exit code manually
>
> An interdiff against [1] is below, although I stripped out all the noisy
> "printf '%s\n'" to test_write_lines() differences, of which there were a
> lot, since they drowned out the other more significant changes.
>
> Thanks to Elijah, Hannes, Jonathan Nieder, Jonathan Tan, Junio, Luke,
> Peff, and Stefan for comments on [1].

Thanks for this series,
I think it is good to include it as is and build on top if needed. I had some
comments on the earlier part of the series, but that is really just the cherry
on top of the cake.

Thanks,
Stefan