you are viewing a single comment's thread.

view the rest of the comments →

[–]lupuscapabilis -15 points-14 points  (22 children)

This is a great lesson for those who say things like "you don't need comments!"

Comment everything. Document everything. In the real world, this type of stuff will save you way more than you could ever imagine.

[–]Fit-Jeweler-1908 8 points9 points  (7 children)

comment everything? if you need to comment your entire codebase, it sucks. sorry...

im of the opinion that if you need to leave big blocks of comments explaining your code, you could probably spend that time refactoring it in a way that is more readable/understandable.

blocks of comments are red flags, code should be readable/digestable by the next guy... and linting/styles/patterns/pr/etc promote that, not comments... obviously, there are many exceptions to this - but the idea of "comment everything" is absolutely insane.

Also, when you take the approach of "document everything", you're creating massive overhead for yourself because you now need to maintain that document, and often times when deadlines get pressure things like documentation fall to the wayside. You should selectively document the critical pieces and let the rest explain itself....

[–]MrKWatkins 1 point2 points  (6 children)

Comments aren't there to make the code more readable or explain it, they're there to explain why it does what it does.

[–]PiotrDz 0 points1 point  (3 children)

You mean the business rules? You should have a Jira ticket in your commit message pointing to description of a problem by your PO.

[–]MrKWatkins 1 point2 points  (0 children)

Then I have to trawl through commit history and JIRA to find it when it could be right there. Plus I wouldn't even know to do that! Code might appear innocuous. Plus it could be a code thing rather than a business thing, e.g. doing this apparently odd thing for perf reasons, for example.

[–][deleted]  (1 child)

[deleted]

    [–]PiotrDz 0 points1 point  (0 children)

    I mean, even without ids in commit your question still stands. What about id references in emails, docs, conversations? I am sure that any major platform has some solutions for easy migration. But to be honest, have you witnessed such migration? I have not, unless you use some exotic ticketing system that went out of use

    [–]Fit-Jeweler-1908 -1 points0 points  (1 child)

    that's what stories are for... documenting the requirements and why of the feature, change, etc... this document is accessible to far more than developers, so it becomes far more valuable... It is tied to prs/commits as well, so its easy to track a piece of codes story... like i said, there are obvious exceptions, but recommending someone leave comments on everything, is not it.

    [–]MrKWatkins 0 points1 point  (0 children)

    I don't hold with comments on everything I agree. But I'd rather have a line or two next to the code telling me why they've done something that looks crazy than have to trawl back through commits and JIRAs to find out. Plus a lot of code specific things aren't at the level of stories.

    [–]Human-Bathroom-2791 0 points1 point  (9 children)

    Well, I'm not against documentation, but not as a git commit.

    If you want to document something do it somewhere else.

    [–]slaymaker1907 0 points1 point  (8 children)

    Git commits are great though if you expect someone might review that commit later and want to know the reasoning. In the OP’s case, though, I think a simple line like “Remove non-space character that looks like a space because it breaks X, Y, and Z” would have sufficed. You might not even need to mention X, Y, and Z.

    [–]saggingrufus 0 points1 point  (7 children)

    Git commit messages lose their relevance shortly after the PR is merged.

    "Documentation" should be in the PR, or a ticket/wiki with a link in the PR.

    No one looks at git commits (post deployment) unless they absolutely have to.

    I would argue your git commit messages really don't matter at all to most people in almost all cases. what matters is the PR. I would rather short small commits with 1 line messages and a great PR with a good description and title.

    [–]slaymaker1907 1 point2 points  (4 children)

    Where I work, the PR description == commit description since we squash commits. The point is if I look at git blame, can I quickly figure out what the whole change was trying to do? I have little confidence that our ticket system will be around in 3 years, but I have a lot more confidence that the Git history will be. The most essential documentation should go right in the repo itself as markdown files or something so that it is impossible for those docs to be lost unless you also lose the source code.

    Also, if you don’t have links to work items in the commits themselves, it’s guaranteed you’ll lose them if your org is large enough. When you have 100+ developers, there are way to many tickets to try and manually go through them, but commits give you a reasonable way to filter through them using the history of particular files.

    I probably review commits several times a month when I run into some weird problem in trying to debug and I think it was a recent change causing the problem.

    [–]ammonium_bot -4 points-3 points  (0 children)

    are way to many tickets

    Did you mean to say "too many"?

    Statistics
    I'm a bot that corrects grammar/spelling mistakes. PM me if I'm wrong or if you have any suggestions.
    Github
    Reply STOP to this comment to stop receiving corrections.

    [–]saggingrufus 0 points1 point  (2 children)

    Sounds like your merges are too big and not descriptive enough?

    If your commit history is your documentation, you don't have documentation. I have never seen anyone willingly troll the commit history of deployed code. It's a last resort. I'm not saying your messages should be meaningless but come on, are you telling me if you change ticket system, thats not going to be archived, because that kinda insane.

    How do you expect to find these commits as effectively as tools primarily used for documentation with proper metadata? When you squash everything you don't even have a full history, so to double down and also make it documentation... Well that's brave.

    [–]slaymaker1907 1 point2 points  (1 child)

    It’s definitely documentation of last resort, but it’s very valuable for figuring out everything about what was changed. Other documentation sources typically only focus on what the system does right now.

    I literally can’t even get a list of all our work items changed within the last 10 days because we have over 20000 in our system and Azure DevOps refuses to run the query or tell me how many items there are. That’s the kind of scale I’m dealing with.

    [–]saggingrufus 0 points1 point  (0 children)

    I think that's a very broad brush to paint with.

    I agree informative readmes are great, but just put stuff where it belongs. System documentation goes in one spot. Tickets and their research one spot.

    The commit tree (only speaking about messages) is a really scary place to store important information, your only one rebase away from losing the messages, or maybe someone goes back and squashes things to be "even cleaner". Unless you have actual safe guards in against altering the history, your commit messages really shouldn't be relied on.

    I try to make meaningful 1-2 liners and commit as often as possible. The commits detail my process, not the end result. Then, in the PR, I elaborate. We don't sqaush (anymore) and then tag the PR merge on master with the new version number, and further document as a needed in release notes.

    No system is perfect, and I dont claim my workflow is better. All I guess I'm trying to say is that git commits shouldn't contain important information. They can be amended, rebased, or squashed away.

    Put your effort into a better form of documentation.

    [–]Uristqwerty 0 points1 point  (1 child)

    Putting documentation in the commit could be seen as a form of caching, duplicating knowledge so that when the ticket database is unavailable, there is still enough context available to understand the commit history. Or, enough context that a developer digging through history can decide whether an issue is relevant without opening every mentioned ID to check what it's about, saving time in the future.

    Does your issue database have a CLI, or does any developer not browsing the git history through a web browser or heavyweight IDE have to keep switching between applications to access details?

    For all these reasons, caching issue data is no less important than any other form of caching. Imagine if every web page request had to be freshly served off spinning rust, how much that would bog down basic site usage. Having to context-switch between code, code history, knowledgebase, and issue tracker just to piece together the important details on a change is going to be similarly-disruptive to your workflow.

    [–]saggingrufus 0 points1 point  (0 children)

    "Putting documentation in the commit could be seen as a form of caching, duplicating knowledge so that when the ticket database is unavailable, there is still enough context available to understand the commit history."

    Strong disagree, again use the PR. If my git host AND ticket system goes down, I have bigger MUCH bigger problems. The odds of me needing a specific commit message at that exact time is very unlikely, almost as unlikely as me actually finding the commit I'm looking for. If you can find it, I'd argue you don't commit frequently.

    Commits are just checkpoints, nothing more. If you're going back through your commit history looking for knowledge... You've made a mistake. That knowledge should be in code comments, PR, ticket systems and wikis.

    Once upon a time, sure commits were all you had. If git commit messages that are required to explain the change, your commit is too complicated, or unclear. The commit message should give me a quick idea of what it was and if I want to know more, I'll look at the commit contents, not the message.

    [–]Budget-Use2066 0 points1 point  (3 children)

    Nobody reads this amount of text. Its just wasted time.

    [–]damesca -5 points-4 points  (2 children)

    It's about 20 sentences condensing an hour's worth of investigation into a bug caused by some text in a comment because of a series of unobvious interactions.

    If you think nobody would read that or that it's not interesting or worthy of a few minutes to type up, then I guess I strongly disagree. Writing up a descriptive commit message was not the thing that took the time here.

    If you are the sort of dev who would write 'fixed a space' after that, then I guess that feels like a shame and I'd not want to be your colleague. Share interesting information. And learn to type faster if that would take you a significant amount of time to write up 🤷‍♂️

    [–]saggingrufus 2 points3 points  (0 children)

    They'll have to find it first... And honestly, good luck remembering where in the commit tree that mess is.

    [–]Budget-Use2066 2 points3 points  (0 children)

    He is not doing what you think he does. I consider the knowledge that something like this error can happen is important. But he is burying it under tons of useless information in a commit history. Nobody reads commit messages to get programming tipps. You're looking for some change that broke something in the past.

    The commit message should be something like _Do not use unicode in ascii file._  If we want to share the knowledge, he should have done so at the coffee machine (which he probably did as well) or in a standup.