-
Notifications
You must be signed in to change notification settings - Fork 55
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
Why can't the base of the stack be the actual main branch? #122
Comments
This used to be because GH didn't let you change the base, but apparently this API has started working so there is very little downside, we should do it |
What I'd like to see is for the base to be the previous PRs head. Especially as when the previous branch has been merged and deleted, GitHub will automatically update the PR's base to the branch it was merged into. |
Having the base be the previous PR's head is possible to implement, but it comes at a cost: today, you can reorder PRs in a ghstack and it just works. However, if you try to reorder in this world order, it doesn't work, because e.g., PR 2 (stacked on PR 1) still has all of PR 1's history in it, and so PR 1's history will start showing up if you retarget to master. I think the only way around it is to force push in this case, which might be an acceptable tradeoff. |
I'd certainly be happy with that that as a tradeoff personally. In fact that is more or less how I manage my stacks manual currently. |
Yeah ok. IDK when I'll get around to implementing this but I would definitely be happy to accept a PR that adds this as an option |
@ezyang Question related to this: If we made the new base for the first diff in the Ghstack main, is there anything else that would break if one were to use the Github UI to land the first diff in the stack? If not, I would love to take a crack at this, so that I could try to support using Ghstack with the Github Merge Queues feature. |
Nope, it would work fine (though I would recommend making sure you only do squash). However, the later PR wouldn't get automatically retargeted, so you'd need some integration to do that. |
On this, GitHub's behavior recently changed in that if you have a stack of PRs like
(Just reproduced it here: https://github.com/robinst/github-stacked-prs-test/pulls?q=is%3Apr) |
Oooh interesting! Is this documented anywhere? I couldn't find it in https://github.blog/ |
I couldn't find anything official, but it's easy to reproduce and a comment here also mentions it: https://stackoverflow.com/questions/32658070/what-happens-to-existing-pull-requests-when-the-target-branch-is-deleted#comment126821938_32658536 |
I spent some more time thinking about how to do this and there's another problem. It's pretty common to do a squash to merge in commits to main. This means that the merge base for main and the retargeted second PR on the stack will be quite far into the past, and each branch will incorporate the "same" change in a way that Git does not know is the same. So if there were further changes on top of it, you'll likely merge conflict; or worse, Git might get confused and apply the diff twice (when it should have only been applied once.) Luckily, I bet @robinst's reported behavior likely only occurs if you do an honest to goodness merge (which won't have this problem), so if you do a squash, you need a ghstack fixup step anyway, and we probably can manually setup the virtual merge in that case anyway. |
Another thing to note is that this would be a change in the ghstack refspec format, which actually is a BC breaking change since there are some tools relying on its format. So this would be something like a ghstack 2.0 type affair (or more likely, ghstack submit2 or something versioning like that) |
I've been dogfooding this feature on master and a few things I've noticed re: interaction with GitHub:
|
This is more of question. The README page correctly points out that merging commits from the github UI becomes impossible when using ghstack. One thing that would help with this is if the actual base of the branch the real
main
and not a brach created by ghstack. I am sure there is a reason for it but why is the base branch calledgh/username/1/base
, what would be the downside of basing the PR against main?The text was updated successfully, but these errors were encountered: