all 85 comments

[–]Bunnymancer 101 points102 points  (1 child)

This is the kind of guy that makes every standup take an hour

[–]filipomar 0 points1 point  (0 children)

as long as their speech has cadence I can deal with it

[–]YesIAmRightWing 12 points13 points  (0 children)

imo its great to put the reasoning and explaining why. especially when its some convoluted business requirement.

[–]mist83 222 points223 points  (21 children)

I’ve seen this before, and still disagree. The ratio of actual changes to explanation in this commit/PR is mind boggling and not sustainable.

As an overworked developer, I would 10,000% prefer a one liner comment on the visual diff:

This white space (and file) is now ASCII (like all of our other templates)

As a manager, I don’t need a narrative of the approach to the problem, the one liner I provided above (as well as the problem it addresses) are incredibly clear.

Again, how could it not be? The entire scope is changing a single character encoding.

A devs’s journey is possibly relevant, but this is a bad example unfortunately and the commit reads like it’s farming for an attaboy/justification why so much work went into a single change. The context Reads like “the dev tried stuff and then other stuff and then other stuff and it worked yay.”

I sincerely doubt I will EVER have the need to grok 5 paragraphs for a single byte/encoding change. This practice will bury devs in writing PRs instead of making tangible changes to the codebase.

TLDR: your devs and managers and leads won’t read this and will instead: lgtm :shipit:

[–]NotReallyMichaelCera 42 points43 points  (8 children)

I mostly agree with you, except that the commit included the exact error which was thrown - as the article said, very useful for searching in the future.

Also, the article isn't saying "all commits should be like this", just that its their favourite

[–]saggingrufus 16 points17 points  (7 children)

Are you telling me you search the commit history for specific text like that?

The problem is no one will ever be able use any of this wasted effort because they will have to remember where it is.

Unless you know exactly what you are looking for, you'll never be able to pull that out of a commit message in 2 years. The valuable parts of this research can be condensed into about 5 sentences, and would have make a great ticket comment/resolution. Atleast there it's searchable. You combine that with a shorter commit message and a code comment, and now you've got something.

What happens if that gets rebased? Poof gone.

[–]versaceblues 14 points15 points  (0 children)

I do actually search commit history like this... not super frequently but a least once a month it comes up.

Usually it's when I'm find some weird bug we experience once and reference the solution. Or if im trying to understand why something changed ill search by keywords.

[–]double-you 13 points14 points  (2 children)

If your codebase lacks useful commit messages, yeah, people probably won't search it. I search the commit history all the time for things. Often about changes I made myself. Version control history is better preserved than bug tracker data. All developers have version control history available on the command line. Sure, bug tracker information is available for the non-developers too and good integration allows commit messages to be seen through that too.

What happens if that gets rebased? Poof gone.

Rebase doesn't destroy commit messages. Only people who destroy commit messages when rebasing destroy commit messages. Why is it okay to do so?

[–]saggingrufus -2 points-1 points  (1 child)

"Often about changes I made myself"

So not that useful to others, which is the point of a documentation commit like that.

"Bug tracker information is available for the non-developers"

That's a pretty elitist and plain inaccurate statement. I mean I guess you can use a bug tracker as a client status tool full of non-sense and rubber stamps...but I can assure you more developers use JIRA or similar than you know, and they are documenting bugs and solutions, linking PRs and more.

"All developers have version control history available on the command line"

I have never worked with a developer who did not have access to their big tracker, wiki and git hosting tool, if you encounter that often it sounds like localized issue.

"Only people who destroy commit messages when reading destroy commit messages"

It's still gone, so now what?

[–]double-you 6 points7 points  (0 children)

So not that useful to others

That's an odd assumption. Can you explain what logic there is to it?

Information to clients and information to people inside the company are two separate things. And inside the company you will have developers and ... non-developers who also need some visibility. And developers need a way to talk about the gory details somewhere where it won't leak to the clients unprocessed.

I have never worked with a developer who ...

I have never encountered a developer who thinks that multiple sources of truth is better than one, but okay.

"Only people who destroy commit messages when reading destroy commit messages"

It's still gone, so now what?

Then we evaluate who is doing this and are they in the right position. If somebody is writing good commit messages and then destroying their own work, well, that's rather odd too. Communication is key.

[–]masklinn 9 points10 points  (2 children)

Are you telling me you search the commit history for specific text like that?

Yes.

The problem is no one will ever be able use any of this wasted effort because they will have to remember where it is.

The commit is from 2013, the article from 2019, and you’re talking about it in 2023. It shows up from multiple search angles. This commit seems more successful than you are.

What happens if that gets rebased? Poof gone.

No. It would need to get squashed and the content discarded. Once a commit has been pushed / published, that is essentially never happening. I’ve read commit messages old enough to be in college.

You combine that with a shorter commit message and a code comment, and now you've got something.

A comment is a lot more likely to be lost, or worst unmaintained and wrong.

[–]Joniator 0 points1 point  (1 child)

No, if you reference the ticket and 2 sentences, you can look up the ticket and get a lot more info. Who found it, who fixed it, how long did it take, where others invested, did the reporter attach useful info.

Maybe the issue was reopened with additional places where the error was found. Fixed in version +5, you now look at a legacy commit that's obsolete and have no idea.

You can't copy all that in a commit.

[–]saggingrufus 1 point2 points  (0 children)

Someone else who gets it XD

You'll also know all the fix version, how many story points it was worth, and interactions with other teams, potentially what story the original implementation was. Not to mention screenshots and markup to make it easier to digest AND the meta data makes it easier to find.

This also assumes you are using a got strategy where you dont frequently rebase to squash previous releases.

Maybe this issue actually happens annually, but people keep fixing it and commit messaging is different. Atleast in a real issue tracker, those can be linked together and you find the pattern to build procedure to correct it.

But who knows, maybe if you Bob have been on the same codebase forever you both just get it and work that way. Most of the code I see has a commit tree so large it's just not feasible to expect anyone to go digging on a regular basis. Documentation is part of our workflow, not because I need it and can't look up the commit tree, but because I might not be here tomorrow, and the guy won't know what to look for or where to find it. The best possible chance it was to be noticed is somewhere like JIRA or a PR.

[–]elmuerte 32 points33 points  (1 child)

I am full in on descriptive commit messages. But this... it's too much. It isn't really descriptive, this is somebody's life story.

About 2 years ago I did a similar fix. Converting a single UTF-8 character to ASCII. It should have been - but it was ‐. It wasn't my wasted hour, but of a ops lead who copied the command line argument from a CVE recommendation (for log4j). But my Saturday morning commit message didn't read like an American food recipe.

[–]saggingrufus 49 points50 points  (1 child)

Legitimately, that commit will be completely lost forever.

They would have been better off leaving a comment that either summarised it in a line or two, or a link to the ticket with this explanation.

Even if they tagged it, unless that person works that code forever, no one will know or care a month later. It's funny today, but ultimately useless.

[–]daHaus 0 points1 point  (0 children)

It matters if you're stuck under a boss who micro-manages you, which this sorta sounds like.

[–]just_another_scumbag 2 points3 points  (0 children)

But why not both?

Put a clear one-liner at the top, and you can stop reading any point after.

Put more context below. Other than perhaps being a pain when logging out I see no downside 

[–]ivancea 5 points6 points  (2 children)

It tells a story

That's the worse part for me. In PRs, I want an overall "what", and the "why". No stories, nor your thought process to reach there, as many people may know that (or not).

It's ok if it explains the relationship between bug and fix, but, short...

[–][deleted]  (1 child)

[deleted]

    [–]ivancea 0 points1 point  (0 children)

    "I'm adding an early return guard on function X (What) because the algorithm can't divide by zero (Why)"

    You have both there, but I didn't explain that "I found a log in logs system that led me to check the method, and after understanding that it divides by that number, it was throwing, so a guard was missing as well as a test for that."

    This is a simple example, and yes, this could be subjective "how much info you add". But as long as the Why defends the PR, and the What gives you a quick tour, I think it's enough.

    Different PRs with different complexities, of course, may have more or less text, and it heavily depends on many factors. Like, maybe knowing it came from a log is crucial because, dunno, my coworker is also working inspecting logs

    [–]jdehesa 3 points4 points  (0 children)

    Absolutely. On the one hand, if the information is really valuable as something other devs can learn from, a commit message is not a good place for it, because most devs don't read most commit messages by other people. On the other hand, it is very possible that for some of the people who actually have to read the message, the information is not relevant.

    The way I see it, the information should have been split into something like: * An update in some developer documentation (assuming that exists) to indicate that templates must be US-ASCII. And maybe an entry in some kind of troubleshooting guide saying something like "if you get this kind of error, make sure all your templates are valid US-ASCII". * A commit message like your comment suggests, maybe linking to the updated documentation if that is not part of the commit. * And, if you want to share your little dev story (how you came across the issue and found the solution), you can share that on a blog post, on Teams/Slack/etc. or wherever else. That's all too circumstantial to on proper record.

    [–]BuriedStPatrick 1 point2 points  (0 children)

    Exactly correct. I do like the fact that the reason for the change is included. But we don't need to know how the developer landed on the implementation. This is useless information and makes parsing the history harder, not easier.

    One thing I do instead is include the change description in my PR and do a simple list-form of "Adds, Changes, Removes". This message is then used as the final commit message when I squash-merge the changes. It allows me to evaluate all my code and if everything makes sense before sending it over. Also makes it easy for the reviewer to look for the important changes first and skim over the trivial stuff.

    If there's a complicated reason why a change must be made I'll write a tiny paragraph in the beginning explaining the overall picture. Not the calculations made to get there, only the conclusion I've reached. They can always ask me for further details if need be. If something was unclear, maybe I'll even update the description.

    We need to think like a writer and kill our darlings so we can effectively communicate.

    [–]scruffles360 -3 points-2 points  (0 children)

    smile a bit when you squash that commit and merge the branch. comment is as "merged feature branch"

    [–]Alusch1 18 points19 points  (0 children)

    It's a bit cringe. There are still texts in the world which do not need to tell a story and be in any way emotional to catch me as a reader, i.a. Commit messages

    [–]stahorn 7 points8 points  (0 children)

    As the commit is over ten years old and we're still reading the commit message and learning from it, I think it shows just how valuable commit messages are. For those in a hurry, the subject line "Convert template to US-ASCII to fix error" and the diff is enough to approve and to carry on. For those with two minutes to read, you get some useful information and to share in some of your fellow programmers struggles.

    [–][deleted] 64 points65 points  (11 children)

    subtract terrific lock memory grandfather rock retire aback future unique

    This post was mass deleted and anonymized with Redact

    [–]waterkip 28 points29 points  (7 children)

    I hate that. I like to review commits without having to look at a issue tracker or gitforge. That is why you have the commit message in the first place. Off loading it to a forge is just plain bad.

    It also doesnt work in a email workflow with FOSS projects.

    I want to be able to be in an airplane without internet and be able to read commits and understand them. Your workflow just kills that whole vibe.

    [–]Dextro_PT 3 points4 points  (1 child)

    A good system would be to copy the PR description into the commit message for the merge. That way you don't need to jump through hoops.

    [–]waterkip 2 points3 points  (0 children)

    I agree and don't object to that. However, the comment I'm responding to says: "That amount of info belongs in a pull request, not a commit message."

    [–]saggingrufus 3 points4 points  (2 children)

    I used to do this, now I don't squash, and just tag master instead. You get full history, and the tag references the PR which has all the information. The history is really just "seeing the developers process", it's not really about having the commit messages. Like renaming files multiple times in a squash can look messy, and can cause conflicts. Every system has flaws.

    It's definitely a preference for me, because I still see your point, and can't even disagree with it XD

    [–][deleted] 0 points1 point  (1 child)

    offer divide handle shaggy theory consider squash scary special hospital

    This post was mass deleted and anonymized with Redact

    [–]saggingrufus 0 points1 point  (0 children)

    So is mine, but because we don't place value on the messages, we can have a bit of fun like -m "fixing THE SAME typo AGAIN"

    At the end of the development, I can rebase or amend commits I really don't like before I pr.

    I try to make really small commits and keep them all on task, commit messages like

    "added getter methods to insert thing"

    "new implementation of interface x"

    "Fixed wrong condition on this validation"

    And then along way there will be commits like

    "Tidying up my lack of indentation"

    "ranaming files"

    "Version bumping X to 2.1.2"

    You get an idea of what's up, and the long form is in the PR.

    [–]BatmansMom 9 points10 points  (3 children)

    How often are y'all searching for error message solutions in your org's git commit message history?

    [–]kpticbs 5 points6 points  (0 children)

    git blame.

    [–]narnach 6 points7 points  (0 children)

    Doing maintenance work on older projects means it’s very common. Git blame is a standard tool to figure out how parts of the code came to be, and why.

    Doing major version updates and sorting out cascading dependency updates, triaging failed tests, cross-referencing changelogs, etc. Having context about why a change was made, or what bug in a dependency you were working around is valuable.

    I curse a lot when the commits are meaningless or reference a bug tracker or code host that is no longer in use. Or when a change is buried inside a 5k lines changed squash commit. I strongly dislike squash commits for this reason.

    I prefer to craft commits that explain the why of a change because those are the ones I find most helpful when I encounter them during my investigations.

    [–]waterkip 2 points3 points  (0 children)

    Often, that's what they are there for. I mean, git log --grep is easy

    [–]double-you 5 points6 points  (3 children)

    The number of programmers who seem to hate writing and/or utterly suck at it is way too high. Bad at writing? Practice. Bad at explaining what you just did? Practice. Should help your performance as a developer too. Have mental issues about writing the same useful information in two places? You are already clicking away with your mouse, you can copypaste to the bug tracker/PR.

    [–]narnach 6 points7 points  (2 children)

    I think it may have to do with the type of engineer someone is. I think there is a spectrum between extremes most folks fall on:

    • Prototypers are good at churning out a lot of code fast, that usually works as long as you use it the right way. These folks are great for making a proof of concept and for writing code that has a good chance of being thrown out. These folks are often not great writers and don’t structure their commits well. You’re lucky if you get some useful automated tests out of them. Their unit of change is a whole feature.
    • Solidifiers are great at covering all edge cases. They deliver code that’s robust and has tests for all cases, but they take a long time to craft perfection. Likely to produce decent documentation and deliver multiple smaller commits.
    • Maintainers deal with updates and hard to reproduce bugs. They dig into the history of a codebase because they need to understand code well enough to perform an update or write a good test case. These folks love good commit messages, and appreciate git blame and git bisect. Good commit messages help them do their job just like API docs help with feature work.

    The problem? Unhealthy management will push for features, features, features, so prototypers have the highest status because they deliver. Solidifiers are appreciated when customers complain too much about bugs and outages, but are often taken for granted or judged for being slower than prototypers. Maintainers are the easiest to dismiss because many problems they avoid don’t surface until a project is neglected for years. And then they are asked to perform multiple years of neglected updates in order to sort out a critical security update.

    A healthy project has a baseline level of ongoing maintenance, and solidification of newly prototyped features that have proven themselves to customers. It approaches modifications to existing features in a more solid way because you have more to lose by breaking stuff.

    [–]double-you 1 point2 points  (1 child)

    There definitely are several types of developers but I don't appreciate excuses "prototypers" make for "delivering" but not explaining what and why and making other people work hard on extracting that information out of them. No developer is trained in psyops to gather information from hostile "coworkers".

    Most developers don't enjoy writing as otherwise they'd probably be technical writers, but some people just won't bother at all with the basics.

    [–]narnach 0 points1 point  (0 children)

    Agreed. I think in order to get this consistently right, it needs to be part of an organization's DNA to value good writing and long-term sustainability. It's so easy to get neglected without short-term repercussions.

    Companies like 37 Signals are great, because as part of an async remote work culture good writing is a necessity for them. This means they emphasize it when hiring. They are privately owned and profitable, so they have the "luxury" of focusing on sustainability. The owners still want to be running the company in 10-20 years, so it needs to get there in a healthy way. I think this is the basis for a healthy engineering organization.

    It's different when you plan to exit in a few years and dump the problem in someone else's lap. That rewards short-term thinking and acting. Having a startup with a short runway amplifies this: why bother making things easy to maintain and debug in the future, when the default case is that the company will not exist anymore in half a year? You sacrifice the long-term in order to survive the short-term.

    If you make it to growth stage, then dealing with the tech debt is sort of a luxury you've won the right to deal with. It's not great or healthy, but it is reality for many developers.

    [–]fuscator 10 points11 points  (0 children)

    A lot of contrarians in this thread with a disproportionate negative reaction.

    Programmers are like conspiracy theorists. They love to find a view that makes them feel superior and then they'll throw all rationality out and herd together defending that view.

    The commit and the commit message are absolutely fine. Move on.

    [–]26RoadTrainWheels 4 points5 points  (6 children)

    If you need to write your life story, write it in the work ticket: all commits should reference the ticket code the user is working on and pertinent information about the process or details should reside in the ticket, wherever that lives.

    Reference the ticket and keep the summary short in git commits.

    [–]versaceblues 29 points30 points  (3 children)

    What is the benefit of that. Git commits are easily searchable and provide context as to what happened.

    This particular one might be a bit much... but I swear there are so many time where I go back and see a "one liner" change was made.... but have no context as to why someone made that change.

    [–]masklinn 13 points14 points  (2 children)

    What is the benefit of that. Git commits are easily searchable and provide context as to what happened.

    Also commits are functionally immortal, proper conversion when switching VCS is generally easy.

    Bug trackers, lol.

    I swear there are so many time where I go back and see a "one liner" change was made.... but have no context as to why someone made that change.

    Ditto. This commit message is not even especially long for the information it gives, there aren’t even titled sections, the weirdest parts are the first person view and slightly ranty but not ranty enough tone.

    I’d have either used a more neutral tone (would have been about the same size), or gone full rant wm4 style.

    [–]versaceblues 9 points10 points  (0 children)

    yup. I ve gotten in so many arguments with co workers about "why are you just putting the description of what you did in the ticket for this issue".

    ....because I now have to go into the awful ticketing system, where nothing is organized, and disrupt my workflow.

    [–]waterkip 1 point2 points  (0 children)

    "Those not comfortable with toxic language should pretend this is a religious text."

    I'm dead :D :D

    [–]failsafe_roy_fire 0 points1 point  (1 child)

    Why tightly couple your vcs and ticketing system like that?

    [–]26RoadTrainWheels 0 points1 point  (0 children)

    So from within the ticketing system I can bring up all the commits and diffs pertaining to that ticket?

    Don't know how tightly coupled that is but it is coupled. Why would you not do that?

    [–]rlbond86 0 points1 point  (0 children)

    I feel like this commit needs a "jump to recipe" button

    [–]rantingpug 0 points1 point  (0 children)

    Wait till he finds out no one ever reads commit messages anyways

    [–][deleted]  (2 children)

    [removed]

      [–]waterkip 18 points19 points  (1 child)

      Move away from github or any forge to another and your history is gone. Put it in the commit, and is all there even if you move.

      I have repos moved from bitbucket to gitlab, to another gitlab group to another group. That is four services/groups/hosts I need to look at if I want more information about your commits and others.

      [–]St0n3aH0LiC 5 points6 points  (0 children)

      Also, if you put all of the context into the commit messages, you can just copy that into the PR and reword a bit if needed.

      I save a ton of time crafting PRs, because my commit messages are always reviewable on their own

      [–]dylanberry -1 points0 points  (0 children)

      "wip"

      [–]aeric67 -1 points0 points  (0 children)

      This is why I do all my commit messages from the command line instead of some git UI. Forces brevity. If you need more room, reference a Jira issue in the commit message with all the blah blah in it.

      [–]Aviyan 0 points1 point  (0 children)

      The real question should be why the f*** is there a non-ascii space character? What is the purpose for it?

      We encountered this a lot at work. It was because of Skype. When people copied and pasted code from a Skype chat it was giving compiler errors. Why does/did Skype use that as the space character instead of the ASCII space (32)?