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

Why can't the base of the stack be the actual main branch? #122

Open
turbo1912 opened this issue Nov 18, 2022 · 13 comments
Open

Why can't the base of the stack be the actual main branch? #122

turbo1912 opened this issue Nov 18, 2022 · 13 comments

Comments

@turbo1912
Copy link

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 called gh/username/1/base, what would be the downside of basing the PR against main?

WARNING. You will NOT be able to merge these commits using the normal GitHub UI, as their branch bases won't be master. Use ghstack land $PR_URL to land a ghstack'ed pull request.
@ezyang
Copy link
Owner

ezyang commented Nov 18, 2022

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

@spence-novata
Copy link

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.

@ezyang
Copy link
Owner

ezyang commented Dec 1, 2022

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.

@spence-novata
Copy link

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.

@ezyang
Copy link
Owner

ezyang commented Dec 1, 2022

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

@Gamrix
Copy link

Gamrix commented Apr 11, 2023

@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.

@ezyang
Copy link
Owner

ezyang commented Apr 11, 2023

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.

@robinst
Copy link

robinst commented Sep 7, 2023

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 b -> a -> main, and merge the PR for a and then delete branch a, GitHub will automatically retarget b to main. On the PR it shows up as a message like this:

Base automatically changed from a to main

(Just reproduced it here: https://github.com/robinst/github-stacked-prs-test/pulls?q=is%3Apr)

@aspiers
Copy link

aspiers commented Sep 7, 2023

GitHub's behavior recently changed in that if you have a stack of PRs like b -> a -> main, and merge the PR for a and then delete branch a, GitHub will automatically retarget b to main.

Oooh interesting! Is this documented anywhere? I couldn't find it in https://github.blog/

@robinst
Copy link

robinst commented Sep 9, 2023

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

@ezyang
Copy link
Owner

ezyang commented Dec 2, 2023

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.

@ezyang
Copy link
Owner

ezyang commented Dec 2, 2023

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)

@ezyang
Copy link
Owner

ezyang commented Dec 16, 2023

I've been dogfooding this feature on master and a few things I've noticed re: interaction with GitHub:

  • You can use the regular GitHub UI to merge commits, but there are some caveats. Merges work great. However, if you squash merge, anything stacked on top of the PR will "conflict", because Git is ostensibly trying to re-merge the branch changes from the base PR and failing. If you git pull --rebase and then ghstack --direct that will typically resolve things without causing anymore conflicts. There's probably a way to cook up a GitHub action that will automatically fix this
  • You probably want to modify GitHub settings to use PR body as the commit message, since people typically generate failure useless ghstack update messages commit-by-commit. One side effect of this is I moved the ghstack navigation links to a separate comment, so they don't get uselessly glommed into your commit message. It's possible we should change this for old-style ghstack, not sure.
  • Twitter seems to think that in complicated rebase situations, we should just force push: https://twitter.com/ezyang/status/1735456825133216249 This requires extra implementation that's not done yet

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

No branches or pull requests

6 participants