Skip to content

Editing pull request: Can we rebase instead of merge? #215

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

Closed
Mariatta opened this issue May 31, 2017 · 6 comments
Closed

Editing pull request: Can we rebase instead of merge? #215

Mariatta opened this issue May 31, 2017 · 6 comments
Labels

Comments

@Mariatta
Copy link
Member

Mariatta commented May 31, 2017

Regarding this section:

In step 3, is it possible to do rebase instead of merge?

When we do git merge, we could be getting a whole bunch of unrelated changes in the PR.

@methane
Copy link
Member

methane commented May 31, 2017

-1 from me.
rebase rewrites history.
If we push rewritten history to contributor's branch and the contributor has local change,
the contributor may be surprised when they do git pull.

Since we use "squash and merge" button when merging pull request,
there are no reason to use rebase.

@ncoghlan
Copy link
Contributor

ncoghlan commented May 31, 2017

It's possible to skip the merge entirely - that's just the example we used of a change someone might want to make.

It's probably better to simplify the example to:

$ git add <changes>
$ git commit -m "<commit message>"

And then add a separate note saying to only use git merge rather than git rebase if you need to bring in changes from the PR's target branch (for the reason @methane describes - we don't want to rewrite the contributor's branch history).

@serhiy-storchaka
Copy link
Member

I concur with @methane.

It's possible to skip the merge entirely - that's just the example we used of a change someone might want to make.

Unfortunately AppVeyor build failed if it can't merge the branch to master without conflicts (which are likely if Misc/NEWS is changed).

@Mariatta
Copy link
Member Author

My concern is that when a PR was created quite a long while ago, when we do a merge from a more recent origin\master, it can bring in too many unrelated changes, making it difficult to review perhaps.
But I agree with the point about not wanting to rewrite the contributor's branch history.
I'll leave the instruction as is for now.
Thanks :)

@ncoghlan
Copy link
Contributor

ncoghlan commented Jun 2, 2017

@Mariatta For those cases, it may make sense to ask the contributor if they'd mind doing a rebase before doing a merge ourselves and/or asking them if they're OK with us doing one.

The main problem we want to avoid is doing a rebase without the contributor being aware it's happening.

@methane
Copy link
Member

methane commented Jun 2, 2017

when a PR was created quite a long while ago, when we do a merge from a more recent origin\master, it can bring in too many unrelated changes, making it difficult to review perhaps.

I think Github won't show commits from master branch.
Github will show only one "merge commit".

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

No branches or pull requests

4 participants