4 ms·

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, congra

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.
    - matijsvzuijlen 3y ago
      The idea is that --amend would be used before pushing the commit that it amends, and so before a reviewer ever sees the original commit.
      - pjerem 3y ago
        The idea of PR review is that the submission is imperfect and that there will be fixes. Like, "typo fix" fixes.
        dllthomas 3y ago
        Yes, but the history presented upthread looks much more like someone working before submitting a PR for review, rather than changes made in response to review. How much to amend the history at each step is several separate questions.
    - keybored 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. A `git commit --fixup` commit can be made which does that. Then when the review is done `git rebase -i --autosquash`. Like squashing but with fine-grained control.
      - Terr_ 3y ago
        Yeah, fixup commits are the answer when it comes to responses to reviewers, who can easily see just what changed since the last time. It's even better if there's some system that ensures authors remember to do the squashing, such as by prohibiting actions that would bring fixup commits into the main development branch. Personally, I use `--fixup` even before making a PR, particularly if there's some work that I want to split into "refactoring prep" versus "the new feature."
  - jcelerier 3y ago
    And then you have to go through the sysadmin team to temporarily enable push --force on your branch
- williamdclt 3y ago
  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
  - keybored 3y ago
    IMO PRs are supposed to do one thing. But they might end up doing a few more things like refactor, clean up whitespace, or even add a new function in order to facilitate the change. And all of these can be put into their own commits. Now you can make like five PRs for each of those commits. But that seems similar to making five <issue tracker> issues for those commits. You’re already there in the PR. You might not need the overhead of N external items for N commits.
  - dllthomas 3y ago
    A PR should do one thing at a higher level than a commit should do one thing (both at a different level than a function should do one thing). I expect we're agreed that most PRs should be a single commit with a small number of changes in a small number of places, but IME it's not rare to have situations where dividing the changes into groups makes things clearer but where elevating those groups to the level of PR would make things less clear.
manojlds 3y ago
We can do rebase and squash on our side while also developing with silly commits but not pushing them.
oaw-bct-ar-bamf 3y ago
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‘
- aulin 3y ago
  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.
- keybored 3y ago
  > 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.
cess11 3y ago
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.

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.
  - matijsvzuijlen 3y ago
    The idea is that --amend would be used before pushing the commit that it amends, and so before a reviewer ever sees the original commit.
    - pjerem 3y ago
      The idea of PR review is that the submission is imperfect and that there will be fixes. Like, "typo fix" fixes.
      - dllthomas 3y ago
        Yes, but the history presented upthread looks much more like someone working before submitting a PR for review, rather than changes made in response to review. How much to amend the history at each step is several separate questions.
  - keybored 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. A `git commit --fixup` commit can be made which does that. Then when the review is done `git rebase -i --autosquash`. Like squashing but with fine-grained control.
    - Terr_ 3y ago
      Yeah, fixup commits are the answer when it comes to responses to reviewers, who can easily see just what changed since the last time. It's even better if there's some system that ensures authors remember to do the squashing, such as by prohibiting actions that would bring fixup commits into the main development branch. Personally, I use `--fixup` even before making a PR, particularly if there's some work that I want to split into "refactoring prep" versus "the new feature."
- jcelerier 3y ago
  And then you have to go through the sysadmin team to temporarily enable push --force on your branch
williamdclt 3y ago
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
- keybored 3y ago
  IMO PRs are supposed to do one thing. But they might end up doing a few more things like refactor, clean up whitespace, or even add a new function in order to facilitate the change. And all of these can be put into their own commits. Now you can make like five PRs for each of those commits. But that seems similar to making five <issue tracker> issues for those commits. You’re already there in the PR. You might not need the overhead of N external items for N commits.
- dllthomas 3y ago
  A PR should do one thing at a higher level than a commit should do one thing (both at a different level than a function should do one thing). I expect we're agreed that most PRs should be a single commit with a small number of changes in a small number of places, but IME it's not rare to have situations where dividing the changes into groups makes things clearer but where elevating those groups to the level of PR would make things less clear.

manojlds 3y ago

We can do rebase and squash on our side while also developing with silly commits but not pushing them.

oaw-bct-ar-bamf 3y ago

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‘

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

cess11 3y ago

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.