Hacker News Reader

Comment by deathanatos | Hacker News Reader

Heh… there are really two types of coders. Those who things commits should have a single, obvious, minimal purpose, and who will split off unrelated changes into separate commits…

… and those who tag you as a reviewer on +8,298, -1,943 commits/PRs with the commit message "JIRA-PROJ-84138".

View on HN

> … and those who tag you as a reviewer on +8,298, -1,943 commits/PRs with the commit message "JIRA-PROJ-84138".

At my workplaces, we've told people who do this to break up their larger commit into smaller ones before reviewing. If they haven't done that initially, well, their life is going to get harder for a few days.

I like it. Inflict the pain where it belongs.

Say hello to a long list of smallish commits full of random, unrelated changes and with commit messages "fixed various small issues", "continued implementing <feature>", etc

Generally it's based on the feature / ticket in JIRA or whatever software you use. If the ticket is causing a commit with +8,298, -1,943, then we'd go back and break up the ticket itself into smaller tickets and then ask the coder to assign the changes to each ticket. There is no way we'd merge changes with such large file addition/deletions.

I'm good with that! It's faster, and easier, to review many smaller PRs, than one large one, IME. (Although also IME, in actual time, larger PRs get "reviewed" faster by not getting reviewed at all.)

(I'd want a better commit message than those, though. But they might just be examples for the sake of discussion.)

I tried to start getting people to follow a rule of "if it's a cosmetic/stylistic change, so long as it passes CI, +1". (Nowadays I work in a language what has a decent auto-formatter, and CI just runs & enforces that…) There's a whole slew of similar changes that fall under that umbrella, if you can have the test for it. (I.e., if I can encode my review into a program that CI runs … then yay! For PRs that meet that, if CI is happy I'm happy.)

You can break a car into small pieces, but you won’t learn anything from a test drive if it doesn’t run.

Yes … it should run at every stage.

There are times new work does result in larger commits, like, a few hundred lines. But I've had some 20k delta PRs dropped on me and it's like, let's be honest, the review will be shallow, at best.

Cars are not like software. The software should run at the initial stage N and at all subsequent stages N+X. If it doesn't and requires some large PR to continue working, you've got a fundamental problem there.

There is a third type of coder: one doing commits with single, obvious, minimal purpose, that still sometimes end up being +8,298, -1,943 - but with a sensible, detailed message explaining what's being done and why.

This happens in environments where it takes hours for CI to let your change pass, making small commits prohibitively expensive in terms of time and infrastructure.

(And yes, I know the answer is: make it so CI that's part of review takes minutes, not hours.)

And then there are the cases where the diff is a single line, but the commit message over a hundred lines because the change (or more likely its justification) is not obvious.

That too.

I've never seen a 1:100 code to commit message lines ratio so far, but I've seen a few 2-3 line changes with a paragraph or two long explanations. I cherish those. Same if the explanation is in a comment. In fact, if I spot something like this, I tend to praise it publicly on the team chat.

I had one case where a single weird line added much earlier messed up a seemingly unrelated piece of code I've been developing. It took me a while to figure out that something is emitting compiler flags that, with surgical precision, prevent the very thing I was attempting - and then find it nested deep in the build configuration. At that point I wanted to strangle the person who put it there - but a paragraph of commentary attached to that line, plus some extra context in the commit message, made me change my reaction to "oh. OH. I see the point now.", and I ended up commending the author instead.

I’ve done 150 lines on what I think was a two-line diff before, and >50 on a one-line diff a few times. But I am known for my verbose commit messages. (After two years working on a twenty-odd-year-old commercial code base that had had a dozen or so people working on it constantly, I had around two thirds of the longest commit messages. My longest was something like 400 lines, but most of that was a list of affected class names or similar, on a diff of tens of thousands of lines from a mostly-automated refactoring.)

I’ve definitely also added multiple paragraphs of in-code comments to an otherwise-single-character change, where it’s an ongoing consideration rather than something that can reasonably be left in a commit message alone. Then my commit messages gets to be brief, directing you to read the added comment instead.

See for example commits by Jeff King on the Git project. :) It’s impossible to infer those messages from the diffs alone.

Hey, at least they referenced the (hopefully appropriate) JIRA ticket!

Reply on news.ycombinator.com