Nobody likes resolving merge/rebase/cherry-pick conflicts. I also don’t, but I’ve found a scheme how to reduce the built-in headache. In this post I’m going to share my findings as well as practical examples.
Warming up
First let’s brush up on merge conflicts with some shortened logs from this example Git repository:
The above alphabetically sorted list of pets starts with the bottom commit adding a dog. Then the Git history diverges: Both a cat and a rabbit get added independently of each other to different locations. And only because of the latter Git is able to automatically merge the two branches into the top commit.
Exactly that is not possible in the second example as both rabbit and dog get added directly below cat. Git won’t produce the top commit as above by itself as it can’t know which animal(s) have to appear in which order. In this case the git merge command fails and “merges” the animals as follows:
@@@ -1,2 -1,2 +1,6 @@@ * cat ++<<<<<<< HEAD +* rabbit ++======= + * dog ++>>>>>>> dog
In this simple case it’s even not enough to just remove Git’s markers despite the changes from both sides are only additional. While actually performing the above merge I had to re-sort the list to keep it in alphabetical order.
Mastering actual challenges
Now, as we’ve warmed up, let’s have a look at a little more complicated example in a real-world repository – Icinga 2. Let’s imagine we’re about to release v2.12.7 and I have to backport (cherry-pick) #9267. If I checkout v2.12.6 and git cherry-pick 575af4c98 362adcab1 6193a911b, I get the following conflict in lib/remote/configpackageutility.hpp:
static void WritePackageConfig(const String& packageName);
static void WriteStageConfig(const String& packageName, const String& stageName);
<<<<<<< HEAD
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, bool reload);
=======
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate,
bool reload, const Shared<Defer>::Ptr& resetPackageUpdates);
static bool ValidateFreshName(const String& name);
>>>>>>> 362adcab1 (ConfigPackageUtility: Don't reset ongoing package updates on config validation success and process is going to be reloaded)
};
}
On the first sight this seems like yet another additional-only pair of changes. The below one even seem to be an addition to the above one. So let’s just use the below one, right? Wrong! That wouldn’t even compile as there’s no ValidateFreshName() in Icinga v2.12.x. At this point I can look up 362adcab1 and search for that location. Or, my preferred solution: Before any of the above operations I’ve done git config --global merge.conflictStyle diff3 (once). Because of this I get the conflict in this format instead:
static void WritePackageConfig(const String& packageName);
static void WriteStageConfig(const String& packageName, const String& stageName);
<<<<<<< HEAD
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, bool reload);
||||||| parent of 362adcab1 (ConfigPackageUtility: Don't reset ongoing package updates on config validation success and process is going to be reloaded)
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, bool reload);
static bool ValidateFreshName(const String& name);
=======
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate,
bool reload, const Shared<Defer>::Ptr& resetPackageUpdates);
static bool ValidateFreshName(const String& name);
>>>>>>> 362adcab1 (ConfigPackageUtility: Don't reset ongoing package updates on config validation success and process is going to be reloaded)
};
}
Much better! Now there’s also the merge-base in the middle. I. e.: The HEAD (v2.12.x), compared to the merge-base (v2.14.x), has “removed” ValidateFreshName(). (Actually ValidateFreshName() is being added in v2.14.x, but if we’d revert everything back to v2.12.x, we’d really remove that function.) On the other side 362adcab1, compared to the merge-base, adds a parameter to TryActivateStageCallback(). Now I have to apply both changes, at the top and at the bottom, to the merge-base in the middle. I significantly simplify that by splitting both changes in the less-difficult and the more-difficult one. In this case the less difficult is clearly the removal of ValidateFreshName() (middle to top). Once I’ve made my decision, I just take the more difficult change by removing the below Git marker (in my case):
static void WritePackageConfig(const String& packageName);
static void WriteStageConfig(const String& packageName, const String& stageName);
<<<<<<< HEAD
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, bool reload);
||||||| parent of 362adcab1 (ConfigPackageUtility: Don't reset ongoing package updates on config validation success and process is going to be reloaded)
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, bool reload);
static bool ValidateFreshName(const String& name);
=======
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate,
bool reload, const Shared<Defer>::Ptr& resetPackageUpdates);
static bool ValidateFreshName(const String& name);
};
}
Now “the only” thing I have to do is to compare the middle block with the top one (in my case) and apply this diff to the code outside the markers. I. e. I have to remove ValidateFreshName():
static void WritePackageConfig(const String& packageName);
static void WriteStageConfig(const String& packageName, const String& stageName);
<<<<<<< HEAD
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, bool reload);
||||||| parent of 362adcab1 (ConfigPackageUtility: Don't reset ongoing package updates on config validation success and process is going to be reloaded)
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, bool reload);
static bool ValidateFreshName(const String& name);
=======
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate,
bool reload, const Shared<Defer>::Ptr& resetPackageUpdates);
};
}
Then I can just remove the rest of Git’s garbage and I’m done:
static void WritePackageConfig(const String& packageName);
static void WriteStageConfig(const String& packageName, const String& stageName);
static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate,
bool reload, const Shared<Defer>::Ptr& resetPackageUpdates);
};
}
Did someone say “accurate”?
By the way it’s the same with the conflict in lib/remote/configpackageutility.cpp:
Process::Ptr process = new Process(Process::PrepareCommand(args));
process->SetTimeout(Application::GetReloadTimeout());
<<<<<<< HEAD
process->Run(std::bind(&TryActivateStageCallback, _1, packageName, stageName, activate, reload));
||||||| parent of 362adcab1 (ConfigPackageUtility: Don't reset ongoing package updates on config validation success and process is going to be reloaded)
process->Run([packageName, stageName, activate, reload](const ProcessResult& pr) { TryActivateStageCallback(pr, packageName, stageName, activate, reload); });
=======
process->Run([packageName, stageName, activate, reload, resetPackageUpdates](const ProcessResult& pr) {
TryActivateStageCallback(pr, packageName, stageName, activate, reload, resetPackageUpdates);
});
>>>>>>> 362adcab1 (ConfigPackageUtility: Don't reset ongoing package updates on config validation success and process is going to be reloaded)
}
void ConfigPackageUtility::DeleteStage(const String& packageName, const String& stageName)
I mean, I could just pick the middle-to-bottom diff as-is. That would be easy, but not accurate. I would steal credits for the lambda refactoring in the name of the developer who added resetPackageUpdates. Instead I have to take the other diff and add resetPackageUpdates there:
Process::Ptr process = new Process(Process::PrepareCommand(args));
process->SetTimeout(Application::GetReloadTimeout());
process->Run(std::bind(&TryActivateStageCallback, _1, packageName, stageName, activate, reload, resetPackageUpdates));
}
void ConfigPackageUtility::DeleteStage(const String& packageName, const String& stageName)
If I had to include the lambda refactoring as a functional dependency of resetPackageUpdates addition, I had to also backport, i. e. cherry-pick, that commit(s). Before the others, of course.
Customers who like to resolve Icinga merge conflicts…
… for the ease and accuracy may also like the Icinga Master as NWS app. Easy in the frontend, but accurate in the backend. And free of charge for 30 days!
