Hacker News Reader

Comment by ricardobeat | Hacker News Reader

> Proper style? Formatting. Imperative voice. Capitalization. Hard wraps.

Correct, those don't matter much. On the other hand, describing what the change is about matters a lot.

It is 10x easier to review a merge request when you can see discrete steps that make sense (ex: update method X, update route Y that calls X, update all code paths that use route Y, update client to handle new data) vs one big jumble of "stuff", "fix it", "update getCustomers" that touches 27 places at once.

View on HN

If the update is going to touch 27 places at once, how is the developer meant to communicate such a change thru commit messages?

By grouping the changes logically (by system, context, feature, up to you), as in the example I gave. Sometimes it's as simple as putting backend & frontend changes into separate commits for easier review.

Of course you still need an overarching description in the merge request - should be really easy to write, when you have all the steps taken right in front of you.

This assumes that people work in logical, sequential chunks. I don't. I do a little here, a little there, and then it all starts to form the larger picture.

Not necessarily, I don't think anyone works linearly, but you can still keep your commits organized. I might have 25 dirty files and a bunch of WIP commits; when I'm done I will reset everything, and take the time to stage them one by one (or `git add -p`) into commits that make sense, and will help a reviewer understand what's happening. It's worth the extra few minutes.

For a more concrete example, say you're adding a new screen to a mobile app, I'll probably split into chunks like these:

    - add new `/foo` endpoints to API
    - add `NewScreen` (can be reviewed as a feature, does the screen contain / do what it's suppose to)
    - add `NewScreen` to navigation stack
    - link to `NewScreen` from settings page
    - add tracking for `NewScreen`

Each of these have very different purposes, and I find being able to focus on the different aspects (business logic, infrastructure stuff, analytics) helps a lot. They can also be reverted cleanly if needed without destroying all of the work.

You can still review the entire PR at once, with the option to drill down into individual commits if desired.

Assuming your original commits are atomic but not complete (that is each feature / chunk of work is spread over multiple commits as you changed things up or fixed things you’d forgotten) `rebase -i` might be significantly superior to resetting and redoing everything by hand.

That's why working on a branch, and once ready, taking the time to reorganize your commits into logical steps, is a basic act of kindness towards the people who will review your changes, and towards your own future self.

You don't need to work in logical, sequential chunks. You just should organize the work after the fact before making the PR. I work the same way.

I would argue that if you can't communicate the purpose of the change in a simple commit message then you have probably combined too many things into the commit.

Breaking this rule is one of my bad habits and it's annoying for my colleagues to review and increases the risk of including some unintentional change ("I'll just edit this config file for some local testing...") as it gets lost in the noise.

>If the update is going to touch 27 places at once, how is the developer meant to communicate such a change thru commit messages?

I make the commit messages also for myself, how many time you find some code and wonder why is it like that, then you search git and find if lucky a nice message with a ticket id and if you are lucky again the ticket has clear details on what was the issue that was fixed and what behavior was requested.

What was changed, why, and link to relevant ticket(s).

To counter, any merge request with more than 2-3 commits is probably too big anyway

Not a fan of those arbitrary limits. It's all about being able to quickly understand the changes and their reason to be. If splitting the updates in smaller increments help, then so be it, and I'll take ten tidy small commits over two large ones any day.

PR guidelines - Git best practices generally - are one of those situations where you must provide concrete guidelines but also never, ever make them a hard rule. If you don't have any you'll get endless "fix" "oops" +9876/-1234 changes, and if you make them a hard rule you'll never get anything useful done as people spend all day trying to please the aesthetic preference of a team lead who probably hasn't written a more than a single two line commit in years.

Huh? The number of commits is irrelevant - the number of changed lines/ files is what matters. 20 commits can result in a couple of lines in one file changing (I've seen it happen) and 1 commit can have 100s of files changed (usually when some commonly used symbol has been renamed).

Reply on news.ycombinator.com