Avoid fetching an rebuilding git gems when it's not necessary #6711
Conversation
|
Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack. For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide |
|
I have no idea why travis choked, by I hope you dont mind me adding some tests ;) |
Absolutely not. |
|
@bundlerbot r+ |
6711: Avoid fetching an rebuilding git gems when it's not necessary r=segiddins a=casperisfine Kind of a followup to #4272 ### The issue As demonstrated by the updated test case, whenever a gem is changed (even a non-git one), bundler re-fetch all git gems, and recompile their extensions (if any). You can repro that issue by running the modified `git_spec.rb` against master. ### The proposed fix In that patch I simply make `Source::Git` skip the `fetch` step if the `cached_revision` matches the install path. Since `install_path` uses only the first 10 characters of the full revision, there is a very very small chance of a collision happening, but It's so small that I think it can be ignored. However my understanding of the codebase is too limited to be 100% sure the git gem would be properly updated if it's definition (`branch` / `ref` / `etc`) is updated. I tried to write a test case for this but couldn't figure out how to create a repo with multiple revisions. I'll keep digging, but I figured I might as well ask for feedback at this stage. @segiddins any thoughts on this ? (since you fixed #4272), any pointers on how to better test this? cc @rafaelfranca @jules2689 Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Samuel Giddins <segiddins@segiddins.me>
Build succeeded |
ghost
merged commit
|
🎉 thanks a lot for improving the coverage ! |
Kind of a followup to #4272
The issue
As demonstrated by the updated test case, whenever a gem is changed (even a non-git one), bundler re-fetch all git gems, and recompile their extensions (if any).
You can repro that issue by running the modified
git_spec.rbagainst master.The proposed fix
In that patch I simply make
Source::Gitskip thefetchstep if thecached_revisionmatches the install path.Since
install_pathuses only the first 10 characters of the full revision, there is a very very small chance of a collision happening, but It's so small that I think it can be ignored.However my understanding of the codebase is too limited to be 100% sure the git gem would be properly updated if it's definition (
branch/ref/etc) is updated. I tried to write a test case for this but couldn't figure out how to create a repo with multiple revisions. I'll keep digging, but I figured I might as well ask for feedback at this stage.@segiddins any thoughts on this ? (since you fixed #4272), any pointers on how to better test this?
cc @rafaelfranca @jules2689