Hacker News new | past | comments | ask | show | jobs | submit login

But how often that ideal is true? It assumes that developers don't make mistakes in any of the commits, or never add "temporary" code which is removed in later commits of the feature branch. This is the reason that I tend to review a single diff, so that I don't have to browse through numerous commits and check if some line of code in a commit ends up being in the final merge diff.



No, it assumes that developers clean up their branch history as necessary before submitting it for code review.


That seems like a lot of work just to make code review easier. Of course, you can clean up the branch history by squashing commits, but that's the same as reviewing the complete merge diff..


Not just code review. It also significantly helps reviewing history later, which is done for any number of reasons, including tracking down where bugs were introduced, understanding the reason for a given change, etc.

I've worked in projects that always keep a clean history. And I've worked in projects where developers don't bother cleaning up history before pushing. And in every non-trivial project that follows the latter I've always hit cases where I try to track down something through the history only to find that the history doesn't capture the meaning of the code. This is not just bad merge behavior, but also things like lumping unrelated changes together into a single commit.

Even something just as simple as reading recent history to keep up with the changes being made to the codebase is significantly simpler if developers are diligent about keeping a clean history. For example, the Rust project has a lot of activity, but also has a strict code review policy that means that nearly all commits are done with a clean history (very occasionally, long-lived branches are allowed through that have control merges, but those are rare). And as a result, even though I no longer have the time to actively contribute, I've still managed to keep with every non-trivial change made to Rust merely by periodically reading through the history since my last pull. I can't imagine trying to do that if the history weren't clean.


Arguably the only reason it could be a lot of work is when the submitter doesn't really understand the code that multiple mixed up and muddled commits have resulted in. By giving your reviewer a mess, you're only asking for the reviewer to make sense of it instead of doing it yourself.

Instead, breaking up the feature into logical pieces allows you to documented every step properly in a separate commit message.

If you properly understand what you're submitting for review, then it isn't really much work at all. It's even easier if, during original development, you commit often. But you can always split commits up during rebasing, too.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: