threads / patch / 64296

patcht1016: make sure to use specified GPG

Subject: [PATCH] t1016: make sure to use specified GPG

## tl;dr

8 messages between Oct 10, 2025 and Oct 24, 2025. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Junio C Hamano· Oct 10, 2025, 21:14 UTC · lore

c348192a (t1016: clean up style, 2024-10-22) fixed a coding style violation that has an extra space between redirection operator ">" and the redirection target, but at the same time, replaced the use of "git config" to set a configuration variable to be used by the remainder of tests with "test_config". The pattern employed here is that the first set-up test prepares the environment to be used by subsequent tests, which then use the settings left by this set-up test to perform their tasks. Using test_config in the first set-up test means the config setting made by the set-up test is reverted at the end of the first set-up test, which totally misses the point.

Go back to use "git config" to fix this.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The commit in question was from October last year, and I didn't
   notice it until I looked at how the test script evolved.  It is a
   bit embarrassing that we didn't catch it during review.
 t/t1016-compatObjectFormat.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t1016-compatObjectFormat.sh +1 −1
diff --git a/t/t1016-compatObjectFormat.sh b/t/t1016-compatObjectFormat.sh
index 8341a2fe83..cb6d308f1d 100755
--- a/t/t1016-compatObjectFormat.sh
+++ b/t/t1016-compatObjectFormat.sh
@@ -116,7 +116,7 @@ do
 		git config core.repositoryformatversion 1 &&
 		git config extensions.objectformat $hash &&
 		git config extensions.compatobjectformat $(compat_hash $hash) &&
-		test_config gpg.program $TEST_DIRECTORY/t1016/gpg &&
+		git config gpg.program $TEST_DIRECTORY/t1016/gpg &&
 		echo "Hello World!" >hello &&
 		eval hello_${hash}_oid=$(git hash-object hello) &&
 		git update-index --add hello &&

-- 
2.51.0-616-gc7d8d4fd8b
Todd Zullinger· Oct 12, 2025, 14:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1016: make sure to use specified GPG

Junio C Hamano wrote:
Show 18 quoted lines
> c348192a (t1016: clean up style, 2024-10-22) fixed a coding style
> violation that has an extra space between redirection operator ">"
> and the redirection target, but at the same time, replaced the use
> of "git config" to set a configuration variable to be used by the
> remainder of tests with "test_config".  The pattern employed here is
> that the first set-up test prepares the environment to be used by
> subsequent tests, which then use the settings left by this set-up
> test to perform their tasks.  Using test_config in the first set-up
> test means the config setting made by the set-up test is reverted at
> the end of the first set-up test, which totally misses the point.
> 
> Go back to use "git config" to fix this.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  * The commit in question was from October last year, and I didn't
>    notice it until I looked at how the test script evolved.  It is a
>    bit embarrassing that we didn't catch it during review.
Interesting.  And well-spotted.

This _does_ seem to resolve the failures in our CI and in the Fedora build system. I was able to run a few test builds. With this fix, the tests were successful where they were not without it.

I remember suspecting the gpg calls were not using the wrapper command in gpg.program. I even tried forcing the --faked-system-time for all the tests to check that theory, unsuccessfully.

Oddly, I ran into test failures after fixing the GPG2 prereq long before c348192afe (t1016: clean up style, 2024-10-22) was in place. Perhaps I was hitting a different issue initially? Then, when I looked at it again I didn't think about gpg.program again, since I'd already tried to force the gpg wrapper which sets --faked-system-time.

It's both annoying and embarrassing if it is that simple and I missed it after looking a few times, to be sure. But I'll be happy with the end result all the same. :)

Show 16 quoted lines
>  t/t1016-compatObjectFormat.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t1016-compatObjectFormat.sh b/t/t1016-compatObjectFormat.sh
> index 8341a2fe83..cb6d308f1d 100755
> --- a/t/t1016-compatObjectFormat.sh
> +++ b/t/t1016-compatObjectFormat.sh
> @@ -116,7 +116,7 @@ do
>  		git config core.repositoryformatversion 1 &&
>  		git config extensions.objectformat $hash &&
>  		git config extensions.compatobjectformat $(compat_hash $hash) &&
> -		test_config gpg.program $TEST_DIRECTORY/t1016/gpg &&
> +		git config gpg.program $TEST_DIRECTORY/t1016/gpg &&
>  		echo "Hello World!" >hello &&
>  		eval hello_${hash}_oid=$(git hash-object hello) &&
>  		git update-index --add hello &&
-- 
Todd
Junio C Hamano· Oct 12, 2025, 15:11 UTC · re: Todd Zullinger · lore

Re: [PATCH] t1016: make sure to use specified GPG

Todd Zullinger <tmz@pobox.com> writes:
Show 22 quoted lines
> Interesting.  And well-spotted.
>
> This _does_ seem to resolve the failures in our CI and in
> the Fedora build system.  I was able to run a few test
> builds.  With this fix, the tests were successful where they
> were not without it.
>
> I remember suspecting the gpg calls were not using the
> wrapper command in gpg.program.  I even tried forcing the
> --faked-system-time for all the tests to check that theory,
> unsuccessfully.
>
> Oddly, I ran into test failures after fixing the GPG2 prereq
> long before c348192afe (t1016: clean up style, 2024-10-22)
> was in place.  Perhaps I was hitting a different issue
> initially?  Then, when I looked at it again I didn't think
> about gpg.program again, since I'd already tried to force
> the gpg wrapper which sets --faked-system-time.
>
> It's both annoying and embarrassing if it is that simple and
> I missed it after looking a few times, to be sure.  But I'll
> be happy with the end result all the same. :)

FWIW, GitHub CI jobs are failing t1016 at the tip of 'seen' (which has this change), but only some and not all the jobs, which may indicate there are timeing-dependent flakes involved. I didn't dig further, though.

Show 16 quoted lines
>>  t/t1016-compatObjectFormat.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/t/t1016-compatObjectFormat.sh b/t/t1016-compatObjectFormat.sh
>> index 8341a2fe83..cb6d308f1d 100755
>> --- a/t/t1016-compatObjectFormat.sh
>> +++ b/t/t1016-compatObjectFormat.sh
>> @@ -116,7 +116,7 @@ do
>>  		git config core.repositoryformatversion 1 &&
>>  		git config extensions.objectformat $hash &&
>>  		git config extensions.compatobjectformat $(compat_hash $hash) &&
>> -		test_config gpg.program $TEST_DIRECTORY/t1016/gpg &&
>> +		git config gpg.program $TEST_DIRECTORY/t1016/gpg &&
>>  		echo "Hello World!" >hello &&
>>  		eval hello_${hash}_oid=$(git hash-object hello) &&
>>  		git update-index --add hello &&
Todd Zullinger· Oct 12, 2025, 18:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1016: make sure to use specified GPG

Junio C Hamano wrote:
Show 29 quoted lines
> Todd Zullinger <tmz@pobox.com> writes:
> 
>> Interesting.  And well-spotted.
>>
>> This _does_ seem to resolve the failures in our CI and in
>> the Fedora build system.  I was able to run a few test
>> builds.  With this fix, the tests were successful where they
>> were not without it.
>>
>> I remember suspecting the gpg calls were not using the
>> wrapper command in gpg.program.  I even tried forcing the
>> --faked-system-time for all the tests to check that theory,
>> unsuccessfully.
>>
>> Oddly, I ran into test failures after fixing the GPG2 prereq
>> long before c348192afe (t1016: clean up style, 2024-10-22)
>> was in place.  Perhaps I was hitting a different issue
>> initially?  Then, when I looked at it again I didn't think
>> about gpg.program again, since I'd already tried to force
>> the gpg wrapper which sets --faked-system-time.
>>
>> It's both annoying and embarrassing if it is that simple and
>> I missed it after looking a few times, to be sure.  But I'll
>> be happy with the end result all the same. :)
> 
> FWIW, GitHub CI jobs are failing t1016 at the tip of 'seen' (which
> has this change), but only some and not all the jobs, which may
> indicate there are timeing-dependent flakes involved.  I didn't dig
> further, though.

Ahh. When I saw this patch and checked the actions, I looked at this job:

    https://github.com/git/git/actions/runs/18418984778

and the failures all seemed to be unrelated to the test. Looking again, I see the ps/ci-avoid-broken-sudo-on-ubuntu branch was merged to seen to fix that problem.

Between that initial "success other than unrelated failures" and my multiple successful runs in the previously-failing Fedora build system, I thought it might have been fixed. :/

When I poked at these failures in the Fedora builds many months ago, I was able to get the full test-results output via a bit of a gross hack to tar it up and print it to the build log as base64 text (only when there are failures):

    # tar up test-results & $testdir, then print base64 encoded output
    #
    # copy $testdir contents to test-results to avoid absolute paths with tar
    cp -a $testdir/* t/test-results/
    begin='-----BEGIN BASE64 MESSAGE-----'
    end='-----END BASE64 MESSAGE-----'
    printf '\n%s\n' 'test-results and trash directory output follows; decode via:'
    printf '%s\n' "sed -n '/^${begin}$/,/^${end}$/{/^${begin}$/!{/^${end}$/!p}}' \
        build.log | base64 -d >output.tar.zst"
    printf '%s\n' "$begin"
    tar --warning=no-file-ignored -C t -cf - test-results/ | zstdmt -17 | base64
    printf '%s\n' "$end"

I don't know if we have or might want something like that for the Github/Gitlab CI.

Even with the test-results directory contents, I didn't manage to work out what the issue was. Maybe some additional debug output from the code and tests to show the environment that is being used when gpg gets called to make/verify the signatures could help. I didn't try to adjust the code. I did add some debugging to the gpg wrapper and used that wrapper by default, without success.

But I imagine that's mostly an issue with my unfamiliarity with the code being tested. (The hacker version of "it's not you, it's me.")

-- 
Todd
Junio C Hamano· Oct 23, 2025, 20:52 UTC · re: Todd Zullinger · lore

Re: [PATCH] t1016: make sure to use specified GPG

Todd Zullinger <tmz@pobox.com> writes:
Show 14 quoted lines
> Junio C Hamano wrote:
>> Todd Zullinger <tmz@pobox.com> writes:
>> 
>>> Interesting.  And well-spotted.
>>>
>>> This _does_ seem to resolve the failures in our CI and in
>>> the Fedora build system.  I was able to run a few test
>>> builds.  With this fix, the tests were successful where they
>>> were not without it.
>> ...
>> FWIW, GitHub CI jobs are failing t1016 at the tip of 'seen' (which
>> has this change), but only some and not all the jobs, which may
>> indicate there are timeing-dependent flakes involved.  I didn't dig
>> further, though.

Let's merge this fix down, even though it does not seem to have any effect improving the situation of flaky tests here.

Todd Zullinger· Oct 24, 2025, 01:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1016: make sure to use specified GPG

Junio C Hamano wrote:
Show 19 quoted lines
> Todd Zullinger <tmz@pobox.com> writes:
> 
>> Junio C Hamano wrote:
>>> Todd Zullinger <tmz@pobox.com> writes:
>>> 
>>>> Interesting.  And well-spotted.
>>>>
>>>> This _does_ seem to resolve the failures in our CI and in
>>>> the Fedora build system.  I was able to run a few test
>>>> builds.  With this fix, the tests were successful where they
>>>> were not without it.
>>> ...
>>> FWIW, GitHub CI jobs are failing t1016 at the tip of 'seen' (which
>>> has this change), but only some and not all the jobs, which may
>>> indicate there are timeing-dependent flakes involved.  I didn't dig
>>> further, though.
> 
> Let's merge this fix down, even though it does not seem to have
> any effect improving the situation of flaky tests here.

Sounds good, thanks. It's the right thing to do, even if it didn't clear up the issues which led to you noticing.

-- 
Todd
Junio C Hamano· Oct 13, 2025, 15:38 UTC · re: Andrew Kreimer · lore

Re: [PATCH] t1016: make sure to use specified GPG

Andrew Kreimer <algonell@gmail.com> writes:
> On Fri, Oct 10, 2025 at 02:14:00PM -0700, Junio C Hamano wrote:
>> Go back to use "git config" to fix this.
>
> Apologies for the mess!

No need to apologize. A bug in reviewed patch is our collective failing, and the author does not have sole ownership of it.

Thanks.

← back to recent threads