From: "brian m. carlson" <sandals@crustytoothpaste•net>
To: redoste <redoste@redoste•xyz>
Cc: git@vger•kernel.org, Jeff King <peff@peff•net>,
Junio C Hamano <gitster@pobox•com>,
Fabian Stelzer <fs@gigacodes•de>,
Elijah Newren <newren@gmail•com>
Subject: Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile
Date: Sun, 6 Jul 2025 00:28:43 +0000 [thread overview]
Message-ID: <aGnDO3HIAQgsRbnJ@fruit.crustytoothpaste.net> (raw)
In-Reply-To: <DB4HMD2HLG24.1V8Y9JDW6BACQ@redoste.xyz>
[-- Attachment #1: Type: text/plain, Size: 4201 bytes --]
On 2025-07-05 at 22:50:43, redoste wrote:
> 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.
> > 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
next prev parent reply other threads:[~2025-07-06 0:28 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-04 23:08 [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile redoste
2025-07-05 19:21 ` Jeff King
2025-07-05 20:07 ` brian m. carlson
2025-07-05 22:50 ` redoste
2025-07-05 23:04 ` redoste
2025-07-06 0:28 ` brian m. carlson [this message]
2025-07-06 3:13 ` Jeff King
2025-07-06 17:20 ` redoste
2025-07-06 17:14 ` redoste
2025-07-06 17:34 ` [PATCH v2] " redoste
2025-07-06 21:21 ` brian m. carlson
2025-07-07 9:02 ` Patrick Steinhardt
2025-07-07 9:35 ` Phillip Wood
2025-07-07 14:25 ` redoste
2025-07-07 15:26 ` Phillip Wood
2025-07-07 16:22 ` redoste
2025-07-07 16:42 ` Phillip Wood
2025-07-07 18:48 ` [PATCH v3] " redoste
2025-07-07 20:57 ` Junio C Hamano
2025-07-08 6:47 ` Patrick Steinhardt
2025-07-08 13:59 ` Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aGnDO3HIAQgsRbnJ@fruit.crustytoothpaste.net \
--to=sandals@crustytoothpaste$(echo .)net \
--cc=fs@gigacodes$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=redoste@redoste$(echo .)xyz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox