Improve selected files performance lag in Commit View#2780
Conversation
|
Did you look into why pathspec matching was used in the first place? I have a vague memory of it, unexpectedly, being faster than specifying straightforward paths, maybe from other places in the codebase. |
Yes, I looked into the history. There's no performance benefit to pathspec matching — it's actually the opposite. History of this code:
Why pathspec matching is not faster:
Consistency with the rest of the codebase:
This PR's approach ( |
|
Thank you for this first pass at an answer. You mention that PR #639 is the one that enabled pathspec matching, but that doesn't answer the question of why it did! I read through the comments there, and thankfully they explain things quite clearly: there was a bug in libgit2 that caused incorrect behavior when checking out indexes without pathspec matching. The GitUp PR worked around that by re-enabling matching, and doing the necessary escaping. So your PR removes this no-longer-relevant fix, which is nice :) Tangential, but: reading the LLM output in your messages is to me unpleasant, and pretty confusing. The outputs themselves are vague and inconsistent; but more importantly, I'm much more interested in your own thoughts, @delacrixmorgan! It's hard to tell, in those messages, what parts are yours, and what's just a generation glitch. For instance, when you say “There's no scenario where glob matching outperforms direct comparison”, this is directly contradicted by the contents of the PR you link in the paragraph right before that. But I have no idea if you made this mistake in reasoning, or if you never reasoned about it at all. It all makes for a strange and difficult conversation! |
|
Hey @Cykelero, Thanks for going through with the PR. Honestly, I don't know a single thing about Objective C or Mac app development, I am a mobile engineer working mostly with Kotlin and Java. That's why it's a bit difficult for me to write my opinions about it. GitUp is a tool that I depend heavily on daily and this behaviour has been really annoying for me for a while now. Hence, I just thought why not have Claude take a hit at it and hopefully will make it a better experience for everyone as well. I do my due diligence by testing it locally and had risk assessment from Claude. Also, try to keep the changes to a bare minimum. Then, I hope that experienced developers and contributors like you guys can validate whether the solution is a good one or not. Hopefully that answers your question. |
|
Thanks @delacrixmorgan, I think this is on me to have a clear policy for AI usage expectations. I appreciate the honesty, and frankly, the targeted performance fixes seem reasonable. I think I need to at state that having a PR that has a human opinion to summarize the why behind the changes before the giant AI change summary would be needed going forward. If you don't fully understand the why yourself, please state this from the beginning so that I know what the expectations are on my end for reviewing. I also expect that you yourself have tested the changes locally and haven't observed regressions and that the PR indeed addresses the claim. |
Problem
In the Commit View, selecting all files and pressing Delete to discard changes has a noticeable lag compared to using the "Discard All" button, especially with many files.
The root cause is in
checkoutFilesToWorkingDirectory:fromIndex:error:— withoutGIT_CHECKOUT_DISABLE_PATHSPEC_MATCH, libgit2 treats each file path as a glob/fnmatch pattern. This causes O(N × M) comparisons where every index entry is checked against every pathspec using pattern matching. With hundreds of files, this becomes very slow.Fix
Added
GIT_CHECKOUT_DISABLE_PATHSPEC_MATCHto the checkout strategy inGCIndex.m. This tells libgit2 to treat paths as exact literals, enabling fast binary-search lookup instead of glob matching. Also removed the unnecessaryNSRegularExpression escapedPatternForString:call since paths are now matched literally (it was a workaround for the pattern-matching behavior).Risk
Low. The paths passed to this function are always literal file paths from diff deltas — never glob patterns. The previous
escapedPatternForString:call confirms the intent was always exact matching. This is consistent with howGCRepository+HEAD.malready usesGIT_CHECKOUT_DISABLE_PATHSPEC_MATCHfor similar checkout operations.