Hacker News Reader

Comment by mrisoli | Hacker News Reader

I write detailed commit messages for every single commit I make(even though commits would be squashed on merges), I write detailed PR descriptions that included before/after screenshots in multiple resolutions whenever relevant. Never once did I have any indication that someone took their time to read descriptions or commit messages.

In my previous job, I received some feedback from my manager that some people complained about code quality of my work, I was dumbfounded, I have no problem with being criticised, but I at least wanted to be able to learn from it and adapt. I went back through every single PR I did at the company and found little to no comments and it made me feel personally attacked with very little evidence. I then went on my peers PRs and see what I could learn from them, most PRs included highly non-descriptive commit messages and descriptions often only included a link to a JIRA ticket which most of the time was only visible to their direct team members and not me. From that I took the position of being more demanding on not just code quality during reviews but also descriptions, I didn't manage to influence a single developer to improve documenting efforts. That was a major reason why I eventually left the company.

I continue to be a believer in taking your time to write good commit messages, I have yet to collect dividends from it however.

View on HN

Yep, no one ever reads them and the cost to benefit ratio is extremely low.

Writing good comments is far more important. Don't explain what the code does - that's what the code is for, explain the why and the background information in the code. Additional, explain what the code does at the function level or at the module level - at a much higher abstraction level basically than the line of the code.

No one ever looks at the commit messages when trying to figure out what the code does as it convolves history and state of the repository, having to contextually wrap your head around when and how this commit was added.

Inline documentation? Perfect.

I look at commit messages quite often working on a 10+ year old code base.

Bisect leads to the commit, the commit message explains why the change was made. I take the explanation from 10+ years ago and determine if the reason still applies today.

Or, I bisect and there's a low quality commit from 10 years ago from a person long since departed that simply says "fixup" and I'm dead in the water.

Commit messages are supposed to be short. "Fixed stuff" is totally wrong. I usually write "Added ability to do foo with bar when baz is true."

Commit messages aren't mutually exclusive to the inline documentation.

I am making the case that inline documentation is far more important than commit messages.

There's no general rule about what length a commit message should be, except that often people try to keep the first paragraph as a single short line for better readability in systems like GitHub and the git CLI.

When additional detail is helpful about the motivation for or gotchas related to a commit, starting a new paragraph and elaborating there is totally fine.

But I agree with you, they aren't mutually exclusive to inline documentation. The inline documentation is more important when the explanation is relevant to understanding the code which results from the commit, whereas a clear commit message is more important when the information is to illuminate the reasons for (or history surrounding) the change itself.

Agree with all you've said. Commit messages do serve a slightly different purpose of historical context.

I was taught by a previous manager always to follow: `add/update/remove/fix {feature/bug} by/with {reason} ({further explanation if required})`

This also follows what I've seen at a few companies since then. Would you say that's a general rule?

There's now an effort to (loosely) standardise this convention: https://www.conventionalcommits.org/en/v1.0.0/#summary

Ah! That seems slightly different from the format the person you're replying to described, but I have seen this version in use at one job.

There was only used by one person who gave no context on why he used that format, so it's probably no coincidence in such an example that to me the extra structure seemed to add only opacity and no real value.

I can absolutely imagine that being different in a company that used this convention widely and built tooling around it. Doubly so when a lot of the staff is junior enough that it's helpful guidance for structuring thoughts around what the commit does.

I've never seen that anywhere I've worked, though it's a readable enough format that it wouldn't bother me if someone decided to adopt it for a good reason like compatibility with parsing by dev workflow dashboards or other automatic tooling.

Though, I'm not sure it's a helpful restriction for commits that clarify, refactor, or clean up code. One can certainly reference an issue tracker number as the {feature/bug} element that gives the necessary context, but the strictness spreads the information around more widely than is natural.

If someone was adopting that convention as a workaround for helping team members learn how to communicate, they're probably going to find that it's a very incomplete workaround.

I should note this is normally done on a feature branch (with the ticket number attached). i.e. feature/{ticket-#}-{ticket-title}

When merging you can pretty easily see what ticket each commit was for. Seems superfluous to put the same information in every commit messages.

Thanks for the clarity. Yeah, I agree it would be unhelpfully repetitive if every commit in a given feature branch merge has nearly basically the same title. That's not best practice anywhere I've worked.

By contrast, referencing the ticket number (but not necessarily ticket title) is common practice in the PR message, and seems much more helpful.

"Supposed" to?

Git was made with the intention that commit messages look like whole emails, describing not only why the change was made but also the thought process behind it, why this particular solution was chosen instead of some other etc.

What you describe is a decent header, although most people would probably prefer "Add ability to" (not "Added ability to", this is a custom that goes way back before git).

Inline documentation, or comments, is something else entirely. That is a moving target that can describe intended usage, remote APIs, and hard to understand passages. Commit messages describes a particular changeset, at a fixed point in time.

Google it, and see that most people's ideas of a good commit message goes way beyond the header ("shortlog") line.

GitHub discourages making commit messages longer than a certain number of characters; it’s interesting for me to see other opinions on that.

You're talking about the title of the commit message. Yes, the title is supposed to be under 50 characters. Now add a blank line after it and you can write all you want (line wrap at 72 chars).

If you use markdown Syntax in your commit body, github and other tools will gladly render it correctly. You can grep for commit messages with "git log --grep <pattern>" and it will also search commit bodies as well. It's beautiful!

Adding on to sibling comment, on GitHub the additional lines they're talking about go in the box before the commit title.

Could be selection bias. People who comment about good commit messages are most likely people who pay attention to commit messages. That does not necessarily mean they make up a majority of committers.

Commit messages and code comments serve different purposes. The commit message should be about what you changed, so they help you find the interesting commit from a list of commit messages, whereas comments in the code should be about why the code is the way it is. Is it because of a bug in a library? Is it arbitrary? A quick fix that still has some debt? Did someone waste a lot of time on this?

Commit messages are about the change, whereas comments are about the state of the code.

Yup, way too many times I’ve encountered code that’s written in a peculiar way obviously for a reason, most likely to work around some issue, but it’s not clear what the issue is and whether it is otherwise resolved so that the peculiar code can be dropped. I bisect and it says “fix bug”, now I need to carry that crap forever or worry about reintroducing some unknown bug.

(In case someone points out a comment is more suitable: comments are less likely to survive refactors, and people who write terrible commit messages also tend to be terrible commenters. Informative commit messages supplement good comments.)

Why is the reason to peculiarity not in the comments in the code itself?

Comments aren't supposed to state the obvious and at the time it probably was. Commit messages are redundant by definition (everything said in the message is supposedly already there in the diff) so they are like magnets for stating the seemingly obvious.

> Commit messages are redundant by definition (everything said in the message is supposedly already there in the diff)

Commit messages are far easier to parse and understand compared to a moderate size diff. By reading the commit message before looking at the diff, it really helps in terms of understanding the context of the diff and what to expect.

More than that, I find moderately sized 'git' diffs in PRs to be unreadable. I frequently have to open the full code block to see what is taking place in the source.

Commit messages provide intent, and should reference the original change request to see what the commit was intended to fix.

I would say the whole chain matters.

A good change request (Jira) with defects, and how to recreate the fault, as well as relevant requirements (by reference, not cut-n-paste)

Good comments on the code that is changing. Don't document the defect and how you fixed it, just comment what the code does (if the code is not perfectly clear),

Then a good commit message, referencing the change request, to tie it all together.

It all matters.

I think the issue with comments is that they lose all temporal and situational context. You see a comment next to code, but you have no idea if it's actually still addressing that piece of code. How out of date is that comment? Nobody knows. But if you look at the git history of all the edits specific to a part of code, you'll have a full view of what happened and why and by whom. I think the big issue is that programmers aren't taught to properly leverage git in this holistic way - instead it's just "version control", when it can be such a crucial and informative and helpful part of a process around documentation, writing good code and working in a team.

> I think the big issue is that programmers aren't taught to properly leverage git in this holistic way

You are assuming here that all programmers even "do" git. A lot don't. A whole lot of them. I worked with some companies that would have some horrible horrible version control system only and I just wanted to cry. Sure, git has its warts, but once you get it, it's a tool that can be _incredibly_ productive. I hate having good tools taken away.

> Yep, no one ever reads them and the cost to benefit ratio is extremely low

I read them quite often. Whenever there is a regression detected between 2 software revisions that have been deployed somewhere the first thing I do is checking commits and commit messages for packages that have changed to find the more obvious reasons for a behavior difference. Checking code diffs at that point would be a lot more effort.

> No one ever looks at the commit messages when trying to figure out what the code does as it convolves history and state of the repository

Do people not use git blame to check what they're changing before they change it? That's how I check the history around the changes I'm going to make before I make them.

>> Yep, no one ever reads them and the cost to benefit ratio is extremely low.

You're projecting. YOU never read them. I used to be the same way. Now I see other people reading them and I've started to myself. I follow other people's efforts and progress. It's a great way to learn not only the code but to communicate better.

Yes, in the literal sense "no one ever reads them" is obviously false as you provided contradictory evidence. But I find it equally obvious that the commenter meant it figuratively.

Is there any data to support your or his claim as more representative of the population?

I can't say I _read_ commit messages all the time. I do however, _look_ for commit messages all the time.

And am frequently disappointed.

The comments I put in commits are things you'd be horrified to see littering your codebase (though I did it that way once upon a time too). They are the "whys" behind a change. Sometimes they have relatively little to do with individual lines of code, and don't need to be maintained like comment blocks should be. The temporal binding is absolutely intended.

Comments and commit messages both matter. For different reasons.

Comments are often wrong, for the wrong reasons. People that change code, but don't update the comments to reflect the code change are evil. But then, people that put to much superfluous information in comments, that should just be left to the code, are also evil.

BTW, if your code implements a part of a standard, try putting the standard, and what subsection, your code addresses. Just the reference not the text. 802.11G for example (old fart) or TA55, or RFC 1536. Whatever. Then list the date of the document and the subsection.

I had to modify a Reed-Solomon implementation many years ago, and the original two authors based the implementation on a particular textbook. They gave the ISBN and then chapter/paragraph/table/illustration references on each block. I went and got a copy of the book, and it all lined up perfectly. Best documented code I have ever encountered.

All the "code quality" people write shitty systems. You are too zoomed in if you think code quality is really important. It's kind of important, but your system likely has much more important things wrong with it than the "code quality". Things you could actually get fired for, or seriously reprimanded if the winds don't blow in your favor.

Almost all "code quality" discussion in PRs is lightweight value judgements with no rigor or consistency. Just what random bikeshedding objection that person happened to think of, maybe they had a bad night's sleep? Who knows...

Remnants of an era where most software was low stakes and had low number of users. We now have many more "-ilities" we are judged on, that are measurable and that actually matter, so ignore the code quality trolls stuck in 2005.

It seems to me you are confusing code quality with style. Code quality is most certainly important. Style less so even though it should be consistent. Almost all bike shedding comments are style.

Most of the code quality comments I make (and recieve) on PRs are about naming and comments, and they're mostly for future changes and maintenance.

Good example from a couple days ago: someone made a helper method for compressing something, with the signature

    string Compress(string value)

It was actually doing gzip followed by base64. My comment was to rename it to something like Base64Compress as well as change the documentation comment to state it was doing both. I wouldn't want someone else to inadvertently call that in the future if they stumbled across it, not realizing it was returning base64 and either double-base64 encode it, unnecessarily have base64 when not needed, or even end up with a bigger string than input because of the 33% increase. In a future PR, misuse of that Compress() method would be impossible to spot unless you happened to remember what it was actually doing.

Other times it'll be something like a constant 30s timeout hard-coded, and I like that to have comments like:

    // This usually takes <1s, so 30s timeout for this is generous

    // 30s is max or (calling code) times out anyway

Why? Because if we need to change that, the comment helps let us know it's arbitrary and safe to modify or not. Without a comment (or good non-squashed commit message) someone in the future (maybr you) is doomed to waste time rediscovering a bug you already know about or researching if it's safe to change.

People who don't care about code quality write brittle, hard-to-maintain systems - which is occasionally fine, depending on expected longevity of the system.

If you're interviewing for a job that involves any sort of maintenance over a long-lived project, organizational attitudes towards code quality are a useful smell test to how miserable it might get. I bear the scars of a couple of wholesale refactors (that cowboy coders wanted no part in, to no ones surprise) to improve readability/maintainability. Merging those gigantic PRs and having everything work was satisfying; it almost made up for the hair pulling. Also satisfying was catching a surprising number of hidden bugs caused by sloppy programming (some logical errors are obvious when you give functions, arguments and variables good names)

> Also satisfying was catching a surprising number of hidden bugs caused by sloppy programming (some logical errors are obvious when you give functions, arguments and variables good names)

I agree, and I've found the same adding tests to existing code. It often uncovers bugs that have been there for years, and there's a certain schadenfreude from doing that.

Sometimes it turns out that the bug doesn't really occur only because that function never happens to be called in a certain way. I've also run into cases where when I'm trying to figure out how to reproduce the bug for real that I uncover a another bug that is blocking it. Both of these are examples of brittle code: the right change or separate bug fix is going to expose this, and hope your QA is good enough to find it.

I've also been able to identify and fix a couple long-standing bugs this way: the kind where it's been observed in production a handful of times and in more than one deployed environment, but no one had ever figured out reliable reproduction steps. It's a great moment when suddenly you realize all this crazy behavior can be fully explained.

> I've also run into cases where when I'm trying to figure out how to reproduce the bug for real that I uncover a another bug that is blocking it.

During a JS project refactor, I ran into a couple of those "This should be broken, why is it working?" headscratchers. In my case, 4 out of 5 times was that there was an earlier "quick-fix" further up the call-chain that papered over the bug for certain use-cases, but did not generalize well; I feel that that sort of bug is harder to catch using (unit) tests.

Code quality is absolutely important, although there seems to be a lot of variation in what people really mean when they talk about code quality. To me it's proper design, readable, maintainable code, that's easy to test and easy to change. To others it means the actual unit testing itself. So some it means the style you tend to enforce through linting rules.

Style is in my opinion very arbitrary. It's good to be consistent, and for that reason I guess it's good to have rules that are enforced, but with the number of times I have to revisit my changes because apparently I'm violating some obscure style rule, and therefore the build server rejects my change (or sometimes it even rejects it because someone else's change violated something, which makes me wonder how that ever got through), it feels like it's a waste of time to be too strict about it. A bit of personal style isn't going to kill anyone.

I disagree completely. The code isn't just for the computer - a human being will need to understand and maintain it in the future, and often that human being won't be the author. Having a second person look at the code and say "I don't understand what's going on here, please make it clearer" or "this violates some convention we use for a reason" is a worthwhile exercise.

Sure it'll be inconsistent, and it'll lead to some changes that are idiosyncratic to the reviewer, while leaving other issues in place. But that's true of copy-editing as well, and I've never heard someone argue that it's not worth getting editorial feedback because that feedback might depend on the person.

Perhaps you had some experience with someone who cared about "code quality" in a way that rubbed you the wrong way, and they also designed a shitty system. Or perhaps your idea of what code quality means just differs from mine. I've spent enough time reading and fixing up left-behind cowboy code to spare a thought for the next engineer who'll have to come along and touch that system.

Please don’t be discouraged and keep on writing quality code. Most managers at big companies are soft engineers who weren't really into the engineering aspects of the job. They’re more interested in team cohesion and deliverables. In my experience the first things to go when there is a date looming over the team’s head is code quality (code review becomes code approve) and then documentation, followed by tests. It’s really up to the engineers at the end of the day to maintain the quality of their codebase. Managers don't care unless it’s so bad that they can’t onboard new people or things are constantly on fire, but I’m pretty you can onboard a new grad into almost any mess and they’ll eventually figure out how to poke the right things, and fires don’t reflect poorly on the manager.. they just happen as if acts of nature breed unstable rushed code.. so :shrug:. Keep in mind a downside of working at a bigger company is the overarching philosophy that most code will be rewritten in 2 years. Add in that most rank and file employees are just ladder climbing anyway and, well, I think you get a pretty realistic picture. The linux kernel is good on merit and demands good engineering. The exploratory product idea that probably wont be around 6 months after launch... no so much. So find a team that actually cares about the quality of their engineering, if you have that luxury. If not perhaps seek work with a smaller company or on a project that is here to stay. Things with open source components tend to calibrate well in that department in my experience because the code is the product. Also whatever the case, lead by example. It may feel futile but it does work.

> Never once did I have any indication that someone took their time to read descriptions or commit messages.

Just because others don't use a tool for documenting code doesn't mean the tool shouldn't be used. I make sure the team I work with documents the what was done and why in their commit messsages. I also make sure that, when applicable, they describe the bug that was fixed and how it was fixed in the commit, or how performance was tested by applying a certain fix in the commit message.

Several times over the years, people have gone back to those commit messages by finding them via git blame and figuring out what was done months or years ago and preventing regressions by reading the commit messages.

> Never once did I have any indication that someone took their time to read descriptions or commit messages.

I do admit that I have been tempted to do things like:

git commit --allow-empty-message

for changes and see if anyone would notice.

I highly doubt _you_'ll be the one to pay dividends for detailed commit messages.

I now write PRs descriptions and commit messages for my future self. I know that nobody in my Team reads them, it has happened to me that a question is asked and the answer was already in the description backed by a commit

Spending time on commit messages that will be essentially removed when the PR is squashed seems... not very productive

And that’s why PR squashing is counter-productive. Good luck trying to track down an issue when bisect leads you to a 1500 line commit containing an entire feature.

Repo commit history is an artifact the team produces, as much as the code it contains.

Yep. I actually think PR squashing should be something you ask of someone else when they don’t take the time to clean up their commit history. If they don’t have the discipline to do it then one big sloppy commit is better than 25... I rarely squash my PRs because I always take the time to provide meaningful context surrounding my work.

How do you convince other people of this though? That's been what I've struggled with. I hate squashing too, but I can't convince anybody.

I like to remind people that pull requests are an abstraction over a set of commits. I show people that they can click on each individual commit in a PR and see the granular delta. Surprisingly often I’ve learned that people really don't know you can do this! If they argue that you cant revert entire features remind them that merge commits are a thing. If they don't like lots of merge commits littering the history suggest that most people should be FF merging (rebase and push) anyway. In my experience git has exactly tue right tooling for many diverse workflow scenarios and the problem more often than not is people don't understand their tools. Help educate them.

If your team insists on 1 PR == 1 commit (or if you use an authoritarian tool that allows enforcing it like gh or gerrit) then be abundantly pedantic about making sure that only changes related to a single unit of work are being introduced in new PRs. I find that calling people out when they make sloppy changes starts to get them thinking that it would be really nice to just keep this typo fix in an isolated commit and not require a new PR for it. When I have seen that model be successful, it’s usually the case that the team agrees that it is more important to ask someone to break out an unrelated change than to merge a sloppy PR.

> If your team insists on 1 PR == 1 commit

Yeah that's the scenario I'm thinking about. The trouble I have with this is two things:

- People inherently don't make the PRs as granular as a commit would be, because it puts load on other processes (like CI, review overhead, etc.) and slows things down that way. So you end up with PRs that are hundreds of lines instead of self-contained commits that are dozens of lines long (or less). They still fit under the same goal (and hence PR), but aren't as granular as they could/should be, which makes them harder to revert.

- It's far easier for people not to care about their commit messages and commit granularity than to care, and honestly, with CLI tooling, I can't claim it's easy. I use TortoiseGit and rebase all the time to keep my commits organized, and it's far easier for me to shuffle around commits that way, though it's still non-negligible overhead.

On the other hand, CI is precisely one argument I get for squashing: because rebasing/merging would imply some commits are untested, others specifically don't want them in the tree (they find it misleading when they bisect). I don't really find this compelling—I find it harmful to lose so much granularity information just because you didn't run a test against it (especially when it's already not the case that every commit passes all the tests) because it makes it impossible to look back and revert a tiny commit later, but I can't really convince others the trade-off is worth it. And to make this work well and keep a good history, you need to rebase to specifically avoid spurious commits like "fixed lint" or "removed whitespace", which generates additional overhead (and makes for harder reviewing when you rebase).

My position is it should be up to the author to decide whether a PR should be merged/squashed/rebased, because the answer can be different in different situations and the author would know best, but I have a hard time convincing anybody... people would rather have a uniform workflow instead.

It’s less common but there are actually CI setups that verify each commit in a PR rather than the tip of the branch.

That seems immensely more painful for a nontrivial project though. It'll clog the CI for everyone and nobody will go back and rebase to make sure every single commit is a self-contained passing unit.

It's trivially easy to do if you run something like:

  git rebase --exec "test_command"

where test_command is one or more commands that you run to test the code for that commit. I typically do this for unit tests and integration tests.

If the tests fail for some reason, you can amend the commit to fix the test and then run

  git rebase --continue

to test the next commit.

In practice, one should be doing -all of these things- in different circumstances.

Rebase and push -- along with some squashing of redundant work-- is great. There's some degree of annoyance if intermediate states don't build, so it requires care to know that all the individual between states are probably safe (because CI, etc, generally don't help you here).

Merge commits are fine, though they do require a higher degree of acumen for people exploring the history.

And there's plenty of times when a developer submits a moderate sized 3 commit PR that whoever looks at the pull can decide that it's better to squash and make into one commit.

I share this opinion, however teams I've worked with are fond of squashing and drop words like "cleaner" or "more maintainable" without having proper evidence and I usually can't convince people to avoid this practice.

If you consistently have 1500 LOC PRs you are really doing PRs wrong. Our median PR is 150 LOC, altough we do rarely (a couple of times a year) have "jumbo" PRs of 1K-2K LOC, but making better individual commits is not going to help with understanding those. Those are the PRs that have extensive documentation and design consideration in the PR description.

I may exaggerate for effect, but 150LOC developed over two weeks is still vastly more surface area than a localized 5LOC change developed over the course of a few hours.

Of course circumstances vary and it depends on verbosity and idioms of your preferred language, framework and codebase. Maybe 150LOC is the minimum it takes to express a logical change unit.

My point is not about specific “maximum line count in a commit” but about a commit expressing a single logical change concept, and not a whole chapter of a book.

> making better individual commits is not going to help with understanding those … "jumbo" PRs of 1K-2K LOC.

This is where we disagree, it seems. You appear very categorical here, could you clarify why you think this is the case?

By default a squash would collect all commit messages.

Also, I agree with aeontech that artificially mandating PRs to be squashed is very counter productive and often hinders bisect. Large features should be implemented in atomic units as a series of commits.

I squash before I open the PR. Then I have last say in the commit message.

Also it gives me a chance to figure out what I actually worked on.

I (and my team) always run an interactive rebase before opening PR. This gives us a chance to reorder, squash and cleanup individual “work in progress” commits into a logical set of atomic, meaningful commits, without turning the entire PR into a single commit.

On my team, we will open a PR with a logical set of commits. As reviews are posted, we will address those comments by using fixup! or squash! commits (using the title of the commit we want to amend). fixup commits are used for code changes and squash commits are used for commit message changes.

In the end before merging, we'll run

git fetch orgiin

git rebase -i --autosquash --keep-empty origin/master

git log -p --reverse origin/master..

git diff @{u}..

The first command will update the remote tracking branches. The second one will start the interactive rebase and order the commits based on the fixup! and squash! tags we used during the review. The third one will show the commit messages and associated diffs for the branch so that we can check that things make sense. The last one is to verify that we didn't inadvertently introduce a change during the rebase (though we will see a diff if someone else merged a PR into master before this rebase).

will verify that no diff was introduced during the rebase compared to the upstream branch as referenced by the PR.

Thank you, learned something today :)

No problem :) One thing I forgot to mention about squash! commits was to use the --allow-empty flag when running git commit, so that we can make a commit without having to actually change any files.

Then, when doing the interactive rebase with the --autosquash and --keep-empty flags, you'll get to a point where your editor opens up with text saying that "this is a combination of X commits". At that point, you can delete the text down to the text of the last squash commit, exit the editor, and it will replace the commit message for the corresponding commit with the text that was in the squash commit message.

Use a GUI and look at it while you work.

Using a collaboration tool to figure out what actually you worked on is placing this burden on other people instead of yourself.

Reply on news.ycombinator.com