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

[Phillip Wood <phillip.wood123@gmail.com>]

On 07/07/2025 10:02, Patrick Steinhardt wrote:
> On Sun, Jul 06, 2025 at 07:34:49PM +0200, redoste wrote:
>> +test_expect_success GPGSSH 'check temporary files clean up when signing commits' '
>> +	test_config gpg.format ssh &&
>> +	eval $(ssh-agent) &&
>> +	test_when_finished "kill ${SSH_AGENT_PID}" &&
>> +	mkdir tmpdir &&
>> +	TMPDIR="$(pwd)/tmpdir" &&
>> +	export TMPDIR &&
> 
> 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.

> 
> 	mkdir tmpdir &&
> 	TMPDIR="$(pwd)/tmpdir" &&
> 	(
> 		export TMPDIR &&
> 		ssh-add "${GPGSSH_KEY_PRIMARY}" &&
> 		echo 1 >file && git add file &&
> 		git commit -a -m inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
> 		echo 2 >file &&
> 		git commit -a -m file -S"${GPGSSH_KEY_PRIMARY}"
> 	) &&
> 	find tmpdir -type f >tmpfiles &&
> 	test_line_count = 0 tmpfiles

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

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.

Thanks

Phillip

> 
> Patrick
> 
>> +	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
>> +	echo 1 >file && git add file &&
>> +	git commit -a -m inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
>> +	echo 2 >file &&
>> +	git commit -a -m file -S"${GPGSSH_KEY_PRIMARY}" &&
>> +	find tmpdir -type f >tmpfiles &&
>> +	test_line_count = 0 tmpfiles
>> +'
>> +
>>   test_expect_failure GPGSSH 'detect fudged commit with double signature (TODO)' '
>>   	sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
>>   	sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
>> -- 
>> 2.49.0
>>
>>
>