Repository navigation
Push rebased PRs before merging #9391
Description
I've noticed that PRs sent by people outside of Node.js core (i.e. people who can't merge their own PRs) show as closed on GitHub even though technically speaking they were rebased and merged.
I'm not 100% sure as I haven't tested it but I believe that if whoever merges the PR pushes the rebased commits first - which can be done if the "allow edits from maintainers" box is checked - and then does a fast-forward merge, GitHub should mark the PR as merged.
Someone should test it on an open PR. If it doesn't work, oh well, but if it does it would be kinda nice if the merge policy was changed to use this so people could see which PRs were merged and which were closed for some other reason, which is currently impossible to determine when looking at the list of PRs.
Activity
mscdex commentedon Nov 1, 2016
It's a pain for collaborators to push to a PR because AFAIK you have to clone/check out the PR author's fork first, unless there is some trick that I'm not aware of that prevents having to do that.
gibfahn commentedon Nov 1, 2016
This was added to the merging PRs guide in #8774, (the comments there have some useful information about checking out PR branches @mscdex , and there's also this).
But the conclusion was that it should only be done when merging in people's own PRs. Maybe we should add something in the PR template that allows people to opt-in?
addaleax commentedon Nov 1, 2016
It may only help a little, but you don’t need to have a git repository listed locally as a remote to push to it, e.g. you can do
git push git@github.com:somebody/node.git somebranch.Fishrock123 commentedon Nov 1, 2016
This only works if PR authors have made their PR branch available for us to push to...
sam-github commentedon Nov 1, 2016
Which they mostly won't.
There was some excitement about the enhancements github made to green-button merges, @Fishrock123 IIRC, you thought it would allow squashing, fast-forwarding, and adding commit metadata all from github UI, did that work out?
targos commentedon Nov 1, 2016
Why not ? The option is checked by default.
sam-github commentedon Nov 1, 2016
That's surprising, the github docs say otherwise:
But it looks like github has enabled it by default. Curious, does that allow force-pushing? Because its force-pushing we would need to do, which seems like a dangerous thing to allow by default.
Fishrock123 commentedon Nov 1, 2016
@sam-github there are currently two options:
Unfortunately that doesn't fully cover our needs but can cover simple cases.
Fishrock123 commentedon Nov 1, 2016
(Note: I think it appends to the commit message, so it may make an invalid commit message. Try it on your own repos FIRST!)
MylesBorins commentedon Nov 1, 2016
I'm - 1 on this idea.
this will involve force pushing over other individuals feature branches.
Even with permission I am uncomfortable with our collaborators making
destructive changes to other people's remotes
while having the purple merge label on a commit is nice to have, I do not
think it is worth the complexity and potential human conflict this could
create
On Tue, Nov 1, 2016, 8:41 AM Jeremiah Senkpiel notifications@github.com
wrote:
gibfahn commentedon Nov 1, 2016
@thealphanerd I agree, it's also an issue if something goes wrong and you merge in a bad commit, if you force-push to someone's fork then you lose your backup.
Perhaps we could leave the PR open after updating, and allow the person who created the branch to force-push themselves (after checking that the commit in master is okay)? I'm not sure if that would work in Github.
fhinkel commentedon Nov 1, 2016
The closed PR should usually have a comment with "Landed in xxxx". What's the problem with it showing as closed rather than merged? By the way, collaborator's PRs are also closed rather than merged.
sam-github commentedon Nov 1, 2016
Depends on the collaborator, see #9257. I force-pushed the branch, so it matches a commit in the history and shows as merged.
Its a nicety, having the merged status, but it doesn't work in general with how node merges code, which isn't supported well by github.
MylesBorins commentedon Nov 1, 2016
from a backporting perspective I would prefer to see the "landed in"
message in all prs... can be very easy to miss if there is more than 1
xommit
On Tue, Nov 1, 2016, 12:02 PM Sam Roberts notifications@github.com wrote:
strugee commentedon Nov 1, 2016
@fhinkel those comments aren't helpful when looking at https://github.com/nodejs/node/pulls?q=is%3Apr+is%3Aclosed.
I should also point out that checking out someone else's repo could be a pain unless it's automated, which should be relatively easy to do.
strugee commentedon Nov 1, 2016
@thealphanerd what if you asked contributors to squash themselves, but then whoever merged would rebase on top of
master, force-push and fast-forward? That'd be much less destructive, IMHO. Although it might make it harder to contribute, which is bad...evanlucas commentedon Nov 1, 2016
-1 for me. I think the "Landed in ..." comments should be required. It sure makes cutting releases easier
Fishrock123 commentedon Nov 3, 2016
I don't think this is workable because of how GitHub works.
Ideally we'd be able to mark a PR as merged and point so some commits.