Re: [PATCH] Revert "osxkeychain: state to skip unnecessary store operations"

[Junio C Hamano <gitster@pobox.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.

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

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