4 Replies Latest reply on Aug 22, 2014 12:58 PM by rhauch

    To fast-forward or not to fast-forward when merging PRs

    rhauch

      GitHub pull-request pages always have handily listed the commands necessary to merge a PR into a branch. I almost always use these, since they always perfectly matched what we wanted to do. Until recently, the commands basically entailed something like this:

      Step 1: From your project repository, check out a new branch and test the changes.

      git checkout -b rhauch-mode-2201

      master git pull git@github.com:rhauch/modeshape.git mode-2201

      Step 2: Merge the changes and update on GitHub.

      git checkout master

      git merge --no-ff rhauch-mode-2201

      git push origin master

      However, recently the second command in step 2 includes the "--no-ff" flag:

      git merge --no-ff rhauch-mode-2201

       

      Initially, I was irritated with the change, since we've always kept our history clean. The "--no-ff" flag enables the "no-fast-forward" option, which will prevent simply adding the commits to the branch if they can be fast-forwarded (added at the end). This StackOverflow question offers a great summary of the difference between the old and new behavior.

       

      I've spent some time thinking about it, and I now wonder if it would be better to have our history show which commits were made together as part of a change. If we did, then we would start seeing merge commits. It would also mean that, as part of our merge operation, we no longer need to rebase to clean things up. In fact, if we did this, then we could even optionally use the GitHub "Merge" button (though I still prefer to run a build locally). And, perhaps best of all, our history would more accurately reflect the work that's done in our individual topic branches, and how those are merged into the various official branches.

       

      The advantage of continuing the way we have is that the history is relatively clean and contains a simple, orderly progression of commits.

       

      Does anyone have an opinion of which way to proceed?

        • 1. Re: To fast-forward or not to fast-forward when merging PRs
          hchiorean

          Even though I'm not a fan of merge commits (IMO they pollute the history), I do see an advantage in being able to tell which group of commits came off a topic branch. This is especially useful when a topic branch contains multiple commits not necessarily all related to the same MODE issue. I also like the idea of being able (for simple commits) to just merge via the GitHub UI.

           

          That being said, I also like seeing a clean history of commits, without the "noise" of merge commits. So I'm conflicted as to which option is better

          • 2. Re: To fast-forward or not to fast-forward when merging PRs
            rhauch

            Here's a summary of the pros/cons that were originally put together by jason.greene in another location:

             

            The benefits:

              1. The original history from the author is preserved
              2. The author does not have to toss their branch to avoid a conflict introduced by a pull after their PR is merged
              3. Changes introduced by conflict resolution are kept separate from the authors. So you know if the problem was caused by the merge or the change itself
              4. PRs sometimes include multiple commits, and a merge commit allows you to see which commits encompass the overall change
              5. Due to 5, bisecting is quicker
              6. It’s easier to revert a merge commit
              7. Github PRs automatically close when you perform a merge (versus only automatically closing if merging can be done only with a fast-forward)
              8. You can use the big green button with automated CI

             

            There are however some drawbacks:

              1. If you revert a merge, you need to create a new merge to bring it back. This can be a little confusing if you do it wrong. To bring back the reverted merge, you first have to rebase the old PR and submit a new PR to bring it back, and then you have to revert the revert. The reason this seems strange is because a “revert” isn’t a real undo, its a new historical event that is just an application of the reverse delta. So thats why to bring it back you want to revert the revert, or do a whole new PR. The explicit thing that you do NOT want to do is to remerge the original PR, because it will actually look like it works, but really it does nothing because what happens is the that when you do a merge, the commits you merge are permanently in your history. If you later on merge them again (like when you undo a merge and try to bring it back), it just notices its already in your history, and just returns leaving the merge undone.
              2. You have to know how to interact with merge commits in the tools (e.g. revert requires -m 1)
              3. git log, for whatever reason defaults to date ordering instead of topographical ordering, which can look confusing since it doesn’t represent when changes were actually merged. Thats solvable using git log —topo-order

             

            Merging merge commits is of course nasty, but you don’t have to allow it. You can just require that authors rebase their history when they need it to be more current vs a git pull. Merging then follows a nice clear one level nesting.

             

            Wildfly has already started always using merge commits.

             

            If ModeShape were to adopt using merge commits, then the process would be as follows:

             

            1. The author creates the PR as usual, except that there is (usually) no need to rebase your commits before you create the PR. You can rebase, but it would not be required for many PRs.
            2. One of the ModeShape committers attempts to merge the PR (with "--no-ff") and run a build. Git will merge automatically for some PRs, but others might have conflicts that have to be resolved. If that's the case, the committer will attempt to resolve all of the minor/obvious ones. But if the conflicts are more complicated, the author may ask the author to rebase the commits in the PR.
            3. If the author has to make any changes (or rebase), then they do this and update the PR. Then back to B.
            4. The PR is marked as closed, and the commit history is updated and always includes a merge commit.

             

            This varies from our existing procedure:

             

            1. The author creates the PR, making sure to rebase the commits so that they are based upon the latest official commit.
            2. One of the ModeShape committers attempts to merge the PR and run a build. If a merge commit is needed, the author rebases the PR's commits locally (changing the commit IDs from that listed on the PR). If there are any merge conflicts, the committer attempts to rebase the PR's commit(s) and fix any minor conflicts.
            3. If the author has to make any changes (or rebase), then they do this and update the PR. Then back to B.
            4. The PR is marked as closed, and the commit history is updated and never includes a merge commit.

             

            So what's the difference? Firstly, the history will start containing merge commits for every merge, and this contains information about who the committer was and what if any changes (conflict resolutions) they introduced. Secondly, most of the time the author does not have to rebase the commits before they create the PR, so it's a bit easier for them. Thirdly, the commit IDs from the PR will always appear in the history (they did not before if the committer rebased).

            • 3. Re: To fast-forward or not to fast-forward when merging PRs
              rhauch

              We've decided to go ahead and start using merge commits.

              • 4. Re: To fast-forward or not to fast-forward when merging PRs
                rhauch

                Using non-fastforwarding merges is a bit strange at first, but I've started to prefer them. It’s actually quite nice to be able to see which commits were related and made in a single PR, without having to parse the commit messages.


                In fact, the following aliases in ~/.gitconfig are quite helpful, especially "git graph" with the above-mentioned merges.

                 

                [alias]

                     graph = log --pretty=oneline --graph --abbrev-commit

                     hist = log --pretty=oneline --decorate --abbrev-commit --topo-order

                 

                For example, here's a portion of the output of "git graph" from today, and this uses different color traces to show the evolution of the different commits:


                *   6cd3a27 Merge pull request #1214 from hchiorean/MODE-2278

                |\ 

                | * c6ab818 MODE-2278 Fixed the validation of the prefix of a namespace (according to the space, must be an NCName)

                * |   179b252 Merge pull request #1208 from okulikov/MODE-2234

                |\ \ 

                | * | cb4ff74 MODE-2234: Enable "save" button when something is not saved only

                | * | dda816d MODE-2234: Move save button close to content  area

                * | |   2628c97 Merge pull request #1212 from hchiorean/MODE-2185

                |\ \ \ 

                | |_|/ 

                |/| |  

                | * | 5de23e0 MODE-2185 Removed the old REST service (/v1) and retrofitted the existing tests to reflect this change.

                | * | 38d2e24 MODE-2185 Removed the ModeShape REST client (modeshape-web-jcr-rest-client module). The functionality required by the remote JDBC Driver was moved to the modeshape-jd

                |/ / 

                * |   32b6740 Merge pull request #1209 from hchiorean/MODE-2275

                |\ \ 

                | |/ 

                |/|  

                | * e6c2add MODE-2275 Fixed Schemata caching by the JcrNodeTypeManager, which caused newly registered node types to not be visible as long as they are registered with the same name

                |/ 

                *   1dd8fab Merge pull request #1207 from hchiorean/MODE-2255

                |\ 

                | * 349cbba MODE-2255 Added 2 test cases for the reported issue, but both pass fine.

                * |   85f1090 Merge pull request #1206 from okulikov/MODE-2233

                |\ \ 

                | |/ 

                |/|  

                | * f3d4c6c MODE-2233: Move backup/restore actions to specific admin page

                * |   e71082a Merge pull request #1202 from elvisisking/MODE-2274

                |\ \ 

                | * | e9fc425 MODE-2274 Java sequencer doesn't handle files with no package statement Added a test case to verify the JdtRecorder can process Java files that do not have a package.

                * | |   ab430de Merge pull request #1201 from elvisisking/MODE-2272

                |\ \ \ 

                | * | | c43887a MODE-2272 Java Sequencer Should Be Thread-Safe The JavaFileSequencer was holding state by a field referencing the source file recorder. I removed that field and now

                | |/ / 

                * | | c69013f Updated the README file

                * | |   48bbf05 Merge pull request #1203 from hchiorean/MODE-2277

                |\ \ \ 

                | |/ / 

                |/| |  

                | * | 2c38121 MODE-2277 Added the ability to avoid performing versioning and locking validations if these are not used in a repository.

                |/ / 

                * |   136296b Merge pull request #1200 from hchiorean/MODE-2266

                |\ \ 

                | * | 3fc39ce MODE-2266 Added more performance tests for adding lots of nodes, both in deep and flat hierarchies.

                * | |   e94b249 Merge pull request #1198 from rhauch/mode-2276

                |\ \ \ 

                | |/ / 

                |/| |  

                | * | 50e98c6 MODE-2276 Improved efficiency of methods to add/remove/check mixins

                | * | bb93ad3 MODE-2276 Replaced the old logic to find child node definitions with the new logic

                | * | 22c7f11 MODE-2276 Changed how child node definitions are found when adding a child

                |/ / 

                * |   354786f Merge pull request #1197 from hchiorean/MODE-2271