{"thread":{"id":"63744","subject":"[PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","startedAt":"2025-07-05T00:27:21Z","lastAt":"2025-07-08T13:59:41Z","messageCount":21,"participants":["redoste","Jeff King","brian m. carlson","Patrick Steinhardt","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"521340","messageId":"20250704230829.29696-1-redoste@redoste.xyz","threadId":"63744","inReplyTo":null,"subject":"[PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"redoste","fromEmail":"redoste@redoste.xyz","sentAt":"2025-07-04T23:08:28Z","receivedAt":"2025-07-05T00:27:21Z","isPatch":true,"sender":{"key":"redoste@redoste.xyz","avatar":"https://avatars.githubusercontent.com/u/12833420?v=4"},"body":"Detaching the filename string from the tempfile structure used to cause\ndelete_tempfile() to fail and the temporary file was not cleaned up.\n\nSigned-off-by: redoste <redoste@redoste.xyz>\n---\n gpg-interface.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0896458de5..bdcc8c2a2e 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -1048,7 +1048,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \t\t\t\t    key_file->filename.buf);\n \t\t\tgoto out;\n \t\t}\n-\t\tssh_signing_key_file = strbuf_detach(&key_file->filename, NULL);\n+\t\tssh_signing_key_file = xstrdup(key_file->filename.buf);\n \t} else {\n \t\t/* We assume a file */\n \t\tssh_signing_key_file = interpolate_path(signing_key, 1);\n-- \n2.49.0\n\n"},{"id":"521346","messageId":"20250705192113.GB2496172@coredump.intra.peff.net","threadId":"63744","inReplyTo":"20250704230829.29696-1-redoste@redoste.xyz","subject":"Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-05T19:21:13Z","receivedAt":"2025-07-05T19:21:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 05, 2025 at 01:08:28AM +0200, redoste wrote:\n\n> Detaching the filename string from the tempfile structure used to cause\n> delete_tempfile() to fail and the temporary file was not cleaned up.\n\nGood catch. I can reproduce this easily with:\n\n  git -c gpg.format=ssh \\\n      -c user.signingkey=key::does-not-exist \\\n      commit --allow-empty -S -m foo\n\nwhich creates /tmp/.git_signing_key_tmp* and never cleans it up.\n\nI wonder if it is worth adding a test, or if it would be too weirdly\nfocused on this obscure case to be very useful against future\nregressions.\n\n> Signed-off-by: redoste <redoste@redoste.xyz>\n\nWe look for a real name in the sign-off trailer, since it indicates an\nacceptance of the DCO and the ability to legally contribute the patch to\nthe project. See the section of Documentation/SubmittingPatches starting\nwith the '[[dco]]'. Or here:\n\n  https://git-scm.com/docs/SubmittingPatches#sign-off\n\nLooking at your web page, it looks like you may prefer not to associate\nyour online identity with a legal name. I can't remember if we've dealt\nwith this before. I'm adding brian to the cc, who has given a lot of\nthought to naming and privacy issues.\n\n(In this particular case, I suspect the patch is trivial enough not to\neven be copyright-able, so we may be able to just let it go in this\ninstance.)\n\n\nThe patch itself looks good:\n\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 0896458de5..bdcc8c2a2e 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -1048,7 +1048,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n>  \t\t\t\t    key_file->filename.buf);\n>  \t\t\tgoto out;\n>  \t\t}\n> -\t\tssh_signing_key_file = strbuf_detach(&key_file->filename, NULL);\n> +\t\tssh_signing_key_file = xstrdup(key_file->filename.buf);\n>  \t} else {\n>  \t\t/* We assume a file */\n>  \t\tssh_signing_key_file = interpolate_path(signing_key, 1);\n\nI think we could avoid even xstrdup() here and just use the key_file\npointer directly (it lasts until the end of the function, which is all\nwe need). But the call to interpolate_path() on the other side of\nconditional does always allocate, so we'd need extra book-keeping to\ndecide whether to free or not. The extra copy is worth it to keep the\ncode simpler.\n\n\nOne other thing I noticed while looking at this function: the tempfile\ndeletion code should be OK with a NULL entry (that is considered an\n\"inactive\" tempfile). So:\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0896458de5..fc48f93e11 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -1102,10 +1102,8 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \tremove_cr_after(signature, bottom);\n \n out:\n-\tif (key_file)\n-\t\tdelete_tempfile(&key_file);\n-\tif (buffer_file)\n-\t\tdelete_tempfile(&buffer_file);\n+\tdelete_tempfile(&key_file);\n+\tdelete_tempfile(&buffer_file);\n \tif (ssh_signature_filename.len)\n \t\tunlink_or_warn(ssh_signature_filename.buf);\n \tstrbuf_release(&signer_stderr);\n\nwould simplify the code a little bit. But it may not be worth worrying\ntoo much about (and is certainly orthogonal to your patch).\n\n-Peff\n"},{"id":"521347","messageId":"aGmGCmkwC1HlSyog@fruit.crustytoothpaste.net","threadId":"63744","inReplyTo":"20250705192113.GB2496172@coredump.intra.peff.net","subject":"Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-07-05T20:07:38Z","receivedAt":"2025-07-05T20:07:45Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-07-05 at 19:21:13, Jeff King wrote:\n> On Sat, Jul 05, 2025 at 01:08:28AM +0200, redoste wrote:\n> \n> > Detaching the filename string from the tempfile structure used to cause\n> > delete_tempfile() to fail and the temporary file was not cleaned up.\n> \n> Good catch. I can reproduce this easily with:\n> \n>   git -c gpg.format=ssh \\\n>       -c user.signingkey=key::does-not-exist \\\n>       commit --allow-empty -S -m foo\n> \n> which creates /tmp/.git_signing_key_tmp* and never cleans it up.\n> \n> I wonder if it is worth adding a test, or if it would be too weirdly\n> focused on this obscure case to be very useful against future\n> regressions.\n\nI don't have a strong view either way, but I do wonder if it's a good\nidea to have the testsuite poking around in `/tmp`, although maybe if we\nhonour `TMPDIR` then it would be possible to do in a tidy way.\n\n> > Signed-off-by: redoste <redoste@redoste.xyz>\n> \n> We look for a real name in the sign-off trailer, since it indicates an\n> acceptance of the DCO and the ability to legally contribute the patch to\n> the project. See the section of Documentation/SubmittingPatches starting\n> with the '[[dco]]'. Or here:\n> \n>   https://git-scm.com/docs/SubmittingPatches#sign-off\n> \n> Looking at your web page, it looks like you may prefer not to associate\n> your online identity with a legal name. I can't remember if we've dealt\n> with this before. I'm adding brian to the cc, who has given a lot of\n> thought to naming and privacy issues.\n\nI don't know if we have a strict policy.  I do know that there are\ndevelopers who always go by a pseudonym, such as chromatic[0], the\ncontributor to Perl, and obviously we'd want to allow them to\ncontribute. We also let people use shortened forms of their names or\ninitials (for instance, Jeff King).\n\nI also have some friends who are trans and have transitioned or are in\nthe process of transitioning but have simply not gotten around to\ngetting legal paperwork done[1].  Obviously they have a distinct and\nidentifiable name that they go by and we'd allow them to use a preferred\nname.\n\nThere might also be good reasons that a contributor might not want to\nuse a legal name: harassment, threats, employer hostility, fame[2], or a\nhostile government, to name a few.  I think those are legitimate reasons\nto contribute pseudonymously.\n\nSo I would say that if someone has a distinct and identifiable identity\nthat is pseudonymous and that is generally used and visible in the\npublic sphere online, that's probably good enough.  While I'm not a\nlawyer, it's my understanding that in many locales, making a legal\npromise of sorts (such as a sign-off) is equally binding whether made\nwith one's real name or a pseudonym, so I don't see a problem with the\nlegal aspect of it.\n\n[0] https://en.wikipedia.org/wiki/Chromatic_(programmer)\n[1] In some locales this involves hiring an attorney, getting paperwork\nfrom a doctor, and getting a court order, so it can be expensive and\nkind of a hassle to do.  It may also not be legally possible to do that\nin some places.\n[2] Notably the frontman of the band Weezer, Rivers Cuomo, is involved\nin coding under his real name (https://github.com/riverscuomo), but\nperhaps a CEO, musician, actor, or other famous person might not want\ntheir open-source contributions to be associated with their real name.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"521348","messageId":"DB4HWL9M36X4.2GND0UHYOS8Q0@redoste.xyz","threadId":"63744","inReplyTo":"DB4HMD2HLG24.1V8Y9JDW6BACQ@redoste.xyz","subject":"Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"redoste","fromEmail":"redoste@redoste.xyz","sentAt":"2025-07-05T23:04:05Z","receivedAt":"2025-07-05T23:22:25Z","isPatch":true,"sender":{"key":"redoste@redoste.xyz","avatar":"https://avatars.githubusercontent.com/u/12833420?v=4"},"body":"On Sun Jul 6, 2025 at 00:50 CEST, redoste wrote:\n> Maybe something like this?\nSorry, it's my first time contributing via a mailing list and I stupidly\ncopy-pasted the output of `git diff` without checking the whitespaces.\nHere is the proper version of the diff that can be applied:\n\ndiff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh\nindex 065f780636..359dc8eba8 100755\n--- a/t/t7528-signed-commit-ssh.sh\n+++ b/t/t7528-signed-commit-ssh.sh\n@@ -85,6 +85,7 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen\n \teval $(ssh-agent) &&\n \ttest_when_finished \"kill ${SSH_AGENT_PID}\" &&\n \tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n+\texport TMPDIR=$(pwd) &&\n \techo 1 >file && git add file &&\n \tgit commit -a -m rsa-inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n \techo 2 >file &&\n@@ -95,7 +96,8 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen\n \tgit commit -a -m ecdsa-inline -S\"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n \techo 4 >file &&\n \ttest_config user.signingkey \"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n-\tgit commit -a -m ecdsa-config -S\n+\tgit commit -a -m ecdsa-config -S &&\n+\t! ls .git_signing_key_tmp*\n '\n \n test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '\n\n-- \nredoste\n"},{"id":"521349","messageId":"aGnDO3HIAQgsRbnJ@fruit.crustytoothpaste.net","threadId":"63744","inReplyTo":"DB4HMD2HLG24.1V8Y9JDW6BACQ@redoste.xyz","subject":"Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-07-06T00:28:43Z","receivedAt":"2025-07-06T00:28:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-07-05 at 22:50:43, redoste wrote:\n> On Sat Jul 5, 2025 at 22:07 CEST, brian m. carlson wrote:\n> > On 2025-07-05 at 19:21:13, Jeff King wrote:\n> > I don't have a strong view either way, but I do wonder if it's a good\n> > idea to have the testsuite poking around in `/tmp`, although maybe if we\n> > honour `TMPDIR` then it would be possible to do in a tidy way.\n> I looked into adding a test, but I didn't find any other tests checking\n> for temporary files and I agree that messing in /tmp doesn't feel really\n> appropriate for the testsuite.\n> \n> Maybe something like this?\n> \n> diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh\n> index 065f780636..359dc8eba8 100755\n> --- a/t/t7528-signed-commit-ssh.sh\n> +++ b/t/t7528-signed-commit-ssh.sh\n> @@ -85,6 +85,7 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen\n> \teval $(ssh-agent) &&\n> \ttest_when_finished \"kill ${SSH_AGENT_PID}\" &&\n> \tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n> +\texport TMPDIR=$(pwd) &&\n> \techo 1 >file && git add file &&\n> \tgit commit -a -m rsa-inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n> \techo 2 >file &&\n> @@ -95,7 +96,8 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen\n> \tgit commit -a -m ecdsa-inline -S\"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n> \techo 4 >file &&\n> \ttest_config user.signingkey \"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n> -\tgit commit -a -m ecdsa-config -S\n> +\tgit commit -a -m ecdsa-config -S &&\n> +\t! ls .git_signing_key_tmp*\n>  '\n\nWhat I would recommend is adding another test (that is, another\n`test_expect_success` assertion) that when signing we don't leave any\ntemporary files behind.  You can then squash that into your patch and\nsend a v2.\n\nAs for `ls`, I wouldn't use that in scripting.  I'd use something like\nthis:\n\n----\nmkdir tmpdir &&\nTMPDIR=\"$(pwd)/tmpdir\" &&\nexport TMPDIR &&\n# Other code\nfind tmpdir -type f >files &&\ntest_line_count = 0 files\n----\n\nI think `find` is typically better in scripting and also in this case\nwith a separate directory as the temporary directory we'll find any left\nover temporary files, not just the pattern we've fixed here.\n\nOur coding guidelines point out that some shells don't like `export`\nwith assignments, so we do those as two lines.  Normally I would write\nwhat you have in my day-to-day life, but we run on a variety of systems\nthat most people will never use.\n\n>  test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '\n> \n> I can add it in a v2 if you think it's a good way to test it.\n\nI think the basic premise is sound, but a separate assertion would make\nit easier to readers why we're setting the temporary directory.\n\n> > I also have some friends who are trans and have transitioned or are in\n> > the process of transitioning but have simply not gotten around to\n> > getting legal paperwork done[1].\n> This is the exact reason why I'm not very comfortable with using my\n> legal or real name, (well, it's mostly because I still can't find a name\n> I like).\n> And since it's a simple patch that's probably not even copyrightable, I\n> figured out that using a pseudonym was fine.\n> \n> Since I knew that the Linux kernel changed their documentation to remove\n> the use of \"real name\", I thought it was more common and didn't relly\n> think about it a lot.\n> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330\n> \n> I'm sorry, I should have read the git documentation more thoroughly.\n> \n> If it's really an issue I don't mind signing off with a different and\n> more distinctive name.\n\nNo, I think this is fine.  You weren't obligated to explain that and our\npolicies should gracefully handle this situation regardless, but given\nwhat I said before and this context, I think the name you have is fine.\nGit unfortunately has poor support for replacing names and I would't\nwant you to have to put your deadname into our history for all of\neternity.\n\nI'll send a patch to fix the policy.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"521350","messageId":"DB4HMD2HLG24.1V8Y9JDW6BACQ@redoste.xyz","threadId":"63744","inReplyTo":"aGmGCmkwC1HlSyog@fruit.crustytoothpaste.net","subject":"Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"redoste","fromEmail":"redoste@redoste.xyz","sentAt":"2025-07-05T22:50:43Z","receivedAt":"2025-07-06T01:19:45Z","isPatch":true,"sender":{"key":"redoste@redoste.xyz","avatar":"https://avatars.githubusercontent.com/u/12833420?v=4"},"body":"On Sat Jul 5, 2025 at 22:07 CEST, brian m. carlson wrote:\n> On 2025-07-05 at 19:21:13, Jeff King wrote:\n>> I wonder if it is worth adding a test, or if it would be too weirdly\n>> focused on this obscure case to be very useful against future\n>> regressions.\n>\n> I don't have a strong view either way, but I do wonder if it's a good\n> idea to have the testsuite poking around in `/tmp`, although maybe if we\n> honour `TMPDIR` then it would be possible to do in a tidy way.\nI looked into adding a test, but I didn't find any other tests checking\nfor temporary files and I agree that messing in /tmp doesn't feel really\nappropriate for the testsuite.\n\nMaybe something like this?\n\ndiff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh\nindex 065f780636..359dc8eba8 100755\n--- a/t/t7528-signed-commit-ssh.sh\n+++ b/t/t7528-signed-commit-ssh.sh\n@@ -85,6 +85,7 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen\n\teval $(ssh-agent) &&\n\ttest_when_finished \"kill ${SSH_AGENT_PID}\" &&\n\tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n+\texport TMPDIR=$(pwd) &&\n\techo 1 >file && git add file &&\n\tgit commit -a -m rsa-inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n\techo 2 >file &&\n@@ -95,7 +96,8 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen\n\tgit commit -a -m ecdsa-inline -S\"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n\techo 4 >file &&\n\ttest_config user.signingkey \"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n-\tgit commit -a -m ecdsa-config -S\n+\tgit commit -a -m ecdsa-config -S &&\n+\t! ls .git_signing_key_tmp*\n '\n \n test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '\n\nI can add it in a v2 if you think it's a good way to test it.\n\n> I also have some friends who are trans and have transitioned or are in\n> the process of transitioning but have simply not gotten around to\n> getting legal paperwork done[1].\nThis is the exact reason why I'm not very comfortable with using my\nlegal or real name, (well, it's mostly because I still can't find a name\nI like).\nAnd since it's a simple patch that's probably not even copyrightable, I\nfigured out that using a pseudonym was fine.\n\nSince I knew that the Linux kernel changed their documentation to remove\nthe use of \"real name\", I thought it was more common and didn't relly\nthink about it a lot.\nhttps://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330\n\nI'm sorry, I should have read the git documentation more thoroughly.\n\nIf it's really an issue I don't mind signing off with a different and\nmore distinctive name.\n\n-- \nredoste\n"},{"id":"521352","messageId":"20250706031337.GA3041790@coredump.intra.peff.net","threadId":"63744","inReplyTo":"aGnDO3HIAQgsRbnJ@fruit.crustytoothpaste.net","subject":"Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-06T03:13:37Z","receivedAt":"2025-07-06T03:13:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 06, 2025 at 12:28:43AM +0000, brian m. carlson wrote:\n\n> On 2025-07-05 at 22:50:43, redoste wrote:\n> [...]\n> > Since I knew that the Linux kernel changed their documentation to remove\n> > the use of \"real name\", I thought it was more common and didn't relly\n> > think about it a lot.\n> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330\n> > \n> > I'm sorry, I should have read the git documentation more thoroughly.\n> > \n> > If it's really an issue I don't mind signing off with a different and\n> > more distinctive name.\n> \n> No, I think this is fine.  You weren't obligated to explain that and our\n> policies should gracefully handle this situation regardless, but given\n> what I said before and this context, I think the name you have is fine.\n> Git unfortunately has poor support for replacing names and I would't\n> want you to have to put your deadname into our history for all of\n> eternity.\n> \n> I'll send a patch to fix the policy.\n\nAh, I hadn't heard about that change in the kernel. I agree that it\nwould make sense to loosen our text to match.\n\nThanks (in advance) for sending that doc update, and for your earlier\ncomments in the thread. I agreed with everything you wrote there. FWIW,\nmy personal inclinations lean much more towards allowing privacy and\nanonymity, but as a reviewer for the project I wanted to make sure we\nwere not violating our own policy in a way that would eventually bite us\nin a legal sense. The notion of \"identifiable but pseudonymous\" you\nbrought up seems like a good middle ground (at least to my layman's\nears).\n\nAnd thanks redoste for your patch. ;) Sorry if bringing up the signoff\nissue caused any hassle (but hopefully it will lead to an improvement to\nour docs).\n\n-Peff\n"},{"id":"521368","messageId":"DB553KB95680.28G0KIGGWCW9L@redoste.xyz","threadId":"63744","inReplyTo":"aGnDO3HIAQgsRbnJ@fruit.crustytoothpaste.net","subject":"Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"redoste","fromEmail":"redoste@redoste.xyz","sentAt":"2025-07-06T17:14:37Z","receivedAt":"2025-07-06T17:24:42Z","isPatch":true,"sender":{"key":"redoste@redoste.xyz","avatar":"https://avatars.githubusercontent.com/u/12833420?v=4"},"body":"On Sun Jul 6, 2025 at 02:28 CEST, brian m. carlson wrote:\n> I think `find` is typically better in scripting and also in this case\n> with a separate directory as the temporary directory we'll find any left\n> over temporary files, not just the pattern we've fixed here.\nThanks for the feedback!\n\nI used `ls` since it was the easiest way to have something that fails\nquickly if a file doesn't exists (and `test -e` doesn't support having\nmultiple files), but I agree, using `find` this way is definitely\ncleaner.\n\n> I'll send a patch to fix the policy.\nThanks!\n\n-- \nredoste\n"},{"id":"521369","messageId":"DB557SHQD36C.2MWS0EROQIBO5@redoste.xyz","threadId":"63744","inReplyTo":"20250706031337.GA3041790@coredump.intra.peff.net","subject":"Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"redoste","fromEmail":"redoste@redoste.xyz","sentAt":"2025-07-06T17:20:08Z","receivedAt":"2025-07-06T17:37:49Z","isPatch":true,"sender":{"key":"redoste@redoste.xyz","avatar":"https://avatars.githubusercontent.com/u/12833420?v=4"},"body":"On Sun Jul 6, 2025 at 05:13 CEST, Jeff King wrote:\n> And thanks redoste for your patch. ;) Sorry if bringing up the signoff\n> issue caused any hassle (but hopefully it will lead to an improvement to\n> our docs).\n\nNo worries! I completely understand your position as a reviewer and I\ndon't mind if it helps to improve the situation and will help future\ncontributors.\n\n-- \nredoste\n"},{"id":"521370","messageId":"20250706173450.12995-1-redoste@redoste.xyz","threadId":"63744","inReplyTo":"20250704230829.29696-1-redoste@redoste.xyz","subject":"[PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"redoste","fromEmail":"redoste@redoste.xyz","sentAt":"2025-07-06T17:34:49Z","receivedAt":"2025-07-06T17:42:42Z","isPatch":true,"sender":{"key":"redoste@redoste.xyz","avatar":"https://avatars.githubusercontent.com/u/12833420?v=4"},"body":"Detaching the filename string from the tempfile structure used to cause\ndelete_tempfile() to fail and the temporary file was not cleaned up.\n\nWhile it's possible to get rid of the allocation and copy from\nxstrdup(), it keeps the code symetric with the other branch since\ninterpolate_path() also allocates and ssh_signing_key_file is freed\nin both cases.\n\nHelped-by: brian m. carlson <sandals@crustytoothpaste.net>\nSigned-off-by: redoste <redoste@redoste.xyz>\n---\nv1->v2:\n * add a test case that checks for temporary files after signing commits\n * add small explaination about the use of xstrdup() in the commit\n   body\n\n gpg-interface.c              |  2 +-\n t/t7528-signed-commit-ssh.sh | 16 ++++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0896458de5..bdcc8c2a2e 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -1048,7 +1048,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \t\t\t\t    key_file->filename.buf);\n \t\t\tgoto out;\n \t\t}\n-\t\tssh_signing_key_file = strbuf_detach(&key_file->filename, NULL);\n+\t\tssh_signing_key_file = xstrdup(key_file->filename.buf);\n \t} else {\n \t\t/* We assume a file */\n \t\tssh_signing_key_file = interpolate_path(signing_key, 1);\ndiff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh\nindex 065f780636..1a8d96f355 100755\n--- a/t/t7528-signed-commit-ssh.sh\n+++ b/t/t7528-signed-commit-ssh.sh\n@@ -390,6 +390,22 @@ test_expect_success GPGSSH 'check config gpg.format values' '\n \ttest_must_fail git commit -S --amend -m \"fail\"\n '\n \n+test_expect_success GPGSSH 'check temporary files clean up when signing commits' '\n+\ttest_config gpg.format ssh &&\n+\teval $(ssh-agent) &&\n+\ttest_when_finished \"kill ${SSH_AGENT_PID}\" &&\n+\tmkdir tmpdir &&\n+\tTMPDIR=\"$(pwd)/tmpdir\" &&\n+\texport TMPDIR &&\n+\tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n+\techo 1 >file && git add file &&\n+\tgit commit -a -m inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n+\techo 2 >file &&\n+\tgit commit -a -m file -S\"${GPGSSH_KEY_PRIMARY}\" &&\n+\tfind tmpdir -type f >tmpfiles &&\n+\ttest_line_count = 0 tmpfiles\n+'\n+\n test_expect_failure GPGSSH 'detect fudged commit with double signature (TODO)' '\n \tsed -e \"/gpgsig/,/END PGP/d\" forged1 >double-base &&\n \tsed -n -e \"/gpgsig/,/END PGP/p\" forged1 | \\\n-- \n2.49.0\n\n"},{"id":"521380","messageId":"aGro2h1VsSEYYCmp@fruit.crustytoothpaste.net","threadId":"63744","inReplyTo":"20250706173450.12995-1-redoste@redoste.xyz","subject":"Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-07-06T21:21:30Z","receivedAt":"2025-07-06T21:21:38Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-07-06 at 17:34:49, redoste wrote:\n> Detaching the filename string from the tempfile structure used to cause\n> delete_tempfile() to fail and the temporary file was not cleaned up.\n> \n> While it's possible to get rid of the allocation and copy from\n> xstrdup(), it keeps the code symetric with the other branch since\n> interpolate_path() also allocates and ssh_signing_key_file is freed\n> in both cases.\n> \n> Helped-by: brian m. carlson <sandals@crustytoothpaste.net>\n> Signed-off-by: redoste <redoste@redoste.xyz>\n\nYup, this looks good to me.  Thanks so much for the patch; I always\nappreciate keeping the temporary directory tidy.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"521419","messageId":"aGuNCGNk96DK4GzX@pks.im","threadId":"63744","inReplyTo":"20250706173450.12995-1-redoste@redoste.xyz","subject":"Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-07T09:02:00Z","receivedAt":"2025-07-07T09:02:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jul 06, 2025 at 07:34:49PM +0200, redoste wrote:\n> diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh\n> index 065f780636..1a8d96f355 100755\n> --- a/t/t7528-signed-commit-ssh.sh\n> +++ b/t/t7528-signed-commit-ssh.sh\n> @@ -390,6 +390,22 @@ test_expect_success GPGSSH 'check config gpg.format values' '\n>  \ttest_must_fail git commit -S --amend -m \"fail\"\n>  '\n>  \n> +test_expect_success GPGSSH 'check temporary files clean up when signing commits' '\n> +\ttest_config gpg.format ssh &&\n> +\teval $(ssh-agent) &&\n> +\ttest_when_finished \"kill ${SSH_AGENT_PID}\" &&\n> +\tmkdir tmpdir &&\n> +\tTMPDIR=\"$(pwd)/tmpdir\" &&\n> +\texport TMPDIR &&\n\nI think this exported environment variable now leaks into subsequent\ntests, doesn't it? We may want to do it in a subshell.\n\n\tmkdir tmpdir &&\n\tTMPDIR=\"$(pwd)/tmpdir\" &&\n\t(\n\t\texport TMPDIR &&\n\t\tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n\t\techo 1 >file && git add file &&\n\t\tgit commit -a -m inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n\t\techo 2 >file &&\n\t\tgit commit -a -m file -S\"${GPGSSH_KEY_PRIMARY}\"\n\t) && \n\tfind tmpdir -type f >tmpfiles &&\n\ttest_line_count = 0 tmpfiles\n\nPatrick\n\n> +\tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n> +\techo 1 >file && git add file &&\n> +\tgit commit -a -m inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n> +\techo 2 >file &&\n> +\tgit commit -a -m file -S\"${GPGSSH_KEY_PRIMARY}\" &&\n> +\tfind tmpdir -type f >tmpfiles &&\n> +\ttest_line_count = 0 tmpfiles\n> +'\n> +\n>  test_expect_failure GPGSSH 'detect fudged commit with double signature (TODO)' '\n>  \tsed -e \"/gpgsig/,/END PGP/d\" forged1 >double-base &&\n>  \tsed -n -e \"/gpgsig/,/END PGP/p\" forged1 | \\\n> -- \n> 2.49.0\n> \n> \n"},{"id":"521421","messageId":"05d66f9c-78b6-4624-a0e3-32235df05105@gmail.com","threadId":"63744","inReplyTo":"aGuNCGNk96DK4GzX@pks.im","subject":"Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-07T09:35:38Z","receivedAt":"2025-07-07T09:35:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/07/2025 10:02, Patrick Steinhardt wrote:\n> On Sun, Jul 06, 2025 at 07:34:49PM +0200, redoste wrote:\n>> +test_expect_success GPGSSH 'check temporary files clean up when signing commits' '\n>> +\ttest_config gpg.format ssh &&\n>> +\teval $(ssh-agent) &&\n>> +\ttest_when_finished \"kill ${SSH_AGENT_PID}\" &&\n>> +\tmkdir tmpdir &&\n>> +\tTMPDIR=\"$(pwd)/tmpdir\" &&\n>> +\texport TMPDIR &&\n> \n> I think this exported environment variable now leaks into subsequent\n> tests, doesn't it? We may want to do it in a subshell.\n\nThat's a very good point.\n\n> \n> \tmkdir tmpdir &&\n> \tTMPDIR=\"$(pwd)/tmpdir\" &&\n> \t(\n> \t\texport TMPDIR &&\n> \t\tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n> \t\techo 1 >file && git add file &&\n> \t\tgit commit -a -m inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n> \t\techo 2 >file &&\n> \t\tgit commit -a -m file -S\"${GPGSSH_KEY_PRIMARY}\"\n> \t) &&\n> \tfind tmpdir -type f >tmpfiles &&\n> \ttest_line_count = 0 tmpfiles\n\nThe idiomatic way to test for an empty file is \"test_file_is_empty \n<file>\" which avoids spawning wc.\n\nAs this test is an abridged version of the other test that uses \nssh-agent I think it would be more efficient to tweak that test to check \nthere are no temporary files left over. Our test suite is slow and \ntweaking an existing test to improve our coverage instead of adding a \nnew test means we don't make it any slower than necessary.\n\nThanks\n\nPhillip\n\n> \n> Patrick\n> \n>> +\tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n>> +\techo 1 >file && git add file &&\n>> +\tgit commit -a -m inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n>> +\techo 2 >file &&\n>> +\tgit commit -a -m file -S\"${GPGSSH_KEY_PRIMARY}\" &&\n>> +\tfind tmpdir -type f >tmpfiles &&\n>> +\ttest_line_count = 0 tmpfiles\n>> +'\n>> +\n>>   test_expect_failure GPGSSH 'detect fudged commit with double signature (TODO)' '\n>>   \tsed -e \"/gpgsig/,/END PGP/d\" forged1 >double-base &&\n>>   \tsed -n -e \"/gpgsig/,/END PGP/p\" forged1 | \\\n>> -- \n>> 2.49.0\n>>\n>>\n> \n\n"},{"id":"521441","messageId":"DB5W4LH0MI5K.EJ0IILQ1IWR@redoste.xyz","threadId":"63744","inReplyTo":"05d66f9c-78b6-4624-a0e3-32235df05105@gmail.com","subject":"Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"redoste","fromEmail":"redoste@redoste.xyz","sentAt":"2025-07-07T14:25:28Z","receivedAt":"2025-07-07T14:44:52Z","isPatch":true,"sender":{"key":"redoste@redoste.xyz","avatar":"https://avatars.githubusercontent.com/u/12833420?v=4"},"body":"On Mon Jul 7, 2025 at 11:35 CEST, Phillip Wood wrote:\n> On 07/07/2025 10:02, Patrick Steinhardt wrote:\n>> I think this exported environment variable now leaks into subsequent\n>> tests, doesn't it? We may want to do it in a subshell.\n>\n> That's a very good point.\nSure thing, I took inspiration from the previous ssh-agent test and it\nexports the ssh-agent variables without spawning a subshell, so I\nfigured it was fine, but it was only because test_when_finished will\nfail in a subshell.\nI guess it's probably fine to leak only the ssh-agent variables\naccross tests and keep TMPDIR in a subshell.\n\n> The idiomatic way to test for an empty file is \"test_file_is_empty \n> <file>\" which avoids spawning wc.\nThanks, I will update this.\n\n> As this test is an abridged version of the other test that uses \n> ssh-agent I think it would be more efficient to tweak that test to check \n> there are no temporary files left over. Our test suite is slow and \n> tweaking an existing test to improve our coverage instead of adding a \n> new test means we don't make it any slower than necessary.\nI moved this test out of the other ssh-agent one according to the advice\nfrom brian on the previous version. I agree that spawning multiple\nssh-agent is definitly not the best for speed, but is it worth it to\nmake the test failure less clear?\nI feel like it's not the best to keep checks for temporary files clean\nup in a test that (initially) only claims to sign commits.\n\n-- \nredoste\n"},{"id":"521444","messageId":"7130651b-76a6-4eb1-93cf-c9e237d398d7@gmail.com","threadId":"63744","inReplyTo":"DB5W4LH0MI5K.EJ0IILQ1IWR@redoste.xyz","subject":"Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-07T15:26:42Z","receivedAt":"2025-07-07T15:26:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/07/2025 15:25, redoste wrote:\n> On Mon Jul 7, 2025 at 11:35 CEST, Phillip Wood wrote:\n>> On 07/07/2025 10:02, Patrick Steinhardt wrote:\n>>> I think this exported environment variable now leaks into subsequent\n>>> tests, doesn't it? We may want to do it in a subshell.\n>>\n>> That's a very good point.\n> Sure thing, I took inspiration from the previous ssh-agent test and it\n> exports the ssh-agent variables without spawning a subshell, so I\n> figured it was fine, but it was only because test_when_finished will\n> fail in a subshell.\n> I guess it's probably fine to leak only the ssh-agent variables\n> accross tests and keep TMPDIR in a subshell.\n\nHopefully having the ssh-agent variables set after the agent has been \nkilled doesn't affect the later tests\n\n>> The idiomatic way to test for an empty file is \"test_file_is_empty\n>> <file>\" which avoids spawning wc.\n> Thanks, I will update this.\n> \n>> As this test is an abridged version of the other test that uses\n>> ssh-agent I think it would be more efficient to tweak that test to check\n>> there are no temporary files left over. Our test suite is slow and\n>> tweaking an existing test to improve our coverage instead of adding a\n>> new test means we don't make it any slower than necessary.\n> I moved this test out of the other ssh-agent one according to the advice\n> from brian on the previous version. I agree that spawning multiple\n> ssh-agent is definitly not the best for speed, but is it worth it to\n> make the test failure less clear?\ntest_file_is_empty will print a diagnostic message if it fails so it \nshould be clear what has caused the test failure\n\n> I feel like it's not the best to keep checks for temporary files clean\n> up in a test that (initially) only claims to sign commits.\n\nYou can always tweak the test title if you want. The way I see it is \nthat the changes that are being tested are related to commit signing as \nthe invariant that we want to assert is that temporary files are cleaned \nup after signing commits.\n\nThanks\n\nPhillip\n"},{"id":"521449","messageId":"DB5YM57RVQTT.35ZS2BA5SHY@redoste.xyz","threadId":"63744","inReplyTo":"7130651b-76a6-4eb1-93cf-c9e237d398d7@gmail.com","subject":"Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"redoste","fromEmail":"redoste@redoste.xyz","sentAt":"2025-07-07T16:22:25Z","receivedAt":"2025-07-07T16:22:40Z","isPatch":true,"sender":{"key":"redoste@redoste.xyz","avatar":"https://avatars.githubusercontent.com/u/12833420?v=4"},"body":"On Mon Jul 7, 2025 at 17:26 CEST, Phillip Wood wrote:\n> test_file_is_empty will print a diagnostic message if it fails so it \n> should be clear what has caused the test failure\nUh, okay, this makes sense and I think it will be clear enough.\nHowever there is only `test_file_not_empty`, `test_file_is_empty`\ndoesn't exist. I will implement it in an other commit.\n\n> The way I see it is that the changes that are being tested are related\n> to commit signing as the invariant that we want to assert is that\n> temporary files are cleaned up after signing commits.\nI see.\n\nI will update this for a v3.\nThanks for the feedback!\n\n-- \nredoste\n"},{"id":"521450","messageId":"2ae656ee-cf3e-42a7-815a-016df23ac051@gmail.com","threadId":"63744","inReplyTo":"DB5YM57RVQTT.35ZS2BA5SHY@redoste.xyz","subject":"Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-07T16:42:00Z","receivedAt":"2025-07-07T16:42:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/07/2025 17:22, redoste wrote:\n> On Mon Jul 7, 2025 at 17:26 CEST, Phillip Wood wrote:\n>> test_file_is_empty will print a diagnostic message if it fails so it\n>> should be clear what has caused the test failure\n> Uh, okay, this makes sense and I think it will be clear enough.\n> However there is only `test_file_not_empty`, `test_file_is_empty`\n> doesn't exist. I will implement it in an other commit.\n\nSorry I'd misremembered the name, it is \"test_must_be_empty\"\n\nPhillip\n>> The way I see it is that the changes that are being tested are related\n>> to commit signing as the invariant that we want to assert is that\n>> temporary files are cleaned up after signing commits.\n> I see.\n> \n> I will update this for a v3.\n> Thanks for the feedback!\n> \n\n"},{"id":"521470","messageId":"20250707184852.16010-1-redoste@redoste.xyz","threadId":"63744","inReplyTo":"20250704230829.29696-1-redoste@redoste.xyz","subject":"[PATCH v3] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"redoste","fromEmail":"redoste@redoste.xyz","sentAt":"2025-07-07T18:48:51Z","receivedAt":"2025-07-07T18:57:26Z","isPatch":true,"sender":{"key":"redoste@redoste.xyz","avatar":"https://avatars.githubusercontent.com/u/12833420?v=4"},"body":"Detaching the filename string from the tempfile structure used to cause\ndelete_tempfile() to fail and the temporary file was not cleaned up.\n\nWhile it's possible to get rid of the allocation and copy from\nxstrdup(), it keeps the code symetric with the other branch since\ninterpolate_path() also allocates and ssh_signing_key_file is freed\nin both cases.\n\nThe exisiting test was updated to check if the temporary files are\nproperly deleted. To prevent TMPDIR from leaking into the other tests, a\nnew subshell is created, however this prevents test_config from working.\nThe cleanup of the config changed in the subshell is done by\ntest_unconfig in a call to test_when_finished outside of it.\n\nHelped-by: brian m. carlson <sandals@crustytoothpaste.net>\nHelped-by: Patrick Steinhardt <ps@pks.im>\nHelped-by: Phillip Wood <phillip.wood@dunelm.org.uk>\nSigned-off-by: redoste <redoste@redoste.xyz>\n---\nv1->v2:\n * add a test case that checks for temporary files after signing commits\n * add small explaination about the use of xstrdup() in the commit\n   body\nv2->v3:\n * merge the test with the previous ssh-agent test\n * export TMPDIR in a subshell to prevent the environment variable from\n   leaking in the next tests\n * use test_must_be_empty instead of test_line_count\n\n gpg-interface.c              |  2 +-\n t/t7528-signed-commit-ssh.sh | 32 ++++++++++++++++++++------------\n 2 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0896458de5..bdcc8c2a2e 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -1048,7 +1048,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \t\t\t\t    key_file->filename.buf);\n \t\t\tgoto out;\n \t\t}\n-\t\tssh_signing_key_file = strbuf_detach(&key_file->filename, NULL);\n+\t\tssh_signing_key_file = xstrdup(key_file->filename.buf);\n \t} else {\n \t\t/* We assume a file */\n \t\tssh_signing_key_file = interpolate_path(signing_key, 1);\ndiff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh\nindex 065f780636..0f887a3ebe 100755\n--- a/t/t7528-signed-commit-ssh.sh\n+++ b/t/t7528-signed-commit-ssh.sh\n@@ -84,18 +84,26 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen\n \ttest_config gpg.format ssh &&\n \teval $(ssh-agent) &&\n \ttest_when_finished \"kill ${SSH_AGENT_PID}\" &&\n-\tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n-\techo 1 >file && git add file &&\n-\tgit commit -a -m rsa-inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n-\techo 2 >file &&\n-\ttest_config user.signingkey \"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n-\tgit commit -a -m rsa-config -S &&\n-\tssh-add \"${GPGSSH_KEY_ECDSA}\" &&\n-\techo 3 >file &&\n-\tgit commit -a -m ecdsa-inline -S\"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n-\techo 4 >file &&\n-\ttest_config user.signingkey \"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n-\tgit commit -a -m ecdsa-config -S\n+\ttest_when_finished \"test_unconfig user.signingkey\" &&\n+\tmkdir tmpdir &&\n+\tTMPDIR=\"$(pwd)/tmpdir\" &&\n+\t(\n+\t\texport TMPDIR &&\n+\t\tssh-add \"${GPGSSH_KEY_PRIMARY}\" &&\n+\t\techo 1 >file && git add file &&\n+\t\tgit commit -a -m rsa-inline -S\"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n+\t\techo 2 >file &&\n+\t\tgit config user.signingkey \"$(cat \"${GPGSSH_KEY_PRIMARY}.pub\")\" &&\n+\t\tgit commit -a -m rsa-config -S &&\n+\t\tssh-add \"${GPGSSH_KEY_ECDSA}\" &&\n+\t\techo 3 >file &&\n+\t\tgit commit -a -m ecdsa-inline -S\"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n+\t\techo 4 >file &&\n+\t\tgit config user.signingkey \"key::$(cat \"${GPGSSH_KEY_ECDSA}.pub\")\" &&\n+\t\tgit commit -a -m ecdsa-config -S\n+\t) &&\n+\tfind tmpdir -type f >tmpfiles &&\n+\ttest_must_be_empty tmpfiles\n '\n \n test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '\n-- \n2.49.0\n\n"},{"id":"521473","messageId":"xmqq4ivn4t30.fsf@gitster.g","threadId":"63744","inReplyTo":"20250707184852.16010-1-redoste@redoste.xyz","subject":"Re: [PATCH v3] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-07T20:57:39Z","receivedAt":"2025-07-07T20:57:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"redoste <redoste@redoste.xyz> writes:\n\n> v2->v3:\n>  * merge the test with the previous ssh-agent test\n>  * export TMPDIR in a subshell to prevent the environment variable from\n>    leaking in the next tests\n>  * use test_must_be_empty instead of test_line_count\n\nThese sound all good.  Will queue.\n\nThanks.\n"},{"id":"521506","messageId":"aGy_DEehFgw4A6va@pks.im","threadId":"63744","inReplyTo":"xmqq4ivn4t30.fsf@gitster.g","subject":"Re: [PATCH v3] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-08T06:47:40Z","receivedAt":"2025-07-08T06:47:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 07, 2025 at 01:57:39PM -0700, Junio C Hamano wrote:\n> redoste <redoste@redoste.xyz> writes:\n> \n> > v2->v3:\n> >  * merge the test with the previous ssh-agent test\n> >  * export TMPDIR in a subshell to prevent the environment variable from\n> >    leaking in the next tests\n> >  * use test_must_be_empty instead of test_line_count\n> \n> These sound all good.  Will queue.\n\nYup, the patch looks good to me. Thanks!\n\nPatrick\n"},{"id":"521549","messageId":"94caee19-b915-4678-900a-03095663e8a8@gmail.com","threadId":"63744","inReplyTo":"aGy_DEehFgw4A6va@pks.im","subject":"Re: [PATCH v3] ssh signing: don't detach the filename strbuf from key_file tempfile","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-08T13:59:38Z","receivedAt":"2025-07-08T13:59:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/07/2025 07:47, Patrick Steinhardt wrote:\n> On Mon, Jul 07, 2025 at 01:57:39PM -0700, Junio C Hamano wrote:\n>> redoste <redoste@redoste.xyz> writes:\n>>\n>>> v2->v3:\n>>>   * merge the test with the previous ssh-agent test\n>>>   * export TMPDIR in a subshell to prevent the environment variable from\n>>>     leaking in the next tests\n>>>   * use test_must_be_empty instead of test_line_count\n>>\n>> These sound all good.  Will queue.\n> \n> Yup, the patch looks good to me. Thanks!\n\nYes it does to me too, thanks\n\nPhillip\n"}]}