A rebasing vendor-branch workflow for Git

An earlier post laid out a neat way of managing third-party code, in a subdirectory of a larger Git repository, as a vendor branch. Here at Electric Imp we followed that scheme for the subsequent five years, and bitter experience over that time has now convinced us that it doesn’t work well in all circumstances, and a better plan is needed.

The issue is that essentially all local changes to the repository accrete over time into one massive merge commit. If we’re talking about a sizeable amount of code, and if we’ve made lots of changes to it over time, and if upstream have also made lots of changes to it over time, then re-merging that single merge commit soon becomes horrendous. Basically, the original bow-shaped vendor branch workflow does not scale.

So what to do instead? We need to deconstruct that overwhelming single merge commit; we need, ideally, to move from a merging workflow to a rebasing workflow.

The broad outline of the situation is as follows: some time ago we merged an upstream version of some vendor code, but we’ve been hacking on it since. Meanwhile, upstream has produced a newer version whose updates we’d like to take – while keeping our changes, where relevant.

To achieve this, we’re first going to create a new branch, re-import a stock copy of the current upstream version, and then re-apply our changes, one by one, so that the tip of that branch exactly matches the tip of master.

So far, this sounds like a lot of work to achieve literally nothing! But what we now have, is a branch containing just our changes, in a sequence of individual commits. In other words, it’s exactly what’s needed in order to rebase those commits on top of a newer upstream release. So, starting from our re-import, we create a second new branch, then import a stock copy of the new upstream (so that there’s a single commit containing all upstream’s changes between the two), and then rebase our re-apply branch on top of that.

The upstream source in the example below is the same WICED SDK as used in the previous blog post; note that although the whole WICED project was sold by Broadcom to new owners Cypress in the meantime, many references to “Broadcom” still remain in our code, notably in the directory names. This ticks all three boxes for needing the heavyweight integration process: it’s large – 7,000 source files – we’ve made lots of changes, and upstream have also made lots of changes.

Here’s exactly what to do. First we need to know which commits, since the last time we took an upstream version, changed the vendor directory – here, thirdpaty/broadcom. Fortunately, git log is able to tell us exactly that. We need to go back in history (perhaps using gitk’s “find commits touching paths” option) and find the commit where we originally took our current upstream version. In the repo I’m using, that’s ac64f159.

The following command logs all commits that have changed the thirdparty/broadcom directory since then:

git log --format=%H --reverse --ancestry-path ac64f159..master -- thirdparty/broadcom

For me, that lists 198 commits! Because of the --reverse, they’re listed in forwards chronological order: in other words, in the order in which we need to re-apply them on our re-merge branch. Let’s put that list of commits in a file, commits.txt:

git log --format=%H --reverse --ancestry-path ac64f159..master -- thirdparty/broadcom > commits.txt

Now we start a new branch:

git checkout -b pdh-wiced-remerge

...and re-import the current upstream version, here WICED-3.5.2:

rm -rf thirdparty/broadcom
cd thirdparty
7zr x WICED-SDK-3.5.2.7z
cd ..
mv thirdparty/WICED-SDK-3.5.2 thirdparty/broadcom
(clean up the source as necessary, fix CR/LFs etc.)
git add -A thirdparty/broadcom
git commit -m"Import stock WICED 3.5.2"
git tag pdh-wiced-3.5.2-stock

Notice that we tag this commit, as we’ll be referring to it again later.

Now we’d like to replay our long list of local commits on top of that release – to get back, as it were, to where we started. The thing to note here is that we can do this in a completely automated way – there should be no chance of merge conflicts, as we’re re-applying the same commits on top of the same upstream drop. It’s so very automated that we can do each one using a script, which I called apply-just-broadcom:

#! /bin/bash
git checkout $1 -- thirdparty/broadcom
git add -u thirdparty/broadcom
git commit -C $1

This says, first checkout (or, really, apply) the commit named in the script’s first argument – but only where it affects the thirdparty/broadcom directory. Any other parts of the commit aren’t applied. This automatically adds any new files, but it doesn’t delete anything deleted by the commit we’re applying – so we delete those using git add -u. Finally we want to commit those changes to our new branch, but using the commit message they originally had – for which, git commit’s -C option is exactly what we want.

Armed with that script, we can then apply, one-by-one, all the commits we identified before:

git checkout pdh-wiced-remerge
for i in `cat commits.txt` ; do scripts/apply-just-broadcom $i ; done

This will churn through the commits, one by one, re-applying them to the pdh-wiced-remerge branch. Because they’re all getting applied in the same context in which they were originally committed, they should all go in one-after-the-other with no conflicts or warnings. (If they don’t, perhaps your re-import of the old upstream didn’t quite match the original import, so fix that and start again.)

And now we should be, quite circuitously, back where we started, with the tip of the pdh-wiced-remerge branch matching master exactly:

git diff pdh-wiced-remerge master

...which should show no differences at all. What you’d see in gitk is something like the image to the right, showing master with your branch coming off it. And the branch contains the (re-)import commit, then all the work that’s been done since. Scrolled way, way off the top of that screenshot, about 150 commits further up, is the tip of the pdh-wiced-remerge branch.

Optionally but usefully, you can now tidy up the branch to make it easier to apply to the new release. For instance, if the branch contains patches that were later reverted, you can use interactive rebase to remove both the patch and the revert, for the same result but avoiding any chance of the patch later causing conflicts. Doing this should still leave no diff between your branch and master.

Even more optionally, but still usefully, I needed at this stage to rewrite a bunch of the commit messages. Those numbers in square-brackets in the commit summaries, are instructions to our git server to link the commits with the related user-story (or bug) in Pivotal Tracker. (The reason they don’t all have one, is that some of the original commits were themselves on bow-shaped branches, and only the merge commit carried the Pivotal link.) I wanted to remove those links, so that pushing my branch didn’t spam every Pivotal story associated with a thirdparty/broadcom commit, some of which were years old by this stage. But there are tons of them, so I wanted to rewrite the messages programmatically. It’s definitely out-of-scope for this blog post, but I ended up using the powerful and not-to-be-trifled-with git filter-branch command, in conjunction with a tiny sed script:

git filter-branch --msg-filter "sed -e 's/\[\#\([0-9]*\)\]/(\1)/g'" master..pdh-wiced-remerge

This monstrosity rewrites every commit message between master and pdh-wiced-remerge, to replace square-bracket Pivotal links with curved-bracket “links” that don’t trigger the automatic Pivotal integration.

Anyway, that aside, we’re now in a position to merge the new WICED release, upstream version 3.6.3. So let’s tag where we are now, so we can refer to it later:

git checkout pdh-wiced-remerge
git tag pdh-wiced-3.5.2-merged

We want to branch our WICED-3.6.3 branch from the stock 3.5.2 re-import, so that there’s a diff that just shows upstream’s changes between 3.5.2 and 3.6.3. So that’s:

git checkout pdh-wiced-3.5.2-stock
git checkout -b pdh-wiced-3.6.3

And now we can do the same dance as before, removing the old thirdparty/broadcom and replacing it with the new one:

rm -rf thirdparty/broadcom
cd thirdparty
7zr x WICED-SDK-3.6.3.7z
cd ..
mv thirdparty/WICED-SDK-3.6.3 thirdparty/broadcom
(clean up the source as necessary, fix CR/LFs etc.)
git add -A thirdparty/broadcom
git commit -m"Import stock WICED 3.6.3"
git tag pdh-wiced-3.6.3-stock

Again, because this is a complete replacement, there’s no chance of merge conflicts.

Now just the rebasing operation itself remains. Because we tagged all the important points, we can use those tag names in the rebase command:

git rebase --onto pdh-wiced-3.6.3-stock pdh-wiced-3.5.2.stock pdh-wiced-remerge

This will be a lengthy rebasing operation, and merge conflicts are likely to occur. These can be fixed “in the usual way” – this isn’t a process usable be people who aren’t already experienced in fixing rebase conflicts, so I won’t say much more about that fixing here. But note that the rebase operation always tells you which commit it got stuck on – so you can go and look at the original version of that commit to check what its intention was, and also at the pdh-wiced-3.6.3-stock commit, containing exactly upstream’s changes between 3.5.2 and 3.6.3, in the hope that you can glean some insight into what upstream’s intention was.

If you’ve previously been using our original vendor-branch scheme, the first of those commits will be the worst trouble to integrate, as it will be the single big-bang merge commit from the previous integration. But at least you can console yourself now that future integrations will be no harder than this – as opposed to the previous scheme where they just kept getting harder.

Once you’ve completed the rebase, the repository will look basically as it is on the right. Notice that the rebase operation has “moved” the branch pdh-wiced-remerge so that it’s now on top of the 3.6.3 merge; it’s not still where the pdh-wiced-3.5.2-merged tag points to. (Rebasing always moves the branch that you’re rebasing; it never moves any tags.)

Now you get to build and test the pdh-wiced-remerge branch; it’s likely that it currently doesn’t even compile, and here is where you make any major changes needed to deal with the new drop. (Minor, textual changes may have already been fixed during the rebase process.) Add any new commits as necessary, on the pdh-wiced-remerge branch, until everything builds, runs, and passes all its tests. This may or may not be particularly arduous, depending on the nature of the changes made upstream. (But either way it’s still less arduous than the same thing would have been with the merging workflow.)

Now all that remains is to merge the pdh-wiced-remerge branch back to master as a normal bow-shaped branch:

git checkout master
git merge --no-ff pdh-wiced-remerge

And you should end up with a repository that looks like the one at left: a bow-shaped branch containing, in order, the re-imported 3.5.2; the newly-imported 3.6.3; the rebased versions of all your previous fixes; and any new fixes occasioned by 3.6.3 itself.

Notice that the replayed fixes leading up to the tag pdh-wiced-3.5.2-merged don’t appear in the branch as finally merged, but that the stock WICED 3.5.2 commit does. This is probably what you want: it’s important to have a commit that denotes only the changes made upstream between releases, but it’s not very important to record the replayed fixes – after all, those commits were generated by a script to start with, so the information in them can’t be that significant.

So now 3.6.3 is merged. And when the next upstream release comes along, you get to repeat the whole process.

But what if several upstream releases have happened since you last took one? There are two options: either you do the above process once, taking the latest release – or, you do it repeatedly, once for each intermediate release. The latter is the best option if widespread changes have occurred, as it lets you fix each conflict separately, rather than all in one go. And in fact if you know you’ve got several upstream releases to merge, you don’t have to bother merging back to master each time (especially if you too have 198 commits on your remerge branch): you can keep branching and rebasing and then merge the final result to master in one go, as seen on the right. The branch, as finally merged, would contain, in order:

• stock 3.5.2
• stock 3.6.3
• stock 3.7.0
• ... further upstream releases ... (red)
• fixes from 3.5.2 and earlier (pink)
• fixes from 3.6.3 (pale blue)
• fixes from 3.7.0 (mid-blue)
• ... further sets of fixes ... (green)

All of which means that the tip of that branch would be the latest upstream version, with all relevant fixes applied. Which is what’s needed to be merged.

Bow-shaped branches: using vendor branches in Git

LATER EDIT: the method described here turned out not to scale very well for large vendored libraries with lots of changes. If your Broadcom (or similar) merges are killing you, you should probably be using a rebasing vendor flow instead.

Following on from the one about bow-shaped branches, one of the remaining ways that Git history can get untidy, despite following those precepts, is when you’ve got a long-running “vendor branch” for some third-party code. The usual pattern is, to have a branch containing successive deliveries of the third-party code; when a new delivery needs to be integrated, it’s checked-in unaltered on that branch, and then the branch is merged back to master. This means that the branch effectively has a diff that corresponds only to the vendor’s changes in the new release (relative to the previous release), so that Git’s merge infrastructure helps you integrate those changes with any changes you’ve made yourself to the third-party code.

Which is all to the good, but presents a problem for those striving for tidy Git history. Especially in the case of widespread changes or ugly merges, integrating the new delivery could end up being as much work as a new feature in its own right – which means it really ought to take place on a feature branch. But if you try and do that feature branch as a bow-shaped branch, starting off with a commit that’s the merge from the vendor branch, then the bow-shaped workflow (and indeed the simple rebase workflow) won’t work out of the box: every time you attempt to rebase the feature branch, Git tries to rebase the entire vendor branch, which is hopelessly not what you want.

The obscure Git feature which in this case turns out to be exactly what’s wanted, as if designed for it, is git rerere. Git can be instructed to remember merge-conflict resolutions, and replay them automatically if the precise same conflict is encountered again. This feature isn’t enabled by default, no doubt because it’d confuse people about how their conflicts somehow disappeared, but it’s just what’s needed for rebasing a feature branch that includes a merge from a vendor branch.

Let’s start by having a look at the desired result. On the right is a screenshot from gitk, which shows the newest commits at the top. It shows that the long-lived vendor branch “broadcom” has been merged following a new delivery of the third-party code, and that the work of integrating the new release itself happened on a bow-shaped branch. No superfluous merge commits are visible.

Here’s how to make history that looks like that. First of all I need to turn on the rerere feature:

git config --global rerere.enabled true

Then import the new delivery. This part is much like the process of using vendor branches in any other source-control system:

1. checkout the vendor branch
2. remove the old code entirely
3. untar/unzip the new delivery
4. rename if necessary (usually, at least, removing version numbers from directory names – the delivery depicted here untarred into a directory called WICED-SDK-2.3.1, but we wanted it called thirdparty/broadcom)
5. but don’t touch the source in any other way
6. add the changes, additions, and removals to Git (git add -A)
7. commit everything to the vendor branch with a commit message along the lines of “import Broadcom Wiced 2.3.1”

But then it’s time to merge those changes back to master. The steps are as follows:

1. create a merge branch, here called “pdh-broadcom” (this is the branch that will eventually look bow-shaped)

   git checkout master
   git checkout -b pdh-broadcom

2. merge the vendor branch

   git merge broadcom

   1. if there were merge conflicts, fix them up; don’t do any other edits at this stage
   2. once the merge conflicts are fixed, commit (to the merge branch)

      git commit

3. compile and test the result
4. commit any compile or test fixes to the merge branch
5. test some more, hold code reviews, commit more fixes, etc., until you’re satisfied that it’s ready to merge to master

At this stage, history looks like the picture on the right: a bit like the “ready to merge” stage of a normal bow-shaped branch, except of course with the long tail of the vendor branch heading off into the distant past.

So let’s get hold of the current state of master:

git checkout master
git pull --rebase

Aha, see on the right, someone’s beaten us to the punch again. We need to rebase our work on top of theirs. But here’s the point at which a bit of caution needs to be applied. Blithely using git rebase as if the vendor branch weren’t there, will instruct git to rebase the entire vendor branch on top of master, which will typically be somewhere between undesirable and complete carnage.

There’s a -p (“preserve branching”) option to git rebase option which will stop it doing that, but there is still the issue that re-doing our merge branch, will involve re-doing the merge itself – which will involve re-resolving all the conflicts that we got in stage 2 above.

And that is where git rerere comes in. Because we enabled rerere above, each time we committed the fix to a merge conflict, git remembered the conflict and our resolution of the conflict. And when we re-encounter the same conflict again, as a result of rebasing, git re-resolves it for us:

git checkout pdh-broadcom
git rebase -p master
Resolved 'thirdparty/broadcom/Wiced/WWD/internal/wwd_wifi.c' using previous resolution.
Automatic merge failed; fix conflicts and then commit the result.
Error redoing merge 7a485d1d

In fact, Git is being unduly alarmist here. The merge did technically fail, but rerere has in fact already solved all the issues automatically. So we just need to convince Git of that, noting which files or directories it thought were a problem:

git add thirdparty/broadcom/Wiced/WWD/internal
git commit

So we’ve now got a fully rebased merge branch, ready to merge back to master – as at right. The merge itself is now just like a normal bow-shaped branch:

git checkout master
git merge --no-ff pdh-broadcom

So at last (as at left) we’ve got where we need to be: it’s a merge from a vendor branch, but it looks like a feature branch.

The more I use Git, the more I feel the truth of these two rules of thumb about it: there is always a way to do X, and there is always a use for command Y. Blogging about Git is really an exercise in providing the Y corresponding to a certain X, which is not always obvious.

Bow-shaped branches: a Git workflow

Distributed version control systems, and Git in particular, have for all practical purposes wiped out CVS, Subversion and similar systems. But Git is new enough that there’s still discussion to be had on the best ways of using it: the best Git “workflows”. One goal of a good workflow is that Git history – the graph of all previous revisions – remains readable and comprehensible. Or, put conversely: one symptom of a bad workflow is a convoluted history that looks like a Tube map, or worse.

One way in which git history can end up looking a tangled mess, is if the history graph includes lots of merges caused by doing git pull when upstream has also been updated. Those merges don’t really add that much information – only that two things happened simultaneously, which is a perfectly normal occurrence in teams of more than one person. So most development teams, at least once they’ve been using Git for a while, decide that merges caused by git pull cause unnecessary tangles, and git pull --rebase gets mandated instead.

But what if development occurs on “feature branches” which are always getting merged? This case, too, can be dealt with using rebasing: in this case, rebasing the branch before merging it. But a pure rebase workflow serialises everything into a straight line, losing the information that these commits, in some sense, belong together.

So what’s really needed is a hybrid of the rebasing and merging styles: something that keeps the feature commits together, but also keeps history looking neat (and, in case of disaster, allows easy reverting of the whole feature). What’s needed is bow-shaped feature branches.

Let’s look at the desired end-result first. On the right is a snapshot from gitk --all, showing a feature branch that I’ve just merged, ready to push. As always with gitk, the newest commits are shown at the top. Two different routes join the same two endpoints, spanning the addition of my new feature: one direct, and another taking many small steps, so that the overall effect is a bow shape. This means that, though each individual commit is readily accessible, so too (by comparing the top and bottom of the bow) is the overall effect of the whole feature. Notice how the commits are all bunched together, but with a limited, local amount of branching: a run of N such branches consecutively would still, in some sense, have O(1) complexity, not O(N). (At least, it would have O(1) concurrently-active development branches, which is believably a useful measure of the complexity, and the comprehensibility burden, of history.)

Someone looking back through history who’s only interested in the large-scale changes, can thus move back via the “express train” lines and not the “stopping train” (or “local train” in US parlance) that goes via every commit. But they still wouldn’t have to follow many different branch lines, as they would in a pure merge workflow, because each bow-shape happens on top of the last, not in parallel with it.

So how to build history graphs that look like that? On the left is gitk’s view of my local repository after doing some work on a feature: in this case a feature made up of seven commits. (Commit early, commit often; but stay bisectable.) If I just did a git pull followed by git push, I’d get a merge with commits on both sides; if I did the same but pulling with --rebase, I’d get a straight-line history with my commits at the top.

In order to get the bow shape, though, I’m going to need to retrospectively turn my commits into a branch. Fortunately, that’s straightforward in Git. First, I create a new branch, named after my initials and my feature (the branch itself won’t be pushed upstream, but its name appears in the merge message, so it ought to be fairly descriptive):

git checkout -b pdh-regexp

At this point my repository looks like the picture on the right.

Then I need to wind back my local master branch to match origin/master:

git checkout master
git reset --hard origin/master

Now my repository looks like the picture on the left: a feature-branch ready to merge. (If at this point you can't see your feature branch, make sure you used the --all option to gitk.) This is the point at which to do any smartening-up of the branch, using interactive rebase or similar, that is needed. It’s also the point at which to do a code review with one of your colleagues – a practice that’s strongly recommended, for various reasons.

[Edited 2014-Apr-05→] Now if by this stage master has moved on – in other words, if it’s not still pointing to the base of my feature-branch as shown in the pictures – then I’ll first need to rebase my branch on top of master, so that it looks like the gitk pictures:

git checkout master
git pull --rebase
git checkout pdh-regexp
git rebase master
git checkout master

And now I’m once more at the ready to merge stage, as seen in gitk.

At this point a normal git merge would do a fast-forward merge, returning me to the picture above on the right – so what’s needed instead, is an explicitly no-fast-forwards merge (which can never produce conflicts, because nothing else has happened to the top of master). First I’d better just check that I’m ready to merge:

git checkout master
git branch --contains HEAD

which will print a list of branches, including at least master itself, that contain the commit at the top of master. If my branch isn’t in the list, then I wasn’t, in fact, ready to merge – which means that I need to rebase on top of master again. If, though, my branch is in the list, then it’s time to do the bow-shaped merge itself:

git merge --no-ff pdh-regexp

And now I’m where I need to be, like the picture above right. [←end edits]

So it’s time to push:

git push
To peter@git.electricimp.com
! [rejected] master -> master (non-fast-forward)
error: failed to push some refs to 'peter@git.electricimp.com/ei.git'
To prevent you from losing history, non-fast-forward updates were rejected
Merge the remote changes (e.g. 'git pull') before pushing again. See the
'Note about fast-forwards' section of 'git push --help' for details.

Ahh, someone else has pushed a different feature while I was doing all that. So I need to go round the loop again.

git reset --hard origin/master

This has the effect of undoing the (trivial) merge commit, and the repository ends up like the picture on the left.

Now I can fetch the new commits to the top of master:

git pull --rebase
git checkout pdh-regexp
git rebase master
git checkout master

Of course, git rebase master might produce conflicts, which should be cleared up in the usual way. (And if the conflicts are too hard to clear up, maybe a real merge is called-for, so that history records that the project had a hard time here.) But if all goes smoothly, I end up with the picture on the right: a feature branch ready to merge, as above – the only difference being that the branch is now on top of a newer commit as the head of origin/master. So I can carry on from that point.

Of course, sometimes a feature is too small to merit all of this ... mechanism. If something goes in in just one or two commits, then it’s probably not complex enough for its presence in history to be controversial: for those, just use the pure rebase workflow. But once a feature has grown to three or four commits, it’s worthwhile making it as clear as possible, to those who follow us, what’s gone on. As for an upper limit: I’ve seen bow shapes with 20–30 commits which still looked fairly sane. Any feature which takes more commits than that, could probably do with being subdivided anyway: not least because the rebasing is probably becoming a major pain at that scale. Again, perhaps a real merge commit is called-for, and again this helps signal to future historians that here is a point at which perhaps the project was having a bit of a hard time.

Is “Factory Method” An Anti-Pattern?

Let’s take another look at this version of the “two implementations, one interface” code from that one about portability:

// Event.h v8
class Event {
public:
  virtual ~Event() {}
  virtual void Wake() = 0;
  ...
};

std::auto_ptr<Event> CreateEvent();

// Event.cpp
std::auto_ptr<Event> CreateEvent()
{
  ... return whichever derived class of Event is appropriate ...
}

What I didn’t say at the time is that this is sort-of the Factory Method pattern, though a strict following of that pattern would instead have us put the CreateEvent function inside the class as a static member, Event::Create(). And the pattern also includes designs where CreateEvent is a factory method on a different class from Event, but it‘s specifically “self-factory methods” such as Event::Create that I’m concerned with here.

(As an aside: the patterns literature comes in for a lot of criticism for being simplistic. Which it is: the GoF book could be a quarter the size if it engaged in less spoon-feeding and pedagoguery. (And by pedagoguery I mean pedagoguery.) But in a sense the simplicity and obviousness of the patterns they’re describing is the whole point: thanks to the patternistas (patternauts?), a lot of simple and obvious things that often come up in programmers’ natural discourse about code, and that didn’t previously have names, now do. Reading a patterns book might not make your code much better unless you’re a n00b, but that’s not what it’s for. It’s for making your discourse about code better. In any n>1 team, being able to discourse well about code is a vital skill.)

But what I also didn’t say at the time, is that whether CreateEvent is a member or not, so long as it’s in event.h, this code appears to have cyclic dependencies — to be a violation of the principle of “levelisability” set out in the Lakos book.

What’s going on, as you can see on the left, is that, although the source file dependencies themselves don’t exhibit any cycles, viewing, as Lakos does, each header and its corresponding .cpp file as forming a component — the three grey rectangles — produces a component dependency graph with cycles: win32/event ↔ event ↔ posix/event.

One way around that would be to move CreateEvent out into its own component — a freestanding event factory — as seen on the right. With this change, the design is fairly clearly levelisable at both the file level and the component level. This refactoring is an example of what Lakos (5.2) calles escalation: knowledge of the multifarious nature of events has been kicked upstairs to a higher-level class that, being higher-level, is allowed to know everything. (The file event.cpp now gets a question-mark because, as the implementation file for what may now be a completely abstract class, it might not exist at all — or it might exist and contain the unit tests.)

But is it worth it? We’ve arguably complicated the code — requiring users of events to know also about event factories — for what’s essentially the synthetic goal of levelisability: synthetic in the sense that it won’t be appearing in any user-story. Any subsequent programmer working on the code would be very tempted to fold the factory back into event.h under the banner of the Factory Method pattern.

Moreover, in this case the warning is basically a false positive: if Event is truly an abstract class (give or take its factory method), then the apparent coupling between, say, posix/event and event is not in fact present: posix/event can be unit-tested without linking against event.o nor win32/event.o. (Not that, in this particular example, posix/event and win32/event would both exist for the same target — but factory methods obviously also get used for cases where both potential concrete products exist in the same build.) Though conversely, if Event had any non-abstract methods — any with implementations in event.cpp — then it’d be a true positive not a false positive, as all the different event types would be undesirably link-time coupled together.

One reason that the refactoring is worth it, is the same sort of reason that fixing compiler warnings, i.e. altering the code so they don’t trigger, is worth it, even in instances when the warning doesn’t point out a bug: because if you let warnings proliferate, real ones will get lost in the noise, and ideally you aim for the zero-warnings state in order that the introduction of any new warning is an easily-visible alert telling you that there’s a new potential bug to check for. Steve Maguire is talking here about unit tests, but the same applies to compiler warnings: “[W]hen they corner a bug, they grab it by the antennae, drag it to the broadcast studio, and interrupt your regularly-scheduled program”.

Exactly like compiler warnings, cyclic-dependency warnings — which are really design warnings — are sometimes false positives, but likewise it’s worth aiming for the “zero design warnings” state, because it makes new design warnings stand out so. I ran a cycle-checker script (in the spirit of the ones in Lakos) over my own Chorale project, and the result was that it effectively shone a spotlight on all the parts of the code where the design was a bit icky. Every cycle it produced — one was dvb::Service ↔ dvb::Recording, another was all the parts of db::steam depending on each other in a big loop — was a place where I’d at some time thought, “Hmm, this isn’t quite right, but it works for now and I’ll come back and do it properly”. And of course without anything to remind me, I never had gone back and done it properly.

So it turns out that you can’t have both factory methods and levelisability. You have to pick one or the other. And levelisability is better.