Hacker News Reader

Comment by jakub_g | Hacker News Reader

A commit should be like UNIX tools: should do one thing and do it well. If the commit is adding hell lots of new features, fixing bugs, and doing heavy refactoring all at once, it's way too much.

When developing say a new widget (or heavily extending an existing one), you can develop basic functionality, commit, add bells and whistles, commit, and some more stuff, commit, send for code review, do the fixes, commit, ask for re-review maybe, and then squash the commits into one commit if needed. With DVCSes like Git it's extremely easy, and reviewing smaller commits is easier than having to go through a behemoth (of course it always depends on your particular situation).

I sometimes tend to open half-baked pull requests to ask for quick early feedback if I'm doing things the right way, and what to be careful about, and continue developing in background while the more senior person takes a look at the code.

View on HN

Does that really work well in practice, though? I find that I might get the base functionality down to a workable state, but then as you add the bells and whistles you're likely to find and fix your own bugs, realize you need to refactor, etc.

You can make all those changes in distinct, clean commits, but if the reviewer works commit by commit, they have to follow the evolution of your code rather than simply review the final state. You might argue that provides a basis for a better review, but it's also much more time consuming for the reviewer.

When you find an issue with previous commit while adding new things, you can make a temporary commit, fix the issue, commit once again and reorder commits, then meld the fix into the previous one. I'm pretty comfortable with doing things like that but some guys not that much. You have to like this workflow and work from the start like this.

You can of course split a huge commit into smaller ones using sth like `git add -p` but it's way more work than just doing lots of small commits regularly and then squashing them into medium-sized commits before the review. I like to do temporary commits regularly as "checkpoints", meaning, "this code works", and anytime I make a stupid mistake and break something, I can analyze a short diff to find the bug I just introduced rather than trying to figure out the bug just by looking at the code. It's usually a lot faster for me.

You don't have to push immediately to review: you commit a few commits locally, until you're sure of the architecture. Once you're sure, start pushing the earlier ones out to review.

If you're really unsure of the design, then tackle that in a design doc first. Discuss what the API will look like, write out sample call sites to show its use. Otherwise, yeah, you'll probably end up re-writing it.

Small commits have another benefit: they can be moved (cherry picked, for example to backport) or reverted easily.

When the feature is complete and ready for review, just put the final commit id in with the ticket. Compare it to some version before ticket was being worked on?

I think it's easier to look at two different versions of checkins, than to hoard code and make a single huge checkin.

Reply on news.ycombinator.com