Rebase and merge pull requests | Hacker News Reader

Rebase and merge pull requests

github.com

181 pointsdwaxe95 comments

This is great, but as chrisseaton asked on Twitter [1], how does it interact with CI?

The trees in the rebased commits are new and may never have been tested. Is GitHub going to expose a `refs/pulls/123/rebase` ref (cf. the existing `refs/pulls/123/merge`) for running through CI before the button can be pressed?

EDIT: The docs [2] say:

  You aren't able to automatically rebase and merge on GitHub when:
  
  * Your pull request has merge conflicts.
  * Rebasing the commits from the base branch into the head branch
    runs into conflicts.
  * Rebasing your commits is considered "unsafe", such as when a
    rebase is possible without merge conflicts but would produce a
    different result than a merge would.

So I guess the point is moot. You can’t rebase a branch unless the resulting tree is identical to the one you’d get from a merge, and that’s the tree that runs through CI.

[1] https://twitter.com/ChrisGSeaton/status/780441251992731648

[2] https://help.github.com/articles/about-pull-request-merges/#...

I had the same reaction: why isn't there a "rebase now" button, with a separate "merge" button you can click after CI etc. runs?

I'm surprised there is no "commit tree" drawing to illustrate this.

Basically it adds the commits to the master branch without a merge commit:

e.g.:

   master: A - B - C
   feature: D - E
   master (after merge): A - B - C - D - E

See: https://help.github.com/articles/about-pull-request-merges

Merging with --no-ff the just-rebased branch would've had the effect of adding a merge commit detailing what branch was indeed merged then.

Personally I like the combined approach where the feature is first rebased (which I think should be done if possible because then it guarantees a clean commit with no conflicts) and then merged e.g.

   master:      A - B - C
   feature:     D - E
   post-master: A - B - C --------- F
                         \- D - E -/

So in the view of the master the entire branch is one clean commit, but the branch history is still preserved.

That would be more useful, as git log tools tend not to deal fantastically well with concurrently running branch, especially with interleaved side-branches (fork branch 1, fork branch 2, merge branch 1, merge branch 2, history looks like garbage in most Git tools)

You can get a lot better results with `git log -m --first-parent`, which effectively shows you only the merge commits, and shows all changes that were merged in as if they belonged to the merge commit.

I have a `git log0` alias for this, as well as `git blame0` and `git show0`.

> You can get a lot better results with `git log -m --first-parent`

Yes, however that only works in the CLI, not in any Git GUI I know of (though GTK Bazaar log would pretty much do that, folding merge commits & following the mainline by default).

I don't understand why you want to rebase in this scenario. All you're doing is throwing away information (the point where the branch diverged), but you're not recovering anything or making the history any "cleaner". Not only this, but you're introducing a potential for error, because a branch that merges cleanly may not necessarily rebase cleanly (e.g. if the branch made a change that would conflict, but then backed out that change in a subsequent commit).

Well this is a great addition! I've worked in places where `squash and merge` was standard procedure and others (including currently) where `rebase and merge` was the standard.

It's nice to see the later finally getting some love so I don't have to keep going back to my own terminal after my PR is approved.

Best part I think is that it's possible to make this the only allowed merge strategy for a repo.

> so I don't have to keep going back to my own terminal after my PR is approved.

Why would you have to go back to your terminal after the PR is approved? In either case, the merges are done by the admin..

This would be true if we had one person responsible for the merge! We have a small team of developers and proper rebase and merge is the responsibility of the PR author after it has passed code review + testing.

In short: all devs are admins.

edit: anyway this would still be a huge help to the sole admin if we had one.

Because sometimes the admin just tells you "ok, rebase then I will merge" and don't bother doing the rebase himself...

If there is a conflict during the rebase, the developer working on the new feature will have more information on how to fix it. So it makes sense for the developer to rebase before submitting.

Why would you have a separate "admin" do that? It doesn't require a separate skillset; the developer is in the best position to shepherd their feature into production, and by giving them ownership of their feature they can stick closer to solving the actual problem, and likely get more satisfaction from their work as well.

> others (including currently) where `rebase and merge` was the standard.

Isn't the usual standard "rebase && merge --no-ff?" Yielding a merge commit but lower branch interleaving in the logs? This is going to dump all the branch's commit into the mainline.

Some projects (like Chromium) squash all of the contributor's commits into a single commit representing their entire local branch history (it was local, no one needs to see their "aw shit I broke the tests" commit), and then rebases that commit and lands it directly on top of master.

Github already supported that with the second merge method they introduced ("squash").

I wonder if this preserves the link to the PR the commits were issued from - I'd hate to lose the ability to reference code review comments for a commit.

> Rebases automatically set the committer of the rebased commits to the current user, while keeping authorship information intact. The pull request's branch will not be modified by this operation.

What does "keeping authorship information intact" mean? It says it resets the committer - when I'm going through `git log` or `git blame` output, doesn't this mean that I won't actually see the correct author? I don't want to have to dig through commit messages to get an accurate understanding of who wrote what.

Committer and author are tracked separately, and by default you only see the author. Try: git log --pretty=fuller

Ah, I didn't know that. Thanks!

This is great. On the other hand, no one mentioned Gitlab here yet as squash merge remains as an ee-only (paid-only) feature in Gitlab all this long.

https://gitlab.com/gitlab-org/gitlab-ee/issues/150 https://gitlab.com/gitlab-org/gitlab-ce/issues/4106

I'm sure it will enter CE fairly soon now. The Gitlab vs GitHub competition is good for all of us.

Merge and rebase for pull requests might soon land in RhodeCode too.

The inability to do rebase in CE is my biggest pain point with Gitlab right now.

Yay now it's only missing rebase then merge with --no-ff to be useful to me!

Your use case is super weird. If you're going to do a --no-ff merge why would you rebase?

My guess is it keeps the history as linear as possible while still being able to undo the merge if necessary.

Precisely correct, rebasing prior to merging ensures the branch about to be merged has no conflicts with the one being merged to, or that conflicts have been fixed in the branch prior to the merge… and merging with --no-ff ensures that the history of the branch gets kept "visually" when doing Git archaeology, as well as that the merge was "wanted"

> […] ensures the branch about to be merged has no conflicts with the one being merged to

So does merging.

> or that conflicts have been fixed in the branch prior to the merge

Conflicts that show up in rebasing may not necessarily show up during a merge. Having to fix a bunch of conflicts that only occur because you rebased is tedious and introduces a source of potential error.

It is useful for when you want the commits to be clearly grouped in the log (because they belong together conceptually), but you don't want to squash them all together (because, for example, you still want to be able to browse/bisect/revert some of those commits in the future). Also, it is easy to revert the merge commit if you want to undo the whole group at once.

> It is useful for when you want the commits to be clearly grouped in the log (because they belong together conceptually)

Surely there must be a way to tell git to show history in that form even with branches that haven't been rebased?

Tell git? sure. Tell git CLI tools? …

It's easy. Use the --topo-order flag. This makes it show commits in topological order, which should group all commits on the branch together.

I guess it could be useful for putting conflict fixes in the commits themselves rather than all in a lump in the merge commit. And it could also help have useful commits for bisecting.

From an aesthetic perspective, rebasing and ff merging also destroys the history of what commit is related to what work branch.

It is to have a single merge commit which is simpler to revert.

It also stored metadata about the branch (its name) so that you can recover and recreate it if necessary: imagine if you found out that two branches had been merged in the wrong order, and the refs had been deleted. With rebase && --no-ff, the merge commit literally tells you the ref that was attached to the second parent when the merge occurred.

After being initially skeptical I was convinced by a colleague that this is a good model.

The merge commit acts as a kind of "pushlog"; it tells you what actually landed together as a single unit. It probably also tells you what passed CI; although many projects state that you shouldn't have individual commits that don't pass CI it's rare that this is enforced below the level of the PR. That should be good for bisection. Because the commits are always rebased onto master (and of course you never allow merge commits other than from code being integrated into master) your history is relatively clean, you don't get multiple overlapping branches, but something like

  M1 ------- M2 -------- M3
   \ B1 - B2 / \ C1 - C2 /

Because the merges are --ff-only you don't get the confusing situation where the merge commits themselves contain changes.

In principle this seems like the ideal way to use the tools git provides. However I understand there are some minor rough edges e.g. with magic needed to tell git bisect how to only try the merge commits.

Yeah, looking at the commit history tree visually convinced me.

Then was convinced even more after having to revert a ff merge.

> your history is relatively clean, you don't get multiple overlapping branches

Right, but what's the benefit of that? You still need to understand a history with merges in, in which case it seems like you might as well get the safety advantages of not rebasing.

what are the safety advantages?

Rebasing tends to induce more conflicts than merging, tends to lead to force-pushing which is often dangerous ( "--force-with-lease" is safer but a lot longer than "-f" ), and has to be done after PR review which bypasses one set of checks particularly in the case where conflicts have to be fixed.

presumably github will only let you rebase-merge if there are no conflicts. In my experience though rebase tends to have more conflicts requiring manual intervention vs merge, but that feels safer to me than merge occasionally automatically resolving the conflict incorrectly.

Github also has protection to prevent branches from being forced, if you are only forcing a feature branch with no collaborators, or at the end of collaboration, it's not so bad, especially when git tells you the old hash you just overwrote and you have the reflog, but again github's rebase-then-merge should make that a non issue.

I really like this workflow, but my major grip is that the autogenerated merge commits are terribly ugly and uninformative. I'm sure you can override them, but no one seems to every do it. The workflow is awesome at visually segregating your logically separate pieces of work, but you never actually end up giving it a proper label like "Added feature" (which took a refactor and 10 commits)

I came here to say the same thing. The rebase option is awesome IMO because it causes git to show the commits in the chronological position of when they were added to master rather than jumbled in back to when the author originally started working on it. But we still want merge commits to make it easy to find the pull request that introduced the change.

This strikes me as perhaps the worst merge method possible for git. Sometimes people call your main branch soup, and this will make that an incredibly accurate description.

People are bizarrely terrified of merge commits.

Then don't use it. I'd been happy to just have an option to support fast forward merges without merge commits, but this really nails my preferred work flow.

> support fast forward merges without merge commits

Isn't that essentially what this is? It's a rebase followed by a fast-forwarded merge, there should be no merge commit.

Well, what I'm saying is that I'd be happy even if I had to rebase manually in the case that master had moved on since the pull request had been created.

Indeed they are and you're going to get merge commits when you're continuing work on a branch and you're using branches rather than having people use separate repos (which is the superior choice).

I'm amazed at how many companies allow branches rather than allowing repo forks.

I, too used to wonder about this. But in git merge commits basically break bisect and revert. You can't tell git to only consider feature merges for it's bisection (which is normally what you want), nor will revert on a merge commit behave the way people expect. And not everyone enjoys reading long essays about to work around these problems.

Could you elaborate? I find it hard to believe that merge commits break bisect, since git bisect was developed for use by the Linux kernel developers, and the Linux kernel repository uses merge commits heavily.

As an advocate for both merges and bisect, even I have to admit that the experience of using bisect over merges can be quite terrible. I would prefer if bisect had a working --first-parent mode, because most of the time that's what I really want. Git-core seems against this, though. Not sure if it's on technical or aesthetic grounds.

Let's assume you found a serious bug in production. Probably one of the recently landed feature branches contains the offending change. If you rebase feature branches into master as a single squashed commit, things are straightforward: you run a git bisect HEAD $LAST_KNOWN_GOOD and then you might revert the offending commit and re-deploy before you dig into the problem deeper and fix the bug.

If you have a normal merge based workflow, your bisect will likely require some babysitting because git doesn't offer an easy way to just bisect your merge commits into master. Firstly of course there will also be many more commits to consider, most of which you don't really care about at this point and which will slow down the bisect. More importantly some of the intermediate commits from the feature branches will almost certainly be broken, because typically not every single commit even goes through CI. You can work around that by hacking something up to ignore these commits but it's probably gonna be a pain.

Once you found the problematic feature you want to back out you then will spend half an hour googling how to revert merge commits.

Of course, there are advantages to having the "intermediate" commits available and if git bisect were better designed the experience with merge branches should be strictly better, but in practice it's enough of a pain that people who semantically prefer merges opt for a rebase-based workflow.

> More importantly some of the intermediate commits from the feature branches will almost certainly be broken, because typically not every single commit even goes through CI. You can work around that by hacking something up to ignore these commits but it's probably gonna be a pain.

git bisect skip. Also, if your maintainers aren't doing a `git rebase -x "make test" origin/master` before merging then you're the ones to blame. Not merge commits.

> Once you found the problematic feature you want to back out you then will spend half an hour googling how to revert merge commits.

`git revert <merge commit>`. Or if by "revert" you mean "change the history to the state it used to be in" you can do `git reset --hard <old HEAD using git log --first-parent>`.

> Of course, there are advantages to having the "intermediate" commits available and if git bisect were better designed the experience with merge branches should be strictly better, but in practice it's enough of a pain that people who semantically prefer merges opt for a rebase-based workflow.

git bisect is actually quite well designed considering it works given any two commits (even if the histories are different). To be fair, it doesn't handle histories where a bug is fixed and broken multiple times -- but there's no nice way of dealing with that (it's a hard problem to solve without iterating through each commit).

Merge commits don't break bisect or revert. You may revert to a DAG node that develop (or whatever your source branch) never pointed to, but that's just the reality of developing on branches. Bisect works over a linearization of the DAG, and may similarly test intermediate states that don't make sense. But again, it just reflects the reality of a team of developers working concurrently, sometimes on the same branch, sometimes on different branches.

Merge commits don't break bisect. People who submit pull requests that don't build, run, and pass tests at every commit break bisect. Rebasing doesn't fix that problem.

Don't build a series of commits where a later one fixes an earlier one; organize them into a logical series of changes, such that the project works after each commit.

That's too much work to guard against a rare occurrence. Most junior engineers would think that that workflow is excessively fussy and belt-and-braces (because they haven't seen how useful bisect is).

I'd love to see some of the major CI systems (like Travis, Appveyor, and Gitlab CI) supporting "build and test every commit" rather than "build and test the final commit".

Agreed, although to do this at acceptable cost you'll often also want a decent build system (a la blaze, that can rebuild and re-test just what needs rebuilding and re-testing) and a lot of CI slaves. That's not a typical setup.

Right now build systems don't even do the "build and test the final commit" bit right; it should of course test against a pre-merge (not on the branch) but again that tends to be a pain to set up.

In practice rebase heavy-workflows squash a lot of commits away; in fact I think the standard workflow with phabricator is sqashing everything in the feature branch into a single commit and then rebasing that in. This has obvious downsides, but it means it's easy to revert and bisect works, which probably does in many cases outweight the downsides.

Squashing everything is better than applying a series of patches that don't bisect, though it's not quite as nice as providing one logical change per commit.

Fully agreed. The problem is that one CI-tested logical change per commit tends to be too painful to set up. I think the attractions of rebase-based workflows are mostly due to tooling limitations (in particular shortcomings of git and CI systems).

It is pretty good for single-commit branches, though the previous squash commit already handled that case fine.

Then please don't use it. In the mean time, here's how a flat tree looks to contributors:

https://github.com/jleclanche/fireplace/commits/master

Versus a merge tree:

https://github.com/hashicorp/terraform/commits/master

Consider that the git log is one of the first thing contributors will look at. The cleaner it is, the better the experience.

I've been waiting for this for a long time. Thank you github!

The merge tree looks much more useful me; links to the PR with discussion is a lot nicer in a collaborative team than a series of commits by individuals.

What if your workflow primarily isn't PR-oriented? eg you have discussions on a different channel than Github.

Point is, it's good to have both. And personally, I find terraform's git log unreadable and unusable.

I still think it's better to have a logical grouping of related work. I prefer merge commits, but even squash merges at least achieve this as well.

I find terraform's git log to be more readable. You can get a quick overview for which commits corresponds to which feature by following the first-parent chain.

This is why people want to rebase.

Now we could use some UI improvements to the github commits page to make the merge method not look so ugly. Lets say: collapse merge commits into a single gray line and roll up the commits made on the branch(es) underneath the merge commit line, maybe indented.

I don't believe the problems with visualization of merge commits are insurmountable. Nearly all the command line tools (other than, really notably, bisect) allow a --first-parent flag that makes navigating them much simpler. Even gitk supports this mode, so doing it in a gui is also possible.

In terms of the preservation of useful information, I vastly prefer the merge tree. And using merge trees appropriately and never using merge trees at all are both matters of discipline.

I think both approaches are fine to be honest. I usually go for a merge tree because I don't want to bother with rebase, and whether it's a flat tree or a merge tree, I can usually find whatever I'm looking for in the log.

Whether it's faster to search for something in a flat tree or not remained to be proven. It feels like it's mostly a matter of personal taste.

You should be pushing for GitHub to improve their commit display instead of celebrating them adding something that lets you contort your history to work around it.

Right, because GitHub is obviously the only tool I, or any of my contributors, use that has a git history. (Yes, I know about whatever command line flags you're suddenly feeling the urge to tell me about)

Edit: And for fucks sake can't I be happy GitHub implemented an outstanding feature request I've personally been asking for years? The cynicism from some of the people on this site really gets to me sometimes. Stop.

Cynicism? It's not cynicism. It's the fact that a lot of people, including me, think rebase-before-merge is an antipattern and should be discouraged.

"Complaining about features that people ask for getting implemented" is a far worse antipattern.

It seems you don't actually understand the complaint, and are resorting to a schoolyard "no you" tactic to try and deflect.

The problem is that this merge option is considered by many to be actively harmful, and GitHub adding support for this is basically them tacitly saying that this is a perfectly fine merge method to use. This will almost certainly cause an increase in the number of people using this merge method, which is a shame as it really should be discouraged instead of encouraged.

The other problem is the main argument in favor of this merge method is the fact that GitHub displays non-linear commit histories terribly. That should not be an argument in favor of having a poor merging strategy, that should be an argument in favor of GitHub revamping their commit history display to actually be useful.

I fully understand the complaint. I also understand that this is not up to you to decide.

It is not up to you to decide other people's and teams' workflows. It is not up to you to decide what is or isn't harmful in the context of another team. And it is certainly not up to you to decide that GitHub shouldn't provide features their paying customers are asking for, because of concerns of a completely different context and workflow.

I appreciate that you are more than familiar with git, but that doesn't give you free reign to decide what is and isn't an antipattern for other people. In fact, you should understand far better than the average HN user just how absurd your reasoning is if you'd step back from it a little.

What you call antipattern, a lot of people call feature. If GitHub now clashes with you on a philosophical level, consider moving to GitLab (oh, wait, GL has supported this and far worse for a far longer time).

The developers using rebase flows do not affect your life. They do not affect your work. They, and their "antipatterns", are as harmful to you as an iOS user is to an Android one. And your merge flow does not affect me. What does affect me is reading constant unwarranted negativity on Hacker News, a community where my expectations have so far been above those of reddit's unending stream of cynicism. And it bugs me far, far more that this has been your reaction to a company listening to its customer base, than you or I could ever care about deciding what flow is or isn't an antipattern.

Bloody hell.

It sounds to me like you're saying that nobody can criticize anything, which is complete bullshit. Yes I absolutely can declare that something is an antipattern. You're free to disagree with me, but that doesn't mean I can't express my opinion, or that I can't be disappointed when a company like GitHub tacitly endorses the antipattern instead of improving their tooling to eliminate the main reason why people reach for that antipattern.

> The developers using rebase flows do not affect your life.

Yes they do. If they didn't affect my life, I wouldn't care. But every time I have to deal with an open-source project that adopts this workflow, it affects me. Every time I end up working on a project that's adopted this workflow, it affects me. And it affects me in more subtle ways too, such as tooling being geared around a largely-linear history instead of a history with lots of merges (e.g. GitHub's commit history display).

> And it bugs me far, far more that this has been your reaction to a company listening to its customer base

They're barely listening. There's a long list of far more valuable changes they could make, that they're not doing, because they seem to not really care. It took an extremely long time, a very high-profile blog post calling them out, and competition from GitLab for them to finally start improving the core of their product (pull requests), and they're still missing a lot of useful functionality (e.g. rebase tracking). And of course they haven't even touched the commit history display, even though, as has been mentioned multiple times, it really sucks for a non-linear history. In fact, most of their product hasn't improved in a long time.

> Rebases automatically set the committer of the rebased commits to the current user, while keeping authorship information intact

I don't understand this. The commits are rewritten - do they overwrite the author to the current user, or don't they? Or is there a distinction between "author" and "committer" I'm not aware of? Which one does blame use?

Here are some resources for you:

Difference between author and committer in Git? -- http://stackoverflow.com/questions/11856983/why-git-authorda...

Why git AuthorDate is different from CommitDate? -- http://stackoverflow.com/questions/11856983/why-git-authorda...

What does “authored 7 days ago; committed 14 hours ago” mean on GitHub? -- http://webapps.stackexchange.com/questions/70383/what-does-a...

I found all of that by googling "author vs committer git".

The gist is that, yes, there is a distinction. If your friend, say, gives you a diff and you apply it with `git apply`, you're the committer and your friend is the author.

EDIT:

Regarding your other questions ("Which one does blame use?"):

If you run

    $ man git-blame

This is right at the top:

    NAME
           git-blame - Show what revision and author last modified each line of a file

Can someone explain the benefit of this? vs. "squash and merge"?

If you want to keep the individual commits rather than squash the into a single commit. This is useful if the author of the PR actually wrote good commits that you don't want to make into a single commit. I'm also thinking of the case where a squash could turn a series of commits into a single commit with a massive amount of changes that is harder to grok when going back historically.

So it's like the old "merge", but doesn't create another commit when you close the PR?

Yes -- a merge merges one branch into another, and creates a merge commit while doing so.

A rebase rewrites history, as if the PR had been written on master directly. There is no merge commit and no evidence of a previous branch. Some people prefer rebasing over merging because it gives you a linear commit history.

This is not correct. What you're describing is two different styles of merging: fast-forward and no fast-forward, as indicated by the `--no-ff` and `--ff-only` flags provided to `git-merge`

`git-rebase` will rewrite the history of the current branch. In this case, github will rewrite the history of the branch that you would like to merge such that the base commit (hence the name "rebase") or the commit that was branched off of is the latest commit on the default branch (usually master)

Then when you merge it, you'll get a merge commit or not depending on whether or not you do a fast-forward merge

I personally think that commit history should be kept as long as it makes sense. But in some cases, you don't want to keep some of the commit messages if they don't add anything to development flow.

For example,

- Force SSL flag when connecting to server

- Use new syntax for SSL flag

- Wrap flag implementation to its own method

- Add forgotten tests

This branch's main goal was to use ssl flag on external requests but ended up having several commits that does not really matter to other developers. Then you want to squash this branch into a commit and merge.

Rebase and merge example,

- Improve sorting by adding cache

- Implement a separate config class to fetch caching times.

- Modify search implementation to use cache config class

These are rather important commits by themselves and better keep them separated in your history.

> These are rather important commits by themselves and better keep them separated in your history.

Which you could already do with a regular merge. Just because the commits are fine as part of a branch doesn't mean you want to keep them as part of mainline.

> Just because the commits are fine as part of a branch doesn't mean you want to keep them as part of mainline.

Can you elaborate on this? I would be happy to learn more about it.

Parent is missing the point that you were making, which is the benefit of not squashing vs. squashing.

What masklinn means is that when you just merge a PR, the history is retained as-is (same as with rebase). But instead of the history being flat, it's bumpy.

> Parent is missing the point that you were making, which is the benefit of not squashing vs. squashing.

I'm not missing any point, I'm saying you already get that benefit with the original merge strategy.

Neat. They've been adding a lot of good stuff recently.

I really wish there was a fourth option: "Squash and Create a Merge Commit." It's nice to have a single commit per feature, but also see when it was merged into master and by who.

Reply on news.ycombinator.com