Re: [PATCH] osxkeychain: lock for exclusive execution

From: Jeff King <peff@peff.net>
Date: May 10, 2024, 20:01 UTC
Message-ID: <20240510200114.GC1954863@coredump.intra.peff.net>
In-Reply-To: <D7A8539F-E33C-44F3-A7BF-5F5D4A26F2A4@boanderson.me>

On Fri, May 10, 2024 at 04:02:03PM +0100, Bo Anderson wrote:

Show 5 quoted lines

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

The short answer is that Git always passes a credential which has been used successfully to the helpers to record (if they want to). That's how stuff gets stored in the first place. And those parallel fetches have no knowledge of what the other ones are doing, so they all try to store.

But the more interesting question is: why do we tell helpers to store a credential that we got from helpers in the first place? The behavior is mostly an artifact of how the original implementation behaved, as it did not record the source of the credential.

And I think there are several problems with that, besides inefficiency and locking. See this old patch, which fixes it by remembering when a credential came from a helper:

  https://lore.kernel.org/git/20120407033417.GA13914@sigill.intra.peff.net/

But we didn't merge it because some people rely on the behavior of helpers feeding back to themselves. I outlined some solutions there, but it would definitely be a change in behavior that people would have to adapt to.

Some possible alternatives:

  - we could remember _which_ helper we got the credential from, and
    avoid invoking it again.

  - we could record a bit saying that the credential came from a helper,
    and then feed that back to helpers when storing. So osxkeychain
    could then decide not to store it.

Both of those solve the repeated stores, but still let credentials populate across helpers (which I still think is a questionable thing to do by default, per the discussion in that thread, but is the very thing that some people rely on).

-Peff