15

I am not certain I can trust Git to merge automatically. Here is a scenario.

Create a program in the master:

    MOVE 0 TO I.
A.
    PERFORM X-PROC.
    IF I IS EQUAL TO 25 THEN GO TO A.

Developer 1 makes a branch and notices that there is a bug: an infinite loop. He fixes it:

    MOVE 0 TO I.
A.
    ADD 1 TO I.
    PERFORM X-PROC.
    IF I IS EQUAL TO 25 THEN GO TO A.

Meanwhile Developer 2 makes a branch and fixes the bug in her own way:

    MOVE 0 TO I.
A.
    PERFORM X-PROC.
    ADD 1 TO I.
    IF I IS EQUAL TO 25 THEN GO TO A.

Both developers test their code and find it correct. Both merge to the master:

    MOVE 0 TO I.
A.
    ADD 1 TO I.
    PERFORM X-PROC.
    ADD 1 TO I.
    IF I IS EQUAL TO 25 THEN GO TO A.

The infinite loop is back.

It seems to me that this problem must happen often in any sort of distributed development environment. When I tested this, Git did not report a merge conflict. Sometimes this problem could go undetected for a long time. A regression test should find it, but regression tests are merged in Git too, so we can't trust them either.

What can I do about this? Do I need to do a code reading after every merge?

EJoshuaS - Stand with Ukraine
  • 11,977
  • 56
  • 49
  • 78
Mark Lutton
  • 6,959
  • 7
  • 41
  • 57

1 Answers1

19

Do I need to do a code reading after every merge?

Yes, of course.

Automerge algorithms are helpful, but they're not magic; they just include the changes that were made to both sides of a file if they don't conflict. There's no guarantee that the resultant change compiles or is even not gibberish. There's no guarantee that the logic isn't a complete trainwreck. (Some have speculated that the Heartbleed bug was the result of an automerge changing the logic subtly and was not caught in review.)

This is true of any version control tool that does an automerge (which, presuming you're using something that was written in the last 15 years or so, almost certainly does.) Though this is not to impeach automerge, which resolves two changes to the same file and generally does a fine job; this is also true of a merge in general. If you modify some file A and I modify some file B, there's no guarantee that the merge makes sense.

Best practice: you should always review your merges before you commit or push them, even when they automerge successfully.

Edward Thomson
  • 74,857
  • 14
  • 158
  • 187
  • 5
    Test suites/unit tests are a wonderful tool to reduce the workload if you're having gargantuan merges (which you should not usually have) -- but dont fall to the false sense of security and neglect sanity-checking your merges, just because your tests all pass! – Nevik Rehnel May 07 '14 at 18:27
  • 1
    One technique that I use to make verifying a merge less painful is to take the diff of a branch before and after a merge with another branch, and then compare that diff (using a diffing program) to the diff of the changes made in the other branch. If there's no difference between the diffs, then I can have higher confidence that the merge wasn't bungled. –  May 13 '14 at 03:39
  • Thanks NevikRehnel and @cupcake - I would love or hear everybody's strategies for sanity checking merges! – Edward Thomson May 13 '14 at 04:39
  • 1
    Test suites get merged too. The same merge that breaks the code may alter the test so that it still passes. – Mark Lutton Sep 29 '15 at 00:53
  • 2
    Unfortunately many websites and learning materials talk about git branch/merges in a way that implies that if the physical text merges ok, then the underlying logic of the code is ok, which of course it's not. This has been perpetuated as a default behaviour too and give give a godlike status as version control. – John Stock Feb 17 '21 at 09:02