4 ms·

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 l

by teeray 3y 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.

whoitwas 3y ago
Agreed. A good policy is to include ticket number and a short description.
- btschaegg 3y ago
  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.
- teeray 3y ago
  I save that and one or more “Reviewed-by” trailers for the merge commit message (Yes! You can write commit messages there too!)
MichaelMug 3y ago
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
- dllthomas 3y ago
  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.
  - a_e_k 3y ago
    Yes, I'll never understand the "fix typo" commits when `git commit --amend` is so straightforward.
    - Terr_ 3y ago
      Better-still is `git commit --fixup {rev}` and `git rebase -i --autosquash`, since then the fix(up) can be placed into the most-appropriate commit, which isn't necessarily the most-recent one. Of course, that assumes a you're already got a workforce that is able to do resolve minor conflicts when something else near the typo got tweaked in a separate commit, so I'll grant that `--amend` is easier when starting out.
      - tux3 3y ago
        Better still, git-absorb figures out in which recent commit your fixups should go on its own (based on which file it touches)
    - williamdclt 3y ago
      As a reviewer, —amend means I need to reread the whole commit to see what changed. A tiny commit means it takes me seconds. My opinion is that a PR should be small enough that it’s desirable to have it squashed into a single commit at the end anyway.
      - 3y ago
deleted 3y ago
[deleted]
globular-toast 3y ago
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.
- keybored 3y ago
  > 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).
QuantumSeed 3y ago
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
- doix 3y ago
  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 https://github.com/tummychow/git-absorb
  - mb7733 3y ago
    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.

by teeray 3y ago

whoitwas 3y ago

Agreed. A good policy is to include ticket number and a short description.

btschaegg 3y ago
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.
teeray 3y ago
I save that and one or more “Reviewed-by” trailers for the merge commit message (Yes! You can write commit messages there too!)

MichaelMug 3y ago

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

dllthomas 3y ago
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.
- a_e_k 3y ago
  Yes, I'll never understand the "fix typo" commits when `git commit --amend` is so straightforward.
  - Terr_ 3y ago
    Better-still is `git commit --fixup {rev}` and `git rebase -i --autosquash`, since then the fix(up) can be placed into the most-appropriate commit, which isn't necessarily the most-recent one. Of course, that assumes a you're already got a workforce that is able to do resolve minor conflicts when something else near the typo got tweaked in a separate commit, so I'll grant that `--amend` is easier when starting out.
    - tux3 3y ago
      Better still, git-absorb figures out in which recent commit your fixups should go on its own (based on which file it touches)
  - williamdclt 3y ago
    As a reviewer, —amend means I need to reread the whole commit to see what changed. A tiny commit means it takes me seconds. My opinion is that a PR should be small enough that it’s desirable to have it squashed into a single commit at the end anyway.
    - 3y ago

deleted 3y ago

[deleted]

globular-toast 3y ago

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.

keybored 3y ago
> 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).

QuantumSeed 3y ago

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

doix 3y ago
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 https://github.com/tummychow/git-absorb
- mb7733 3y ago
  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.