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. > 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@xxxxxxxxxxxxxxxxxxx/ > 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@xxxxxxxxx> > --- > > 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. > + 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' '
