Re: [PATCH 0/2] describe and diff: implement --no-optional-locks

From: Benjamin Woodruff <github@benjam.info>
Date: Mar 10, 2025, 20:50 UTC
Message-ID: <bbc8a0ef-737c-44ba-9786-f5456f5ce71b@app.fastmail.com>
In-Reply-To: <xmqqo6y87m4d.fsf@gitster.g>

On Mon, Mar 10, 2025, at 11:53 AM, Junio C Hamano wrote:

Show 9 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> But maybe that is lost in the noise of reading the files to actually do
>> diffs, etc? I dunno. I expect it is more important for status, which
>> probably does not need to read the whole file contents in most cases
>> (and which may be run a lot from the user's prompt, etc).
>
> Yeah, and old timers who run "diff --raw" as if it were a quick
> analogue for "status" also would notice.

Thanks for your reviews, Junio and Jeff.

>>>> I must admit that I didn't imagine "describe" is something that
>>>> somebody would run a lot in the background

It might help if I start with some more context of why I wrote this patch. We've got a tool that uses the Rust `vergen-gitcl` crate to call `git describe --dirty`. You can see that code here: https://github.com/vercel/next.js/pull/76889/files

We use `vergen-gitcl` to generate version identifiers for an on-disk cache. This cache stores results of thousands of functions and has no backwards compatibility. We want to invalidate it when *any* of the code changes. `git-describe` felt like a good fit for that, as it gives us a unique identifier that's still reasonably user-friendly.

However, we discovered that we'd frequently end up with stale git lockfiles. This appeared to be due some combination of IDE tools that run the build in the background (i.e. the rust-analyzer LSP), behavior that causes builds to sometimes get killed before completion, and the fact that `git describe --dirty` takes a lock.

I've worked around this on our end, by re-implementing `describe`'s `--dirty` flag on top of `status`: <https://github.com/rustyhorde/vergen/pull/406>

So, from my end, there's no urgency to get this change in, or to add support for `diff` (we only care about `describe`). Rather, I felt like this might be a footgun for other users, and wanted to do the right thing by submitting an upstream change.

Show 9 quoted lines

>>> git describe and git diff may update the index in the background for
>>> similar performance reasons to git-status.
>>
>> That is a wrong reasoning that is completely opposite, though.
>> 
>> The commands at the Porcelain level, like "status" and "diff",
>> refresh the index for the CORRECTNESS purposes.
>
> Right, but "status" supports --no-optional-locks already.

Does this mean the documentation in `git-status` is incorrect? It implies that the background refresh is only for performance reasons. That's where I got this idea from: <https://git-scm.com/docs/git-status#_background_refresh>

It's also worth noting that libgit2 does not do this background refresh by default (`GIT_DIFF_UPDATE_INDEX` and `GIT_STATUS_OPT_UPDATE_INDEX`). I think that makes sense for libgit2's typical use-cases, but it is a divergence in behavior.

Show 11 quoted lines

>> Yeah, certainly it is a potential source of confusion to have a
>> conceptually read-only operation take locks or modify the repo state.
>>
>> [...]
>>
>> It seems like a big and possibly risky departure from what we've done
>> for so many years. I'm inclined not to rock the boat too much. ;)
>
> Certainly not right now.  But adding a command line option is even
> worse as we would have to carry the support for it for practically
> forever X-<.

What about if we reduce this to documentation changes? That alone would've saved me a lot of pain of trying to figure out what does and doesn't take a lock:

- Explicitly document that `--no-optional-locks` only changes behavior
  for `git status`.

- Document that the `--dirty` flag on `describe` will lead to
  `status`-like background refresh behavior.

- Change the documentation for status's background refresh to indicate
  that there is a subtle difference in behavior caused by
  `--no-optional-locks`? I lack sufficient context on how best to
  describe or explain this.

- Leave `git-diff` documentation as-is. I think the current
  documentation of `diff.autoRefreshIndex` is sufficient?