From: takashikkbn@... Date: 2019-08-10T03:52:44+00:00 Subject: [ruby-core:94240] [Ruby master Misc#16093] Prohibit a "foxtrot merge" instead of a merge commit Issue #16093 has been updated by 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 make a merge commit and push it to GitHub, which you may not like and 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 committing 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? ---------------------------------------- Misc #16093: Prohibit a "foxtrot merge" instead of a merge commit https://bugs.ruby-lang.org/issues/16093#change-80547 * Author: k0kubun (Takashi Kokubun) * Status: Open * Priority: Normal * Assignee: ---------------------------------------- ## 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](https://github.com/ruby/ruby-commit-hook/commit/d7759cca6282741ecc9c46053166a1f5f5779c10#diff-7e61afe505452158c45dfa32ea7d7a14) 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](https://github.com/mame/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"](https://blog.developer.atlassian.com/stop-foxtrots-now/) 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](https://github.com/ruby/ruby-commit-hook/blob/master/bin/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. -- https://bugs.ruby-lang.org/ Unsubscribe: