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

Don't require a deep fetch for forks #2847

Closed
infinisil opened this issue Apr 10, 2024 · 11 comments · Fixed by #2849 or #2857
Closed

Don't require a deep fetch for forks #2847

infinisil opened this issue Apr 10, 2024 · 11 comments · Fixed by #2849 or #2857

Comments

@infinisil
Copy link

Subject of the issue

Currently this action always performs a deep fetch of the repository, but that doesn't seem to be required, at least anymore (could a GitHub server update make this be possible suddenly?).

Here's a demo where I clone shallowly from the original repo (https://github.com/nixos/nixpkgs/ in this case, which is pretty big), make a commit, and push the result to my fork at https://github.com/infinisil/nixpkgs (which exists already, but it's not up-to-date with the original repo), which takes only a couple seconds:

$ git clone https://github.com/NixOS/nixpkgs --depth=1
Cloning into 'nixpkgs'...
remote: Enumerating objects: 68538, done.
remote: Counting objects: 100% (68538/68538), done.
remote: Compressing objects: 100% (43539/43539), done.
remote: Total 68538 (delta 2653), reused 59426 (delta 2191), pack-reused 0
Receiving objects: 100% (68538/68538), 47.91 MiB | 13.42 MiB/s, done.
Resolving deltas: 100% (2653/2653), done.
Updating files: 100% (40885/40885), done.

$ echo 'A change' >> README.md

$ git commit -a -m 'A change'
[master accef3639] A change
 1 file changed, 1 insertion(+)

$ git remote add mine git@github.com:infinisil/nixpkgs.git

$ git push mine HEAD:a-change
Enumerating objects: 5, done.
Counting objects: 100% (5/5), done.
Delta compression using up to 8 threads
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 300 bytes | 300.00 KiB/s, done.
Total 3 (delta 2), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
remote: 
remote: Create a pull request for 'a-change' on GitHub by visiting:
remote:      https://github.com/infinisil/nixpkgs/pull/new/a-change
remote: 
To github.com:infinisil/nixpkgs.git
 * [new branch]          HEAD -> a-change

Not doing this leads to having to wait 10 minutes for the action to complete in this case, e.g. https://github.com/NixOS/nixpkgs-check-by-name/actions/runs/8633608062/job/23667098826. And pushing to a fork is required in this case.

Related issues:

@willbush
Copy link

I was just searching through this repo briefly and I wonder if this comment is related?

@peter-evans
Copy link
Owner

Hi @infinisil

could a GitHub server update make this be possible suddenly?

I think you might be right. It seems to be working now without doing the fetch with --unshallow! 😮

Thank you for pointing it out! I'll release a fix for this.

@peter-evans
Copy link
Owner

Released as v6.0.3 / v6.

Thank you!

@peter-evans
Copy link
Owner

@infinisil Just saw your comment here: #2849 (comment)

This fix did actually address your original issue, but now the problem has shifted to the later fetch of the pull request branch. I can't hardcode that to depth=1, because there will be cases where some users have multiple commits on the branch.

I have an idea to fix it. I'll experiment with it next week.

@peter-evans
Copy link
Owner

@infinisil, would you be willing to test a new version of the action before I release it? It's not easy for me to test this with a real-world case like yours. I've made a change to limit the fetch depth when fetching the pull request branch.

You can try the new version like this:

uses: peter-evans/create-pull-request@branch-fetch-depth

@infinisil
Copy link
Author

Sure! Just did, and it seems to work: https://github.com/NixOS/nixpkgs-check-by-name/actions/runs/8707862728/job/23883876037 ✨ (tweag/nixpkgs#91, this is a PR from a fork to another fork, but I don't think that would influence the result)

@peter-evans
Copy link
Owner

peter-evans commented Apr 16, 2024

Sure! Just did, and it seems to work: https://github.com/NixOS/nixpkgs-check-by-name/actions/runs/8707862728/job/23883876037 ✨ (tweag/nixpkgs#91, this is a PR from a fork to another fork, but I don't think that would influence the result)

Thanks for testing, but this isn't doing the same as your original run here: https://github.com/NixOS/nixpkgs-check-by-name/actions/runs/8633608062/job/23667098826

The PR branch needs to exist before the action runs. Otherwise the fetch just fails and it creates a new branch. The perf problem comes when the branch exists and the fetch pulls all the history for the PR branch. So please keep fork/test-some-change existing on the fork repo and run it again so the fetch occurs.

@infinisil
Copy link
Author

Ah good point, I now updated the fork to PR against and reran the action, looks like that also works: https://github.com/NixOS/nixpkgs-check-by-name/actions/runs/8707862728/job/23892105339

@peter-evans
Copy link
Owner

Ah good point, I now updated the fork to PR against and reran the action, looks like that also works: https://github.com/NixOS/nixpkgs-check-by-name/actions/runs/8707862728/job/23892105339

Looks good! Down to 15 seconds from 10 minutes. Thank you for testing it.

I'm slightly nervous about releasing this because there is a possibility that someone is using the action in a way that I've not anticipated and this update clobbers their workflow. So just be aware that I might need to roll this back if it causes some issue for a lot of users. I think it will be a rare edge case if it causes any problem at all.

@peter-evans
Copy link
Owner

Released as v6.0.4 / v6.

@infinisil
Copy link
Author

So just be aware that I might need to roll this back if it causes some issue for a lot of users. I think it will be a rare edge case if it causes any problem at all.

Sounds good, no worries, thanks a lot for the quick fix!

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