{"thread":{"id":"64467","subject":"[PATCH] Revert \"osxkeychain: state to skip unnecessary store operations\"","startedAt":"2025-11-12T07:01:24Z","lastAt":"2025-11-14T03:37:13Z","messageCount":5,"participants":["Koji Nakamaru via GitGitGadget","Junio C Hamano","Koji Nakamaru","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530560","messageId":"pull.1998.git.1762930881599.gitgitgadget@gmail.com","threadId":"64467","inReplyTo":null,"subject":"[PATCH] Revert \"osxkeychain: state to skip unnecessary store operations\"","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-11-12T07:01:21Z","receivedAt":"2025-11-12T07:01:24Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru \n\nThis reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.\n\nThat commit was trying to skip to store a credential returned by\n\"git-credential-osxkeychain get\" by setting\n\"state[]=osxkeychain:seen=1\". However, this state[] is kept even if a\ncredential returned by \"git-credential-osxkeychain get\" is invalid and\nanother subsequent helper's \"get\" returns a valid credential. Another\nsubsequent helper (such as [1]) may expect git-credential-osxkeychain to\nstore the valid credential so that \"store\" cannot be skipped by just\nchecking \"state[]=osxkeychain:seen=1\".\n\nIn order to solve this issue, the state[] mechanism can be refined or\n\"osxkeychain:seen\" can encode the whole information of the last\n\"get\". For now, let's revert the change.\n\n[1]: https://github.com/hickford/git-credential-oauth\n\nReported-by: Petter Sælen \nSigned-off-by: Koji Nakamaru \n---\n Revert \"osxkeychain: state to skip unnecessary store operations\"\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1998%2FKojiNakamaru%2Frevert%2Fe1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1998/KojiNakamaru/revert/e1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1998\n\n .../osxkeychain/git-credential-osxkeychain.c | 11 -----------\n 1 file changed, 11 deletions(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 611c9798b3..1f49ab8548 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -12,7 +12,6 @@ static CFStringRef username;\n static CFDataRef password;\n static CFDataRef password_expiry_utc;\n static CFDataRef oauth_refresh_token;\n-static int state_seen;\n \n static void clear_credential(void)\n {\n@@ -172,9 +171,6 @@ static OSStatus find_internet_password(void)\n \n \tCFRelease(item);\n \n-\twrite_item(\"capability[]\", \"state\", strlen(\"state\"));\n-\twrite_item(\"state[]\", \"osxkeychain:seen=1\", strlen(\"osxkeychain:seen=1\"));\n-\n out:\n \tCFRelease(attrs);\n \n@@ -288,9 +284,6 @@ static OSStatus add_internet_password(void)\n \tCFDictionaryRef attrs;\n \tOSStatus result;\n \n-\tif (state_seen)\n-\t\treturn errSecSuccess;\n-\n \t/* Only store complete credentials */\n \tif (!protocol || !host || !username || !password)\n \t\treturn -1;\n@@ -402,10 +395,6 @@ static void read_credential(void)\n \t\t\toauth_refresh_token = CFDataCreate(kCFAllocatorDefault,\n \t\t\t\t\t\t\t (UInt8 *)v,\n \t\t\t\t\t\t\t strlen(v));\n-\t\telse if (!strcmp(buf, \"state[]\")) {\n-\t\t\tif (!strcmp(v, \"osxkeychain:seen=1\"))\n-\t\t\t\tstate_seen = 1;\n-\t\t}\n \t\t/*\n \t\t * Ignore other lines; we don't know what they mean, but\n \t\t * this future-proofs us when later versions of git do\n\nbase-commit: 4badef0c3503dc29059d678abba7fac0f042bc84\n-- \ngitgitgadget\n"},{"id":"530599","messageId":"xmqqv7jfryet.fsf@gitster.g","threadId":"64467","inReplyTo":"pull.1998.git.1762930881599.gitgitgadget@gmail.com","subject":"Re: [PATCH] Revert \"osxkeychain: state to skip unnecessary store operations\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-12T16:47:06Z","receivedAt":"2025-11-12T16:47:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Koji Nakamaru via GitGitGadget\" writes:\n\n> From: Koji Nakamaru \n>\n> This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.\n\nOK. Let's make a mental note that e1ab45b2 (osxkeychain: state to\nskip unnecessary store operations, 2024-05-15) appeared in v2.46 or\nso.\n\n> That commit was trying to skip to store a credential returned by\n> \"git-credential-osxkeychain get\" by setting\n> \"state[]=osxkeychain:seen=1\". However, this state[] is kept even if a\n> credential returned by \"git-credential-osxkeychain get\" is invalid and\n> another subsequent helper's \"get\" returns a valid credential. Another\n> subsequent helper (such as [1]) may expect git-credential-osxkeychain to\n> store the valid credential so that \"store\" cannot be skipped by just\n> checking \"state[]=osxkeychain:seen=1\".\n>\n> In order to solve this issue, the state[] mechanism can be refined or\n> \"osxkeychain:seen\" can encode the whole information of the last\n> \"get\". For now, let's revert the change.\n\nIs anybody actively working on the proper solution?\n\nIn a patch series that replaces the old commit with a more proper\nsolution, it could be a reasonable layout of the series to make the\nfirst patch a revert like this patch to give the proper solution a\nclean slate to work from, but this looks different.\n\nIf the problem you are trying to solve here were a regression that\nhappened after Git 2.51 was released, a revert is totally warranted\nat this point in time, even during the pre-release freeze period.\n\nBut it does not even look like a recent regression. Wouldn't\nreverting this change at this point give existing users who are\naccustomed to the current behaviour another regression, essentially\nrobbing Peter to pay Paul? In such a case, I do not think \"let's\nrevert now and then hopefully a proper solution can come later\" is a\ngood approach.\n\nThanks.\n\n> [1]: https://github.com/hickford/git-credential-oauth\n>\n> Reported-by: Petter Sælen \n> Signed-off-by: Koji Nakamaru \n> ---\n> Revert \"osxkeychain: state to skip unnecessary store operations\"\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1998%2FKojiNakamaru%2Frevert%2Fe1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1998/KojiNakamaru/revert/e1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1998\n>\n> .../osxkeychain/git-credential-osxkeychain.c | 11 -----------\n> 1 file changed, 11 deletions(-)\n>\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index 611c9798b3..1f49ab8548 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -12,7 +12,6 @@ static CFStringRef username;\n> static CFDataRef password;\n> static CFDataRef password_expiry_utc;\n> static CFDataRef oauth_refresh_token;\n> -static int state_seen;\n> \n> static void clear_credential(void)\n> {\n> @@ -172,9 +171,6 @@ static OSStatus find_internet_password(void)\n> \n> \tCFRelease(item);\n> \n> -\twrite_item(\"capability[]\", \"state\", strlen(\"state\"));\n> -\twrite_item(\"state[]\", \"osxkeychain:seen=1\", strlen(\"osxkeychain:seen=1\"));\n> -\n> out:\n> \tCFRelease(attrs);\n> \n> @@ -288,9 +284,6 @@ static OSStatus add_internet_password(void)\n> \tCFDictionaryRef attrs;\n> \tOSStatus result;\n> \n> -\tif (state_seen)\n> -\t\treturn errSecSuccess;\n> -\n> \t/* Only store complete credentials */\n> \tif (!protocol || !host || !username || !password)\n> \t\treturn -1;\n> @@ -402,10 +395,6 @@ static void read_credential(void)\n> \t\t\toauth_refresh_token = CFDataCreate(kCFAllocatorDefault,\n> \t\t\t\t\t\t\t (UInt8 *)v,\n> \t\t\t\t\t\t\t strlen(v));\n> -\t\telse if (!strcmp(buf, \"state[]\")) {\n> -\t\t\tif (!strcmp(v, \"osxkeychain:seen=1\"))\n> -\t\t\t\tstate_seen = 1;\n> -\t\t}\n> \t\t/*\n> \t\t * Ignore other lines; we don't know what they mean, but\n> \t\t * this future-proofs us when later versions of git do\n>\n> base-commit: 4badef0c3503dc29059d678abba7fac0f042bc84\n"},{"id":"530641","messageId":"CAOTNsDwmMb2P9J9=GDJwyYRihdKQHixcX=GkdL8j6uNL=L6smQ@mail.gmail.com","threadId":"64467","inReplyTo":"xmqqv7jfryet.fsf@gitster.g","subject":"Re: [PATCH] Revert \"osxkeychain: state to skip unnecessary store operations\"","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2025-11-13T08:17:06Z","receivedAt":"2025-11-13T08:17:18Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"On Thu, Nov 13, 2025 at 1:47 AM Junio C Hamano wrote:\n>\n> \"Koji Nakamaru via GitGitGadget\" writes:\n>\n> > From: Koji Nakamaru \n> >\n> > This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.\n>\n> OK. Let's make a mental note that e1ab45b2 (osxkeychain: state to\n> skip unnecessary store operations, 2024-05-15) appeared in v2.46 or\n> so.\n\nI see.\n\n> > That commit was trying to skip to store a credential returned by\n> > \"git-credential-osxkeychain get\" by setting\n> > \"state[]=osxkeychain:seen=1\". However, this state[] is kept even if a\n> > credential returned by \"git-credential-osxkeychain get\" is invalid and\n> > another subsequent helper's \"get\" returns a valid credential. Another\n> > subsequent helper (such as [1]) may expect git-credential-osxkeychain to\n> > store the valid credential so that \"store\" cannot be skipped by just\n> > checking \"state[]=osxkeychain:seen=1\".\n> >\n> > In order to solve this issue, the state[] mechanism can be refined or\n> > \"osxkeychain:seen\" can encode the whole information of the last\n> > \"get\". For now, let's revert the change.\n>\n> Is anybody actively working on the proper solution?\n>\n> In a patch series that replaces the old commit with a more proper\n> solution, it could be a reasonable layout of the series to make the\n> first patch a revert like this patch to give the proper solution a\n> clean slate to work from, but this looks different.\n>\n> If the problem you are trying to solve here were a regression that\n> happened after Git 2.51 was released, a revert is totally warranted\n> at this point in time, even during the pre-release freeze period.\n>\n> But it does not even look like a recent regression. Wouldn't\n> reverting this change at this point give existing users who are\n> accustomed to the current behaviour another regression, essentially\n> robbing Peter to pay Paul? In such a case, I do not think \"let's\n> revert now and then hopefully a proper solution can come later\" is a\n> good approach.\n\nI see. I'll work on the following approach and submit another patch.\n\n> > \"osxkeychain:seen\" can encode the whole information of the last\n> > \"get\".\n"},{"id":"530673","messageId":"aRZqLp__WdA4hbuD@fruit.crustytoothpaste.net","threadId":"64467","inReplyTo":"pull.1998.git.1762930881599.gitgitgadget@gmail.com","subject":"Re: [PATCH] Revert \"osxkeychain: state to skip unnecessary store operations\"","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-13T23:30:54Z","receivedAt":"2025-11-13T23:30:56Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-11-12 at 07:01:21, Koji Nakamaru via GitGitGadget wrote:\n> From: Koji Nakamaru \n> \n> This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.\n> \n> That commit was trying to skip to store a credential returned by\n> \"git-credential-osxkeychain get\" by setting\n> \"state[]=osxkeychain:seen=1\". However, this state[] is kept even if a\n> credential returned by \"git-credential-osxkeychain get\" is invalid and\n> another subsequent helper's \"get\" returns a valid credential. Another\n> subsequent helper (such as [1]) may expect git-credential-osxkeychain to\n> store the valid credential so that \"store\" cannot be skipped by just\n> checking \"state[]=osxkeychain:seen=1\".\n\nI believe the intended approach here is that if we do a get and the\ncredential is invalid, we return the same state[] header to erase, but\nwe should not send it to subsequent gets for a new credential. However,\nwe do need to send it to subsequent gets (which will not have an\nintervening erase) if this is a multistage request because otherwise\nmultistage requests will not be able to keep state, which NTLM and\nKerberos require. Does that make sense?\n\nMy guess is that the problem here is that we reuse the credential\nstructure without resetting it somewhere in the HTTP code rather than a\nproblem in this particular helper. That is probably my fault, but in my\ndefence I would not say that the structure of the HTTP code is very easy\nto follow.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"530675","messageId":"CAOTNsDx77ni29S1tGNi-3Nhb=XpT2x72gg4PesxGXfZO0Ke5qw@mail.gmail.com","threadId":"64467","inReplyTo":"aRZqLp__WdA4hbuD@fruit.crustytoothpaste.net","subject":"Re: [PATCH] Revert \"osxkeychain: state to skip unnecessary store operations\"","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2025-11-14T03:37:00Z","receivedAt":"2025-11-14T03:37:13Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"On Fri, Nov 14, 2025 at 8:30 AM brian m. carlson\n wrote:\n>\n> On 2025-11-12 at 07:01:21, Koji Nakamaru via GitGitGadget wrote:\n> > From: Koji Nakamaru \n> >\n> > This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.\n> >\n> > That commit was trying to skip to store a credential returned by\n> > \"git-credential-osxkeychain get\" by setting\n> > \"state[]=osxkeychain:seen=1\". However, this state[] is kept even if a\n> > credential returned by \"git-credential-osxkeychain get\" is invalid and\n> > another subsequent helper's \"get\" returns a valid credential. Another\n> > subsequent helper (such as [1]) may expect git-credential-osxkeychain to\n> > store the valid credential so that \"store\" cannot be skipped by just\n> > checking \"state[]=osxkeychain:seen=1\".\n>\n> I believe the intended approach here is that if we do a get and the\n> credential is invalid, we return the same state[] header to erase, but\n> we should not send it to subsequent gets for a new credential. However,\n> we do need to send it to subsequent gets (which will not have an\n> intervening erase) if this is a multistage request because otherwise\n> multistage requests will not be able to keep state, which NTLM and\n> Kerberos require. Does that make sense?\n>\n> My guess is that the problem here is that we reuse the credential\n> structure without resetting it somewhere in the HTTP code rather than a\n> problem in this particular helper. That is probably my fault, but in my\n> defence I would not say that the structure of the HTTP code is very easy\n> to follow.\n\nThanks for the explanation. I misunderstood how state[] was intended to\nwork. The current behavior seems rather natural now that I understand\nit. It might be useful if we could optionally specify that it be\ndiscarded under predefined conditions. Also, as you and others\npreviously discussed in [1], this topic is delicate and interesting.\n\n[1]: https://lore.kernel.org/git/20240510200114.GC1954863@coredump.intra.peff.net/\n\n--\nKoji Nakamaru\n"}]}