On Thu, Nov 14, 2019 at 05:00:29PM -0800, Denton Liu wrote: > Patches 1-20 perform some general test cleanup to modernise the style. > They should be relatively uncontroversial and can be merged earlier (or > as a separate series) if desired. The reason these tests were identified > for cleanup was because they failed under `set -o pipefail`. > > Patches 21-27 should be considered RFC. In an attempt to catch git > commands failing in the upstream of a pipe, we enable `set -o pipefail` > on Bash. This may result in some funny-looking shell script constructs > (e.g. needing to wrap `grep` since it may "fail") but overall, I think it > is an improvement since we catch failure in more cases. Using pipefail can have unexpected consequences for other commands if they rely on SIGPIPE/EPIPE to signal the left-hand side of the pipe. E.g., this: $ set -o pipefail $ yes | head >/dev/null $ echo $? will consistently yield 141, since "yes" will always die to SIGPIPE after "head" stops reading from it. But much worse, this can be racy. Take something like this (which is a real snippet in our test suite): git status -s -b | head -1 The "head" process will quit after reading one line. What's the exit code of "git status"? It's either "0", if it managed to write everything into the pipe buffer before the simultaneously-running "head" closed the pipe. Or it's 141, if it didn't and got SIGPIPE. You could argue that "git status" should not be on the left-hand side of a pipe, but the same holds for any command on the left-hand side. E.g., grepping in our test scripts yields some bits like: t/t7003-filter-branch.sh: echo "$faux_gpg_tag" | sed -e s/XXXXXX/$sha1/ | head -n 6 > expect && t/t9400-git-cvsserver-server.sh: test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5 | head -n 1))" = "empty/1.1/" && And it's not just "head" that doesn't read all of its input. Another common one is "grep -q", which can quit as soon as it sees a match. So I have a feeling that pipefail is going to create more headaches than it solves. -Peff
