Update git.fetch calls to use depth=1 #2810
Conversation
webmonarch
changed the title
There was a problem hiding this comment.
I've gone back and reviewed the places where git fetch is used, and tried to remind myself why depth=1 cannot be used in some cases. (I also have tests covering these cases.)
There are four cases where git fetch is used. The two that I think are affecting you are the ones inside the if (workingBase != base) block, which you have added depth=1 to. I think these changes might actually be ok for all use cases! I've run all the tests for this and it seems fine. 👍
The depth=1 change you have made in the tryFetch function definitely will not work in all cases. This fetch is used for the pull request branch and if it has more than 1 commit it won't work correctly.
The other git fetch case (which you didn't add depth=1 to) also will not work. Unfortunately, that case in particular needs to "unshallow" the branch in order to work correctly. But I don't think that particular case affects you anyway. It's only used when using the push-to-fork feature.
So let's remove the --depth=1 in the tryFetch function and see if the remaining changes still solve the perf issue for you.
|
Thanks @peter-evans for taking the time. I updated the PR based on your guidance. Back to you! |
|
The tests are failing, but I thought I checked them the other day. 🤔 I'll investigate. |
|
I didn't see how to run the docker part of the test suite locally. LMK! UPDATE: I guess I need to have Docker running... 😬 |
|
If it's ok, I'll commit test changes directly to your branch. |
|
I thought I could push to your branch because it says "Maintainers are allowed to edit this pull request" but it didn't work. So I'll merge this to a feature branch and add the necessary changes before reopening a PR. |
peter-evans
merged commit 53d7eb4
into
peter-evans:shallow-fetch-base
…2816) * Update git.fetch calls to use depth=1 (#2810) * When base is set, fetch depth=1 * PR Feedback - remove depth=1 from tryFetch function * push-to-fork fix * test updates to handle shallow fetch of base --------- Co-authored-by: Eric Webb <[email protected]>
|
@webmonarch Thank you for this contribution! A nice performance improvement for this case. This is released as |
This works for me locally, but I am not sure of the total implications to other use-cases. I explicitly didn't update the push to fork use-case when mentioned it's not supported.
For me, it went from 18+ minutes -> 7s. YMMV
Fixes #2809