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, 09:35 UTC
Message-ID: <05d66f9c-78b6-4624-a0e3-32235df05105@gmail.com>
In-Reply-To: <aGuNCGNk96DK4GzX@pks.im>

On 07/07/2025 10:02, Patrick Steinhardt wrote:

Show 11 quoted lines

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

Show 13 quoted lines

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

Show 20 quoted lines

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