Comment by teeray
2 years ago
Remember that the more you treat commit history as something to be read, the better these tools get. If you’re just jamming “-m” on `git commit` with nonsense like “fix bug”, “pr comments”, “ci”, then you’re going to have a bad time debugging with Git. Also, if you have mandatory squash and merge, congrats, you’re destroying history all the time that can help you debug things.
This is one of the biggest issues I face at work. I always see "added X", which git can already tell me.
> Also, if you have mandatory squash and merge, congrats, you’re destroying history all the time that can help you debug things.
Well the alternative is 100s of commits like this:
fix bug
added file
fix typo
fix typo
update test
test
test
test
fix typo
added file
Ours isn’t much better. Each commit message has a mandatory ticket number you have to enter. So for details you need to jump to the ticket.
The tickets often don’t have meaningful information in them apart from ‚for details see the attachments‘
Attachments are often either not uploaded or nowadays we do not have read access as they were created by a different team working for a different customer but forwarded to the ‚central backend team’ and for know how protection purposes we are not even allowed to see the attachments.
Effectively resulting in the information that is available telling us: ‚we did stuff‘
I don't know about at your workplace, but in our (non-git) repo the intent of the ticket number isn't so you can enter details about the commit in the ticket. It's to link tickets to particular work items being tracked in the ticket system. You should still add as much detail as possible about what the commit is doing in the commit itself. If anyone really wants to, they can always aggregate all commits against a ticket to get an overall idea of the flow for that task, which might span branches or repos, and of course the tickets can be linked to higher ones tying that commit into the larger plan. But none of that is as important as explaining exactly what the commit's code changes did. So yeah, I would never use the ticket system as a replacement for commit comments.
We've migrated our ticket management system to a few vendors. So a ticket number might come from the system 3 iterations ago... and of course migration wasn't perfect. Because that's the one ticket that didn't get migrated.
> Ours isn’t much better. Each commit message has a mandatory ticket number you have to enter. So for details you need to jump to the ticket.
In my perfect world people would write self-contained commit messages. Links to non-essential elaborations is fine.
But what we have is often (ticket + some bare-bones explanation of what the code change does without explaining why). Okay, so that’s annoying that I have to click through to a slow issue tracker instance for every commit.
But maybe people just want an easily changeable (unlike immutable commits) place to write down what this is about. Is the issue tracker that? Ideally (or second-to-ideally; see first paragraph) the title and description explain what the problem is. If this was a long back and forth issue then hopefully someone has updated the description to point to exactly what the PR/commits are supposed to do. More likely though is that the issue is a stream of consciousness:
- Naive title
- Naive description
- Back and forth troubleshooting in the comments
- The tech lead shoots in with “so, i guess <discard everything> and do X”
And that’s the average good case. I’ve been complaining recently (maybe on two occasions) that I as a secondary (to the tech lead) PR reviewer can’t even easily figure out what the PR is supposed to do based on the issue.
So there is no curation or editing. Even though the issue tracker is elevated to be the source of code change truth by mandating that commits need to have the issue id in the title but almost nothing else is demanded of the commit message.
Now compare all of that to—no matter what is in the issue tracker, no matter if it has good or bad information—taking five minutes in order to write a commit message (or just a PR description which you can use in the merge commit) on a change which took in total three hours to work on with all the back and forth and testing and debugging. Now the snapshot of your understanding of the issue at the time of writing the commit/PR stays there forever, without any need for hyperlinks or external tools.
Same here and tickets most of the time come from bugs so the title and discussion (if any) there are about the problem. The commit message should be about the cause and the solution. Instead people just copy and paste bug title into the message.
Do we work at the same place
That's one alternative. Another alternative is cleaning that up in a rebase into a series of (hopefully) easy to follow individual commits that do one thing, and then a merge commit pulling in the branch with a description of the change as a whole (and a reference to the pr and any relevant tickets). There are other alternatives as well that make various tradeoffs between effort for the author, effort for the reviewer, ease of reading the git history, applicability of various tools, etc.
Yes, I'll never understand the "fix typo" commits when `git commit --amend` is so straightforward.
12 replies →
I think you start from a different opinion of what a PR looks like. You say “commits that do one thing”, but in my workplace PRs already are supposed to be small and do one thing: I _want_ them to be squashed into a single atomic commit that’s easy to revert if needed
2 replies →
Why would you write such commit messages instead of describing what changed and, if needed, why?
I find it quite rude to change a codebase and not leave an explanation in the version control metadata. Over the lifetime of the application that's the source of truth. You can type in whatever in Jira or Trello, what's in version control will be built and shipped anyway, and they're unreliable. Sooner or later someone will think it's a good idea to 'clean up' and delete stuff, or someone decides to migrate to another project management supplier and issue-tags in commit messages become dead links.
We can do rebase and squash on our side while also developing with silly commits but not pushing them.
The point of squash is that the "history" is nonsense like "fix" etc. There are two types of commit: versions and checkpoints. The latter are just to help you develop and can include stuff like "end of day" that should never end up on master. Squashing is a way to turn those into versions. Blindly squashing every branch down into one commit is stupid, though.
> The point of squash is that the "history" is nonsense like "fix" etc. […]
Using squash when appropriate is good. But then it should be generalized to “rebase” since that doesn’t ever imply a certain strategy like “squash everything”.
> Blindly squashing every branch down into one commit is stupid, though.
Yep, which is what OP is complaining about (mandatory squash).
I usually keep all my work in a stash until I'm ready to create the PR. I used to preface my intermediate commits with "WIP -" and then reset the branch and re-commit everything when I was ready to create the PR, but that was just too much effort
You don't need to reset the branch. Using rebase -i, you can squash all your WIP commits that way.
You can also use it to split and individually pick changes if you want to clean up your commits.
You can automatically fix up commits after review commits using git absorb [1].
[1] https://github.com/tummychow/git-absorb
As a big fan of interactive rebase, when cleanint up a chunk of commits I do a reset as reset the OP described (smetimes mid rebase). It's convenient for inspecting all your changes together, and you can pick and choose changes into different commits easily too... Pretty much equivalent, but I find it more straightforward for some things.
Agreed. A good policy is to include ticket number and a short description.
I save that and one or more “Reviewed-by” trailers for the merge commit message (Yes! You can write commit messages there too!)
Meh, you can easily twist that into a bad policy, too (and yes, people do that). At work, some of my colleagues litter many repos (I don't usually have to work with, thankfully) with dozens of commits having the same ticket ID and the title of the ticket as their subject. Usually pushed straight to trunk (full of foxtrot merges if two people work at the same time). And in the message body, then, there's usually the unhelpful "fix typo".
My opinion: Write readable messages, scope your commits to simple changes, put them on a branch, use `--autosquash`, put the ticket ID in the merge commit's message. It really isn't hard.