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.

Sign up for GitHub

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

Jump to bottom

Add BranchPushIndividually repo config #374

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Add BranchPushIndividually repo config #374

merged 1 commit into from
Dec 18, 2023

Conversation

scosenza
Copy link

@scosenza scosenza commented Dec 17, 2023 •
edited
Loading

Problem

Atomically pushing all stacked branches at once can fail in larger and slower monorepos with pre-commit hook timeouts e.g.

% git spr update --detail
> git rev-parse --show-toplevel
> git fetch
> git rebase origin/master --autostash
> github fetch pull requests
> git log --format=medium --no-color origin/master..HEAD
> git branch --no-color
> git log --format=medium --no-color origin/master..HEAD
> git status --porcelain --untracked-files=no
> git push --force --atomic origin eddc899e328aa4b7819dd62f15dfdf5cbb489efb:refs/heads/spr/master/895c5bb2 cc639c1396bc68569e7f7887d2e60c167a543b19:refs/heads/spr/master/2a17fcc7 8bc3daa4d188043c2f226ac4c58a0688266aa2a0:refs/heads/spr/master/128d0ed9 cd1ae69fcc4c110a246036d27624594f57295a1a:refs/heads/spr/master/5a24cfff 6eafcb9f8bbb52923ddc2d3c6ef97630d087b9c3:refs/heads/spr/master/cf90150f 36e061e05f4d7ce8302abd29c08d3140606198d1:refs/heads/spr/master/010f3a1d 866a77d361596116afe465ec0e12057ce3a3e892:refs/heads/spr/master/4586c009 aa0c0d60bdade274a1d8cbf66ac148a4b2ce0549:refs/heads/spr/master/0035f666 d0cae29d8c770cfaefb54444a7b45ef65a247077:refs/heads/spr/master/878d183d 2117d0ad6b262a052a8a19656259f82eff5688d8:refs/heads/spr/master/9fd490a9 e7cafc67a60e91a85aea0792725efbc3a109c3a9:refs/heads/spr/master/065af94b 9fc3978ee0e40e2b73f84959f399afa643003369:refs/heads/spr/master/c9e6d2f0 f77c77080714336724ce30e07c8ca9b0e48c03de:refs/heads/spr/master/8660d178 d2cfc4ce7a80d2e83daea6ef49c771ee1dd3d7bc:refs/heads/spr/master/a9f2063f
git error: remote: pre-receive/global-checks.sh: execution exceeded 5s timeout
To <redacted>.git
 ! [remote rejected]             aa0c0d60bdade274a1d8cbf66ac148a4b2ce0549 -> spr/master/0035f666 (pre-receive hook declined)
 ! [remote rejected]             36e061e05f4d7ce8302abd29c08d3140606198d1 -> spr/master/010f3a1d (pre-receive hook declined)
 ! [remote rejected]             e7cafc67a60e91a85aea0792725efbc3a109c3a9 -> spr/master/065af94b (pre-receive hook declined)
 ! [remote rejected]             8bc3daa4d188043c2f226ac4c58a0688266aa2a0 -> spr/master/128d0ed9 (pre-receive hook declined)
 ! [remote rejected]             cc639c1396bc68569e7f7887d2e60c167a543b19 -> spr/master/2a17fcc7 (pre-receive hook declined)
 ! [remote rejected]             866a77d361596116afe465ec0e12057ce3a3e892 -> spr/master/4586c009 (pre-receive hook declined)
 ! [remote rejected]             cd1ae69fcc4c110a246036d27624594f57295a1a -> spr/master/5a24cfff (pre-receive hook declined)
 ! [remote rejected]             d0cae29d8c770cfaefb54444a7b45ef65a247077 -> spr/master/878d183d (pre-receive hook declined)
 ! [remote rejected]             eddc899e328aa4b7819dd62f15dfdf5cbb489efb -> spr/master/895c5bb2 (pre-receive hook declined)
 ! [remote rejected]             2117d0ad6b262a052a8a19656259f82eff5688d8 -> spr/master/9fd490a9 (pre-receive hook declined)
 ! [remote rejected]             6eafcb9f8bbb52923ddc2d3c6ef97630d087b9c3 -> spr/master/cf90150f (pre-receive hook declined)
 ! [remote rejected]             9fc3978ee0e40e2b73f84959f399afa643003369 -> spr/master/c9e6d2f0 (pre-receive hook declined)
 ! [remote rejected]             f77c77080714336724ce30e07c8ca9b0e48c03de -> spr/master/8660d178 (pre-receive hook declined)
 ! [remote rejected]             d2cfc4ce7a80d2e83daea6ef49c771ee1dd3d7bc -> spr/master/a9f2063f (pre-receive hook declined)
error: failed to push some refs to '<redacted>.git'
panic: exit status 1

goroutine 1 [running]:
github.com/ejoffe/spr/git/realgit.(*gitcmd).MustGit(0x140000b0100?, {0x140000da000?, 0x10?}, 0x140000c6800?)
	/Users/runner/work/spr/spr/git/realgit/realcmd.go:44 +0x4c
github.com/ejoffe/spr/spr.(*stackediff).syncCommitStackToGitHub(0x14000122d20, {0x1400018f620?, 0x140003840f1?}, {0x140003c0480, 0xe, 0x100bcc0dc?}, 0x1400021a4b0)
	/Users/runner/work/spr/spr/spr/spr.go:549 +0x578
github.com/ejoffe/spr/spr.(*stackediff).UpdatePullRequests(0x14000122d20, {0x1010395b0?, 0x101355460}, {0x101355460, 0x0, 0x0}, 0x0)
	/Users/runner/work/spr/spr/spr/spr.go:180 +0x790
main.main.func4(0x140001fa7e0?)
	/Users/runner/work/spr/spr/cmd/spr/main.go:155 +0x100
github.com/urfave/cli/v2.(*Command).Run(0x140001fa7e0, 0x14000127280)
	/Users/runner/go/pkg/mod/github.com/urfave/cli/[email protected]/command.go:169 +0x4f8
github.com/urfave/cli/v2.(*App).RunContext(0x140001251e0, {0x1010395b0?, 0x101355460}, {0x14000110120, 0x3, 0x3})
	/Users/runner/go/pkg/mod/github.com/urfave/cli/[email protected]/app.go:341 +0x85c
github.com/urfave/cli/v2.(*App).Run(...)
	/Users/runner/go/pkg/mod/github.com/urfave/cli/[email protected]/app.go:247
main.main()
	/Users/runner/work/spr/spr/cmd/spr/main.go:224 +0x11bc

Solution

Instead of always pushing branches atomically (which remains the default), add a BranchPushIndividually repo config setting which can be set to true to push each branch one at a time e.g.

git spr update
> git rev-parse --show-toplevel
> git fetch
> git rebase origin/master --autostash
> github fetch pull requests
> git log --format=medium --no-color origin/master..HEAD
> git branch --no-color
> git log --format=medium --no-color origin/master..HEAD
> git status --porcelain --untracked-files=no
> git push --force origin eddc899e328aa4b7819dd62f15dfdf5cbb489efb:refs/heads/spr/master/895c5bb2
> git push --force origin cc639c1396bc68569e7f7887d2e60c167a543b19:refs/heads/spr/master/2a17fcc7
> git push --force origin 8bc3daa4d188043c2f226ac4c58a0688266aa2a0:refs/heads/spr/master/128d0ed9
> git push --force origin cd1ae69fcc4c110a246036d27624594f57295a1a:refs/heads/spr/master/5a24cfff
> git push --force origin 6eafcb9f8bbb52923ddc2d3c6ef97630d087b9c3:refs/heads/spr/master/cf90150f
> git push --force origin 36e061e05f4d7ce8302abd29c08d3140606198d1:refs/heads/spr/master/010f3a1d
> git push --force origin 866a77d361596116afe465ec0e12057ce3a3e892:refs/heads/spr/master/4586c009
> git push --force origin aa0c0d60bdade274a1d8cbf66ac148a4b2ce0549:refs/heads/spr/master/0035f666
> git push --force origin d0cae29d8c770cfaefb54444a7b45ef65a247077:refs/heads/spr/master/878d183d
> git push --force origin 2117d0ad6b262a052a8a19656259f82eff5688d8:refs/heads/spr/master/9fd490a9
> git push --force origin e7cafc67a60e91a85aea0792725efbc3a109c3a9:refs/heads/spr/master/065af94b
> git push --force origin 9fc3978ee0e40e2b73f84959f399afa643003369:refs/heads/spr/master/c9e6d2f0
> git push --force origin f77c77080714336724ce30e07c8ca9b0e48c03de:refs/heads/spr/master/8660d178
> git push --force origin d2cfc4ce7a80d2e83daea6ef49c771ee1dd3d7bc:refs/heads/spr/master/a9f2063f
> git push --force origin 4942ee2caf5abf54de4e0fcdb91653dd4d6fd431:refs/heads/spr/master/a12b0c36
...

@scosenza scosenza changed the title WIP: Add BatchPushAtomically repo config so it can be disabled for slow repos that hit timeouts WIP: Add BatchPushAtomically repo config Dec 17, 2023
@scosenza scosenza marked this pull request as ready for review December 17, 2023 01:52
@scosenza scosenza changed the title WIP: Add BatchPushAtomically repo config Add BatchPushAtomically repo config Dec 17, 2023
ejoffe
ejoffe reviewed Dec 17, 2023
View reviewed changes
Copy link
Owner

@ejoffe ejoffe left a comment

Choose a reason for hiding this comment

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

Thank you for contributing!
Looks good.
Can you combine the two commits to a single commit please.

readme.md Outdated
@@ -167,6 +167,7 @@ User specific configuration is saved to .spr.yml in the user home directory.
| forceFetchTags | bool | false | also fetch tags when running 'git spr update' |
| branchNameIncludeTarget | bool | false | include target branch name in pull request branch name |
| showPrTitlesInStack | bool | false | show PR titles in stack description within pull request body |
| batchPushAtomically | bool | true | push all commits in a batch atomically |
Copy link
Owner

Choose a reason for hiding this comment

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

Can you call this branchPushIndividually and have the default false

@scosenza scosenza changed the title Add BatchPushAtomically repo config Add BranchPushIndividually repo config Dec 18, 2023
ejoffe
ejoffe reviewed Dec 18, 2023
View reviewed changes
Copy link
Owner

@ejoffe ejoffe left a comment

Choose a reason for hiding this comment

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

unit tests failing because of a missing --atomic flag in the atomic case.
I don't think you need the --atomic flag in the individual case.

Add BranchPushIndividually repo config
89b3201 
Instead of always pushing branches atomically (which can result in timeouts in larger and slower monorepos), add a BranchPushIndividually repo config setting which enables pushing each branch one at a time.
@ejoffe ejoffe merged commit 1c6edd8 into ejoffe:master Dec 18, 2023
1 check passed
@ejoffe
Copy link
Owner

ejoffe commented Dec 18, 2023

released in v0.14.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejoffe ejoffe ejoffe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@scosenza @ejoffe