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

From: redoste <redoste@redoste.xyz>
Date: Jul 7, 2025, 14:25 UTC
Message-ID: <DB5W4LH0MI5K.EJ0IILQ1IWR@redoste.xyz>
In-Reply-To: <05d66f9c-78b6-4624-a0e3-32235df05105@gmail.com>

On Mon Jul 7, 2025 at 11:35 CEST, Phillip Wood wrote:

Show 5 quoted lines

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

> The idiomatic way to test for an empty file is "test_file_is_empty 
> <file>" which avoids spawning wc.

Thanks, I will update this.

Show 5 quoted lines

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

-- 
redoste