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:
>> 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@xxxxxxxxx/
>
>> 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 <avarab@xxxxxxxxx>
