patchRevert "osxkeychain: state to skip unnecessary store operations"

> 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.

> [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

>
> "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.

> > 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.

> From: Koji Nakamaru <koji.nakamaru@gree.net>
> 
> This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.
> 
> 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".

>
> On 2025-11-12 at 07:01:21, Koji Nakamaru via GitGitGadget wrote:
> > From: Koji Nakamaru <koji.nakamaru@gree.net>
> >
> > This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.
> >
> > 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".
>
> I believe the intended approach here is that if we do a get and the
> credential is invalid, we return the same state[] header to erase, but
> we should not send it to subsequent gets for a new credential.  However,
> we do need to send it to subsequent gets (which will not have an
> intervening erase) if this is a multistage request because otherwise
> multistage requests will not be able to keep state, which NTLM and
> Kerberos require.  Does that make sense?
>
> My guess is that the problem here is that we reuse the credential
> structure without resetting it somewhere in the HTTP code rather than a
> problem in this particular helper.  That is probably my fault, but in my
> defence I would not say that the structure of the HTTP code is very easy
> to follow.