{"thread":{"id":"62347","subject":"[PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","startedAt":"2024-10-16T14:21:44Z","lastAt":"2025-05-08T20:24:38Z","messageCount":22,"participants":["Patrick Steinhardt","Jeff King","Ramsay Jones","Taylor Blau","Kristoffer Haugsbakk","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"505265","messageId":"a4472d6d1551e7c25540c4c8361bcb6b1c9f92ff.1729084997.git.ps@pks.im","threadId":"62347","inReplyTo":null,"subject":"[PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-16T14:21:36Z","receivedAt":"2024-10-16T14:21:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Clients can signal the git-credential-cache(1) daemon that it is\nsupposed to exit by sending it an \"exit\" command. The details around\nhow exactly the daemon exits seem to be rather intricate as spelt out by\na comment surrounding our call to exit(3p), as we need to be mindful\naround closing the client streams before we signal the client.\n\nThe logic is broken on Cygwin though: when a client asks the daemon to\nexit, they won't see the EOF and will instead get an error message:\n\n fatal: read error from cache daemon: Software caused connection abort\n\nThis issue is known in Cygwin, see for example [1], but the exact root\ncause is not known.\n\nAs it turns out, we can avoid the issue by explicitly closing the client\nstreams via fclose(3p). I'm not sure at all where the claimed atexit(3p)\nhandler mentioned in the comment is supposed to live, but from all I can\nsee we do not have any installed that would close the sockets for us. So\nthis leaves me with a bit of a sour taste overall.\n\nThat being said, I couldn't spot anything obviously wrong with closing\nthe streams ourselves, and it does fix the issue on Cygwin without any\nregressions on other platforms as far as I can see. So let's go for this\nfix, even though I cannot properly explain it.\n\n[1]: https://github.com/cygporter/git/issues/51\n\nSigned-off-by: Patrick Steinhardt \n---\n\nI've Cc'd Adam, who is the maintainer of the Git package in Cygwin, as\nwell as Peff, who is the original author of the below comment. I'd be\nreally happy if one of you could enlighten me here :)\n\nPatrick\n\n builtin/credential-cache--daemon.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex bc22f5c6d24..5a09df5c167 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -156,13 +156,11 @@ static void serve_one_client(FILE *in, FILE *out)\n \t}\n \telse if (!strcmp(action.buf, \"exit\")) {\n \t\t/*\n-\t\t * It's important that we clean up our socket first, and then\n-\t\t * signal the client only once we have finished the cleanup.\n-\t\t * Calling exit() directly does this, because we clean up in\n-\t\t * our atexit() handler, and then signal the client when our\n-\t\t * process actually ends, which closes the socket and gives\n-\t\t * them EOF.\n+\t\t * We must close our file handles before we exit such that the\n+\t\t * client will receive an EOF.\n \t\t */\n+\t\tfclose(in);\n+\t\tfclose(out);\n \t\texit(0);\n \t}\n \telse if (!strcmp(action.buf, \"erase\"))\n-- \n2.47.0.72.gef8ce8f3d4.dirty\n\n"},{"id":"505266","messageId":"20241016145539.GA703747@coredump.intra.peff.net","threadId":"62347","inReplyTo":"a4472d6d1551e7c25540c4c8361bcb6b1c9f92ff.1729084997.git.ps@pks.im","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-16T14:55:39Z","receivedAt":"2024-10-16T14:55:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 16, 2024 at 04:21:36PM +0200, Patrick Steinhardt wrote:\n\n> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> index bc22f5c6d24..5a09df5c167 100644\n> --- a/builtin/credential-cache--daemon.c\n> +++ b/builtin/credential-cache--daemon.c\n> @@ -156,13 +156,11 @@ static void serve_one_client(FILE *in, FILE *out)\n> \t}\n> \telse if (!strcmp(action.buf, \"exit\")) {\n> \t\t/*\n> -\t\t * It's important that we clean up our socket first, and then\n> -\t\t * signal the client only once we have finished the cleanup.\n> -\t\t * Calling exit() directly does this, because we clean up in\n> -\t\t * our atexit() handler, and then signal the client when our\n> -\t\t * process actually ends, which closes the socket and gives\n> -\t\t * them EOF.\n> +\t\t * We must close our file handles before we exit such that the\n> +\t\t * client will receive an EOF.\n> \t\t */\n> +\t\tfclose(in);\n> +\t\tfclose(out);\n> \t\texit(0);\n> \t}\n\nThis breaks the thing the comment was trying to protect against. We want\nto unlink() the socket file before closing the descriptors.\n\nFrom 7d5e9c9849 (which you can find with blame or \"git log --follow\n-Satexit builtin/credential-cache--daemon.c\"):\n\n credential-cache--daemon: clarify \"exit\" action semantics\n\n When this code was originally written, there wasn't much\n thought given to the timing between a client asking for\n \"exit\", the daemon signaling that the action is done (with\n EOF), and the actual cleanup of the socket.\n\n However, we need to care about this so that our test scripts\n do not end up racy (e.g., by asking for an exit and checking\n that the socket was cleaned up). The code that is already\n there happens to behave very reasonably; let's add a comment\n to make it clear that any changes should retain the same\n behavior.\n\nSo with the proposed change, t0301 will now fail racily. We need that\nunlink() to happen before the fclose(). Just calling exit() does things\nin the right order, but it should also be OK to do an explicit:\n\n delete_tempfile(&socket_file);\n fclose(in);\n fclose(out);\n\nThat would probably require making socket_file a global variable. (You\ncan't just return out of the serving loop, since that closes the sockets\nfirst before deleting the tempfile).\n\n> Clients can signal the git-credential-cache(1) daemon that it is\n> supposed to exit by sending it an \"exit\" command. The details around\n> how exactly the daemon exits seem to be rather intricate as spelt out by\n> a comment surrounding our call to exit(3p), as we need to be mindful\n> around closing the client streams before we signal the client.\n> \n> The logic is broken on Cygwin though: when a client asks the daemon to\n> exit, they won't see the EOF and will instead get an error message:\n> \n> fatal: read error from cache daemon: Software caused connection abort\n> \n> This issue is known in Cygwin, see for example [1], but the exact root\n> cause is not known.\n> [...]\n> [1]: https://github.com/cygporter/git/issues/51\n\nI don't see any details at that issue, but I'm not sure how it would fix\nthings. From the client's perspective, they are going to see the\ndescriptor either way. Unless there is some magic that fclose() does\nthat a normal descriptor-close-on-exit does not do.\n\nThat \"Software caused connection abort\" thing seems like a weird\nnot-standard-Unix errno value. Searching for it mostly yields people\ncomplaining about getting it from ssh under cygwin. :)\n\nIf the magic that cygwin needs is actually \"fclose before unlink\", then\nthat is in opposition to other platforms (and I suspect would make t0301\nracy there).\n\n> As it turns out, we can avoid the issue by explicitly closing the client\n> streams via fclose(3p). I'm not sure at all where the claimed atexit(3p)\n> handler mentioned in the comment is supposed to live, but from all I can\n> see we do not have any installed that would close the sockets for us. So\n> this leaves me with a bit of a sour taste overall.\n\nThe mention of atexit is a little oblique these days. We moved from a\nmanual atexit handler to using the tempfile.c handler in 9e9033166b\n(credential-cache--daemon: use tempfile module, 2015-08-10).\n\n> That being said, I couldn't spot anything obviously wrong with closing\n> the streams ourselves, and it does fix the issue on Cygwin without any\n> regressions on other platforms as far as I can see. So let's go for this\n> fix, even though I cannot properly explain it.\n\nRunning t0301 with --stress on Linux failed for me after a minute or so.\n\n-Peff\n"},{"id":"505267","messageId":"20241016150922.GA1848210@coredump.intra.peff.net","threadId":"62347","inReplyTo":"20241016145539.GA703747@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-16T15:09:22Z","receivedAt":"2024-10-16T15:09:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 16, 2024 at 10:55:40AM -0400, Jeff King wrote:\n\n> > The logic is broken on Cygwin though: when a client asks the daemon to\n> > exit, they won't see the EOF and will instead get an error message:\n> > \n> > fatal: read error from cache daemon: Software caused connection abort\n> > \n> > This issue is known in Cygwin, see for example [1], but the exact root\n> > cause is not known.\n> > [...]\n> > [1]: https://github.com/cygporter/git/issues/51\n> \n> I don't see any details at that issue, but I'm not sure how it would fix\n> things. From the client's perspective, they are going to see the\n> descriptor either way. Unless there is some magic that fclose() does\n> that a normal descriptor-close-on-exit does not do.\n> \n> That \"Software caused connection abort\" thing seems like a weird\n> not-standard-Unix errno value. Searching for it mostly yields people\n> complaining about getting it from ssh under cygwin. :)\n> \n> If the magic that cygwin needs is actually \"fclose before unlink\", then\n> that is in opposition to other platforms (and I suspect would make t0301\n> racy there).\n\nThis all seemed eerily familiar. Try this thread:\n\n https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n\nIt looks like the conclusion was that we should adjust errno handling on\nthe client side, but nobody ever followed up with an actual patch.\n\n-Peff\n"},{"id":"505268","messageId":"8dafcaf1-9de6-4f37-89e9-0f71fdc1012b@ramsayjones.plus.com","threadId":"62347","inReplyTo":"a4472d6d1551e7c25540c4c8361bcb6b1c9f92ff.1729084997.git.ps@pks.im","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-10-16T15:12:30Z","receivedAt":"2024-10-16T15:12:40Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 16/10/2024 15:21, Patrick Steinhardt wrote:\n> Clients can signal the git-credential-cache(1) daemon that it is\n> supposed to exit by sending it an \"exit\" command. The details around\n> how exactly the daemon exits seem to be rather intricate as spelt out by\n> a comment surrounding our call to exit(3p), as we need to be mindful\n> around closing the client streams before we signal the client.\n> \n> The logic is broken on Cygwin though: when a client asks the daemon to\n> exit, they won't see the EOF and will instead get an error message:\n> \n> fatal: read error from cache daemon: Software caused connection abort\n> \n> This issue is known in Cygwin, see for example [1], but the exact root\n> cause is not known.\n> \n> As it turns out, we can avoid the issue by explicitly closing the client\n> streams via fclose(3p). I'm not sure at all where the claimed atexit(3p)\n> handler mentioned in the comment is supposed to live, but from all I can\n> see we do not have any installed that would close the sockets for us. So\n> this leaves me with a bit of a sour taste overall.\n> \n> That being said, I couldn't spot anything obviously wrong with closing\n> the streams ourselves, and it does fix the issue on Cygwin without any\n> regressions on other platforms as far as I can see. So let's go for this\n> fix, even though I cannot properly explain it.\n> \n> [1]: https://github.com/cygporter/git/issues/51\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> \n> I've Cc'd Adam, who is the maintainer of the Git package in Cygwin, as\n> well as Peff, who is the original author of the below comment. I'd be\n> really happy if one of you could enlighten me here :)\n> \n> Patrick\n> \n> builtin/credential-cache--daemon.c | 10 ++++------\n> 1 file changed, 4 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> index bc22f5c6d24..5a09df5c167 100644\n> --- a/builtin/credential-cache--daemon.c\n> +++ b/builtin/credential-cache--daemon.c\n> @@ -156,13 +156,11 @@ static void serve_one_client(FILE *in, FILE *out)\n> \t}\n> \telse if (!strcmp(action.buf, \"exit\")) {\n> \t\t/*\n> -\t\t * It's important that we clean up our socket first, and then\n> -\t\t * signal the client only once we have finished the cleanup.\n> -\t\t * Calling exit() directly does this, because we clean up in\n> -\t\t * our atexit() handler, and then signal the client when our\n> -\t\t * process actually ends, which closes the socket and gives\n> -\t\t * them EOF.\n> +\t\t * We must close our file handles before we exit such that the\n> +\t\t * client will receive an EOF.\n> \t\t */\n> +\t\tfclose(in);\n> +\t\tfclose(out);\n> \t\texit(0);\n> \t}\n> \telse if (!strcmp(action.buf, \"erase\"))\n\nHeh, this is very familiar! :)\n\nSee, for example, the mailing list discussion here[1].\n\nIf memory serves, Jeff was against this solution. For what it is worth, my\nrecommended solution is '[RFC PATCH 1A] credential-cache: also handle\nECONNABORTED connection closure'. I still have those patches in my local\ncygwin repo. Ah, let me just add the patch below (this was last rebased\nonto v2.46.0, but it should (hopefully) be fine to apply to master - famous\nlast words).\n\nATB,\nRamsay Jones\n\n[1] https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n\n-------->8--------\nFrom 89526b2b87e39985dc8cd80661662ff087dc1078 Mon Sep 17 00:00:00 2001\nFrom: Ramsay Jones \nDate: Wed, 22 Jun 2022 19:24:44 +0100\nSubject: [PATCH] credential-cache: also handle ECONNABORTED connection closure\n\nSigned-off-by: Ramsay Jones \n---\n builtin/credential-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\nindex 3db8df70a9..defbcc845c 100644\n--- a/builtin/credential-cache.c\n+++ b/builtin/credential-cache.c\n@@ -30,7 +30,7 @@ static int connection_fatally_broken(int error)\n \n static int connection_closed(int error)\n {\n-\treturn (error == ECONNRESET);\n+\treturn (error == ECONNRESET) || (error == ECONNABORTED);\n }\n \n static int connection_fatally_broken(int error)\n-- \n2.47.0\n\n"},{"id":"505269","messageId":"5056cadb-b8c8-4704-a508-1c7036559526@ramsayjones.plus.com","threadId":"62347","inReplyTo":"20241016150922.GA1848210@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-10-16T15:39:58Z","receivedAt":"2024-10-16T15:43:09Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 16/10/2024 16:09, Jeff King wrote:\n> On Wed, Oct 16, 2024 at 10:55:40AM -0400, Jeff King wrote:\n> \n>>> The logic is broken on Cygwin though: when a client asks the daemon to\n>>> exit, they won't see the EOF and will instead get an error message:\n>>>\n>>> fatal: read error from cache daemon: Software caused connection abort\n>>>\n>>> This issue is known in Cygwin, see for example [1], but the exact root\n>>> cause is not known.\n>>> [...]\n>>> [1]: https://github.com/cygporter/git/issues/51\n>>\n>> I don't see any details at that issue, but I'm not sure how it would fix\n>> things. From the client's perspective, they are going to see the\n>> descriptor either way. Unless there is some magic that fclose() does\n>> that a normal descriptor-close-on-exit does not do.\n>>\n>> That \"Software caused connection abort\" thing seems like a weird\n>> not-standard-Unix errno value. Searching for it mostly yields people\n>> complaining about getting it from ssh under cygwin. :)\n>>\n>> If the magic that cygwin needs is actually \"fclose before unlink\", then\n>> that is in opposition to other platforms (and I suspect would make t0301\n>> racy there).\n> \n> This all seemed eerily familiar. Try this thread:\n> \n> https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n> \n> It looks like the conclusion was that we should adjust errno handling on\n> the client side, but nobody ever followed up with an actual patch.\n\nHeh, our emails crossed. Yes, I was hoping that, given that Adam had identified\nthe cygwin commit that caused the regression, some resolution on the cygwin\nside would fix things up. I waited ... and then put it on my TODO list! :)\n\nI did look at the cygwin commit and it wasn't at all obvious what happened.\nIn fact there was a comment about making sure that errno values didn't\nchange as a side-effect!\n\nSorry for being tardy, again ...\n\nATB,\nRamsay Jones\n\n\n\n"},{"id":"505295","messageId":"ZxAiAUdtddwk5RS7@nand.local","threadId":"62347","inReplyTo":"20241016150922.GA1848210@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-16T20:28:49Z","receivedAt":"2024-10-16T20:28:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 11:09:22AM -0400, Jeff King wrote:\n> On Wed, Oct 16, 2024 at 10:55:40AM -0400, Jeff King wrote:\n>\n> > > The logic is broken on Cygwin though: when a client asks the daemon to\n> > > exit, they won't see the EOF and will instead get an error message:\n> > >\n> > > fatal: read error from cache daemon: Software caused connection abort\n> > >\n> > > This issue is known in Cygwin, see for example [1], but the exact root\n> > > cause is not known.\n> > > [...]\n> > > [1]: https://github.com/cygporter/git/issues/51\n> >\n> > I don't see any details at that issue, but I'm not sure how it would fix\n> > things. From the client's perspective, they are going to see the\n> > descriptor either way. Unless there is some magic that fclose() does\n> > that a normal descriptor-close-on-exit does not do.\n> >\n> > That \"Software caused connection abort\" thing seems like a weird\n> > not-standard-Unix errno value. Searching for it mostly yields people\n> > complaining about getting it from ssh under cygwin. :)\n> >\n> > If the magic that cygwin needs is actually \"fclose before unlink\", then\n> > that is in opposition to other platforms (and I suspect would make t0301\n> > racy there).\n>\n> This all seemed eerily familiar. Try this thread:\n>\n> https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n>\n> It looks like the conclusion was that we should adjust errno handling on\n> the client side, but nobody ever followed up with an actual patch.\n\nThanks for digging. It would be great if you both and Ramsay could unify\non an agreeable path forward here.\n\nIn the meantime, I picked this up as 'ps/credential-cache-exit-cygwin'\nin my tree, but let's hold it out from 'seen' as you note it racily\nfails t0301.\n\nThanks,\nTaylor\n"},{"id":"505321","messageId":"20241017023319.GA1858436@coredump.intra.peff.net","threadId":"62347","inReplyTo":"ZxAiAUdtddwk5RS7@nand.local","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-17T02:33:19Z","receivedAt":"2024-10-17T02:33:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:\n\n> > This all seemed eerily familiar. Try this thread:\n> >\n> > https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n> >\n> > It looks like the conclusion was that we should adjust errno handling on\n> > the client side, but nobody ever followed up with an actual patch.\n> \n> Thanks for digging. It would be great if you both and Ramsay could unify\n> on an agreeable path forward here.\n\nI think the patch Ramsay posted elsewhere is the right way forward.\nHopefully he can fill out a commit message with the summary and then we\ncan proceed.\n\n(I'm happy to help with explaining the credential-cache side, but I\nwould just be hand-waving on the cygwin parts).\n\n-Peff\n"},{"id":"505348","messageId":"ZxDOydD7kpSGchiQ@pks.im","threadId":"62347","inReplyTo":"20241017023319.GA1858436@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-17T08:46:07Z","receivedAt":"2024-10-17T08:46:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 16, 2024 at 10:33:19PM -0400, Jeff King wrote:\n> On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:\n> \n> > > This all seemed eerily familiar. Try this thread:\n> > >\n> > > https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n> > >\n> > > It looks like the conclusion was that we should adjust errno handling on\n> > > the client side, but nobody ever followed up with an actual patch.\n> > \n> > Thanks for digging. It would be great if you both and Ramsay could unify\n> > on an agreeable path forward here.\n> \n> I think the patch Ramsay posted elsewhere is the right way forward.\n> Hopefully he can fill out a commit message with the summary and then we\n> can proceed.\n> \n> (I'm happy to help with explaining the credential-cache side, but I\n> would just be hand-waving on the cygwin parts).\n\nSounds great to me -- in that case, let's just drop my patch. I was\nbasically just trying to get somebody to have a look at the issue, and\nit very much seems like I succeeded :)\n\nRamsay, do you want to polish your patch with a commit message or shall\nI pick it up and iterate on it?\n\nThanks all for chiming in!\n\nPatrick\n"},{"id":"505392","messageId":"ZxF5hk5TpKpBF6Xu@nand.local","threadId":"62347","inReplyTo":"20241017023319.GA1858436@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-17T20:54:30Z","receivedAt":"2024-10-17T20:54:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 10:33:19PM -0400, Jeff King wrote:\n> On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:\n>\n> > > This all seemed eerily familiar. Try this thread:\n> > >\n> > > https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n> > >\n> > > It looks like the conclusion was that we should adjust errno handling on\n> > > the client side, but nobody ever followed up with an actual patch.\n> >\n> > Thanks for digging. It would be great if you both and Ramsay could unify\n> > on an agreeable path forward here.\n>\n> I think the patch Ramsay posted elsewhere is the right way forward.\n> Hopefully he can fill out a commit message with the summary and then we\n> can proceed.\n\nYeah, that's exactly what I was hoping for ;-).\n\nThanks,\nTaylor\n"},{"id":"505400","messageId":"395cd934-f6a4-4f08-8bbc-f5a068ebfa39@ramsayjones.plus.com","threadId":"62347","inReplyTo":"ZxDOydD7kpSGchiQ@pks.im","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-10-17T22:58:33Z","receivedAt":"2024-10-17T22:58:43Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 17/10/2024 09:46, Patrick Steinhardt wrote:\n> On Wed, Oct 16, 2024 at 10:33:19PM -0400, Jeff King wrote:\n>> On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:\n>>\n>>>> This all seemed eerily familiar. Try this thread:\n>>>>\n>>>> https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n>>>>\n>>>> It looks like the conclusion was that we should adjust errno handling on\n>>>> the client side, but nobody ever followed up with an actual patch.\n>>>\n>>> Thanks for digging. It would be great if you both and Ramsay could unify\n>>> on an agreeable path forward here.\n>>\n>> I think the patch Ramsay posted elsewhere is the right way forward.\n>> Hopefully he can fill out a commit message with the summary and then we\n>> can proceed.\n>>\n>> (I'm happy to help with explaining the credential-cache side, but I\n>> would just be hand-waving on the cygwin parts).\n> \n> Sounds great to me -- in that case, let's just drop my patch. I was\n> basically just trying to get somebody to have a look at the issue, and\n> it very much seems like I succeeded :)\n> \n> Ramsay, do you want to polish your patch with a commit message or shall\n> I pick it up and iterate on it?\n\nI can't get to it before the weekend, at the earliest, sorry! :(\n\nIf you fancy picking it up, please be my guest! :)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"505404","messageId":"658fe4fa540a0a5316e11ed43f9139d5ef818ee5.1729226155.git.ps@pks.im","threadId":"62347","inReplyTo":"a4472d6d1551e7c25540c4c8361bcb6b1c9f92ff.1729084997.git.ps@pks.im","subject":"[PATCH v2] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-18T04:36:11Z","receivedAt":"2024-10-18T04:36:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Clients can signal the git-credential-cache(1) daemon that it is\nsupposed to exit by sending it an \"exit\" command. The details around\nhow exactly the daemon exits seem to be rather intricate as spelt out by\na comment surrounding our call to exit(3p), as we need to be mindful\naround closing the client streams before we signal the client.\n\nThe logic is broken on Cygwin though: when a client asks the daemon to\nexit, they won't see the EOF and will instead get an error message:\n\n fatal: read error from cache daemon: Software caused connection abort\n\nThis issue is known in Cygwin for quite a while already [1] [2] and\nseems to come from a change in Cygwin itself. While we haven't figured\nout what exact change that was, the effect is that we see ECONNABORTED\nnow instead of ECONNRESET when trying to read from the socket connected\nto the daemon when the daemon has shut down. It is somewhat dubious that\nerrno changed in this way: read(3p) does not mention ECONNABORTED as a\npossible error code, so the behaviour seems to not be POSIX-compliant.\n\nWe have discussed a couple of workarounds:\n\n - Explicitly close file streams when handling the \"exit\" action in\n `serve_one_client()`. This may easily lead to races because we need\n to be very careful about the order in which we delete sockets.\n\n - We can adapt `serve_one_client()` to not use exit(3) anymore, and\n instead handle the shutdown in the outer loop. This allows us to\n handle the shutdown elsewhere and be less intimate with the calling\n context.\n\n - Start handling ECONNABORTED.\n\nThe last option seems like it is both the easiest and least-risky fix.\nIt is unlikely that it would negatively affect any other platforms given\nthat read(3p) shouldn't even set the code in the first place, and it\nfixes the issue on Cygwin.\n\nFix the issue by handling both ECONNRESET and ECONNABORTED.\n\n[1]: https://github.com/cygporter/git/issues/51\n[2]: https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n\nOriginal-patch-by: Ramsay Jones \nHelped-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n builtin/credential-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\nindex 5de8b9123bf..7789d57d3e1 100644\n--- a/builtin/credential-cache.c\n+++ b/builtin/credential-cache.c\n@@ -30,7 +30,7 @@ static int connection_fatally_broken(int error)\n \n static int connection_closed(int error)\n {\n-\treturn (error == ECONNRESET);\n+\treturn error == ECONNRESET || error == ECONNABORTED;\n }\n \n static int connection_fatally_broken(int error)\n\nbase-commit: a7c6fbb48a5849db1bb1f841ef5403476fed0c0e\n-- \n2.47.0.72.gef8ce8f3d4.dirty\n\n"},{"id":"505405","messageId":"ZxHl5-32sClL6H3Y@pks.im","threadId":"62347","inReplyTo":"395cd934-f6a4-4f08-8bbc-f5a068ebfa39@ramsayjones.plus.com","subject":"Re: [PATCH] builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-18T04:36:55Z","receivedAt":"2024-10-18T04:37:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 17, 2024 at 11:58:33PM +0100, Ramsay Jones wrote:\n> \n> \n> On 17/10/2024 09:46, Patrick Steinhardt wrote:\n> > On Wed, Oct 16, 2024 at 10:33:19PM -0400, Jeff King wrote:\n> >> On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:\n> >>\n> >>>> This all seemed eerily familiar. Try this thread:\n> >>>>\n> >>>> https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n> >>>>\n> >>>> It looks like the conclusion was that we should adjust errno handling on\n> >>>> the client side, but nobody ever followed up with an actual patch.\n> >>>\n> >>> Thanks for digging. It would be great if you both and Ramsay could unify\n> >>> on an agreeable path forward here.\n> >>\n> >> I think the patch Ramsay posted elsewhere is the right way forward.\n> >> Hopefully he can fill out a commit message with the summary and then we\n> >> can proceed.\n> >>\n> >> (I'm happy to help with explaining the credential-cache side, but I\n> >> would just be hand-waving on the cygwin parts).\n> > \n> > Sounds great to me -- in that case, let's just drop my patch. I was\n> > basically just trying to get somebody to have a look at the issue, and\n> > it very much seems like I succeeded :)\n> > \n> > Ramsay, do you want to polish your patch with a commit message or shall\n> > I pick it up and iterate on it?\n> \n> I can't get to it before the weekend, at the earliest, sorry! :(\n> \n> If you fancy picking it up, please be my guest! :)\n\nI've sent v2 using that hack now, noting your original authorship. I\nwant to finally get on top of all of those platform-specific failures :)\n\nThanks!\n\nPatrick\n"},{"id":"505409","messageId":"20241018052952.GE2408674@coredump.intra.peff.net","threadId":"62347","inReplyTo":"658fe4fa540a0a5316e11ed43f9139d5ef818ee5.1729226155.git.ps@pks.im","subject":"[PATCH] credential-cache: treat ECONNABORTED like ECONNRESET","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-18T05:29:52Z","receivedAt":"2024-10-18T05:29:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 18, 2024 at 06:36:11AM +0200, Patrick Steinhardt wrote:\n\n> Subject: builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin\n\nI think this commit message has a few unclear or inaccurate bits,\nbecause it's based on the earlier attempt. E.g., the change is now on\nthe client side, not in credential-cache--daemon.\n\nAnd I think rather than say \"the daemon exit rules are intricate\", we\ncan actually outline the rules. :)\n\nSo here's what I had written after reading through the old thread. It\nwould preferably get Ramsay's Signed-off-by before being applied.\n\n-- >8 --\nFrom: Ramsay Jones \nSubject: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET\n\nOn Cygwin, t0301 fails because \"git credential-cache exit\" returns a\nnon-zero exit code. What's supposed to happen here is:\n\n 1. The client (the \"credential-cache\" invocation above) connects to a\n previously-spawned credential-cache--daemon.\n\n 2. The client sends an \"exit\" command to the daemon.\n\n 3. The daemon unlinks the socket and then exits, closing the\n descriptor back to the client.\n\n 4. The client sees EOF on the descriptor and exits successfully.\n\nThat works on most platforms, and even _used_ to work on Cygwin. But\nthat changed in Cygwin's ef95c03522 (Cygwin: select: Fix FD_CLOSE\nhandling, 2021-04-06). After that commit, the client sees a read error\nwith errno set to ECONNABORTED, and it reports the error and dies.\n\nIt's not entirely clear if this is a Cygwin bug. It seems that calling\nfclose() on the filehandles pointing to the sockets is sufficient to\navoid this error return, even though exiting should in general look the\nsame from the client's perspective.\n\nHowever, we can't just call fclose() here. It's important in step 3\nabove to unlink the socket before closing the descriptor to avoid the\nrace mentioned by 7d5e9c9849 (credential-cache--daemon: clarify \"exit\"\naction semantics, 2016-03-18). The client will exit as soon as it sees\nthe descriptor close, and the daemon may or may not have actually\nunlinked the socket by then. That makes test code like this:\n\n git credential exit &&\n test_path_is_missing .git-credential-cache\n\nracy.\n\nSo we probably _could_ fix this by calling:\n\n delete_tempfile(&socket_file);\n fclose(in);\n fclose(out);\n\nbefore we exit(). Or by replacing the exit() with a return up the stack,\nin which case the fclose() happens as we unwind. But in that case we'd\nstill need to call delete_tempfile() here to avoid the race.\n\nBut simpler still is that we can notice that we already special-case\nECONNRESET on the client side, courtesy of 1f180e5eb9 (credential-cache:\ninterpret an ECONNRESET as an EOF, 2017-07-27). We can just do the same\nthing here (I suspect that prior to the Cygwin commit that introduced\nthis problem, we were really just seeing ECONNRESET instead of\nECONNABORTED, so the \"new\" problem is just the switch of the errno\nvalues).\n\nThere's loads more debugging in this thread:\n\n https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n\nbut I've tried to summarize the useful bits in this commit message.\n\n[jk: commit message]\n\nSigned-off-by: Jeff King \n---\n builtin/credential-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\nindex 5de8b9123b..7789d57d3e 100644\n--- a/builtin/credential-cache.c\n+++ b/builtin/credential-cache.c\n@@ -30,7 +30,7 @@ static int connection_fatally_broken(int error)\n \n static int connection_closed(int error)\n {\n-\treturn (error == ECONNRESET);\n+\treturn error == ECONNRESET || error == ECONNABORTED;\n }\n \n static int connection_fatally_broken(int error)\n-- \n2.47.0.273.g4ed1498264\n\n"},{"id":"505411","messageId":"ZxHy5Q-0KJ0q7c6m@pks.im","threadId":"62347","inReplyTo":"20241018052952.GE2408674@coredump.intra.peff.net","subject":"Re: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-18T05:32:21Z","receivedAt":"2024-10-18T05:32:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 18, 2024 at 01:29:52AM -0400, Jeff King wrote:\n> On Fri, Oct 18, 2024 at 06:36:11AM +0200, Patrick Steinhardt wrote:\n> \n> > Subject: builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin\n> \n> I think this commit message has a few unclear or inaccurate bits,\n> because it's based on the earlier attempt. E.g., the change is now on\n> the client side, not in credential-cache--daemon.\n> \n> And I think rather than say \"the daemon exit rules are intricate\", we\n> can actually outline the rules. :)\n> \n> So here's what I had written after reading through the old thread. It\n> would preferably get Ramsay's Signed-off-by before being applied.\n\nWorks for me. I don't really care whose patch lands, I just want the\nissue to be fixed :) Thanks!\n\nPatrick\n"},{"id":"505446","messageId":"8c4b513a-aa6e-4015-a6c2-e153b6c94491@ramsayjones.plus.com","threadId":"62347","inReplyTo":"20241018052952.GE2408674@coredump.intra.peff.net","subject":"Re: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-10-18T15:33:13Z","receivedAt":"2024-10-18T15:36:23Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 18/10/2024 06:29, Jeff King wrote:\n> On Fri, Oct 18, 2024 at 06:36:11AM +0200, Patrick Steinhardt wrote:\n> \n>> Subject: builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin\n> \n> I think this commit message has a few unclear or inaccurate bits,\n> because it's based on the earlier attempt. E.g., the change is now on\n> the client side, not in credential-cache--daemon.\n> \n> And I think rather than say \"the daemon exit rules are intricate\", we\n> can actually outline the rules. :)\n> \n> So here's what I had written after reading through the old thread. It\n> would preferably get Ramsay's Signed-off-by before being applied.\n\nOh Wow, this is (no surprise) a masterpiece! :)\n\nI would very happily add:\n\nSigned-off-by: Ramsay Jones \n\n(I don't think I could have produced anything half as good in\nseveral weekends!)\n\nThanks!\n\nATB,\nRamsay Jones\n\n> \n> -- >8 --\n> From: Ramsay Jones \n> Subject: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET\n> \n> On Cygwin, t0301 fails because \"git credential-cache exit\" returns a\n> non-zero exit code. What's supposed to happen here is:\n> \n> 1. The client (the \"credential-cache\" invocation above) connects to a\n> previously-spawned credential-cache--daemon.\n> \n> 2. The client sends an \"exit\" command to the daemon.\n> \n> 3. The daemon unlinks the socket and then exits, closing the\n> descriptor back to the client.\n> \n> 4. The client sees EOF on the descriptor and exits successfully.\n> \n> That works on most platforms, and even _used_ to work on Cygwin. But\n> that changed in Cygwin's ef95c03522 (Cygwin: select: Fix FD_CLOSE\n> handling, 2021-04-06). After that commit, the client sees a read error\n> with errno set to ECONNABORTED, and it reports the error and dies.\n> \n> It's not entirely clear if this is a Cygwin bug. It seems that calling\n> fclose() on the filehandles pointing to the sockets is sufficient to\n> avoid this error return, even though exiting should in general look the\n> same from the client's perspective.\n> \n> However, we can't just call fclose() here. It's important in step 3\n> above to unlink the socket before closing the descriptor to avoid the\n> race mentioned by 7d5e9c9849 (credential-cache--daemon: clarify \"exit\"\n> action semantics, 2016-03-18). The client will exit as soon as it sees\n> the descriptor close, and the daemon may or may not have actually\n> unlinked the socket by then. That makes test code like this:\n> \n> git credential exit &&\n> test_path_is_missing .git-credential-cache\n> \n> racy.\n> \n> So we probably _could_ fix this by calling:\n> \n> delete_tempfile(&socket_file);\n> fclose(in);\n> fclose(out);\n> \n> before we exit(). Or by replacing the exit() with a return up the stack,\n> in which case the fclose() happens as we unwind. But in that case we'd\n> still need to call delete_tempfile() here to avoid the race.\n> \n> But simpler still is that we can notice that we already special-case\n> ECONNRESET on the client side, courtesy of 1f180e5eb9 (credential-cache:\n> interpret an ECONNRESET as an EOF, 2017-07-27). We can just do the same\n> thing here (I suspect that prior to the Cygwin commit that introduced\n> this problem, we were really just seeing ECONNRESET instead of\n> ECONNABORTED, so the \"new\" problem is just the switch of the errno\n> values).\n> \n> There's loads more debugging in this thread:\n> \n> https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/\n> \n> but I've tried to summarize the useful bits in this commit message.\n> \n> [jk: commit message]\n> \n> Signed-off-by: Jeff King \n> ---\n> builtin/credential-cache.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\n> index 5de8b9123b..7789d57d3e 100644\n> --- a/builtin/credential-cache.c\n> +++ b/builtin/credential-cache.c\n> @@ -30,7 +30,7 @@ static int connection_fatally_broken(int error)\n> \n> static int connection_closed(int error)\n> {\n> -\treturn (error == ECONNRESET);\n> +\treturn error == ECONNRESET || error == ECONNABORTED;\n> }\n> \n> static int connection_fatally_broken(int error)\n\n"},{"id":"505467","messageId":"ZxLQcekbHEg5YLBH@nand.local","threadId":"62347","inReplyTo":"8c4b513a-aa6e-4015-a6c2-e153b6c94491@ramsayjones.plus.com","subject":"Re: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-18T21:17:37Z","receivedAt":"2024-10-18T21:17:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 18, 2024 at 04:33:13PM +0100, Ramsay Jones wrote:\n>\n>\n> On 18/10/2024 06:29, Jeff King wrote:\n> > On Fri, Oct 18, 2024 at 06:36:11AM +0200, Patrick Steinhardt wrote:\n> >\n> >> Subject: builtin/credential-cache--daemon: fix error when \"exit\"ing on Cygwin\n> >\n> > I think this commit message has a few unclear or inaccurate bits,\n> > because it's based on the earlier attempt. E.g., the change is now on\n> > the client side, not in credential-cache--daemon.\n> >\n> > And I think rather than say \"the daemon exit rules are intricate\", we\n> > can actually outline the rules. :)\n> >\n> > So here's what I had written after reading through the old thread. It\n> > would preferably get Ramsay's Signed-off-by before being applied.\n>\n> Oh Wow, this is (no surprise) a masterpiece! :)\n>\n> I would very happily add:\n>\n> Signed-off-by: Ramsay Jones \n\nThanks, all.\n\nThanks,\nTaylor\n"},{"id":"505469","messageId":"71e8e44e-dbf9-482e-a351-3a82aa1ca5dd@app.fastmail.com","threadId":"62347","inReplyTo":"20241018052952.GE2408674@coredump.intra.peff.net","subject":"Re: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-18T21:27:50Z","receivedAt":"2024-10-18T21:28:30Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Fri, Oct 18, 2024, at 07:29, Jeff King wrote:\n> [jk: commit message]\n>\n> Signed-off-by: Jeff King \n\nJust curious. Why this bracket comment (non-trailer line or whatever it\nis called) and not a regular trailer? All the bracket comments that\nI’ve seen give some comment, often about tweaking the patch or the\ncommit message. In this case though the whole commit message is written\nby one person.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"505513","messageId":"20241019212135.GB589728@coredump.intra.peff.net","threadId":"62347","inReplyTo":"71e8e44e-dbf9-482e-a351-3a82aa1ca5dd@app.fastmail.com","subject":"Re: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-19T21:21:35Z","receivedAt":"2024-10-19T21:21:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 18, 2024 at 11:27:50PM +0200, Kristoffer Haugsbakk wrote:\n\n> On Fri, Oct 18, 2024, at 07:29, Jeff King wrote:\n> > [jk: commit message]\n> >\n> > Signed-off-by: Jeff King \n> \n> Just curious. Why this bracket comment (non-trailer line or whatever it\n> is called) and not a regular trailer? All the bracket comments that\n> I’ve seen give some comment, often about tweaking the patch or the\n> commit message. In this case though the whole commit message is written\n> by one person.\n\nI assigned authorship to Ramsay, so my name is not otherwise mentioned,\nbut appears in the signoff. So it was a way of mentioning what I\ncontributed (both for credit, but also in case anybody has questions\nlater).\n\nI guess \"Commit-message-by:\" would work, too. ;) I guess you could\nlikewise argue that I'm a co-author.\n\nI think in the usual trailer order, it would be:\n\n Signed-off-by: Ramsay\n [jk: add commit message]\n Signed-off-by: me\n\nbut I didn't want to forge his S-o-b without asking first.\n\n-Peff\n"},{"id":"505566","messageId":"ae56dc14-bbae-4d5e-a890-20735131d484@app.fastmail.com","threadId":"62347","inReplyTo":"20241019212135.GB589728@coredump.intra.peff.net","subject":"Comment trailers vs. bracketed lines","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-20T17:08:24Z","receivedAt":"2024-10-20T17:08:47Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"Hi Peff\n\n(editing the subject now which I intended to do from the start)\n\nOn Sat, Oct 19, 2024, at 23:21, Jeff King wrote:\n>> On Fri, Oct 18, 2024, at 07:29, Jeff King wrote:\n>> […]\n>\n> I assigned authorship to Ramsay, so my name is not otherwise mentioned,\n> but appears in the signoff. So it was a way of mentioning what I\n> contributed (both for credit, but also in case anybody has questions\n> later).\n>\n> I guess \"Commit-message-by:\" would work, too. ;)\n\nI’ve done that when someone has given me a non-descript diff. :)\n\n> I think in the usual trailer order, it would be:\n>\n> Signed-off-by: Ramsay\n> [jk: add commit message]\n> Signed-off-by: me\n>\n> but I didn't want to forge his S-o-b without asking first.\n\nI’ve seen those brackets in the log. They used to happen with some\nregularity. At first it made sense since you need a free-form area to\nboth comment and tell everyone that you left the comment. And a trailer\ndoesn’t make sense for that, I thought.[1]\n\nBut thinking about the signoff requirement: you already have all the\ninformation you need from the next trailer, namely the signoff. In\nother words this:\n\n [kh: Added tests]\n Signed-off-by: Kristoffer Haugsbakk \n\nHas the same information as this:\n\n Comment: Added tests\n Signed-off-by: Kristoffer Haugsbakk \n\nBecause the signoff order tells you who left the comment. So I was\nwondering to myself why this uniform approach wasn’t used.\n\n† 1: Since the brackets become “non-trailer values” or something\n (git-interpret-trailers(1)), i.e. the discarded parts of the trailer\n block\n"},{"id":"517503","messageId":"29ff888d-9749-4fbe-9722-95dd71008a2a@app.fastmail.com","threadId":"62347","inReplyTo":"ae56dc14-bbae-4d5e-a890-20735131d484@app.fastmail.com","subject":"Re: Comment trailers vs. bracketed lines","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-05-07T16:41:27Z","receivedAt":"2025-05-07T16:41:59Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"Hi, this is an old thread:\n\nOn Sun, Oct 20, 2024, at 19:08, Kristoffer Haugsbakk wrote:\n> On Sat, Oct 19, 2024, at 23:21, Jeff King wrote:\n>>> On Fri, Oct 18, 2024, at 07:29, Jeff King wrote:\n>>> […]\n>>\n>> I assigned authorship to Ramsay, so my name is not otherwise mentioned,\n>> but appears in the signoff. So it was a way of mentioning what I\n>> contributed (both for credit, but also in case anybody has questions\n>> later).\n>>\n>> I guess \"Commit-message-by:\" would work, too. ;)\n>\n> I’ve done that when someone has given me a non-descript diff. :)\n>\n>> I think in the usual trailer order, it would be:\n>>\n>> Signed-off-by: Ramsay\n>> [jk: add commit message]\n>> Signed-off-by: me\n>>\n>> but I didn't want to forge his S-o-b without asking first.\n>\n> I’ve seen those brackets in the log. They used to happen with some\n> regularity. At first it made sense since you need a free-form area to\n> both comment and tell everyone that you left the comment. And a trailer\n> doesn’t make sense for that, I thought.[1]\n>\n> But thinking about the signoff requirement: you already have all the\n> information you need from the next trailer, namely the signoff. In\n> other words this:\n>\n> [kh: Added tests]\n> Signed-off-by: Kristoffer Haugsbakk \n>\n> Has the same information as this:\n>\n> Comment: Added tests\n> Signed-off-by: Kristoffer Haugsbakk \n>\n> Because the signoff order tells you who left the comment. So I was\n> wondering to myself why this uniform approach wasn’t used.\n>\n> † 1: Since the brackets become “non-trailer values” or something\n> (git-interpret-trailers(1)), i.e. the discarded parts of the trailer\n> block\n\nI was just reminded of this: https://lore.kernel.org/git/xmqqikmce67y.fsf@gitster.g/T/#m68c22c9b7dbc9b295e923a913e9d67e3ab28a2a4\n\nI’m just doing a little bump of this topic in case anyone has any\nthoughts. I hope that is okay.\n"},{"id":"517514","messageId":"xmqqv7qcb52r.fsf@gitster.g","threadId":"62347","inReplyTo":"ae56dc14-bbae-4d5e-a890-20735131d484@app.fastmail.com","subject":"Re: Comment trailers vs. bracketed lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-07T19:20:44Z","receivedAt":"2025-05-07T19:20:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n>> I think in the usual trailer order, it would be:\n>>\n>> Signed-off-by: Ramsay\n>> [jk: add commit message]\n>> Signed-off-by: me\n>>\n>> but I didn't want to forge his S-o-b without asking first.\n>\n> I’ve seen those brackets in the log. They used to happen with some\n> regularity. At first it made sense since you need a free-form area to\n> both comment and tell everyone that you left the comment. And a trailer\n> doesn’t make sense for that, I thought.[1]\n>\n> But thinking about the signoff requirement: you already have all the\n> information you need from the next trailer, namely the signoff. In\n> other words this:\n>\n> [kh: Added tests]\n> Signed-off-by: Kristoffer Haugsbakk \n>\n> Has the same information as this:\n>\n> Comment: Added tests\n> Signed-off-by: Kristoffer Haugsbakk \n>\n> Because the signoff order tells you who left the comment. So I was\n> wondering to myself why this uniform approach wasn’t used.\n\nSimply because we already saw another project like the kernel use\nthe [initial: comment] convention, I think. The \"25% rule\" was\noriginally added specifically to accomdate this kind of comments\nfrequently used in the kernel project, if I am not mistaken.\n\nWe see too many 'jk' and 'js' in the project so the initial may not\nbe all that meaningful if added and it usually is obvious who did\nwhat without, but even with three letter initial,\n\n [khh: Added tests]\n Comment: Added tests\n\nthe existing convention is still shorter than with your \"Comment: \"\nprefix ;-)\n"},{"id":"517599","messageId":"20250508202437.GF18229@coredump.intra.peff.net","threadId":"62347","inReplyTo":"29ff888d-9749-4fbe-9722-95dd71008a2a@app.fastmail.com","subject":"Re: Comment trailers vs. bracketed lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-08T20:24:37Z","receivedAt":"2025-05-08T20:24:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 07, 2025 at 06:41:27PM +0200, Kristoffer Haugsbakk wrote:\n\n> > But thinking about the signoff requirement: you already have all the\n> > information you need from the next trailer, namely the signoff. In\n> > other words this:\n> >\n> > [kh: Added tests]\n> > Signed-off-by: Kristoffer Haugsbakk \n> >\n> > Has the same information as this:\n> >\n> > Comment: Added tests\n> > Signed-off-by: Kristoffer Haugsbakk \n> >\n> > Because the signoff order tells you who left the comment. So I was\n> > wondering to myself why this uniform approach wasn’t used.\n> >\n> > † 1: Since the brackets become “non-trailer values” or something\n> > (git-interpret-trailers(1)), i.e. the discarded parts of the trailer\n> > block\n> \n> I was just reminded of this: https://lore.kernel.org/git/xmqqikmce67y.fsf@gitster.g/T/#m68c22c9b7dbc9b295e923a913e9d67e3ab28a2a4\n> \n> I’m just doing a little bump of this topic in case anyone has any\n> thoughts. I hope that is okay.\n\nIt is the same information _if_ there is another trailer with the ident\nthat comes afterwards. In our project, you'd usually have a sign-off\nthere. But not all projects would, and even in git.git you don't\nnecessarily need one, depending on what is in the comment.\n\nThat said, if I were designing trailers from scratch today, I'd probably\nrequire something that looks more like \"Comment:\" just because it would\nreduce the complexity of the trailer parser (and its \"non trailer\nportions\"). But since we live in a world where we support that for\nhistorical reasons, I don't mind using it (I find it more natural to\nread, but possibly my mind is polluted by years of practice).\n\n-Peff\n"}]}