Why I prefer pessimistic merging

Even though I trust my team.

As a technical lead, I think code reviews are essential to ensuring the quality of the code. On a project I worked on, due to changes in Project Management, I ended up switching between code reviews as standard and code reviews by exception, and in every case, releases delivered with code reviews baked in had fewer bugs, and were far more likely to meet deadlines.

I believe that code reviews done after integration have roughly the same benefit on these metrics as no code reviews at all. Although this guy prefers optimistic merging, I’m not convinced, and I think pessimistic merging is the only way to review code, especially when automated testing and static analysis are part of the review process.

However, my experience is with working with small-ish, cohesive teams, rather than open source projects seeking new contributors, so please bear that context in mind.

  • It’s easier to fix problems pre-merge;
  • Blockages pre-merge are far easier to detect (kanban!) – fix the bottleneck, don’t break the process;
  • Code reviews are the process by which the team takes ownership of the code;
  • The code remains continuously shippable;
  • Each code review is a checkpoint for discussion, and segways into refactoring and other improvements.
Advertisements

5 thoughts on “Why I prefer pessimistic merging

  1. I have similar feelings in many way, but also key differences. I believe that the best merging is no merging at all….the adoption of designs and practices that allow the entire team to work in a single unified branch, with extremely frequent commits [often multiple per hours per developer].

    In terms of code reviews, they [IMPO] must be small, anything over about 40-50 lines is going to be too big to pay detailed attention to the code. [Note: I am specifically talking about reviewing the code, there are many other types of reviews for other purposes].

    As a concrete example, if you had a major set of changes to review would you be able to reliably dtermine if the change from a for to a foreach or a LINQ would be safe or potentially fatal? [This is one of my favorites to spring on people].

    Repeated experience has sown that the vast majority of these can be non-blocking…Make change, publish review request, commit.

    Liked by 1 person

    1. Maybe I’ve always followed the wrong processes, but in my experience, checking before merge always catches more problems and leads to fewer bugs reaching the customer than merge before check.

      Like

      1. Thanks for the response. With the types of designs, and work habits that are prevalent, I 100% agree with you.

        The primary reason for my initial comment was to raise awareness that there exist alternate approaches that can completely eliminate merging.

        I am at a client site, and looking at their codebase right now. 83 commits by 7 developers [and it is just a little after 12:00 here) all into “main” which will be pushed to production provided the large Daily test run (there are smaller earlier ones including “gated” and “CI”).

        None of the work was blocked waiting for a review – I spotted two cases where minor tweaks were made o (already functional) code as the result of the review. And there was NO merging at all.

        Is this for everyone? No. But could this potentially have a major impact on teams? I am sure of it, because I have experienced it.

        Liked by 1 person

      2. It’s good to hear about other experiences. Were you involved in setting the process up or was the team following it when you joined? I imagine there’s a lot of things to get right to make it work. Automated tests and code analysis I’m sure, but what else?

        Like

      3. The overall “process” is something I have been evolving over the past decade [I run a consulting firm that specializes in ALM] integrating various aspects I have found teams doing. I am then engaged by companies to see if this approach is a good ft for them and if so, get retained to help them adopt it.

        Yes, automated tests at multiple levels are key, code analysis plays a significant part. But the two biggest elements:

        1) Very tight communication. I often characterize it as the team becoming “borg” [though that is hyperbole]

        2) Adoption of “unobtrusive” design/implementation practices so that new (or changed) work can safely be performed concurrently in a single “branch”

        I speak fairly often (being based on St. Petersburg that is the hub of the most common venues). Some sessions were presented at VsLive [Washington DC] last year, and more are submitted for the 2017 VsLive tour [as well as other conferences].

        Anyone interested can feel free to drop me a note: david.corbin@dynconcepts.com

        Liked by 1 person

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s