Re: [PATCH 09/10] leak tests: mark passing SANITIZE=leak tests as leak-free

From: Derrick Stolee <derrickstolee@github.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH 09/10] leak tests: mark passing SANITIZE=leak tests as leak-free
Date: Tue, 19 Jul 2022 21:50:57 -0400	[thread overview]
Message-ID: <fa01e4af-3744-e719-6916-4d7776da3d9a@github.com> (raw)
In-Reply-To: <patch-09.10-b75b93822e1-20220719T205710Z-avarab@gmail.com>

On 7/19/2022 5:05 PM, Ævar Arnfjörð Bjarmason wrote:
> Mark those remaining tests that pass when run under SANITIZE=leak with
> TEST_PASSES_SANITIZE_LEAK=true, these were either omitted in
> f346fcb62a0 (Merge branch 'ab/mark-leak-free-tests-even-more',
> 2021-12-15) and 5a4f8381b68 (Merge branch 'ab/mark-leak-free-tests',
> 2021-10-25), or have had their memory leaks fixed since then.
> 
> With this change there's now a a 1=1 mapping between those tests that

nit: here's another use of "1=1" which I read as "one equals one" and
not "one-to-one", so please expand into the words.

> we have opted-in via "TEST_PASSES_SANITIZE_LEAK=true", and those that
> pass with the new "check" mode:
> 
>     GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true make test SANITIZE=leak

Maybe split this line?

	GIT_TEST_PASSING_SANITIZE_LEAK=check \
		GIT_TEST_SANITIZE_LEAK_LOG=true \
		make test SANITIZE=leak

> Note that the "GIT_TEST_SANITIZE_LEAK_LOG=true" is needed due to the
> edge cases noted in a preceding commit, i.e. in some cases we'd pass
> the test itself, but still have outstanding leaks due to ignored exit
> codes.
> 
> The "GIT_TEST_SANITIZE_LEAK_LOG=true" corrects for that, we're only
> marking those tests as passing that really don't have any leaks,
> whether that was reflected in their exit code or not.

This paragraph repeats the previous one, but with different words.
Consider removing it.

>  t/t0027-auto-crlf.sh                | 1 +
>  t/t0032-reftable-unittest.sh        | 1 +
>  t/t0033-safe-directory.sh           | 1 +
>  t/t0050-filesystem.sh               | 1 +
>  t/t0095-bloom.sh                    | 2 ++
>  t/t1405-main-ref-store.sh           | 1 +
>  t/t1407-worktree-ref-store.sh       | 1 +
>  t/t1418-reflog-exists.sh            | 1 +
>  t/t1701-racy-split-index.sh         | 1 +
>  t/t2006-checkout-index-basic.sh     | 1 +
>  t/t2023-checkout-m.sh               | 1 +
>  t/t2205-add-worktree-config.sh      | 1 +
>  t/t3012-ls-files-dedup.sh           | 1 +
>  t/t4017-diff-retval.sh              | 1 +
>  t/t4051-diff-function-context.sh    | 1 +
>  t/t4057-diff-combined-paths.sh      | 1 +
>  t/t4114-apply-typechange.sh         | 1 +
>  t/t4301-merge-tree-write-tree.sh    | 1 +
>  t/t5315-pack-objects-compression.sh | 1 +
>  t/t5351-unpack-large-objects.sh     | 1 +
>  t/t5402-post-merge-hook.sh          | 1 +
>  t/t5503-tagfollow.sh                | 1 +
>  t/t6404-recursive-merge.sh          | 1 +
>  t/t6405-merge-symlinks.sh           | 1 +
>  t/t6408-merge-up-to-date.sh         | 1 +
>  t/t6411-merge-filemode.sh           | 1 +
>  t/t6413-merge-crlf.sh               | 1 +
>  t/t6415-merge-dir-to-symlink.sh     | 1 +
>  t/t6425-merge-rename-delete.sh      | 1 +
>  t/t6431-merge-criscross.sh          | 1 +
>  t/t7060-wtstatus.sh                 | 1 +
>  t/t7062-wtstatus-ignorecase.sh      | 1 +
>  t/t7110-reset-merge.sh              | 1 +
>  t/t7111-reset-table.sh              | 1 +
>  t/t7609-mergetool--lib.sh           | 1 +
>  t/t9100-git-svn-basic.sh            | 1 -
>  t/t9901-git-web--browse.sh          | 1 +

That's a lot of tests that we can mark as having no leaks. Nice.

I imagine that after this series cooks for a while we can consider
running this check in CI. That is, unless it's prohibitively
expensive to run under SANITIZE=leak.

Thanks,
-Stolee