Node: Push rebased PRs before merging

[strugee]

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.

[mscdex]

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]

This was added to the merging PRs guide in https://github.com/nodejs/node/pull/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]

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.

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 [email protected]:somebody/node.git somebranch.

[Fishrock123]

This only works if PR authors have made their PR branch available for us to push to...

[sam-github]

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]

Which they mostly won't.

Why not ? The option is checked by default.

[sam-github]

That's surprising, the github docs say otherwise:

If you would like to allow anyone with push access to the upstream repository to make changes to your PR, then select Allow edits from maintainers.

• https://help.github.com/articles/creating-a-pull-request-from-a-fork/

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]

@sam-github there are currently two options:

• squash all with a custom message
• rebase all and apply

Unfortunately that doesn't fully cover our needs but can cover simple cases.

[Fishrock123]

(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]

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 [email protected]
wrote:

(Note: I think it appends to the commit message, so it may make an invalid
commit message. Try it on your own repos FIRST!)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/9391#issuecomment-257601072, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecVwEJ0oIElAysof2uZn9IWd19VMI3ks5q512qgaJpZM4Klwr_
.

[gibfahn]

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

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]

Depends on the collaborator, see https://github.com/nodejs/node/pull/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]

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 [email protected] wrote:

Depends on the collaborator, see #9257
https://github.com/nodejs/node/pull/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.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/9391#issuecomment-257661683, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecVyiuH4tzP4ld4aS8t_AvVXO2qcHSks5q54y0gaJpZM4Klwr_
.

[strugee]

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

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

-1 for me. I think the "Landed in ..." comments should be required. It sure makes cutting releases easier

[Fishrock123]

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.