Re: [PATCH] t4204 is not sanitizer clean at all

On Fri, Dec 17 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

This change looks good to me.

FWIW this is not a mistake on my part, but something I'm perfectly aware
of. I don't consider it to be "brekage".

We have plenty of place in the test suite where we hide exit codes on
the LHS of a pipe, or where we call a function that doesn't &&-chain its
git invocations.

In those cases we can and usually will "succeed" under LSAN, because it
allows the program to emit its full output, and will abort() at the very
end.

But pipes do not hide ONLY deaths by sanitizer.  And by relying on
the presence of pipe hiding deaths of git tools to mark the script
sanitizer-clean, the TEST_PASSES_SANITIZE_LEAK=true line adds an
unnecessary road-block for those who are cleaning up the "git whose
crash are hidden by being on the left hand side of the pipe"
pattern.

I do not know what to call it if not "breakage".

Yes it's broken as far as the test is concerned. I meant as far as
"GIT_TEST_PASSING_SANITIZE_LEAK" goes I consider it somewhere between
"meh" and "don't care yet".

I.e. these are pretty irrelevant for finding leaks, as we've got a huge
deluge of them elsewhere. At some point we might have a last few stray
memory leaks in git hidden by such patterns, but we're very far away
from that.

Sometimes fixing those is trivial as in 3247919a758 (commit-graph tests:
fix error-hiding graph_git_two_modes() helper, 2021-10-15), and
sometimes we'll find that the test was broken all along in some other
subtle way, as in the a046aa38ca9 (commit-graph tests: fix another
graph_git_two_modes() helper, 2021-10-15) follow-up.

But as to the "roadblock" I don't mind the
TEST_PASSES_SANITIZE_LEAK=true being removed from the script at the
slightest sign of trouble. Nobody should have to shift gears and chase
down some memory leak in "git log" just because they needed it for their
test setup.

And I'd very much prefer that to UNLEAK() just to avoid that
TEST_PASSES_SANITIZE_LEAK=true removal, because it makes fixing the leak
itself harder as far as what topic to target, re-adding
TEST_PASSES_SANITIZE_LEAK=true once it's fixed etc. goes.