Misc #16093: Prohibit a "foxtrot merge" instead of a merge commit

• Status: Rejected
• Assignee: -

[k0kubun (Takashi Kokubun)]

Background

• When we migrated the canonical Ruby repository from Subversion to Git [Misc #14632], in that ticket nobody had objected to allowing a merge commit in the repository.
• At first, we decided to prohibit merge commits because:
  • The first merge commit https://github.com/ruby/ruby/pull/2084 went well. Then we tried the second merge commit for https://github.com/ruby/ruby/pull/2079 but it failed.
  • We struggled to find why only the first one succeeded. That was tricky because ruby-commit-hook's pre-receive hook was updated to a new revision after the first merge happened, and the new revision included a change that accidentally made a merge commit impossible.
  • Because the merge commit made it harder to debug the issue in ruby-commit-hook, we decided to deliberately prohibit to push a merge commit to the master branch for a while to reduce the number of problems to be solved during the early stage of the migration to git. These days the ruby-commit-hook has worked stably.
• Then we also noticed that prohibiting merge commits makes it easier to efficiently list up revisions to be built by the bisect bot rubyfarmer without having a data store.
• If we do not make a merge commit, there's only one way to make a pull request "Merged" on GitHub ruby/ruby:
  • Push commits in the pull requests to the master branch when their parent revision is the same as the master branch's one.
• Therefore, we have not been able to make a pull request "Merged" when a pull request's branch needs to be rebased before pushing it to the master branch.
  • But force-pushing the rebased commits to their author's branch is a bad habit and we do not want to do so.

Problem

• Some contributors get confused when their pull request is committed to the master branch but it's marked as "Closed" on GitHub [Misc #16089].
• Even though we clarified the situation in https://bugs.ruby-lang.org/projects/ruby/wiki/HowToContribute, a first-time contributor could be confused and the person might complain about it to the committer.

Solution

1. Improve the rubyfarmer's implementation to make it work even if we had merge commits. https://github.com/mame/rubyfarmer/pull/1
2. To maintain a consistent linear history in the git log even after allowing merge commits, implement a guard to prevent a "foxtrot merge" in the pre-receive hook. https://github.com/ruby/ruby-commit-hook/pull/19

• Details: https://devblog.nestoria.com/post/98892582763/maintaining-a-consistent-linear-history-for-git

3. Fix bugs in check-email.rb to correctly check merge commits.
4. Allow pushing merge commits to the master branch.

We'll implement 2, 3, and 4 shortly. Please let me know if you have any opinion about this.

[k0kubun (Takashi Kokubun)]

• Subject changed from Disable changing the first parent instead of prohibiting a merge commit to Prohibit a "foxtrot merge" instead of a merge commit

[jeremyevans0 (Jeremy Evans)]

I prefer rebasing commits on master instead of merging to keep a true linear history, as I find it makes reviewing the history simpler. In my opinion, the "Problem" as specified in the post is not a significant issue, and the disadvantages of allowing merge commits outweigh the advantages.

However, I don't have a strong objection if other committers want to allow merge commits.

[k0kubun (Takashi Kokubun)]

Apologies, according to the chat history, it turned out that @mame (Yusuke Endoh) was not the only person opposed to merge commits. I'll update the description to reflect the facts later.

keep a true linear history, as I find it makes reviewing the history simpler.

Yeah, I understand it's a trade-off. In general, it could cause a mess of commit history. Though, while I'm not sure how you define a "true" linear history, my point is that in this particular case we'll be able to see a linear history which will never be reversed as long as we'll have the step 2 of "Solution" and if we use git log --no-merges, just like we did not have any merge commits.

[k0kubun (Takashi Kokubun)]

• Description updated (diff)

I updated the description according to my further investigation of the history. Thank you @mame (Yusuke Endoh) for helping it.
I also prepared a patch to keep a consistent linear history (step 2 of the "Solution") in https://github.com/ruby/ruby-commit-hook/pull/19.

[k0kubun (Takashi Kokubun)]

• Description updated (diff)

[k0kubun (Takashi Kokubun)]

• Description updated (diff)

[k0kubun (Takashi Kokubun)]

• Description updated (diff)

[k0kubun (Takashi Kokubun)]

• Description updated (diff)

[marcandre (Marc-Andre Lafortune)]

Another +1 for no merge commits.

Therefore, we have not been able to make a pull request "Merged" when a pull request's branch needs to be rebased before pushing it to the master branch.

I'm probably missing something, but Github allows rebasing from the UI.

But force-pushing the rebased commits to their author's branch is a bad habit and we do not want to do so.

I'm probably missing something too, but rebasing and force-push a branch used for a PR is a good habit.

[k0kubun (Takashi Kokubun)]

Thank you for your comment.

I'm probably missing something, but Github allows rebasing from the UI.

There are three situations:

1. The parent of a pull request's commits is master:

• It's the only situation which is not "when a pull request's branch needs to be rebased". Just pushing the branch to master is not rejected and it's marked as "Merged".
• We don't need to rebase this.

2. The parent of a pull request's commits is older than master, but it can be merged without a conflict:

• While it can be merged without a rebase, as you're proposing not to use a merge commit, we need to rebase it before pushing it.
• I can see only "Rebase and merge", but it's not a button to just rebase the branch. So "Github allows rebasing from the UI" can be seen as true for this too, but it forces us to push it to GitHub directly, which is not compatible with our repository usage.

3. The parent of a pull request's commits is older than master, and it causes a conflict on merge/rebase.

• It shows the "Resolve Conflicts" button and then the shows a "Commit merge" button. Maybe you intended this one, while it also seems to create a merge commit, though.

Anyway, this is not the main part of the discussion. My problem is not to rebase from the UI and I'm totally fine to do the same thing from a command line. The problem would be actually "rebase" vs "merge" for pushing it to the master branch in the case 2 or 3.

I'm probably missing something too, but rebasing and force-push a branch used for a PR is a good habit.

For using a rebase and a force-push for the case 2 or 3, I have mainly two concerns (not a strict blocker, though) in mind:

• Authenticity of the author is lost. When commits are rebased, their sha is changed and there's no guarantee that the author wrote neither a patch nor a commit message. In the case 2 or 3, we can preserve the original sha and also its GPG sign (if signed) only with a merge commit.
• Force-push to another person's branch may overwrite the person's new push by a race condition.
  • However, this one may not be so critical because these days GitHub shows "xxx force-pushed" on a pull request. We can fix such a mistake easily.

For me, the largest challenge in using merge commits is missing a linear history in the default git log. Though I believe it will be okay as long as we can virtually get such a history by protecting the master branch from reversing the history by a "foxtrot merge" and using git log --no-merges as I said above.

In case you misunderstand me, even after it's permitted, I'm personally NOT going to use merge commits for most of my own commits because there's no need to do so. This ticket is NOT recommending to always use a merge commit for our own commits either, which I believe is a separate topic. We could write "please don't use merge commits when you're not merging a branch of another person" in the CommiterHowTo.
I'm planning to modify make merge-github in the ruby repository to create a merge commit if and only if it's either the case 2 or 3, and use it only when I want to merge a pull request on GitHub, not for my own pull requests because I can rebase and GPG-sign them by myself so that git log becomes linear even without --no-merges without losing authenticity.

Given all that, can you elaborate more on the problems you want to solve by prohibiting merge commits?

[naruse (Yui NARUSE)]

I'm probably missing something, but Github allows rebasing from the UI.

GitHub UI allows "rebase and merge" but

1. Our primary repository is git.ruby-lang.org; we don't use "rebase and merge" on GitHub
2. Because of 1., we need to rebase on local machine. In this case
   2.1. GitHub cannot know a PR is merged or not
   2.2. To notice a PR is merged and should be closed, We need
   2.2.1. add extra empty commit with a commit message "close #xxx"
   2.2.2. modify PR's commit to include "close #xxx"
   2.2.3. I think oth of 2.2.1. and 2.2.2. sounds bad habit
   Therefore "Rebase and merge" strategy is not practical because of commit association.

There's another solution to solve above issue: Squash and merge.
But it looses the commit history inside the topic branch.

[k0kubun (Takashi Kokubun)]

• Related to Misc #16094: Allow only "Rebase and merge" or "Squash and merge" on GitHub master branch, and sync it on git.ruby-lang.org update hook added

[k0kubun (Takashi Kokubun)]

• Status changed from Open to Feedback

As @marcandre (Marc-Andre Lafortune) suggested, I thought of an approach to use "Rebase and merge", [Misc #16094]. In that plan, we'll be able to make a pull request "Merged" without creating a merge commit, while we continue to use git.ruby-lang.org as the Ruby's canonical Git repository.

As we have [Misc #16094], now I'm okay to close this ticket if @jeremyevans and @marcandre (Marc-Andre Lafortune) still do not like the idea to prohibit only foxtrot merges.

[jeremyevans0 (Jeremy Evans)]

k0kubun (Takashi Kokubun) wrote:

As @marcandre (Marc-Andre Lafortune) suggested, I thought of an approach to use "Rebase and merge", [Misc #16094]. In that plan, we'll be able to make a pull request "Merged" without creating a merge commit, while we continue to use git.ruby-lang.org as the Ruby's canonical Git repository.

As we have [Misc #16094], now I'm okay to close this ticket if @jeremyevans and @marcandre (Marc-Andre Lafortune) still do not like the idea to prohibit only foxtrot merges.

I think the allowing the "Rebase and merge" approach is a better solution to the problem than allowing merge commits, so I'm OK to close this.

[marcandre (Marc-Andre Lafortune)]

Looks good to me. If I'm not mistaken, neither "Rebase and merge" nor "Squash and merge" create merge commits, they are just fast forward merges, so both could be choices acceptable in the UI.

[k0kubun (Takashi Kokubun)]

• Status changed from Feedback to Rejected