Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 7, 2025, 15:26 UTC
Message-ID: <7130651b-76a6-4eb1-93cf-c9e237d398d7@gmail.com>
In-Reply-To: <DB5W4LH0MI5K.EJ0IILQ1IWR@redoste.xyz>

On 07/07/2025 15:25, redoste wrote:

Show 12 quoted lines

> On Mon Jul 7, 2025 at 11:35 CEST, Phillip Wood wrote:
>> On 07/07/2025 10:02, Patrick Steinhardt wrote:
>>> I think this exported environment variable now leaks into subsequent
>>> tests, doesn't it? We may want to do it in a subshell.
>>
>> That's a very good point.
> Sure thing, I took inspiration from the previous ssh-agent test and it
> exports the ssh-agent variables without spawning a subshell, so I
> figured it was fine, but it was only because test_when_finished will
> fail in a subshell.
> I guess it's probably fine to leak only the ssh-agent variables
> accross tests and keep TMPDIR in a subshell.

Hopefully having the ssh-agent variables set after the agent has been killed doesn't affect the later tests

Show 13 quoted lines

>> The idiomatic way to test for an empty file is "test_file_is_empty
>> <file>" which avoids spawning wc.
> Thanks, I will update this.
> 
>> As this test is an abridged version of the other test that uses
>> ssh-agent I think it would be more efficient to tweak that test to check
>> there are no temporary files left over. Our test suite is slow and
>> tweaking an existing test to improve our coverage instead of adding a
>> new test means we don't make it any slower than necessary.
> I moved this test out of the other ssh-agent one according to the advice
> from brian on the previous version. I agree that spawning multiple
> ssh-agent is definitly not the best for speed, but is it worth it to
> make the test failure less clear?

test_file_is_empty will print a diagnostic message if it fails so it should be clear what has caused the test failure

> I feel like it's not the best to keep checks for temporary files clean
> up in a test that (initially) only claims to sign commits.

You can always tweak the test title if you want. The way I see it is that the changes that are being tested are related to commit signing as the invariant that we want to assert is that temporary files are cleaned up after signing commits.

Thanks

Phillip