Suggestion: Enable "Rebase + Merge" in GitHub

[filbranden]

It's a slight variation on the "Squash + Merge" feature, but it's more useful for when there are multiple commits in a single PR...

Here are instructions on how to enable it:
https://help.github.com/articles/configuring-commit-rebasing-for-pull-requests/

Needs to be done by an owner of the project.

[keszybz]

We never use it, so I'm happy that it is disabled.

[filbranden]

@keszybz Do you mean "we like to use git merges and don't mind a very non-linear history" (in which case you might want to consider disallowing currently allowed squashes as well) or "when we want to keep a linear history, we just want to squash every PR into a single commit"?

I see many PRs go by with many individual commits, so I think the latter is unlikely...

I believe I've also seen "Squash + Merge" used before, perhaps even when there's a single commit, to avoid getting more PR Merge commits in the history and try to keep it slightly more linear... (But maybe I'm mistaken here?)

[keszybz]

We use "squash & merge" for PRs with a single commit (and in the rare case where there are multiple commits which should be just one logically), and "merge" for PRs with at least two commits.

Unfortunately the github UI is confusing, since the type of merge is sticky, and becomes the default for subsequent merges, hence there are occasional mistakes, in particular merges for with a single commit.

[poettering]

@keszybz Do you mean "we like to use git merges and don't mind a very non-linear history" (in which case you might want to consider disallowing currently allowed squashes as well) or "when we want to keep a linear history, we just want to squash every PR into a single commit"?

We use classic merge commits as a way to group the commits that make up a PR. If we'd permit rebase+merge commits we'd lose that kind of grouping.

We do like linear history though, but we like logical grouping of PRs more. That's why we tend to squash-merge single-commit PRs, since if your PR is a single commit only, then there's no point in grouping.

Hence, if you see a PR and want to merge it, check if it's one commit or more than one. If it's one hit "squash and merge" and otherwise use a regular merge.

[filbranden]

group the commits that make up a PR

That's a fair point... Though many times I see PRs go by with many unrelated commits and the PR description is saying something like "minor fixes" or "assorted changes" or similar...

I'm not 100% sure about how GitHub behaves on Rebase + Merge, I know that in a Squash + Merge it does keep a reference to the PR number when looking at the commit in the GitHub browser. (For example, I Squash + Merge'd 7511655 this morning and following the link you'll see there's a master (#8633) reference to the PR.) I imagine Rebase + Merge would behave the same. (But, admittedly, haven't tested that.)

Also, by "allowing" it, you're not forcing anyone to use it... The default is still a straight Merge. So you'd just give the choice in case it looks appropriate (which it might for those "assorted fixes" PRs...)

Anyways, fine with me if you close it... It was just a suggestion.

[poettering]

You do have a point, if we do PRs that consist of multiple unrelated then rebase+merge makes sense. but then again, maybe the lesson would be not to do such PRs ;-). I mean, I do them every now and then because I am lazy, but @keszybz doesn't like that I do that, I think, and I fully understand why and even agree ;-).

Note that the CI will test the PR as a whole though, hence even if the commits that make up a PR are logically independent, they are CI tested as one single unit, hence keeping the grouping in place for them probably makes some sense still on that level...

Note that the "Squash + Merge" feature of github actually sucks badly: it rewrites the commit a bit too much: it will fill in the author of the PR as the author of the single resulting commit, which very very often is just a bad idea, because one one hand sometimes people post PRs containing commits from other folks, and secondly because there appears to be a theme where people stick to weird usernames in github, even though they use realnames in the commits themselves, and we thus lose the realnames...

I wonder if "Rebase + Merge" would actually behave better in that case, i.e. whether we should change our rule of "Squash + Merge on single-commit PRs" to "Rebase + Merge on single-commit PRs". Maybe github wouldn't rewrite the authors then?

[filbranden]

Note that the "Squash + Merge" feature of github actually sucks badly: it rewrites the commit a bit too much: it will fill in the author of the PR as the author of the single resulting commit, ...

Yes, I've seen that happen a short while ago (and how the author of the PR, sending us one of his colleague's git commits, wasn't completely happy about that...)

I wonder if "Rebase + Merge" would actually behave better in that case, ...

Yes, that was indeed my thought when I squashed/merged one this week, I was thinking that I'd feel "safer" with Rebase since I'd expect it would do "less" to my commits. (I was kind of worried about the commit message, but saw that GitHub preserved the original one of the commit, not the PR, and even gave me the ability to edit it before the push.)

I can try to do some tests around Rebase + Merge. When I have some time (but it's getting super busy right now) I'll try to do that and report back.

But, really, my point with this Issue was that just enabling this shouldn't hurt. If we enable it and never use it, no harm done. If we enable it and regret it, we can disable it again. Really, it shouldn't be that big of a deal! :-)

[filbranden]

So we had another example where squash clobbered the author of a commit and rebase would have been helpful.

More specifically, PR #8874 which @evverx authored and sent to the mailing list, but I sent the PR (since he was having trouble with his GitHub access), was then merged into a605e46 which looks authored by me.

So I went on and did some tests between Merge/Squash/Rebase to see how they worked. I ended up doing all tests with a PR with a single commit (the one from #8874, just rebased it on top of 30 commits before it) and then I merged/squashed/rebased it on top of a605e46~ in different branches.

Results are here:

• Merge:
  https://github.com/filbranden/systemd/commits/test8665_filbranden_merge
  (PR Test Merge filbranden/systemd#1)
• Squash:
  https://github.com/filbranden/systemd/commits/test8665_filbranden_squash
  (PR Test Squash filbranden/systemd#2)
• Rebase:
  https://github.com/filbranden/systemd/commits/test8665_filbranden_rebase
  (PR Test Rebase filbranden/systemd#3)

I guess we don't need to discuss "Merge", since it's the default and everyone kind of knows how it works...

Squash clobbered the author, as we expected. The commit description was updated to include the PR number (#2) at the end of the first line.

The "Rebase" one looks very similar to the "Squash" one, except that the commit description is pristine (the PR number was not added) and the author information was not touched (the commit date was not touched either.)

One concern that was raised was mapping commits back to PRs. That information is actually not lost. Looking at that commit (at filbranden@16a5f1e), below the commit description, there's a small section that shows test8665_filbranden_rebase (#3), which is the destination branch and the PR number. the (#3) links back to filbranden#3 which is the PR that merged (/rebased) it.

So, frankly, my impression is that "Rebase" is superior to "Squash" in about every way. Probably the only case where we would want to use "Squash" is when the PR author is sending incremental fixes and we'd really want to squash it all at merge time. (Though usually I'd prefer asking the author to do so.)

(An aside, but I'd actually prefer using "Rebase" instead of "Merge", I think there are benefits in having a linear history and not having tons of merge commits around... As pointed out, we can always trace the commits back to the PRs for access to the discussion, etc.!)

I also prepared some extra test PRs, in case you would like to test this for yourselves. You might need to accept my invitation to collaborate to my forked repository, then you can look at PRs filbranden#4 (Merge), filbranden#5 (Squash) and filbranden#6 (Rebase) and do the operations for yourselves, then look at the branches and commits, trace the rebased commits back to the PR that merged them, etc.

Let me know if you have any questions. I'm happy to do further experiments, though I really think we should just go ahead and enable this, start using it and disable back if for some reason we're not satisfied...

Cheers!
Filipe

[keszybz]

I enabled rebase&merge now. So the new rules are:

• single commit → rebase
• a bunch of fixup commits that should be one → squash
• more than one commit → merge

Let's see how that goes.

[yuwata]

Hmm. I merged PR #8951 by 'Rebase and merge', then the PR number is not added to the commit message, see b6887d7.

So, I do not think we should use 'Rebase and merge' except the case @filbranden explains:

More specifically, PR #8874 which @evverx authored and sent to the mailing list, but I sent the PR (since he was having trouble with his GitHub access), was then merged into a605e46 which looks authored by me.

[filbranden]

It's not in the commit description (which is not touched by GitHub, and I'd argue that's how it should be!) but you can find it from the GitHub page showing the commit:

[yuwata]

@filbranden Yeah, I noticed that. But, it is hard to (or cannot?) find the PR number from git log.

[filbranden]

Yes, agreed, it's harder from git log... (Though, in a way, that's also somewhat the case for actual "merge" commits as well.)

I guess if you want to look at the PR discussion, you're opening the GitHub website anyways, so looking up the commit there to find the PR is not really that much more trouble...

I imagine that's also exposed in the GitHub API, so technically it should be possible to write a helper to do that mapping too.

[yuwata]

I guess if you want to look at the PR discussion, you're opening the GitHub website anyways, so looking up the commit there to find the PR is not really that much more trouble...

Indeed. OK. Let's close again. Thank you for the comments.

[evverx]

I'm glad that my slip-up has apparently helped to learn something new. Thank you!