[PATCH] t1016: make sure to use specified GPG

[PATCH] t1016: make sure to use specified GPG

[Junio C Hamano]

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(-)

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

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

[Todd Zullinger]

Junio C Hamano wrote:
> 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. :)

>  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

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

[Junio C Hamano]

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.

>>  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 &&

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

[Todd Zullinger]

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.
>>
>> 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

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

[Junio C Hamano]

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.

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

[Todd Zullinger]

Junio C Hamano wrote:
> 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

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

[Andrew Kreimer]

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!

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

[Junio C Hamano]

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.