[PATCH] ci(*-leaks): skip the git-svn tests to save time

[PATCH] ci(*-leaks): skip the git-svn tests to save time

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

I noticed recently that the leak-checking jobs still take a lot of time,
and upon analysis, the git-svn tests contribute significantly to this.

Analyzing a recent CI run, I saw that the Git test suite contains
1,017 tests, running for approximately 5¼ hours total. Of these, 65
git-svn-related tests (~6% of test count) took 42.24 minutes combined,
accounting for ~13.% of the total runtime. This implies that the git-svn
tests are roughly twice as expernsive compared to the other tests.

However, testing git-svn in the leak-checking jobs provides minimal
value: git-svn is implemented as a Perl script, and leak checking only
handles C code. While git-svn does call into Git's built-in commands
that are implemented in C, these are standard Git operations that are
already thoroughly exercised elsewhere in the test suite. Therefore,
running the git-svn tests in the leak-checking jobs only adds to the
overall run time with little value in return.

Given that the leak-checking jobs are particularly time-intensive and
these 42+ minutes of SVN tests per job provide no additional leak
detection value, skip them in the *-leaks jobs to reduce CI runtime.

Assisted-by: Claude Sonnet 4.5
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    ci(*-leaks): skip the git-svn tests to save time
    
    I leaned heavily on AI to implement this patch, in particular when
    analyzing the logs. That's why I added that trailer talking about Claude
    Sonnet. If this is undesirable, please let me know.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2031%2Fdscho%2Fskip-svn-and-leak-tests-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2031/dscho/skip-svn-and-leak-tests-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2031

 ci/lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ci/lib.sh b/ci/lib.sh
index f561884d40..a165c7f268 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -356,6 +356,7 @@ linux-musl-meson)
 	;;
 linux-leaks|linux-reftable-leaks)
 	export SANITIZE=leak
+	export NO_SVN_TESTS=LetsSaveSomeTime
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined

base-commit: 7264e61d87e58b9d0f5e6424c47c11e9657dfb75
-- 
gitgitgadget

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> However, testing git-svn in the leak-checking jobs provides minimal
> value: git-svn is implemented as a Perl script, and leak checking only
> handles C code. While git-svn does call into Git's built-in commands
> that are implemented in C, these are standard Git operations that are
> already thoroughly exercised elsewhere in the test suite. Therefore,
> running the git-svn tests in the leak-checking jobs only adds to the
> overall run time with little value in return.

Very nicely reasoned.  And the implementation of this idea is ...

> diff --git a/ci/lib.sh b/ci/lib.sh
> index f561884d40..a165c7f268 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -356,6 +356,7 @@ linux-musl-meson)
>  	;;
>  linux-leaks|linux-reftable-leaks)
>  	export SANITIZE=leak
> +	export NO_SVN_TESTS=LetsSaveSomeTime
>  	;;

... surprisingly simple.  I very much like it.

Thanks.  Will queue.

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Phillip Wood]

Hi Johannes

On 16/01/2026 17:31, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> I noticed recently that the leak-checking jobs still take a lot of time,
> and upon analysis, the git-svn tests contribute significantly to this.
> 
> Analyzing a recent CI run, I saw that the Git test suite contains
> 1,017 tests, running for approximately 5¼ hours total. Of these, 65
> git-svn-related tests (~6% of test count) took 42.24 minutes combined,
> accounting for ~13.% of the total runtime. This implies that the git-svn
> tests are roughly twice as expernsive compared to the other tests.

Looking at the CI logs for this PR the p4 and cvs tests account for 
another 24 minutes of test time and I suspect they also offer little in 
the way of extra coverage. Unfortunately there is no equivalent of 
NO_SVN_TESTS to disable them - I wonder if building with NO_PYTHON and 
NO_PERL would make sense for the leak test job?

Either way I like the direction of this patch

Thanks

Phillip

> However, testing git-svn in the leak-checking jobs provides minimal
> value: git-svn is implemented as a Perl script, and leak checking only
> handles C code. While git-svn does call into Git's built-in commands
> that are implemented in C, these are standard Git operations that are
> already thoroughly exercised elsewhere in the test suite. Therefore,
> running the git-svn tests in the leak-checking jobs only adds to the
> overall run time with little value in return.
> 
> Given that the leak-checking jobs are particularly time-intensive and
> these 42+ minutes of SVN tests per job provide no additional leak
> detection value, skip them in the *-leaks jobs to reduce CI runtime.
> 
> Assisted-by: Claude Sonnet 4.5
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>      ci(*-leaks): skip the git-svn tests to save time
>      
>      I leaned heavily on AI to implement this patch, in particular when
>      analyzing the logs. That's why I added that trailer talking about Claude
>      Sonnet. If this is undesirable, please let me know.
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2031%2Fdscho%2Fskip-svn-and-leak-tests-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2031/dscho/skip-svn-and-leak-tests-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2031
> 
>   ci/lib.sh | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/ci/lib.sh b/ci/lib.sh
> index f561884d40..a165c7f268 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -356,6 +356,7 @@ linux-musl-meson)
>   	;;
>   linux-leaks|linux-reftable-leaks)
>   	export SANITIZE=leak
> +	export NO_SVN_TESTS=LetsSaveSomeTime
>   	;;
>   linux-asan-ubsan)
>   	export SANITIZE=address,undefined
> 
> base-commit: 7264e61d87e58b9d0f5e6424c47c11e9657dfb75

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Looking at the CI logs for this PR the p4 and cvs tests account for 
> another 24 minutes of test time and I suspect they also offer little in 
> the way of extra coverage. Unfortunately there is no equivalent of 
> NO_SVN_TESTS to disable them - I wonder if building with NO_PYTHON and 
> NO_PERL would make sense for the leak test job?
>
> Either way I like the direction of this patch
>
> Thanks
>
> Phillip

Yup, I generally like this direction, and introducing NO_P4_TESTS
and NO_CVS_TESTS would not be so bad.  Here is how it looks on top
of Dscho's patch.

--- >8 ---
Subject: [PATCH] ci: skip CVS and P4 tests in leaks job, too

Looking at the CI logs, the p4 and cvs tests account for another 24
minutes of test time and they offer minimal value for quite a
similar reason as the previous step.

Let's introduce and use a mechanism to skip these tests to save
some resources.

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/lib.sh       | 2 ++
 t/lib-cvs.sh    | 6 ++++++
 t/lib-git-p4.sh | 5 +++++
 3 files changed, 13 insertions(+)

diff --git a/ci/lib.sh b/ci/lib.sh
index a165c7f268..3ecbf147db 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -356,7 +356,9 @@ linux-musl-meson)
 	;;
 linux-leaks|linux-reftable-leaks)
 	export SANITIZE=leak
+	export NO_CVS_TESTS=LetsSaveSomeTime
 	export NO_SVN_TESTS=LetsSaveSomeTime
+	export NO_P4_TESTS=LetsSaveSomeTime
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined
diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
index 57b9b2db9b..c8b4404888 100644
--- a/t/lib-cvs.sh
+++ b/t/lib-cvs.sh
@@ -2,6 +2,12 @@
 
 . ./test-lib.sh
 
+if test -n "$NO_CVS_TESTS"
+then
+	skip_all='skipping git cvs tests, NO_CVS_TESTS defined'
+	test_done
+fi
+
 unset CVS_SERVER
 
 if ! type cvs >/dev/null 2>&1
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 2a5b8738ea..d22e9c684a 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -16,6 +16,11 @@ P4D_TIMEOUT=300
 
 . ./test-lib.sh
 
+if test -n "$NO_P4_TESTS"
+then
+	skip_all='skipping git p4 tests, NO_P4_TESTS defined'
+	test_done
+fi
 if ! test_have_prereq PYTHON
 then
 	skip_all='skipping git p4 tests; python not available'
-- 
2.53.0-rc0-217-gd590ba4684

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Kristoffer Haugsbakk]

On Sat, Jan 17, 2026, at 19:34, Junio C Hamano wrote:
>>[snip]
> Yup, I generally like this direction, and introducing NO_P4_TESTS
> and NO_CVS_TESTS would not be so bad.  Here is how it looks on top
> of Dscho's patch.
>
> --- >8 ---
> Subject: [PATCH] ci: skip CVS and P4 tests in leaks job, too
>
> Looking at the CI logs, the p4 and cvs tests account for another 24
> minutes of test time and they offer minimal value for quite a
> similar reason as the previous step.
>
> Let's introduce and use a mechanism to skip these tests to save
> some resources.
>
> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>

Nitpick: Using the commit ident

    Phillip Wood <phillip.wood@dunelm.org.uk>

might be slightly better?

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>[snip]

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> On Sat, Jan 17, 2026, at 19:34, Junio C Hamano wrote:
>>>[snip]
>> Yup, I generally like this direction, and introducing NO_P4_TESTS
>> and NO_CVS_TESTS would not be so bad.  Here is how it looks on top
>> of Dscho's patch.
>>
>> --- >8 ---
>> Subject: [PATCH] ci: skip CVS and P4 tests in leaks job, too
>>
>> Looking at the CI logs, the p4 and cvs tests account for another 24
>> minutes of test time and they offer minimal value for quite a
>> similar reason as the previous step.
>>
>> Let's introduce and use a mechanism to skip these tests to save
>> some resources.
>>
>> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>
> Nitpick: Using the commit ident
>
>     Phillip Wood <phillip.wood@dunelm.org.uk>
>
> might be slightly better?

I didn't even realize there are multiple addresses in play,
actually.  I just took it from the e-mail header's Cc: field,
which my MUA copied from From: field of the message I was responding
to, which was the identity of the person who suggested the change
after all ;-).

So, I dunno.

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Phillip Wood]

On 18/01/2026 00:35, Junio C Hamano wrote:
> "Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
> 
>> On Sat, Jan 17, 2026, at 19:34, Junio C Hamano wrote:
>>>> [snip]
>>> Yup, I generally like this direction, and introducing NO_P4_TESTS
>>> and NO_CVS_TESTS would not be so bad.  Here is how it looks on top
>>> of Dscho's patch.
>>>
>>> --- >8 ---
>>> Subject: [PATCH] ci: skip CVS and P4 tests in leaks job, too
>>>
>>> Looking at the CI logs, the p4 and cvs tests account for another 24
>>> minutes of test time and they offer minimal value for quite a
>>> similar reason as the previous step.
>>>
>>> Let's introduce and use a mechanism to skip these tests to save
>>> some resources.
>>>
>>> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>>
>> Nitpick: Using the commit ident
>>
>>      Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> might be slightly better?
> 
> I didn't even realize there are multiple addresses in play,
> actually.  I just took it from the e-mail header's Cc: field,
> which my MUA copied from From: field of the message I was responding
> to, which was the identity of the person who suggested the change
> after all ;-).

The dunelm address is a forwarding address that should keep working if I 
change my email provider. I keep meaning send a patch with a mailmap 
entry but never get round to actually doing it.

Thanks

Phillip

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Phillip Wood]

On 17/01/2026 18:34, Junio C Hamano wrote:
> 
> Yup, I generally like this direction, and introducing NO_P4_TESTS
> and NO_CVS_TESTS would not be so bad.  Here is how it looks on top
> of Dscho's patch.
> 
> --- >8 ---
> Subject: [PATCH] ci: skip CVS and P4 tests in leaks job, too
> 
> Looking at the CI logs, the p4 and cvs tests account for another 24
> minutes of test time and they offer minimal value for quite a
> similar reason as the previous step.
> 
> Let's introduce and use a mechanism to skip these tests to save
> some resources.

The patch looks good to me, it is very convenient that we can put the 
test in the library files rather than each test file. Should we drop 
these tests from the ASan job as well?

Thanks

Phillip

> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   ci/lib.sh       | 2 ++
>   t/lib-cvs.sh    | 6 ++++++
>   t/lib-git-p4.sh | 5 +++++
>   3 files changed, 13 insertions(+)
> 
> diff --git a/ci/lib.sh b/ci/lib.sh
> index a165c7f268..3ecbf147db 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -356,7 +356,9 @@ linux-musl-meson)
>   	;;
>   linux-leaks|linux-reftable-leaks)
>   	export SANITIZE=leak
> +	export NO_CVS_TESTS=LetsSaveSomeTime
>   	export NO_SVN_TESTS=LetsSaveSomeTime
> +	export NO_P4_TESTS=LetsSaveSomeTime
>   	;;
>   linux-asan-ubsan)
>   	export SANITIZE=address,undefined
> diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
> index 57b9b2db9b..c8b4404888 100644
> --- a/t/lib-cvs.sh
> +++ b/t/lib-cvs.sh
> @@ -2,6 +2,12 @@
>   
>   . ./test-lib.sh
>   
> +if test -n "$NO_CVS_TESTS"
> +then
> +	skip_all='skipping git cvs tests, NO_CVS_TESTS defined'
> +	test_done
> +fi
> +
>   unset CVS_SERVER
>   
>   if ! type cvs >/dev/null 2>&1
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index 2a5b8738ea..d22e9c684a 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -16,6 +16,11 @@ P4D_TIMEOUT=300
>   
>   . ./test-lib.sh
>   
> +if test -n "$NO_P4_TESTS"
> +then
> +	skip_all='skipping git p4 tests, NO_P4_TESTS defined'
> +	test_done
> +fi
>   if ! test_have_prereq PYTHON
>   then
>   	skip_all='skipping git p4 tests; python not available'

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> The patch looks good to me, it is very convenient that we can put the 
> test in the library files rather than each test file. Should we drop 
> these tests from the ASan job as well?

That's probably a good idea.

I also was wondering if we want a blanket NO_FOO_TESTS that we can
use instead listing all.  That FOO should not be SCM, though, as the
reason why we exclude the tests is not because they are test about
foreign SCM.  We exclude them as low value because testing them
exercises little code of ours that we may make mistakes these checks
are trying to uncover and that we can fix when they do.
NO_FOREIGN_CODE_TESTS?  I dunno.

Thanks.

> Thanks
>
> Phillip
>
>> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>   ci/lib.sh       | 2 ++
>>   t/lib-cvs.sh    | 6 ++++++
>>   t/lib-git-p4.sh | 5 +++++
>>   3 files changed, 13 insertions(+)
>> 
>> diff --git a/ci/lib.sh b/ci/lib.sh
>> index a165c7f268..3ecbf147db 100755
>> --- a/ci/lib.sh
>> +++ b/ci/lib.sh
>> @@ -356,7 +356,9 @@ linux-musl-meson)
>>   	;;
>>   linux-leaks|linux-reftable-leaks)
>>   	export SANITIZE=leak
>> +	export NO_CVS_TESTS=LetsSaveSomeTime
>>   	export NO_SVN_TESTS=LetsSaveSomeTime
>> +	export NO_P4_TESTS=LetsSaveSomeTime
>>   	;;
>>   linux-asan-ubsan)
>>   	export SANITIZE=address,undefined
>> diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
>> index 57b9b2db9b..c8b4404888 100644
>> --- a/t/lib-cvs.sh
>> +++ b/t/lib-cvs.sh
>> @@ -2,6 +2,12 @@
>>   
>>   . ./test-lib.sh
>>   
>> +if test -n "$NO_CVS_TESTS"
>> +then
>> +	skip_all='skipping git cvs tests, NO_CVS_TESTS defined'
>> +	test_done
>> +fi
>> +
>>   unset CVS_SERVER
>>   
>>   if ! type cvs >/dev/null 2>&1
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index 2a5b8738ea..d22e9c684a 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -16,6 +16,11 @@ P4D_TIMEOUT=300
>>   
>>   . ./test-lib.sh
>>   
>> +if test -n "$NO_P4_TESTS"
>> +then
>> +	skip_all='skipping git p4 tests, NO_P4_TESTS defined'
>> +	test_done
>> +fi
>>   if ! test_have_prereq PYTHON
>>   then
>>   	skip_all='skipping git p4 tests; python not available'

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Phillip Wood]

On 20/01/2026 15:42, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> The patch looks good to me, it is very convenient that we can put the
>> test in the library files rather than each test file. Should we drop
>> these tests from the ASan job as well?
> 
> That's probably a good idea.
> 
> I also was wondering if we want a blanket NO_FOO_TESTS that we can
> use instead listing all.  That FOO should not be SCM, though, as the
> reason why we exclude the tests is not because they are test about
> foreign SCM.  We exclude them as low value because testing them
> exercises little code of ours that we may make mistakes these checks
> are trying to uncover and that we can fix when they do.
> NO_FOREIGN_CODE_TESTS?  I dunno.

Having a single Makefile knob is tempting, the naming as tricky though. 
We're skipping these tests because they're scripted and we already have 
leak coverage for the git commands that they call, the fact that they're 
calling foreign programs is incidental to that. If "git svn" was 
implemented in C then we probably would want to check it for leaks even 
though it called a foreign program. That's a long winded way of saying I 
don't have any better suggestions!

Thanks

Phillip

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> ... If "git svn" was 
> implemented in C then we probably would want to check it for leaks even 
> though it called a foreign program. That's a long winded way of saying I 
> don't have any better suggestions!

I am not sure if I agree.  If Perl interpreter used to run the Perl
version of "git svn" were found leaky, are we willing to go in and
plug leaks there?  Not likely, particularly since it is not what we
ship and we do not have control over which version of Perl the users
have on their systems.  So we say "Perl is foreign and we are not
equipped to plug leaks in various versions of it on users' systems,
so it is not worth spending cycles to test for leaks in it".

If "git svn" were in C, linked with libsvn without using the perl
binding, and libsvn were found leaky, the story is the same.  We do
not control the version of libsvn the users have on their systems,
we are not equipped to plug leaks in there, so it is not our job to
spend cycles to test for leaks in it.

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Phillip Wood]

On 23/01/2026 17:46, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> ... If "git svn" was
>> implemented in C then we probably would want to check it for leaks even
>> though it called a foreign program. That's a long winded way of saying I
>> don't have any better suggestions!
> 
> I am not sure if I agree.  If Perl interpreter used to run the Perl
> version of "git svn" were found leaky, are we willing to go in and
> plug leaks there?  Not likely, particularly since it is not what we
> ship and we do not have control over which version of Perl the users
> have on their systems.  So we say "Perl is foreign and we are not
> equipped to plug leaks in various versions of it on users' systems,
> so it is not worth spending cycles to test for leaks in it".
>
> If "git svn" were in C, linked with libsvn without using the perl
> binding, and libsvn were found leaky, the story is the same.  We do
> not control the version of libsvn the users have on their systems,
> we are not equipped to plug leaks in there, so it is not our job to
> spend cycles to test for leaks in it.

I think that unless the libsvn that linked against was built with 
-fsanitize=leak we wouldn't find any leaks in it anyway. When I wrote my 
original mail I was imagining C implementation that forked "svn" but 
replaced the perl code with C that called the appropriate functions in 
libgit rather than forking git. In that case I think there's an argument 
for checking that our code does not leak. Anyway this is all rather 
hypothetical as we're not likely to rewrite these scripts in C.

Thanks

Phillip

Re: [PATCH] ci(*-leaks): skip the git-svn tests to save time

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I think that unless the libsvn that linked against was built with 
> -fsanitize=leak we wouldn't find any leaks in it anyway. When I wrote my 
> original mail I was imagining C implementation that forked "svn" but 
> replaced the perl code with C that called the appropriate functions in 
> libgit rather than forking git.

It was the scenario I was assuming as well, but I simply forgot to
consider that we want to catch leaks in our "client" code (client
from the point of view of the libsvn library).

And you are right.  It can be done to check our leaks without being
able to touch libsvn to fix their leaks, even though we may have to
filter out noises from the leak checker if there are their leaks we
cannot plug.

> In that case I think there's an argument 
> for checking that our code does not leak. Anyway this is all rather 
> hypothetical as we're not likely to rewrite these scripts in C.

;-).