Repository navigation
Conversation
ktravers
changed the title
ktravers
added
go
skarim
left a comment
skarim
left a comment
There was a problem hiding this comment.
lgtm! applying the flag on all force pushes (but not non-force pushes) makes sense to me ✅
There was a problem hiding this comment.
🟡 Changes recommended
Force pushes discard configured push options, and localized Git errors bypass the fallback.
2 open issues
What changed in this PR
Adds a stacked-PR signal to force pushes, supporting approval retention after rebases that leave the PR diff unchanged.
Changes:
- Sends
pull.stackedon force pushes, with an unsupported-remote fallback. - Adds push-option integration tests and updates rebase fixtures.
| File | Description |
|---|---|
| internal/git/gitops.go | Adds the push option and fallback. |
| internal/git/gitops_test.go | Tests option delivery and unsupported remotes. |
| cmd/rebase_test.go | Enables push options in test remotes. |
Review effort: Balanced
| } | ||
| // Send the "pull.stacked" push option on force pushes, falling back to a | ||
| // plain push for remotes that don't support push options. | ||
| withOpt := append([]string{"push", "-o", "pull.stacked"}, args[1:]...) |
There was a problem hiding this comment.
After researching more into git configs and push options, the suggestion here seems correct. Since we don't support other push options in dotcom, it's very unlikely a user will encounter this. But looks like it is possible for folks using GHES to configure their own push options so we might as well be safe and use the -c push.pushOption=pull.stacked route. The option is only used for the current invocation and not persisted, so we don't have to worry about this editing someone's config.
| // Send the "pull.stacked" push option on force pushes, falling back to a | ||
| // plain push for remotes that don't support push options. | ||
| withOpt := append([]string{"push", "-o", "pull.stacked"}, args[1:]...) | ||
| err := d.runSilent(withOpt...) |
There was a problem hiding this comment.
This looks correct. LC_ALL=C uses the standard locale instead of the user's configured language setting. Ideally we wouldn't use string matching, but there's no dedicated exit code for this situation, so this seems like the best approach.
This PR updates
git.Pushto send a push options argument (-o pull.stacked) on forced push only. Consumers can then read that argument as a signal that the ref update belongs to a stacked pull request and handle appropriately. Specifically, this will support retaining approving reviews on stacked pull requests after a rebase that doesn't change the diff (before and after diffs are "tree same").Alternatives considered: