On Fri, Jan 15, 2021 at 10:39:17AM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

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.

But I'm left confused...do we expect grep to get sigpipe here? I don't
think so. The problem is that grep will return 0 for "I did not match
anything". We would get sigpipe if "commands | here" exited without
reading all of the input.

So the "ok=sigpipe" seems very misleading to me (and likewise, the whole
"test_might_fail ok=sigpipe" exception seems weird; sigpipe is not the
reason we want to use it with grep here; it is because grep might
actually fail).

Whereas the test-filter approach is expressing what we actually are
interested in (ignoring the exit code of grep, no matter what).

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.

I agree this is sometimes a problem. But if we want to inline it, it
seems like the correct spelling here is:

  some | { grep ... || true; } | commands | here

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.

I'm somewhat lukewarm on the whitelist already, as I got bit recently by
a (not yet published) test doing:

  foo() {
	git --with --some --options "$@"
  }
  test_expect_success '...' '
	# this one is expected to succeed
	foo ok &&
	# this one is not
	test_must_fail foo bad
  '

-Peff