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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Jul 6, 2025, 00:28 UTC
Message-ID: <aGnDO3HIAQgsRbnJ@fruit.crustytoothpaste.net>
In-Reply-To: <DB4HMD2HLG24.1V8Y9JDW6BACQ@redoste.xyz>

On 2025-07-05 at 22:50:43, redoste wrote:

Show 31 quoted lines

> On Sat Jul 5, 2025 at 22:07 CEST, brian m. carlson wrote:
> > On 2025-07-05 at 19:21:13, Jeff King wrote:
> > I don't have a strong view either way, but I do wonder if it's a good
> > idea to have the testsuite poking around in `/tmp`, although maybe if we
> > honour `TMPDIR` then it would be possible to do in a tidy way.
> I looked into adding a test, but I didn't find any other tests checking
> for temporary files and I agree that messing in /tmp doesn't feel really
> appropriate for the testsuite.
> 
> Maybe something like this?
> 
> diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
> index 065f780636..359dc8eba8 100755
> --- a/t/t7528-signed-commit-ssh.sh
> +++ b/t/t7528-signed-commit-ssh.sh
> @@ -85,6 +85,7 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen
> 	eval $(ssh-agent) &&
> 	test_when_finished "kill ${SSH_AGENT_PID}" &&
> 	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
> +	export TMPDIR=$(pwd) &&
> 	echo 1 >file && git add file &&
> 	git commit -a -m rsa-inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
> 	echo 2 >file &&
> @@ -95,7 +96,8 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen
> 	git commit -a -m ecdsa-inline -S"key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
> 	echo 4 >file &&
> 	test_config user.signingkey "key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
> -	git commit -a -m ecdsa-config -S
> +	git commit -a -m ecdsa-config -S &&
> +	! ls .git_signing_key_tmp*
>  '

What I would recommend is adding another test (that is, another `test_expect_success` assertion) that when signing we don't leave any temporary files behind. You can then squash that into your patch and send a v2.

As for `ls`, I wouldn't use that in scripting. I'd use something like this:

---- mkdir tmpdir && TMPDIR="$(pwd)/tmpdir" && export TMPDIR && # Other code find tmpdir -type f >files && test_line_count = 0 files ----

I think `find` is typically better in scripting and also in this case with a separate directory as the temporary directory we'll find any left over temporary files, not just the pattern we've fixed here.

Our coding guidelines point out that some shells don't like `export` with assignments, so we do those as two lines. Normally I would write what you have in my day-to-day life, but we run on a variety of systems that most people will never use.

>  test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '
> 
> I can add it in a v2 if you think it's a good way to test it.

I think the basic premise is sound, but a separate assertion would make it easier to readers why we're setting the temporary directory.

Show 18 quoted lines

> > I also have some friends who are trans and have transitioned or are in
> > the process of transitioning but have simply not gotten around to
> > getting legal paperwork done[1].
> This is the exact reason why I'm not very comfortable with using my
> legal or real name, (well, it's mostly because I still can't find a name
> I like).
> And since it's a simple patch that's probably not even copyrightable, I
> figured out that using a pseudonym was fine.
> 
> Since I knew that the Linux kernel changed their documentation to remove
> the use of "real name", I thought it was more common and didn't relly
> think about it a lot.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330
> 
> I'm sorry, I should have read the git documentation more thoroughly.
> 
> If it's really an issue I don't mind signing off with a different and
> more distinctive name.

No, I think this is fine. You weren't obligated to explain that and our policies should gracefully handle this situation regardless, but given what I said before and this context, I think the name you have is fine. Git unfortunately has poor support for replacing names and I would't want you to have to put your deadname into our history for all of eternity.

I'll send a patch to fix the policy.

-- 
brian m. carlson (they/them)
Toronto, Ontario, CA