On Mon, Mar 10, 2025, at 11:53 AM, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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. >>> 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. >> 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?
