BlogBack

Sometimes you really do want a merge conflict

As Koha release manager, one of the things I do is supplement patches written by other community members by adding regression tests. While reviewing and pushing two patches involving search today, I ran into a situation where doing a Git merge ended up giving the wrong results.

Fair warning — the rest of this post dives into technical details of Git.

Since we’re in the period between string freeze and the release of Koha 3.14, a few days ago I created a 3.14.x branch in the main Koha Git repository and announced that I would be pushing to both master and 3.14.x. Specifically, enhancements and patches that changed strings would go only into master, while bugfixes would go into both master and 3.14.x.

When maintaining two branches like this, I had a choice between cherry-picking patches from master into 3.14.x or using topic branches that I would merge into both release branches. Doing the latter is nice because it ensures that a given patch ends up having the same commit hash in both branches.

However, I had to be cautious: I don’t want to inadvertently merge into 3.14.x patches that are meant only for master. To ensure that, I started my topic branches from the 3.14 beta tag, e.g.,

git checkout -b new/bug11137 v3.14.00-beta

Bug 11137 has to do with how the QueryParser mode was incorrectly tossing away parts of a Boolean search, resulting a search for kw:history && kw:earth being treated as if it were simply kw:history.

Oops.

I happened to write the patch fixing that bug, and earlier today a member of the QA team, Jonathan Druart, finished reviewing it and passed QA on it. I applied the patch and pushed it using the following sequence of commands:

git bz apply 11137git rebase -i HEAD^^git commit --amend -s # to apply my signoff as RMgit rebase --continuegit commit --amend -s # to apply my signoff as RMgit rebase --continue # all done!git checkout mastergit merge new/bug11137git checkout 3.14.xgit merge new/bug11137# push to the main Git repositorygit push origin master:master 3.14.x:3.14.x

When I write a patch for Koha that touches core APIs, I also write regression tests. In this case, here’s part of the diff on t/db_dependent/Search.t:

--- a/t/db_dependent/Search.t+++ b/t/db_dependent/Search.t@@ -12,7 +12,7 @@ use YAML;use C4::Debug;require C4::Context;-use Test::More tests => 196;+use Test::More tests => 200;

In other words, the patch series for bug 11137 added four tests.

The next thing I did was move on to bug 10684. The patch for this bug, written by Kyle Hall, addresses a situation where some records that Zebra will happily index are considered invalid by the MARC::Record Perl module. A example of such a record is one that contain a tags whose label contains a punctuation character.

Kyle’s patch did not include a regression test, so I wrote one. Here is part of the patch for the regression test that is relevant to the rest of my story:

--- a/t/db_dependent/Search.t+++ b/t/db_dependent/Search.t@@ -12,7 +12,7 @@ use YAML;use C4::Debug;require C4::Context;-use Test::More tests => 196;+use Test::More tests => 200;

Looks rather familiar, right?

Now, why did the number of tests start at 196, not 200? Because my topic branch also started at v3.14.00-beta.

After doing some testing, I was happy with the main patch and my regression test. I then did this:

git checkout mastergit merge new/bug10684

So far, so good — no merge conflicts were reported.

However, because I have a suspicious nature about such matters, I decided to run prove -v t/db_dependent/Search.t again. Here’s what I got:

Test Summary Report-------------------t/db_dependent/Search.t (Wstat: 65280 Tests: 204 Failed: 4)Failed tests:  201-204Non-zero exit status: 255Parse errors: Bad plan.  You planned 200 tests but ran 204.

What happened?

The patch series for the first bug added four tests, and the patch series for the second bug also added four. 196 + 4 + 4 = 204, right?

But what do we see in t/db_dependent/Search.t?

use Test::More tests => 200;

The lesson? Git isn’t that smart. When it did the merge, it saw that both branches (master and new/bug10684) were changing that line from 196 tests to 200 tests. Git’s default merge algorithm interpreted that as meaning that both branches agreed on what that line should be, and didn’t see a merge conflict.

How to work around that? I created a new branch like this:

git checkout -b new/bug10684_take2 new/bug11137

I then cherry-picked the patches from the old topic branch (new/bug10684) to the new one, then added a patch to set the correct number of tests and used an interactive rebase to clean up the resulting patch series. (I still didn’t get a merge conflict when I did the cherry-pick, which is why I needed to manually write and fold in a follow-up to set the number of tests).

After that, I was home free — I merged new/bug10684_take2 into master and 3.14.x.

Trust, but verify.