Git Commit Structure – How to Handle Commits When Refactoring for Unit Tests

Architecturedesigndesign-patternsgitversion control

I'm trying to get a review for my lists of pros/cons about how to structure commits that came out of a discussion at my work.

Here's the scenario:

I have to add feature X to a legacy code base
The current code base has something I can't mock making unit testing feature X impossible
I can refactor to make unit testing possible, but it results in a very large code change touching many other non-test classes that have little in common with feature X

My company has the following strictly enforced rules:

Each and every commit must be stand alone (compiles, passes test, etc.) We have automation that makes it impossible to merge until these have proven to pass.
Only fast-forward merges are allowed (no branches, no merge commits, our origin repository only has a a single master branch and it is a perfectly straight line)

So the question is how to structure the commits for these 3 things. (refactoring, feature X, and test for feature X) My colleague referred me to this other article but it doesn't seem to tackle the refactoring part. (I agree without the refactoring source and test should be in one commit)
The article talks about "breaking git bisect" and "making sure every commit compiles/passes" but our strict rules already cover that.
The main other argument they give is "logically related code kept together" which seems a bit to philosophical for me.

I see 3 ways to proceed. I'm hoping that you can either a) add to it b) comment on why one of the existing pro/cons is not important and should be removed from the list.

method 1 (one commit): includes feature X, test for feature X, and refactoring

pros:

"Logically related code kept together" (Not sure this is actually a "reason". I would probably argue all 3 methods do this, but some may argue otherwise. However, no one can argue against it here).
If you cherry-pick / revert without merge conflict, it will probably always compile & pass tests
There is never code not covered by test

cons:

Harder to code review. (Why is all this refactoring is done here despite not being related to feature X?)
You cannot cherry-pick without the refactoring. (You have to bring along the refactoring, increasing chance of merge conflict and time spent)

method 2 (two commits): one includes feature X, then two includes refactoring and test for feature X

pros:

Easier to code review both. (Refactoring done only for the sake of testing is kept with the test it is associated with)
You can cherry-pick just the feature. (e.g. for experiments or adding feature to old releases)
If you decide to revert the feature, you can keep the (hopefully) better structured code that came from the refactoring (However, revert will not be "pure". See cons below)

cons:

There will be a commit without test coverage (even though it's added immediately after, philosophically bad?)
Having a commit without test coverage makes automated coverage enforcement hard/impossible for every commit (e.g. you need y% coverage to merge)
If you cherry-pick only the test, it will fail.
Adds load to people wanting to do revert. (They needed to either know to revert both commits or remove the test as part of the feature revert making the revert not "pure")

method 3 (two commits): one includes refactoring, two includes feature X and test for feature X

pros:

Easier to code review the second commit. (Refactoring done only for the sake of testing is kept out of feature commit)
If you cherry-pick / revert either without merge conflict, it should compile & pass tests
There is never code not covered by test (both philosophically good and also easier for automated coverage enforcement)

cons:

Harder to code review the first commit. (If the only value of the refactoring is for test, and the test are in a future commit, you need to go back and forth between the two to understand why it was done and if it could have been done better.)
- Arguably the worst of the 3 for "logically related code kept together" (but probably not that important???)

So based on all this, I'm leaning towards 3. Having the automated test coverage is a big win (and it what started me down this rabbit hole in the first place). But maybe one of you has pros/cons I missed? Or maybe there's a 4th options?

Best Answer

When working on existing code, it's common that you need to refactor the code before you can implement your feature.

This is the mantra from Kent Beck: "Make the change easy (warning: this may be hard), then make the easy change"

To do so, I usually recommend to do frequent little commits. Take baby steps. Refactor progressively:

Each refactoring doesn't change the way the code works, but how it's implemented. It's not "hard to review" since both implementation are equally valid. But the new implementation will make it easier for the change to be made.

Finally, write the test and make it pass. It should be relatively short and to the point. That also makes the commit easier to read.

Therefore I'd go for the 3rd option too. Maybe I'd even have multiple refactoring commits. Or I'd squash them into one before pushing that for review, so there's only one. Or maybe I'd do a first PR that's only refactoring, then a second that's only the feature. It really depends on how much refactoring is needed (keep your PRs short) and your team conventions!

If the only value of the refactoring is for test, and the test are in a future commit, you need to go back and forth between the two to understand why it was done and if it could have been done better

To solve this problem, you need to get your team comfortable in this approach: refactor first, then implement the feature.

I'd suggest you to discuss it with your colleagues and try that out. I'd also recommend you try to practice "over-committing" to get you in the habit of doing smaller commits. It's a useful skill to have when code is tricky, so it's a great exercise to do when code is not!

In any case, I think you've healthy discussion with your colleagues. No doubt you'll find what works for your team!

Related Solutions

Code Review Strategy Before Merging to Master from Feature Branches

There's a variation of your 1st option:

merge master to fb_#1 (not fb_#1 to master) to make it as up-to-date as possible
A teammate reviews changes between master at the point you merged and fb_#1 head
If fb_#1 is ok we merge fb_#1 to master
quick check that the merge is ok

eg.

... ma -- ... -- mm -- ... -- mf  <- master
      \            \         /
       f1 ... fn -- fm -----      <-- fb_#1

where:

ma is the ancestor of master and fb_#1.
fn is the last change on your branch
mm is the commit that was master/HEAD at the time you merged onto your branch (giving fm).

So, you compare mm and fm in your initial review, and then quickly check after merging back mf to make sure nothing significant changed on master during steps 1-3. This seems to have all of the pros and none of the cons for the initial review.

This assumes the review is quick compared to the normal frequency of changes pushed to master, so fm -> mf would often be a fast-forward.

If that is not the case, for whatever reason, the cons will just move from the initial review to the post-merge review, and it may be simpler just to merge directly onto master and do a single review there.

Git – Why aren’t there cherry-pick requests

The thing about pull requests is that it makes known that there are changes that someone wants to bring into the project.

If the owner/maintainer wants to cherry pick parts of the pull request, they can do that from that pull request.

And just because there is a pull request does not mean that the maintainer is not allowed, or incapable, of doing a rebase.

So this is indicative of a style that may be wanted in the history.

You can always display the history without the merge history, or even the other way around.

git log --merges
git log --no-merges

So it boils down to, I think:

The project owner wanting to enforce, or not, a certain style of history
The fact that you can get either set of information easily from Git itself, regardless.

You also mention that "A one-commit change is probably the most common case for pull requests" but I am not sure about that.

For one, the number of commits may be unknown due to the developers habits of doing micro-commits and then rebasing them. Also, it is common for me to see project owners asking for those commits to be squashed in the CONTRIBUTING.txt file, or other communication.

Edit: Of course, I can't answer the question of why doesn't X, Y and Z companies do something. Not sure anyone can, except for those entities.

Best Answer

Related Solutions

Code Review Strategy Before Merging to Master from Feature Branches

Git – Why aren’t there cherry-pick requests

Related Topic