On 29-ene-2024 15:51:33, Junio C Hamano wrote:

Rubén Justo [off-list ref] writes:

quoted

quoted

The point of the t-basic tests is to ensure the lightweight unit
test framework that requires nothing from Git behaves (and keeps
behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]
tests under leak sanitizer is to exercise production Git code to
catch leaks in Git code.

So it is not quite clear if we even want to run this t0080 under
leak sanitizer to begin with.  t0080 is a relatively tiny test, but
do we even want to spend leak sanitizer cycles on it?  I dunno.

IIUC, that would imply building test-tool with a different set of flags
than Git, new artifacts ...  or running test-tool with some LSAN_OPTIONS
options, to disable it ...  or both ... or ...

And that is assuming that with test-tool we won't catch a leak in Git
that we're not seeing in the other tests ...

But t0080 does not even run test-tool, does it?  The t-basic unit
test is about testing the unit test framework and does not even
trigger any of the half-libified Git code.  So I am not sure why
you are bringing up test-tool into the picture.

Of course, test-tool has nothing to do here.  I think I got distracted
because:

  $ ( cd t; ./t0080-unit-test-output.sh )
  Bail out! You need to build test-tool; Run "make t/helper/test-tool" in the source (toplevel) directory

My reasoning was about t/unit-test/bin/t-basic (though also applies to
test-tool), due to:

  $ make SANITIZE=leak -n t/unit-tests/bin/t-basic 
  ...
  echo '   ' LINK t/unit-tests/bin/t-basic;cc   -g -O2 -Wall -I. \
  -DHAVE_SYSINFO -fsanitize=leak -fno-sanitize-recover=leak \
  -fno-omit-frame-pointer -DSUPPRESS_ANNOTATED_LEAKS -O0 \
  -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND \
  -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES \
  -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 \
  -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" \
  -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK \
  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME \
  -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_GETDELIM \
  '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES \
  -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' -o t/unit-tests/bin/t-basic   \
  t/unit-tests/t-basic.o t/unit-tests/test-lib.o common-main.o libgit.a \
  xdiff/lib.a reftable/libreftable.a libgit.a xdiff/lib.a \
  reftable/libreftable.a libgit.a -lz -lpthread -lrt

Note that we inject this flags:
  -fsanitize=leak -fno-sanitize-recover=leak -fno-omit-frame-pointer \
  -DSUPPRESS_ANNOTATED_LEAKS -O0

quoted

Maybe this is tangential to this series but,  while a decision is being
made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check
pass, which is the objective in this series. 

One major reason why we want to set TEST_PASSES_SANITIZE_LEAK to
true is because that way the marked test will be run under the leak
sanitizer in the CI.

What do we expect to gain by running t0080, which is to run the
t-basic unit test, under the leak sanitizer?  Unlike other
t[0-9][0-9][0-9][0-9] tests that exercise Git production code, would
we care about a new leak found in t-basic run from t0080 in the
first place?

Annotating with TEST_PASSES_SANITIZE_LEAK is not a goal by itself.

Indeed.  It points to a horizon.

Annotating the tests that we want to run under the sanitizer and see
them passing with it is.

Maybe this is also a horizon (not reachable by definition), and
expecting "make test" to be leak-free (including t0080) a good path
towards that horizon, IMHO.  But you are right, those leak sanitizer
cycles may not be worth it.

And obviously these tests that exercise
Git production code are very good candidates for us to do so.  It is
unclear if t0080 falls into the same category.  That is why I asked
what we expect to gain by running it.

Thanks.

Thank you for bringing up a good question.

I see you queued this as 4/4.  OK.  I'll consider that if a re-roll for
this series is needed.