RFC: Favour not squashing commits over squashing commits

Development
,
1

In the past, the OpenMRS advice has been that every PR should be a single commit. This is because commits are meant to be done at the level of ideas rather than the level of work. It may take several commits to get a feature working, but every feature should ultimately be represented as a single commit

We have enforced this primarily by putting a stern reminder in the pull request template that asks users to ensure that their PR consists of a single commit only.

However, the practice of ensuring a PR consists of a single commit makes things more difficult both for developers (who have to figure out how to squash commits and potentially deal with fallout from other commits) and reviewers (who then can only see a single snapshot of the code at a time and cannot see how the code has evolved with respect to various comments that have been made on it).

Our pull request tips state:

Squash! Best practice is to commit at the level of an idea (feature/fix). While you may have made 10+ commits while you worked, before you submit a pull request, make yourself look even smarter as if you knew how to do it all at once by “squashing” your changes into one that addresses the ticket.

However, at the end of the paragraph, it reads:

So, consider squashing when creating a pull request, don’t worry about squashing subsequent commits performed in response to the reviewer(s) comments. Ultimately, the person merging the pull request can perform a “squash on merge” via GitHub to make the final result appear a single, clean commit.

We would like to reiterate this to the community. While it is nice if the initial pull request consists of only a single commit, we would discourage you from doing any further squashing once code reviews have started. Ultimately, the person who commits the code will be able to squash things down to a single commit, once the change has been accepted.

In the coming days and weeks, the pull request templates for various repositories will be changed to no longer ask developers to certify that their PR consists of only a single commit. We still strongly encourage committers to squash everything in a PR into a single commit once it is ready to merge.

@mksd @burke @dkayiwa @mogoodrich @mseaton

4 Likes
2

I apologise for the overly normative tone of this post. I’ve been reading specifications all day and I think it filtered in a bit.

1 Like
3

My understanding has always been that if you’re working on a small item like a bug fix, it’s nice to squash commits but if you’re working on this new cool fairly big feature, then it’s actually better to keep that commit history without squashing the commits.

3 Likes
4

I like this workflow:

  1. One initial commit asking for a first review.
  2. Additional commits arising from the feedback loop with the reviewer(s):
    - Feedback/comments/changes requested → further commit(s)
    - More feedback/comments/changes requested → even further commit(s)
    - etc

This way, at the moment when you merge, you can actually see the history of the work made collectively (dev + reviewers) on a ticket/PR… event though then it gets squashed.

3 Likes
5

This is kind of a matter of taste or philosophy. As I see it there are two schools of thought: one that doesn’t care how many commits there are as long each commit is clearly tied to what it’s working on and one that aims at a commit-per-feature or commit-per-ticket.

I’m pretty agnostic as to which of these is the “best”, though I think it’s a good idea if don’t merge all 50 or so commits that end up on some tickets, since it can make the commit history hard to follow.

+1. That’s a much more succinct version of what I was going for.

6

I agree with you!

1 Like
7

I agree with this, thanks @ibacher!

8

Thanks to all of you . Are we then doing some surgical operation here https://wiki.openmrs.org/display/docs/Pull+Request+Tips

9

Do you have a specific recommendation for what we change? I tweaked things slightly, but I don’t actually think the text in the Squash! item is wrong; it’s just been understood in a way that’s less helpful.

2 Likes