Hacker News Reader

Comment by alkonaut | Hacker News Reader

If I get a PR to review I want the set of commits to be clean and represent logical changes. No “oops” commits.

What’s more, I also want only commits that relate to the feature implemented on the branch. Preferably no merges from the parent branch.

To get zero oops commits requires squashing in the branch. Note that this is independent of whether the branch is merged or squashed when the PR is completed. Squashing 10 branch commits to 3 is the important step. If those 3 are squashed to a single on the target branch is less important and is a tradeoff.

As a developer I’ll not care to maintain build/testable code on feature branches, which is my main reason for usually squashing to the target branch. For larger changes I merge (after cleaning up and interactively rebasing), but git bisect will have to use —first-parent to work.

View on HN

Having logical commits are a nice-to-have IMO, but I wouldn't be so strict. An oops commit here or there doesn't necessarily throw off one's understanding of the history of what a particular group of commits is intended to do, and there isn't necessarily anything wrong with an oops commit; it's correcting an issue in a previous commit. Besides, it's more common to look at a PR as a whole rather than as individual commits. I know that sometimes you do, but at that point "oops" commits are rather trivial to look at.

What's more important is that the commit history of the main branch remain sane. That's why you might want to squash-merge, since it mostly solves the issue of having too many extraneously-named commits in a place where one indeed may want to examine individual commits.

Honestly, I couldn't give less of a crap what sort of commits someone has on their branch as long as it's not so many that I can't possibly review all of the code.

Don't get me wrong, because I think it's great if you want to maintain that sort of standard in your own work. I don't find it particularly reasonable to expect this of anyone else because it's more of an aesthetic choice than a practical one, not that either are exclusive of the other in this case.

> Preferably no merges from the parent branch.

Yes. Always rebase. Rebase frequently.

Oopses are so trivial to fix though, it’s literally a second’s work for the author to do and costs more seconds in penalty to the reviewer. So I think it’s just a reasonable etiquette to show that you value your colleagues’ time.

Yeah, that's totally fair, and I think I generally agree. Perhaps I just see it as less of a must. I interpreted your point of view as being relatively strict. If there was some sort there was some sort of standard for always squashing oops commits on a software team I was a part of, I'd probably have an issue with it.

> If I get a PR to review I want the set of commits to be clean and represent logical changes. No “oops” commits.

+100. Though every time this comes up on HN you would be surprised by how much some people just vehemently disagree.

I will also add another requirement - all unit tests should pass at each commit in the PR. That way, you can use the rebase strategy to merge and you would still be able to bisect.

Passing unit tests on each commit is only a realistic goal if developers can run all tests locally in less than (say) an hour. If the test suite is ten hours and you have ten commits on a branch then it quickly becomes silly to bog down your build servers (or cloud bills) with building and testing the intermediate commits over 90h. Using your own machine to do it (blocking it from doing other work) is obviously not a good idea either.

> If the test suite is ten hours and you have ten commits on a branch then it quickly becomes silly to bog down your build servers

That's a fair point, but I would say that if you encourage your devs to make each commit logical, meaningful and self contained, this is worth the extra cost.

Also, you don't need to run ALL unit tests - just the ones affected by commit you are testing. This of course requires a build system with good dependency analysis.

Reply on news.ycombinator.com