Skip to content
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

Update git.fetch calls to use depth=1 #2810

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

webmonarch
Copy link
Contributor

@webmonarch webmonarch commented Mar 7, 2024

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.

image

For me, it went from 18+ minutes -> 7s. YMMV

Fixes #2809

@webmonarch webmonarch changed the title When base is set, fetch depth=1 Update git.fetch calls to use depth=1 Mar 8, 2024
@webmonarch webmonarch marked this pull request as ready for review March 8, 2024 04:51
Copy link
Owner

@peter-evans peter-evans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@webmonarch
Copy link
Contributor Author

Thanks @peter-evans for taking the time. I updated the PR based on your guidance. Back to you!

@peter-evans
Copy link
Owner

The tests are failing, but I thought I checked them the other day. 🤔 I'll investigate.

@webmonarch
Copy link
Contributor Author

webmonarch commented Mar 11, 2024

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... 😬

@peter-evans
Copy link
Owner

peter-evans commented Mar 11, 2024

npm run test should run the unit and integration tests. However, don't worry about it, I realise now that I need to make some changes to a handful of the integration tests to cope with this change. I think the change is ok, it's probably just my tests need to be adjusted. I'll try to work on it tomorrow.

If it's ok, I'll commit test changes directly to your branch.

@peter-evans
Copy link
Owner

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 peter-evans changed the base branch from main to shallow-fetch-base March 12, 2024 13:53
@peter-evans peter-evans merged commit 53d7eb4 into peter-evans:shallow-fetch-base Mar 12, 2024
4 of 5 checks passed
peter-evans added a commit that referenced this pull request Mar 12, 2024
…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 <eric@collectivegenius.net>
@peter-evans
Copy link
Owner

@webmonarch Thank you for this contribution! A nice performance improvement for this case.

This is released as v6.0.2 / v6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base parameter causes extremely slow checkout of our monorepo
2 participants