Re: [PATCH] t5314: check exit code of "rev-parse"

From: René Scharfe <l.s.r@web.de>
Date: Nov 28, 2022, 13:51 UTC
Message-ID: <96a2f70d-c338-a992-a3e4-3e6b82b644ca@web.de>
In-Reply-To: <patch-1.1-45b240740eb-20221128T115740Z-avarab@gmail.com>

Am 28.11.2022 um 13:00 schrieb Ævar Arnfjörð Bjarmason:

> Amend the test added in [1] to check the exit code of the "rev-parse"
> invocations. An in-flight change[2] introduced a memory leak in these
> invocations, which went undetected unless we were running under
> "GIT_TEST_SANITIZE_LEAK_LOG=true".

Checking the return code of rev-parse might be a good idea, but AFAICS none of the patches in the thread around [2] add a leak to it; they are only about pack-objects. So that's not how this patch works.

Show 34 quoted lines

> Note that the in-flight change made 8 test files fail, but as far as I
> can tell only this one would have had its exit code hidden unless
> under "GIT_TEST_SANITIZE_LEAK_LOG=true". The rest would be caught
> without it.
>
> 1. 4cf2143e029 (pack-objects: break delta cycles before delta-search
>    phase, 2016-08-11)
> 2. https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/
> 3. faececa53f9 (test-lib: have the "check" mode for SANITIZE=leak
>    consider leak logs, 2022-07-28)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> Aside from rs/multi-filter-args, we should be checking the return code
> of "git" in these cases.
>
>  t/t5314-pack-cycle-detection.sh | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/t/t5314-pack-cycle-detection.sh b/t/t5314-pack-cycle-detection.sh
> index 73a241743aa..169d8198641 100755
> --- a/t/t5314-pack-cycle-detection.sh
> +++ b/t/t5314-pack-cycle-detection.sh
> @@ -63,13 +63,16 @@ TEST_PASSES_SANITIZE_LEAK=true
>  # Note that the two variants of "file" must be similar enough to convince git
>  # to create the delta.
>  make_pack () {
> -	{
> -		printf '%s\n' "-$(git rev-parse $2)"
> -		printf '%s dummy\n' "$(git rev-parse $1:dummy)"
> -		printf '%s file\n' "$(git rev-parse $1:file)"
> -	} |
> -	git pack-objects --stdout |

THIS is how it works: pack-objects with the patch in the grandparent of [2] and built with SANITIZE=leak fails here, but the test marches on, ignoring its return value.

> -	git index-pack --stdin --fix-thin
> +	grp1=$(git rev-parse "$2") &&
> +	grp2=$(git rev-parse "$1:dummy") &&
> +	grp3=$(git rev-parse "$1:file") &&

Nit: "grp" expands to "group" in my mind.  Here it stands for "git
rev-parse", hmm.  "commit", "dummy_blob" and "file_blob" might be
better names.

Show 6 quoted lines

> +	cat >list <<-EOF
> +	-$grp1
> +	$grp2 dummy
> +	$grp3 file
> +	EOF
> +	git pack-objects --stdout <list >pack &&

Here's the new return code check that makes the test detect the diffopt leak in question.

> +	git index-pack --stdin --fix-thin <pack
>  }
>
>  test_expect_success 'setup' '