-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Fix one performance/correctness regression in #6478 found on Rails repository. #6686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
when the repository contains a lot of merge commits. Also return the merge commit as the changed one if the file or directory was changed as part of the merge, eg. through conflict resolution. Signed-off-by: Filip Navara <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #6686 +/- ##
==========================================
- Coverage 40.8% 40.79% -0.01%
==========================================
Files 421 421
Lines 57571 57570 -1
==========================================
- Hits 23494 23488 -6
- Misses 30951 30955 +4
- Partials 3126 3127 +1
Continue to review full report at Codecov.
|
in a merge commit follow only the first parent.
The rails home page will load from 8s to 6.4s on my macOS after merge this PR. |
@lunny Thanks for trying it. That corresponds with numbers from my local testing. |
@lunny Can you also try the branch at https://github.com/filipnavara/gitea/tree/go-git-perf-fixes? It contains this PR + some optimizations to go-git. On my machine that's enough to push it to around ~4.5 seconds. There's still room for some improvement but I wanted to address the low-hanging fruit first. For the root directory it's now similar to the performance I got before #6478. That may not necessarily be good enough since invoking |
@filipnavara Do you want to update go-git recent version on this PR? |
@lunny not worth it at the moment. I have some go-git PRs waiting in the pipeline that have to be merged first. |
Fix flaw in the commit history lookup that caused unnecessary traversal when the repository contains a lot of merge commits.
Also return the merge commit as the changed one if the file or directory was changed as part of the merge, eg. through conflict resolution. This matches behavior observed on GitHub.
Note that this doesn't fix the listing performance regression just yet. There's still problem when listing directories containing very old files. In that case the commit history has to be traversed all the way back to when the file was last modified.