{"thread":{"id":"61451","subject":"[PATCH] osxkeychain: lock for exclusive execution","startedAt":"2024-05-10T08:07:50Z","lastAt":"2024-05-15T19:41:55Z","messageCount":21,"participants":["Koji Nakamaru via GitGitGadget","Bo Anderson","Koji Nakamaru","Jeff King","brian m. carlson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"494398","messageId":"pull.1729.git.1715328467099.gitgitgadget@gmail.com","threadId":"61451","inReplyTo":null,"subject":"[PATCH] osxkeychain: lock for exclusive execution","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-10T08:07:46Z","receivedAt":"2024-05-10T08:07:50Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru \n\nResolves \"failed to store: -25299\" when \"fetch.parallel 0\" is configured\nand there are many submodules.\n\nThe error code -25299 (errSecDuplicateItem) may be returned by\nSecItemUpdate() in add_internet_password() if multiple instances of\ngit-credential-osxkeychain run in parallel. This patch introduces an\nexclusive lock to serialize execution for avoiding this and other\npotential issues.\n\nSigned-off-by: Koji Nakamaru \n---\n osxkeychain: lock for exclusive execution\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1729%2FKojiNakamaru%2Ffeature%2Fosxkeychian_exlock-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1729/KojiNakamaru/feature/osxkeychian_exlock-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1729\n\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 6a40917b1ef..0884db48d0a 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -414,6 +414,9 @@ int main(int argc, const char **argv)\n \tif (!argv[1])\n \t\tdie(\"%s\", usage);\n \n+\tif (open(argv[0], O_RDONLY | O_EXLOCK) == -1)\n+\t\tdie(\"failed to lock %s\", argv[0]);\n+\n \tread_credential();\n \n \tif (!strcmp(argv[1], \"get\"))\n\nbase-commit: 0f3415f1f8478b05e64db11eb8aaa2915e48fef6\n-- \ngitgitgadget\n"},{"id":"494467","messageId":"D7A8539F-E33C-44F3-A7BF-5F5D4A26F2A4@boanderson.me","threadId":"61451","inReplyTo":"pull.1729.git.1715328467099.gitgitgadget@gmail.com","subject":"Re: [PATCH] osxkeychain: lock for exclusive execution","fromName":"Bo Anderson","fromEmail":"mail@boanderson.me","sentAt":"2024-05-10T15:02:03Z","receivedAt":"2024-05-10T15:02:19Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"Interesting.\n\nSecItemUpdate returning errSecDuplicateItem didn’t make sense to make sense to me so I had a check to see what scenario this happens and it appears to be a scenario where updating in-place fails but replacing it entirely succeeds. However it seems the item might have ultimately still been updated: https://github.com/apple-oss-distributions/Security/blob/0600e7bab30fbac3adcafcb6c57d3981dc682304/OSX/libsecurity_keychain/lib/SecItem.cpp#L2398\n\nThe behaviour is a bit odd and the associated code comment referencing an Apple bug number is perhaps is indicative of that. I guess it perhaps makes sense if you are holding references, but that doesn’t apply to us.\n\nI wonder if a fix here could be to treat errSecDuplicateItem as a successful operation for SecItemUpdate. Can you confirm the keychain item is successfully updated in that scenario?\n\nA broader Git-wide question that you perhaps don’t know the answer to but someone else here might do is: why are we spamming updates to the credential helper? Every parallel fetch instance performing a store operation on the same host seems unexpected to me, particularly if there’s no actual changes.\n\nBo\n\n> On 10 May 2024, at 09:07, Koji Nakamaru via GitGitGadget wrote:\n> \n> From: Koji Nakamaru \n> \n> Resolves \"failed to store: -25299\" when \"fetch.parallel 0\" is configured\n> and there are many submodules.\n> \n> The error code -25299 (errSecDuplicateItem) may be returned by\n> SecItemUpdate() in add_internet_password() if multiple instances of\n> git-credential-osxkeychain run in parallel. This patch introduces an\n> exclusive lock to serialize execution for avoiding this and other\n> potential issues.\n> \n> Signed-off-by: Koji Nakamaru \n> ---\n> osxkeychain: lock for exclusive execution\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1729%2FKojiNakamaru%2Ffeature%2Fosxkeychian_exlock-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1729/KojiNakamaru/feature/osxkeychian_exlock-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1729\n> \n> contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index 6a40917b1ef..0884db48d0a 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -414,6 +414,9 @@ int main(int argc, const char **argv)\n> if (!argv[1])\n> die(\"%s\", usage);\n> \n> + if (open(argv[0], O_RDONLY | O_EXLOCK) == -1)\n> + die(\"failed to lock %s\", argv[0]);\n> +\n> read_credential();\n> \n> if (!strcmp(argv[1], \"get\"))\n> \n> base-commit: 0f3415f1f8478b05e64db11eb8aaa2915e48fef6\n> -- \n> gitgitgadget\n\n"},{"id":"494485","messageId":"CAOTNsDzveWCr4wx2vqJF_YfRkF5QyhHpopqfw-CiG2xcNduC2Q@mail.gmail.com","threadId":"61451","inReplyTo":"C0C8F71D-2A01-4C31-9EB6-AB31FA17C3AB@boanderson.me","subject":"Re: [PATCH] osxkeychain: lock for exclusive execution","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-05-10T18:26:03Z","receivedAt":"2024-05-10T18:26:15Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"Thank you for detailed insights.\n\n> SecItemUpdate returning errSecDuplicateItem didn’t make sense to make sense to me so I had a check to see what scenario this happens and it appears to be a scenario where updating in-place fails but replacing it entirely succeeds. However it seems the item might have ultimately still been updated: https://github.com/apple-oss-distributions/Security/blob/0600e7bab30fbac3adcafcb6c57d3981dc682304/OSX/libsecurity_keychain/lib/SecItem.cpp#L2398\n\n> The behaviour is a bit odd and the associated code comment referencing an Apple bug number is perhaps is indicative of that. I guess it perhaps makes sense if you are holding references, but that doesn’t apply to us.\n\n> I wonder if a fix here could be to treat errSecDuplicateItem as a successful operation for SecItemUpdate. Can you confirm the keychain item is successfully updated in that scenario?\n\nI tested osxkeychain with the modification at the end of this note and\ngot the following log for\n\"git fetch --all --prune --recurse-submodules\". SecItemUpdate()\nsometimes returns\nerrSecDuplicateItem but the keychain item seems okay -- its value is\ncorrect after the command\nfinished -- perhaps because one of successful operations stores the\ncorrect value. Even if every\nstore operation fails, perhaps the originally stored value is kept and\nno damage occurs.\n\n XXX: get\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: wwwauth[]=Basic realm=\"GitHub\"\n XXX: store\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: username=jenkins\n XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n XXXX: -25299\n XXXXX: 0\n XXX: get\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: wwwauth[]=Basic realm=\"GitHub\"\n XXX: store\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: username=jenkins\n XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n XXXX: -25299\n XXXXX: 0\n XXX: get\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: wwwauth[]=Basic realm=\"GitHub\"\n XXX: store\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: username=jenkins\n XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n XXXX: -25299\n XXXXX: 0\n XXX: get\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: wwwauth[]=Basic realm=\"GitHub\"\n XXX: store\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: username=jenkins\n XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n XXXX: -25299\n XXXXX: -25299\n failed to store: -25299\n XXX: get\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: wwwauth[]=Basic realm=\"GitHub\"\n XXX: store\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: username=jenkins\n XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n XXXX: -25299\n XXXXX: 0\n XXX: get\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: wwwauth[]=Basic realm=\"GitHub\"\n XXX: store\n XXXX: protocol=https\n XXXX: host=github.com\n XXXX: username=jenkins\n XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n XXXX: -25299\n XXXXX: -25299\n failed to store: -25299\n ...\n\nThis issue however occurs only when fetch.parallel is configured. If\nfetch.parallel is not\nconfigured, we should not ignore any error (including\nerrSecDuplicateItem). Also, the above unstable\nbehaviour is essentially caused by running osxkeychain instances in\nparallel where some of them\ntreat \"get\" and others treat \"store\". I've also considered treating\nerrSecDuplicateItem of\nSecItemUpdate() as errSecSuccess, but ended up with the current patch\nfor these reasons.\n\n> A broader Git-wide question that you perhaps don’t know the answer to but someone else here might do is: why are we spamming updates to the credential helper? Every parallel fetch instance performing a store operation on the same host seems unexpected to me, particularly if there’s no actual changes.\n\nI agree on this point and would like to know the reason.\n\nKoji Nakamaru\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nb/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 6a40917b1e..0373857731 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -308,10 +308,12 @@ static OSStatus add_internet_password(void)\n NULL);\n\n result = SecItemAdd(attrs, NULL);\n+ fprintf(stderr, \"XXXX: %d\\n\", result);\n if (result == errSecDuplicateItem) {\n CFDictionaryRef query;\n query = CREATE_SEC_ATTRIBUTES(NULL);\n result = SecItemUpdate(query, attrs);\n+ fprintf(stderr, \"XXXXX: %d\\n\", result);\n CFRelease(query);\n }\n\n@@ -333,6 +335,7 @@ static void read_credential(void)\n if (!strcmp(buf, \"\\n\"))\n break;\n buf[line_len-1] = '\\0';\n+ fprintf(stderr, \"XXXX: %s\\n\", buf);\n\n v = strchr(buf, '=');\n if (!v)\n@@ -414,6 +417,7 @@ int main(int argc, const char **argv)\n if (!argv[1])\n die(\"%s\", usage);\n\n+ fprintf(stderr, \"XXX: %s\\n\", argv[1]);\n read_credential();\n\n if (!strcmp(argv[1], \"get\"))\n\n\n2024年5月10日(金) 23:58 Bo Anderson :\n>\n> Interesting.\n>\n> SecItemUpdate returning errSecDuplicateItem didn’t make sense to make sense to me so I had a check to see what scenario this happens and it appears to be a scenario where updating in-place fails but replacing it entirely succeeds. However it seems the item might have ultimately still been updated: https://github.com/apple-oss-distributions/Security/blob/0600e7bab30fbac3adcafcb6c57d3981dc682304/OSX/libsecurity_keychain/lib/SecItem.cpp#L2398\n>\n> The behaviour is a bit odd and the associated code comment referencing an Apple bug number is perhaps is indicative of that. I guess it perhaps makes sense if you are holding references, but that doesn’t apply to us.\n>\n> I wonder if a fix here could be to treat errSecDuplicateItem as a successful operation for SecItemUpdate. Can you confirm the keychain item is successfully updated in that scenario?\n>\n> A broader Git-wide question that you perhaps don’t know the answer to but someone else here might do is: why are we spamming updates to the credential helper? Every parallel fetch instance performing a store operation on the same host seems unexpected to me, particularly if there’s no actual changes.\n>\n> Bo\n>\n> On 10 May 2024, at 09:07, Koji Nakamaru via GitGitGadget wrote:\n>\n> From: Koji Nakamaru \n>\n> Resolves \"failed to store: -25299\" when \"fetch.parallel 0\" is configured\n> and there are many submodules.\n>\n> The error code -25299 (errSecDuplicateItem) may be returned by\n> SecItemUpdate() in add_internet_password() if multiple instances of\n> git-credential-osxkeychain run in parallel. This patch introduces an\n> exclusive lock to serialize execution for avoiding this and other\n> potential issues.\n>\n> Signed-off-by: Koji Nakamaru \n> ---\n> osxkeychain: lock for exclusive execution\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1729%2FKojiNakamaru%2Ffeature%2Fosxkeychian_exlock-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1729/KojiNakamaru/feature/osxkeychian_exlock-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1729\n>\n> contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index 6a40917b1ef..0884db48d0a 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -414,6 +414,9 @@ int main(int argc, const char **argv)\n> if (!argv[1])\n> die(\"%s\", usage);\n>\n> + if (open(argv[0], O_RDONLY | O_EXLOCK) == -1)\n> + die(\"failed to lock %s\", argv[0]);\n> +\n> read_credential();\n>\n> if (!strcmp(argv[1], \"get\"))\n>\n> base-commit: 0f3415f1f8478b05e64db11eb8aaa2915e48fef6\n> --\n> gitgitgadget\n>\n>\n"},{"id":"494497","messageId":"20240510200114.GC1954863@coredump.intra.peff.net","threadId":"61451","inReplyTo":"D7A8539F-E33C-44F3-A7BF-5F5D4A26F2A4@boanderson.me","subject":"Re: [PATCH] osxkeychain: lock for exclusive execution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-10T20:01:14Z","receivedAt":"2024-05-10T20:01:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 10, 2024 at 04:02:03PM +0100, Bo Anderson wrote:\n\n> A broader Git-wide question that you perhaps don’t know the answer to\n> but someone else here might do is: why are we spamming updates to the\n> credential helper? Every parallel fetch instance performing a store\n> operation on the same host seems unexpected to me, particularly if\n> there’s no actual changes.\n\nThe short answer is that Git always passes a credential which has been\nused successfully to the helpers to record (if they want to). That's how\nstuff gets stored in the first place. And those parallel fetches have no\nknowledge of what the other ones are doing, so they all try to store.\n\nBut the more interesting question is: why do we tell helpers to store a\ncredential that we got from helpers in the first place? The behavior is\nmostly an artifact of how the original implementation behaved, as it did\nnot record the source of the credential.\n\nAnd I think there are several problems with that, besides inefficiency\nand locking. See this old patch, which fixes it by remembering when\na credential came from a helper:\n\n https://lore.kernel.org/git/20120407033417.GA13914@sigill.intra.peff.net/\n\nBut we didn't merge it because some people rely on the behavior of\nhelpers feeding back to themselves. I outlined some solutions there, but\nit would definitely be a change in behavior that people would have to\nadapt to.\n\nSome possible alternatives:\n\n - we could remember _which_ helper we got the credential from, and\n avoid invoking it again.\n\n - we could record a bit saying that the credential came from a helper,\n and then feed that back to helpers when storing. So osxkeychain\n could then decide not to store it.\n\nBoth of those solve the repeated stores, but still let credentials\npopulate across helpers (which I still think is a questionable thing to\ndo by default, per the discussion in that thread, but is the very thing\nthat some people rely on).\n\n-Peff\n"},{"id":"494503","messageId":"Zj6EhJi9MgALC5Ti@tapette.crustytoothpaste.net","threadId":"61451","inReplyTo":"20240510200114.GC1954863@coredump.intra.peff.net","subject":"Re: [PATCH] osxkeychain: lock for exclusive execution","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-05-10T20:33:08Z","receivedAt":"2024-05-10T20:33:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-05-10 at 20:01:14, Jeff King wrote:\n> And I think there are several problems with that, besides inefficiency\n> and locking. See this old patch, which fixes it by remembering when\n> a credential came from a helper:\n> \n> https://lore.kernel.org/git/20120407033417.GA13914@sigill.intra.peff.net/\n> \n> But we didn't merge it because some people rely on the behavior of\n> helpers feeding back to themselves. I outlined some solutions there, but\n> it would definitely be a change in behavior that people would have to\n> adapt to.\n> \n> Some possible alternatives:\n> \n> - we could remember _which_ helper we got the credential from, and\n> avoid invoking it again.\n\nThis will break the new `state[]` feature, which relies on being able to\nsee the state after the fact to know whether the operation was\nsuccessful. As an example of the functionality the current approach\nallows, authentication could use an HOTP (like TOTP, but using a counter\ninstead of time) value, and storing the correct used counter on success\nwould be important.\n\nI agree it's not super important if we're just using a username and\npassword, but considering I just added support for arbitrary\nauthentication schemes, which can include things such as limited-use\nOAuth tokens, one-time use passcodes, and certain types of HMAC-based\nsigning, we probably don't want to choose this approach.\n\n> - we could record a bit saying that the credential came from a helper,\n> and then feed that back to helpers when storing. So osxkeychain\n> could then decide not to store it.\n\nThis is actually possible with the new `state[]` feature. `osxkeychain`\ncan simply set that field to something like `osxkeychain:seen=1` and\nsimply do nothing if it sees that field.\n\nAll the credential helper needs to do is declare support for that\nfunctionality with the appropriate capability and emit the field if it\ngets that capability on standard input.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"494505","messageId":"xmqqh6f54czm.fsf@gitster.g","threadId":"61451","inReplyTo":"20240510200114.GC1954863@coredump.intra.peff.net","subject":"Re: [PATCH] osxkeychain: lock for exclusive execution","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-10T20:40:29Z","receivedAt":"2024-05-10T20:40:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> - we could remember _which_ helper we got the credential from, and\n> avoid invoking it again.\n>\n> - we could record a bit saying that the credential came from a helper,\n> and then feed that back to helpers when storing. So osxkeychain\n> could then decide not to store it.\n>\n> Both of those solve the repeated stores, but still let credentials\n> populate across helpers (which I still think is a questionable thing to\n> do by default, per the discussion in that thread, but is the very thing\n> that some people rely on).\n\nWould \"refreshing the last-time-used record\" a valid use case for\nthe behaviour that feeds the successful one back to where the\ncredential came from? Such a helper could instead log the last-time\nthe credential was asked for, and assume that the lack of an explicit\n\"reject\" call signals that the use of the value it returned earlier\nwas auccessfully used, but it is a less obvious way to implement\nsuch a \"this hasn't been successfully used for a long time, perhaps\nwe should expire/ask again/do something else?\" logic.\n"},{"id":"494518","messageId":"20240510220715.GB1962678@coredump.intra.peff.net","threadId":"61451","inReplyTo":"Zj6EhJi9MgALC5Ti@tapette.crustytoothpaste.net","subject":"Re: [PATCH] osxkeychain: lock for exclusive execution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-10T22:07:15Z","receivedAt":"2024-05-10T22:07:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 10, 2024 at 08:33:08PM +0000, brian m. carlson wrote:\n\n> > Some possible alternatives:\n> > \n> > - we could remember _which_ helper we got the credential from, and\n> > avoid invoking it again.\n> \n> This will break the new `state[]` feature, which relies on being able to\n> see the state after the fact to know whether the operation was\n> successful. As an example of the functionality the current approach\n> allows, authentication could use an HOTP (like TOTP, but using a counter\n> instead of time) value, and storing the correct used counter on success\n> would be important.\n> \n> I agree it's not super important if we're just using a username and\n> password, but considering I just added support for arbitrary\n> authentication schemes, which can include things such as limited-use\n> OAuth tokens, one-time use passcodes, and certain types of HMAC-based\n> signing, we probably don't want to choose this approach.\n\nYeah, I think it makes sense to keep the Git side as general as\npossible. So invoking the helper but giving it extra information (so it\ncan decide whether to be a noop or not) seems like the better approach.\n\n> > - we could record a bit saying that the credential came from a helper,\n> > and then feed that back to helpers when storing. So osxkeychain\n> > could then decide not to store it.\n> \n> This is actually possible with the new `state[]` feature. `osxkeychain`\n> can simply set that field to something like `osxkeychain:seen=1` and\n> simply do nothing if it sees that field.\n\nMakes sense. Back in that old thread I showed a patch which would let\nhelpers pass arbitrary fields to each other (or back to themselves), and\nthis works in roughly the same way.\n\n> All the credential helper needs to do is declare support for that\n> functionality with the appropriate capability and emit the field if it\n> gets that capability on standard input.\n\nIf I understand the protocol, it is just:\n\n printf(\"capability[]=state\\n\");\n printf(\"state[]=osxkeychain:seen=1\\n\");\n\nin the helper when it returns a username/password? And I guess the\nmatching parse/check on \"store\".\n\nSounds like that would be easy for folks on macOS to play with.\n\n-Peff\n"},{"id":"494519","messageId":"20240510220920.GC1962678@coredump.intra.peff.net","threadId":"61451","inReplyTo":"xmqqh6f54czm.fsf@gitster.g","subject":"Re: [PATCH] osxkeychain: lock for exclusive execution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-10T22:09:20Z","receivedAt":"2024-05-10T22:09:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 10, 2024 at 01:40:29PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > - we could remember _which_ helper we got the credential from, and\n> > avoid invoking it again.\n> >\n> > - we could record a bit saying that the credential came from a helper,\n> > and then feed that back to helpers when storing. So osxkeychain\n> > could then decide not to store it.\n> >\n> > Both of those solve the repeated stores, but still let credentials\n> > populate across helpers (which I still think is a questionable thing to\n> > do by default, per the discussion in that thread, but is the very thing\n> > that some people rely on).\n> \n> Would \"refreshing the last-time-used record\" a valid use case for\n> the behaviour that feeds the successful one back to where the\n> credential came from? Such a helper could instead log the last-time\n> the credential was asked for, and assume that the lack of an explicit\n> \"reject\" call signals that the use of the value it returned earlier\n> was auccessfully used, but it is a less obvious way to implement\n> such a \"this hasn't been successfully used for a long time, perhaps\n> we should expire/ask again/do something else?\" logic.\n\nThere was some discussion in that old thread about whether that was\nimportant or not. I don't have a strong opinion there. Not refreshing is\na more secure default, but possibly more annoying (and a change from the\nstatus quo).\n\nI do think brian's suggestion to use state[] to pass it back means that\nthe decision is then in the hands of the helper. So \"credential-cache\",\nfor example, could decide whether to refresh its ttl or not, or we could\neven make it configurable with a command-line option for the helper.\n\n-Peff\n"},{"id":"494525","messageId":"xmqqo79d1dtw.fsf@gitster.g","threadId":"61451","inReplyTo":"20240510220920.GC1962678@coredump.intra.peff.net","subject":"Re: [PATCH] osxkeychain: lock for exclusive execution","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-10T22:50:35Z","receivedAt":"2024-05-10T22:50:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do think brian's suggestion to use state[] to pass it back means that\n> the decision is then in the hands of the helper. So \"credential-cache\",\n> for example, could decide whether to refresh its ttl or not, or we could\n> even make it configurable with a command-line option for the helper.\n\nYeah, I read your discussion with brian, and the state[] thing all\nmade sense to me.\n\nThanks.\n"},{"id":"494528","messageId":"Zj6p6df2DOVocbNl@tapette.crustytoothpaste.net","threadId":"61451","inReplyTo":"20240510220715.GB1962678@coredump.intra.peff.net","subject":"Re: [PATCH] osxkeychain: lock for exclusive execution","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-05-10T23:12:41Z","receivedAt":"2024-05-10T23:12:43Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-05-10 at 22:07:15, Jeff King wrote:\n> On Fri, May 10, 2024 at 08:33:08PM +0000, brian m. carlson wrote:\n> > All the credential helper needs to do is declare support for that\n> > functionality with the appropriate capability and emit the field if it\n> > gets that capability on standard input.\n> \n> If I understand the protocol, it is just:\n> \n> printf(\"capability[]=state\\n\");\n> printf(\"state[]=osxkeychain:seen=1\\n\");\n> \n> in the helper when it returns a username/password? And I guess the\n> matching parse/check on \"store\".\n> \n> Sounds like that would be easy for folks on macOS to play with.\n\nYup. It may receive `state[]` fields from other helpers, so it needs to\ncheck that the entries are its own (presumably starting with\n`osxkeychain:`) when it reads them, but otherwise, that's it.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"494532","messageId":"pull.1729.v2.git.1715428542.gitgitgadget@gmail.com","threadId":"61451","inReplyTo":"pull.1729.git.1715328467099.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] osxkeychain: lock for exclusive execution","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-11T11:55:40Z","receivedAt":"2024-05-11T11:55:45Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"Koji Nakamaru (2):\n osxkeychain: lock for exclusive execution\n osxkeychain: state[] seen=1 to skip unnecessary store operations\n\n .../osxkeychain/git-credential-osxkeychain.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\n\nbase-commit: 0f3415f1f8478b05e64db11eb8aaa2915e48fef6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1729%2FKojiNakamaru%2Ffeature%2Fosxkeychian_exlock-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1729/KojiNakamaru/feature/osxkeychian_exlock-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1729\n\nRange-diff vs v1:\n\n 1: 309c17c78f3 = 1: 309c17c78f3 osxkeychain: lock for exclusive execution\n -: ----------- > 2: 1f57718abff osxkeychain: state[] seen=1 to skip unnecessary store operations\n\n-- \ngitgitgadget\n"},{"id":"494533","messageId":"309c17c78f35296dd47e8b203413860eb62b239e.1715428542.git.gitgitgadget@gmail.com","threadId":"61451","inReplyTo":"pull.1729.v2.git.1715428542.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] osxkeychain: lock for exclusive execution","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-11T11:55:41Z","receivedAt":"2024-05-11T11:55:46Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru \n\nResolves \"failed to store: -25299\" when \"fetch.parallel 0\" is configured\nand there are many submodules.\n\nThe error code -25299 (errSecDuplicateItem) may be returned by\nSecItemUpdate() in add_internet_password() if multiple instances of\ngit-credential-osxkeychain run in parallel. This patch introduces an\nexclusive lock to serialize execution for avoiding this and other\npotential issues.\n\nSigned-off-by: Koji Nakamaru \n---\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 6a40917b1ef..0884db48d0a 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -414,6 +414,9 @@ int main(int argc, const char **argv)\n \tif (!argv[1])\n \t\tdie(\"%s\", usage);\n \n+\tif (open(argv[0], O_RDONLY | O_EXLOCK) == -1)\n+\t\tdie(\"failed to lock %s\", argv[0]);\n+\n \tread_credential();\n \n \tif (!strcmp(argv[1], \"get\"))\n-- \ngitgitgadget\n\n"},{"id":"494534","messageId":"1f57718abff1d0e234c4145e833424da7be79311.1715428542.git.gitgitgadget@gmail.com","threadId":"61451","inReplyTo":"pull.1729.v2.git.1715428542.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] osxkeychain: state[] seen=1 to skip unnecessary store operations","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-11T11:55:42Z","receivedAt":"2024-05-11T11:55:48Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru \n\nRecords whether credentials come from get operations and skips\nunnecessary store operations by utilizing the state[] feature, as\nsuggested by brian m. carlson.\n\nSigned-off-by: Koji Nakamaru \n---\n .../osxkeychain/git-credential-osxkeychain.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 0884db48d0a..6ce22a28ed7 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -12,6 +12,7 @@ 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@@ -171,6 +172,9 @@ 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@@ -284,6 +288,9 @@ 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@@ -395,6 +402,10 @@ 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-- \ngitgitgadget\n"},{"id":"494553","messageId":"xmqqseynisc9.fsf@gitster.g","threadId":"61451","inReplyTo":"309c17c78f35296dd47e8b203413860eb62b239e.1715428542.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] osxkeychain: lock for exclusive execution","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-12T04:09:42Z","receivedAt":"2024-05-12T04:09:48Z","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> Resolves \"failed to store: -25299\" when \"fetch.parallel 0\" is configured\n> and there are many submodules.\n\nUse of third-person singular without subject for the \"observation\"\npart is highly unusual the log messages in our codebase.\n\nThe usual way to compose a log message of this project is to\n\n - Give an observation on how the current system work in the present\n tense (so no need to say \"Currently X is Y\", just \"X is Y\"), and\n discuss what you perceive as a problem in it.\n\n - Propose a solution (optional---often, problem description\n trivially leads to an obvious solution in reader's minds).\n\n - Give commands to the codebase to \"become like so\".\n\nin this order.\n\n> The error code -25299 (errSecDuplicateItem) may be returned by\n> SecItemUpdate() in add_internet_password() if multiple instances of\n> git-credential-osxkeychain run in parallel. This patch introduces an\n> exclusive lock to serialize execution for avoiding this and other\n> potential issues.\n\n\"This patch introduces\" -> \"Introduce\"\n\nIs this step still needed, though?\n\n> Signed-off-by: Koji Nakamaru \n> ---\n> contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index 6a40917b1ef..0884db48d0a 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -414,6 +414,9 @@ int main(int argc, const char **argv)\n> \tif (!argv[1])\n> \t\tdie(\"%s\", usage);\n> \n> +\tif (open(argv[0], O_RDONLY | O_EXLOCK) == -1)\n> +\t\tdie(\"failed to lock %s\", argv[0]);\n> +\n> \tread_credential();\n> \n> \tif (!strcmp(argv[1], \"get\"))\n"},{"id":"494554","messageId":"xmqqmsovisc5.fsf@gitster.g","threadId":"61451","inReplyTo":"1f57718abff1d0e234c4145e833424da7be79311.1715428542.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] osxkeychain: state[] seen=1 to skip unnecessary store operations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-12T04:09:46Z","receivedAt":"2024-05-12T04:09:53Z","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> Records whether credentials come from get operations and skips\n> unnecessary store operations by utilizing the state[] feature, as\n> suggested by brian m. carlson.\n\nThis step has a problem description that is even sketchier than the\nprevious one. Anticipate questions by the other developers who read\nthis commit 6 months after it is accepted in the mainline (e.g.,\nWhat problem is there in the current system, why it is bad and worth\nsolving, and how is the patch trying to solve it?) and let your\nproposed log message answer the questions, as you won't be always\nsitting next to these developers.\n\nThanks.\n"},{"id":"494556","messageId":"CAOTNsDwMJigjZKZh74pB_TUdt=LYXO1=2RWEW0KS=_s19Y=8MQ@mail.gmail.com","threadId":"61451","inReplyTo":"xmqqseynisc9.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] osxkeychain: lock for exclusive execution","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-05-12T06:47:01Z","receivedAt":"2024-05-12T06:47:12Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"Thank you for the instruction about a log message. I'll follow it.\n\n> Is this step still needed, though?\n\nFor solving the issue I originally had, just utilizing state[] to skip\n\"store\" operations is enough.\n\nSince the osxkeychain implementation doesn't seem to be aware that it\ncan run in parallel, I thought it would be better to leave this step in\ncase a similar problem occurs. If this reason is weak, I'll remove this\nstep.\n\nKoji Nakamaru\n"},{"id":"494557","messageId":"CAOTNsDyw7xZT+KuE7=ExKk7wARJxAM2xQpv1dMGt+CfUr97pyw@mail.gmail.com","threadId":"61451","inReplyTo":"xmqqmsovisc5.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] osxkeychain: state[] seen=1 to skip unnecessary store operations","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-05-12T07:05:01Z","receivedAt":"2024-05-12T07:05:13Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"Thank you. I'll clean up the whole patch and its description after\ngetting a final reply about exlock.\n\nKoji Nakamaru\n"},{"id":"494832","messageId":"pull.1729.v3.git.1715800868.gitgitgadget@gmail.com","threadId":"61451","inReplyTo":"pull.1729.v2.git.1715428542.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] osxkeychain: lock for exclusive execution","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-15T19:21:05Z","receivedAt":"2024-05-15T19:21:12Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"Koji Nakamaru (2):\n osxkeychain: exclusive lock to serialize execution of operations\n osxkeychain: state to skip unnecessary store operations\n\n .../osxkeychain/git-credential-osxkeychain.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\n\nbase-commit: 83f1add914c6b4682de1e944ec0d1ac043d53d78\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1729%2FKojiNakamaru%2Ffeature%2Fosxkeychian_exlock-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1729/KojiNakamaru/feature/osxkeychian_exlock-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1729\n\nRange-diff vs v2:\n\n 1: 309c17c78f3 ! 1: 3341346c5e6 osxkeychain: lock for exclusive execution\n @@ Metadata\n Author: Koji Nakamaru \n \n ## Commit message ##\n - osxkeychain: lock for exclusive execution\n + osxkeychain: exclusive lock to serialize execution of operations\n \n - Resolves \"failed to store: -25299\" when \"fetch.parallel 0\" is configured\n - and there are many submodules.\n + git passes a credential that has been used successfully to the helpers\n + to record. If \"git-credential-osxkeychain store\" commands run in\n + parallel (with fetch.parallel configuration and/or by running multiple\n + git commands simultaneously), some of them may exit with the error\n + \"failed to store: -25299\". This is because SecItemUpdate() in\n + add_internet_password() may return errSecDuplicateItem (-25299) in this\n + situation. Apple's documentation [1] also states as below:\n \n - The error code -25299 (errSecDuplicateItem) may be returned by\n - SecItemUpdate() in add_internet_password() if multiple instances of\n - git-credential-osxkeychain run in parallel. This patch introduces an\n - exclusive lock to serialize execution for avoiding this and other\n - potential issues.\n + In macOS, some of the functions of this API block while waiting for\n + input from the user (for example, when the user is asked to unlock a\n + keychain or give permission to change trust settings). In general, it\n + is safe to use this API in threads other than your main thread, but\n + avoid calling the functions from multiple operations, work queues, or\n + threads concurrently. Instead, serialize function calls or confine\n + them to a single thread.\n +\n + The error has not been noticed before, because the former implementation\n + ignored the error.\n +\n + Introduce an exclusive lock to serialize execution of operations.\n +\n + [1] https://developer.apple.com/documentation/security/certificate_key_and_trust_services/working_with_concurrency\n \n Signed-off-by: Koji Nakamaru \n \n 2: 1f57718abff ! 2: 146b0ae9146 osxkeychain: state[] seen=1 to skip unnecessary store operations\n @@ Metadata\n Author: Koji Nakamaru \n \n ## Commit message ##\n - osxkeychain: state[] seen=1 to skip unnecessary store operations\n + osxkeychain: state to skip unnecessary store operations\n \n - Records whether credentials come from get operations and skips\n - unnecessary store operations by utilizing the state[] feature, as\n - suggested by brian m. carlson.\n + git passes a credential that has been used successfully to the helpers\n + to record. If a credential is already stored,\n + \"git-credential-osxkeychain store\" just records the credential returned\n + by \"git-credential-osxkeychain get\", and unnecessary (sometimes\n + problematic) SecItemAdd() and/or SecItemUpdate() are performed.\n \n + We can skip such unnecessary operations by marking a credential returned\n + by \"git-credential-osxkeychain get\". This marking can be done by\n + utilizing the \"state[]\" feature:\n +\n + - The \"get\" command sets the field \"state[]=osxkeychain:seen=1\".\n +\n + - The \"store\" command skips its actual operation if the field\n + \"state[]=osxkeychain:seen=1\" exists.\n +\n + Introduce a new state \"state[]=osxkeychain:seen=1\".\n +\n + Suggested-by: brian m. carlson \n Signed-off-by: Koji Nakamaru \n \n ## contrib/credential/osxkeychain/git-credential-osxkeychain.c ##\n\n-- \ngitgitgadget\n"},{"id":"494833","messageId":"3341346c5e6caaad3f222380a82425e1f14b97fa.1715800868.git.gitgitgadget@gmail.com","threadId":"61451","inReplyTo":"pull.1729.v3.git.1715800868.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] osxkeychain: exclusive lock to serialize execution of operations","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-15T19:21:06Z","receivedAt":"2024-05-15T19:21:12Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru \n\ngit passes a credential that has been used successfully to the helpers\nto record. If \"git-credential-osxkeychain store\" commands run in\nparallel (with fetch.parallel configuration and/or by running multiple\ngit commands simultaneously), some of them may exit with the error\n\"failed to store: -25299\". This is because SecItemUpdate() in\nadd_internet_password() may return errSecDuplicateItem (-25299) in this\nsituation. Apple's documentation [1] also states as below:\n\n In macOS, some of the functions of this API block while waiting for\n input from the user (for example, when the user is asked to unlock a\n keychain or give permission to change trust settings). In general, it\n is safe to use this API in threads other than your main thread, but\n avoid calling the functions from multiple operations, work queues, or\n threads concurrently. Instead, serialize function calls or confine\n them to a single thread.\n\nThe error has not been noticed before, because the former implementation\nignored the error.\n\nIntroduce an exclusive lock to serialize execution of operations.\n\n[1] https://developer.apple.com/documentation/security/certificate_key_and_trust_services/working_with_concurrency\n\nSigned-off-by: Koji Nakamaru \n---\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 6a40917b1ef..0884db48d0a 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -414,6 +414,9 @@ int main(int argc, const char **argv)\n \tif (!argv[1])\n \t\tdie(\"%s\", usage);\n \n+\tif (open(argv[0], O_RDONLY | O_EXLOCK) == -1)\n+\t\tdie(\"failed to lock %s\", argv[0]);\n+\n \tread_credential();\n \n \tif (!strcmp(argv[1], \"get\"))\n-- \ngitgitgadget\n\n"},{"id":"494834","messageId":"146b0ae9146f2c575738cb0d6a5008b766aa0c57.1715800868.git.gitgitgadget@gmail.com","threadId":"61451","inReplyTo":"pull.1729.v3.git.1715800868.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] osxkeychain: state to skip unnecessary store operations","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-15T19:21:07Z","receivedAt":"2024-05-15T19:21:14Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru \n\ngit passes a credential that has been used successfully to the helpers\nto record. If a credential is already stored,\n\"git-credential-osxkeychain store\" just records the credential returned\nby \"git-credential-osxkeychain get\", and unnecessary (sometimes\nproblematic) SecItemAdd() and/or SecItemUpdate() are performed.\n\nWe can skip such unnecessary operations by marking a credential returned\nby \"git-credential-osxkeychain get\". This marking can be done by\nutilizing the \"state[]\" feature:\n\n- The \"get\" command sets the field \"state[]=osxkeychain:seen=1\".\n\n- The \"store\" command skips its actual operation if the field\n \"state[]=osxkeychain:seen=1\" exists.\n\nIntroduce a new state \"state[]=osxkeychain:seen=1\".\n\nSuggested-by: brian m. carlson \nSigned-off-by: Koji Nakamaru \n---\n .../osxkeychain/git-credential-osxkeychain.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 0884db48d0a..6ce22a28ed7 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -12,6 +12,7 @@ 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@@ -171,6 +172,9 @@ 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@@ -284,6 +288,9 @@ 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@@ -395,6 +402,10 @@ 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-- \ngitgitgadget\n"},{"id":"494837","messageId":"CAOTNsDzpCs8fiLn4u8z-MH3sRp=G97ZSy--An=i8DpxkK6ec0Q@mail.gmail.com","threadId":"61451","inReplyTo":"pull.1729.v3.git.1715800868.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/2] osxkeychain: lock for exclusive execution","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-05-15T19:41:43Z","receivedAt":"2024-05-15T19:41:55Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"I thought about the issue further. The approach with the \"state[]\"\nfeature reduces problematic \"store\" operations a lot in most cases, it\nis however not perfect to avoid the original issue about parallel\nexecution.\n\nFor example (though quite artificial), let's suppose if two git commands\nstart simultaneously where the credential is input automatically by\n\"expect\" command. Two \"osxkeychain store\" commands will then run\nsimultaneously.\n\nI hope new descriptions are convincing.\n\nKoji Nakamaru\n"}]}