16 ms·

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

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.

systemvoltage 6y ago
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.
- 0xEFF 6y ago
  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.
  - systemvoltage 6y ago
    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.
    - jkaplowitz 6y ago
      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.
      - systemvoltage 6y ago
        Agree with all you've said. Commit messages do serve a slightly different purpose of historical context.
      - cwsx 6y ago
        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?
        MaxBarraclough 6y ago
        There's now an effort to (loosely) standardise this convention: https://www.conventionalcommits.org/en/v1.0.0/#summary https://www.conventionalcommits.org/en/v1.0.0/#summary
- Mekantis 6y ago
  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.
  - RealityVoid 6y ago
    > 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.
- Matthias247 6y ago
  > 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.
- u801e 6y ago
  > 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.
- phkahler 6y ago
  >> 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.
  - alphachloride 6y ago
    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?
- phlakaton 6y ago
  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.
- greyhair 6y ago
  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.
dtech 6y ago
Spending time on commit messages that will be essentially removed when the PR is squashed seems... not very productive
- aeontech 6y ago
  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.
  - dcow 6y ago
    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.
    - mehrdadn 6y ago
      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.
      - dcow 6y ago
        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.
swsieber 6y ago
I highly doubt _you_'ll be the one to pay dividends for detailed commit messages.
dcow 6y ago
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.
u801e 6y ago
> 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.
vp8989 6y ago
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.
- rowanG077 6y ago
  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.
- gregmac 6y ago
  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 or // 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.
deleted 6y ago
[deleted]
hyeomans 6y ago
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

systemvoltage 6y ago

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.

0xEFF 6y ago
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.
- systemvoltage 6y ago
  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.
  - jkaplowitz 6y ago
    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.
    - systemvoltage 6y ago
      Agree with all you've said. Commit messages do serve a slightly different purpose of historical context.
    - cwsx 6y ago
      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?
      - MaxBarraclough 6y ago
        There's now an effort to (loosely) standardise this convention: https://www.conventionalcommits.org/en/v1.0.0/#summary https://www.conventionalcommits.org/en/v1.0.0/#summary
Mekantis 6y ago
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.
- RealityVoid 6y ago
  > 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.
Matthias247 6y ago
> 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.
u801e 6y ago
> 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.
phkahler 6y ago
>> 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.
- alphachloride 6y ago
  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?
phlakaton 6y ago
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.
greyhair 6y ago
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.

dtech 6y ago

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

aeontech 6y ago
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.
- dcow 6y ago
  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.
  - mehrdadn 6y ago
    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.
    - dcow 6y ago
      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.

swsieber 6y ago

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

dcow 6y ago

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.

u801e 6y ago

> 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.

vp8989 6y ago

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.

rowanG077 6y ago
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.
gregmac 6y ago
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 or // 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.

deleted 6y ago

[deleted]

hyeomans 6y ago

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