Re: [PATCH] Revert "osxkeychain: state to skip unnecessary store operations"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 12, 2025, 16:47 UTC
- Message-ID
- <xmqqv7jfryet.fsf@gitster.g>
- In-Reply-To
- <pull.1998.git.1762930881599.gitgitgadget@gmail.com>
"Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Koji Nakamaru <koji.nakamaru@gree.net> > > This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.
OK. Let's make a mental note that e1ab45b2 (osxkeychain: state to skip unnecessary store operations, 2024-05-15) appeared in v2.46 or so.
Show 12 quoted lines
> That commit was trying to skip to store a credential returned by > "git-credential-osxkeychain get" by setting > "state[]=osxkeychain:seen=1". However, this state[] is kept even if a > credential returned by "git-credential-osxkeychain get" is invalid and > another subsequent helper's "get" returns a valid credential. Another > subsequent helper (such as [1]) may expect git-credential-osxkeychain to > store the valid credential so that "store" cannot be skipped by just > checking "state[]=osxkeychain:seen=1". > > In order to solve this issue, the state[] mechanism can be refined or > "osxkeychain:seen" can encode the whole information of the last > "get". For now, let's revert the change.
Is anybody actively working on the proper solution?
In a patch series that replaces the old commit with a more proper solution, it could be a reasonable layout of the series to make the first patch a revert like this patch to give the proper solution a clean slate to work from, but this looks different.
If the problem you are trying to solve here were a regression that happened after Git 2.51 was released, a revert is totally warranted at this point in time, even during the pre-release freeze period.
But it does not even look like a recent regression. Wouldn't reverting this change at this point give existing users who are accustomed to the current behaviour another regression, essentially robbing Peter to pay Paul? In such a case, I do not think "let's revert now and then hopefully a proper solution can come later" is a good approach.
Thanks.
Show 59 quoted lines
> [1]: https://github.com/hickford/git-credential-oauth
>
> Reported-by: Petter Sælen <petter@saelen.eu>
> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
> ---
> Revert "osxkeychain: state to skip unnecessary store operations"
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1998%2FKojiNakamaru%2Frevert%2Fe1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1998/KojiNakamaru/revert/e1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1998
>
> .../osxkeychain/git-credential-osxkeychain.c | 11 -----------
> 1 file changed, 11 deletions(-)
>
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index 611c9798b3..1f49ab8548 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -12,7 +12,6 @@ static CFStringRef username;
> static CFDataRef password;
> static CFDataRef password_expiry_utc;
> static CFDataRef oauth_refresh_token;
> -static int state_seen;
>
> static void clear_credential(void)
> {
> @@ -172,9 +171,6 @@ static OSStatus find_internet_password(void)
>
> CFRelease(item);
>
> - write_item("capability[]", "state", strlen("state"));
> - write_item("state[]", "osxkeychain:seen=1", strlen("osxkeychain:seen=1"));
> -
> out:
> CFRelease(attrs);
>
> @@ -288,9 +284,6 @@ static OSStatus add_internet_password(void)
> CFDictionaryRef attrs;
> OSStatus result;
>
> - if (state_seen)
> - return errSecSuccess;
> -
> /* Only store complete credentials */
> if (!protocol || !host || !username || !password)
> return -1;
> @@ -402,10 +395,6 @@ static void read_credential(void)
> oauth_refresh_token = CFDataCreate(kCFAllocatorDefault,
> (UInt8 *)v,
> strlen(v));
> - else if (!strcmp(buf, "state[]")) {
> - if (!strcmp(v, "osxkeychain:seen=1"))
> - state_seen = 1;
> - }
> /*
> * Ignore other lines; we don't know what they mean, but
> * this future-proofs us when later versions of git do
>
> base-commit: 4badef0c3503dc29059d678abba7fac0f042bc84