opam update: Avoid reloading repository contents when the repo has not changed#5043
Merged
rjbou merged 4 commits intoocaml:masterfrom May 20, 2022
Merged
opam update: Avoid reloading repository contents when the repo has not changed#5043rjbou merged 4 commits intoocaml:masterfrom
rjbou merged 4 commits intoocaml:masterfrom
Conversation
kit-ty-kate
reviewed
Feb 6, 2022
src/state/opamUpdate.ml
Outdated
| job { r with repo_url = new_url } (n-1) | ||
| in | ||
| job repo max_loop @@+ fun repo -> | ||
| job repo max_loop @@+ fun has_changes -> |
Member
There was a problem hiding this comment.
Suggested change
| job repo max_loop @@+ fun has_changes -> | |
| job repo max_loop @@+ fun repo, has_changes -> |
repo is not return by the job function anymore. I belive this would break repositories with redirections from a glance at the code. Could you either return repo as before in a tuple or inside the new [no_changes | changes] state (whichever you think is better)
Member
Author
There was a problem hiding this comment.
you are right of course! I had missed the fact that the repo is returned by job to account for redirections. should be fixed now.
6407321 to
9604c13
Compare
Member
|
note for opam devs: this is going to conflict with #5015 and I think #5015 should be prioritized. I’ll handle rebase of this PR after the merge. @Armael once #5015 is merged I think a simple test could also be added to |
9604c13 to
355bbd7
Compare
Member
Author
|
I have rebased the PR now that #5015 has been merged. I can try to add a test, but I'm not exactly sure how to test the PR, maybe by testing that .tar has not been regenerated by checking the modification date after doing a second |
Collaborator
|
For the test, you can check also check |
355bbd7 to
1c31257
Compare
Member
|
The tests failed with: |
73eb1fc to
530b9fd
Compare
Member
Author
|
Thanks a lot! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently,
opam updatewill reload the state of a repository (reading .opam files, etc) even when the repository has not changed (as detected by the backend).This PR is an attempt at optimizing this case: if the backend says that a repository has not changed, avoid reloading its contents, allowing for quicker
opam updates.(a review is particularly welcome as I am not familiar with this part of opam's code; I might have missed some subtleties wrt synchronization of the on-disk view of the repo and the "repo" in-memory data structure...)