Rebase pull request before merge into master

Everything about development and the OpenMW source code.
Post Reply
elsid
Posts: 20
Joined: 01 Aug 2017, 08:20

Rebase pull request before merge into master

Post by elsid »

I was trying to find a pull request that was merged with https://github.com/OpenMW/openmw/commit ... 292R91-R92 change to see the review comments. So far I wasn't able to do this. I did following:
1. I try to check github web interface. Usually it show the pr where this change is present.
Image
2. I try to go by this link:
Image
Thanks github, but it's a wrong repo...
3. I try to go to gitlab:
Image
Well, nothing...
4. I try to use git:

Code: Select all

git log --graph --format=tformat:'%C(yellow)%h %C(cyan)%aN %C(blue bold)%ar%Creset%Cred%d%Creset %s'
Image
I wan't able to track this line until it merges master...
5. How about more specific tool?
https://stackoverflow.com/questions/847 ... fic-commit

Code: Select all

$ pip3 install --user git-get-merge
$ git-get-merge 1b8d500c0
Might be either of:
commit 31c68c534ca62493d08a3bc6d8e02178eceb4dc9
Merge: c3d7ee5a9 24078d4a7
Author: Miloslav Číž <[email protected]>
Date:   Thu Jun 14 13:23:23 2018 +0200

    Merge branch 'drummyfish/openmw-toggleborders'

commit 5a9e382efe04960e1bed3a309c81ca3d7fc7a1b4
Merge: ccfc07e7e 2a23b5351
Author: Marc Zinnschlag <[email protected]>
Date:   Fri Jun 15 12:23:08 2018 +0200

    Merged pull request #1421
Neither of them show the pr.
6. And the last one try:

Code: Select all

$ git rev-list 1b8d500c0..master --ancestry-path --format=tformat:'%h %aN %s' > ancestry-path.log
$ git rev-list 1b8d500c0..master --first-parent --format=tformat:'%h %aN %s' > first-parent.log
$ diff ancestry-path.log first-parent.log | wc -l
4284
I dont' want to read 4k diff...

So my proposal is to rebase pull requests (merge requests) before merge into master. If pr doesn't have conflicts and has 1-5 commits probably it's fine to merge as is. But if pr contains commits like merge from master and has a several month lifetime then just to make the commits history more clear it should be rebased right before the merge.

For now my only hypothesis about that commit is it was pushed without pr right into master. I'd appreciate if someone will find how did it end up in master. That's another concern. We shouldn't push commits right to the master without pr. I see sometimes such changes. Github allows to forbid such pushes for all users or except admins. We need to sync with gitlab, so I assume admins should be allowed to do this.
User avatar
AnyOldName3
Posts: 2666
Joined: 26 Nov 2015, 03:25

Re: Rebase pull request before merge into master

Post by AnyOldName3 »

I'm generally against rebasing for long-running PRs. If it's something really small that was forgotten about for months, so can be rebased as a single commit, that's fine, but it's an operation that rewrites history, and doing it for something like my shadows PR that needed to adapt to several cases of master breaking something would change the history from working -> merge-with-master -> fix-for-new-changes -> working to obviously-broken -> thing-that-fixes-obvious-problem -> working, meaning all the intermediate commits wouldn't work and potentially wouldn't build.
Post Reply