On Fri, Jan 15 2021, Denton Liu wrote:

Hi Ævar,

First of all, thanks for reviving this series! I hope that Bash accepts
your proposed patch because it would definitely be helpful.

*nod*

On Fri, Jan 15, 2021 at 12:35:12AM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

As the documentation here notes you usually do not want to do:

    test_might_fail grep ...

But instead:

    ! grep ...

However, as a future commit will show it's handy to be able to do:

    some | test_might_fail ok=sigpipe grep | commands | here

To allow "grep" to fail in the middle of a pipe, if we're in a mode
such as a "set -o pipefail" that knows how to accept check intra-pipe
failures.

From what I can see, there presently aren't any other use cases here
except for with grep. I propose writing a wrapper around
grep, à la [0]:

	test_filter () {
		grep "$@" || :
	}

This has two main advantanges: the first would be that we could avoid
complicating the test_must_fail_acceptable() code. The second is that
it would be much less of a mouthful to write and it would be more
readable.

Compare

	some | test_might_fail ok=sigpipe grep | commands | here

to

	some | test_filter | commands | here

I saw your original series/patch. including Junio's suggestion that
test_grep_return_success was a bit too verbose & the suggestion for
"test_filter".

I think the "test_might_fail" in this case is more readable, readable !=
short. I.e. imagine you haven't just been looking at this code & open
that test file. If it's using "test_{might,must}_fail ok=*" you're more
likely to immediately understand it since you've seen those functions in
lots of places before.

If not, then "test might fail ok=sigpipe" is almost so self-documenting
that you don't need to look at the function.

Whereas a "test_filter" for me at least would prompt an immediate "hrm?
what's that?", followed by grepping it and the side-quest of reading the
source for that function we use in a grand total of <10 places.

Anyway, just my 0.02. I also think it makes conceptual sense to just
have a limited whitelist in "test_{might,must}_fail", since in this case
the reason we recommend against its use for non-git doesn't
apply. I.e. we're normally not in the business of testing the OS, but in
this case it's got the useful behavior of knowing how to handle the exit
code & signal special-case, so we might as well use it.

[0]: https://lore.kernel.org/git/3f79d23b40c0586d0351f4d721097be4f7ba26b8.1573779465.git.liu.denton@gmail.com/ (local)

quoted

To test this in t0000-basic.sh we don't actually need to have
test_{might,must}_fail in the middle of a pipe, it'll just that it
accepts e.g. "grep" when we provide ok=sigpipe.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>