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

upload: Check PRs merge into base branch rather than base ref #207

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

jerry-skydio
Copy link
Collaborator

The logic for checking whether a PR was improperly merged was allowing
the target branch name, which could be a relative branch. We don't want
that, we only want changes to merge into the base branch of the entire chain.

This also helps handle some reordering issues.

Topic: reorder3
Reviewers: aaron, brian-k

The logic for checking whether a PR was improperly merged was allowing
the target branch name, which could be a relative branch. We don't want
that, we only want changes to merge into the base branch of the entire chain.

This also helps handle some reordering issues.

Topic: reorder3
Reviewers: aaron, brian-k
@jerry-skydio
Copy link
Collaborator Author

Reviews in this chain:
#207 upload: Check PRs merge into base branch rather than base ref

@jerry-skydio
Copy link
Collaborator Author

# head base diff date summary
0 98a104eb 159f964c diff Dec 30 15:28 PM 1 file changed, 2 insertions(+), 2 deletions(-)

Copy link
Contributor

@aaron-skydio aaron-skydio left a comment

Choose a reason for hiding this comment

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

Hmm. I think it should be fine to merge into either the base branch, or the review.remote_base? For example, sometimes I'll upload a PR relative to someone else's PR as a suggestion/demo, with the intent that they'll merge my PR into theirs before merging theirs into its base. And that sort of thing should be fully revup-supported with #205 (you could already do it with two of your own PRs but I don't have a use case for that)

@jerry-skydio
Copy link
Collaborator Author

if you intend to use the github merge UI you should be using base_branch rather than relative or relative-branch, as the main difference between these is how revup thinks you intended to merge things into them, and how revup should react

base-branch : you intend to merge into this. if pr is merged, revup does not re-create it
relative-branch : you want your diff to be relative, but intend to merge into some other base branch. if relative-branch itself is merged, it is ignored. if your pr gets merged, revup re-creates it
relative : similar to above, but you also control the relative branch / it is created via revup itself. if your pr gets merged, revup re-creates it

@jerry-skydio
Copy link
Collaborator Author

specifically #205 is for when you intend to take over someone else's branch (and this confusion aspect was probably why i didn't originally allow this). if you don't intend to take over the branch you should still use relative-branch (or base-branch perhaps). this is because 2 people pushing to a branch will be confusing -- revup makes no attempt to integrate the push efforts of 2 different uploaders working simultaneously

@aaron-skydio
Copy link
Contributor

Good point, if you're relative to someone else's branch using uploader (which would mean you're planning to push that branch) you might as well just push to that directly instead of creating and merging a PR

@jerry-skydio jerry-skydio merged commit 08c642d into main Jan 2, 2025
5 checks passed
@jerry-skydio jerry-skydio deleted the jerry/revup/main/reorder3 branch January 2, 2025 18:41
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.

2 participants