7 ms·

> why spend time on a ‘nice’ commit history in a (smallish) feature branch when you can squash merge later. Several reasons: * facilitates much better cod

> why spend time on a ‘nice’ commit history in a (smallish) feature branch when you can squash merge later. Several reasons: * facilitates much better code review discussions * enables use of git bisect to locate bugs * allows for informative commit messages associated with the changes * communicates clearly to future self about why changes were made

u801e 3y ago
Another good reason is that having a small commit that changes just one thing is a lot easier to revert without encountering conflicts, even after other features have been committed to the main/master branch.
toast0 3y ago
> * enables use of git bisect to locate bugs This is really only viable if each intermediate commit on a development branch is intended to be bug free. If that's the standard you and your team work with, that's fine, but it's not usually my standard; in a development branch, I may commit things that don't even compile, let alone work, if it's a good point to commit.
- bonzini 3y ago
  The point of the parent comment is exactly that you should clean up the history before merging to a public branch, so that you can use bisect, even if so far you had wip wip doh wip as the commit messages. The way to get there is to have a mix of proper and wip commits.
  - WorldMaker 3y ago
    If merge points are your "known good" points anyway you can just use the powers of the git dag and `git bisect --first-parent` in your main branch to just bisect the merge points. There's no need for rebase/squash and you still get useful git bisect results.
    - bonzini 3y ago
      All commits are good points and potentially useful points. Was the bug in the refactoring? In the feature itself? In the resolution of merge conflicts? You can only answer if you don't squash, and it becomes easier to fix the bug if you know the answer.
      - WorldMaker 3y ago
        Sure, but also no one particularly wants to CI every commit inside a PR, so there is a usefulness in `git bisect --first-parent` as the "first pass" of known CI points (merge commits presumably from PRs) to find the "PR that introduced the problem" and then drill down into every smaller commit to see if you can get additional bisect information (from commits that may or may not have passed CI in the first place in development work-in-progress).
        bonzini 3y ago
        I don't do full CI for every commit but I do run the relevant unit tests (or all of them depending on the change and the project) and ensure that they pass.
        dkubb 3y ago
        I think the point the GP message is making is that, prior to review/merge you extract atomic commits from your WIP that tell a clear, concise story of how the change was made. The reviewer has less built up context so by chunking it like this they can step through each commit one at a time. IMHO the expectation is that each commit would 100% pass CI, so if you decided to extract some commits and merge that early you can. This is especially useful when a 6 commit PR is reviewed, and the first 3 commits are fine but there is more feedback on the last three. The reviewer can split the first 3 good ones out, get them merged and whittle down the PR to the remaining three. The subsequent follow up will be less. IME team velocity goes up with this too, and it encourages small and easy to review commits like a Remove to be extracted and merged early. Since PRs are always as large or larger than commits, I would much rather have a specific commit flagged than have to wade through the whole PR diff. If the PR is not familiar to me, I want to increase my effectiveness narrowing down the cause, so I can fix it faster.
  - watwut 3y ago
    Frankly, people lately spend more time managing commit history then using it. Like, commit history is useful once in a year little bit, maybe, but we spend absurd amount of time trying to make it look nice.
    - gwright 3y ago
      Not my experience, nor my team's experience over almost 10 years of using this approach.
      - Chris_Newton 3y ago
        I’m firmly in your camp on this one, but I’ve noticed that advocating a tidy history gets a lot of push-back online. I think there is an element of self-fulfilling prophecy here. If a team habitually leaves a messy history behind, that history is rarely going to be useful, so naturally the team has low expectations and sees little value in doing anything to curate it. And if a team isn’t used to making an effort to curate its history, they may assume that doing so is expensive because `git rebase -i` is scary and not something they use on auto-pilot for a few seconds at a time. In other news, our developers also create several small PRs every day but each is for an incomplete change that doesn’t stand alone so we’re never quite sure which features are finished in any given build, everyone keeps complaining about being interrupted to do code reviews all the time when the code reviews have no value anyway because they always just say LGTM :+1:, and we have targets that no more than 15% of commits should break production when CI/CD deploys them and that we recover fully within an hour each time that happens. If only there were something we could do to improve all this…
        watwut 3y ago
        I worked in a team where tech lead insisted on nice history. It was a lot of effort all the time and very little to no benefit. He was lead and could influence salaries, his opinion mattered. So, in real life, people rarely pushed back. That is not the same as us sharing the same opinions tho. I became more verbal about history not being useful online.
        3y ago
  - Terr_ 3y ago
    > even if so far you had wip wip doh wip as the commit messages Aside, `git commit --fixup HEAD` is often better than `git commit -m "oops, one more thing"`, since it means you can easily `git rebase -i --autosquash`.
- seba_dos1 3y ago
  Of course I commit a lot of garbage commits that don't work, it's super useful to do so. Those never get pushed out into branches that I share to others though - why would I waste their time having them look at those? What I push out are atomic commits that make sense logically, not an external undo log of my text editor; squashing those on merge provides no benefit and only loses useful information. Squashing should happen before push, not on merge, and there's no reason to have buggy "intermediate" commits recorded in your central remote branch at all.
- u801e 3y ago
  > > * enables use of git bisect to locate bugs > This is really only viable if each intermediate commit on a development branch is intended to be bug free. git rebase has an --exec option that allows you to run a command or set of commands for each commit in the branch. You could rebase your development branch before pushing it up for review and ensure each commit passes coffee linting and tests.
bogeholm 3y ago
> facilitates much better code review discussions Hmm, I usually mark PR’s as draft until ready for review, and then I expect the discussion to be about the current state, not a previous intermediate state. Easiest with small PR’s. > enables use of git bisect to locate bugs Interesting. I know _of_ git bisect, but haven’t used it as part of my workflow. Have you found it useful to bisect commits on a feature branch (which, presumably, represents unfinished work)? > allows for informative commit messages associated with the changes I find using the PR title and accompanying info in GitHub or similar to be quite informative - that should convey the purpose of the change. > communicates clearly to future self about why changes were made See above. Perhaps we work differently, but I find it clearer to read a git history where each commit represents a single, complete feature/fix/refactor instead of intermediate steps.
sangnoir 3y ago
There are a multitude of Git workflows, and opinions on what the basic unit of change is: for some, a feature is atomic, so squash-merging feature branches is perfectly natural. > facilitates much better code review discussions This can be done while adding code to the feature branch > allows for informative commit messages associated with the changes I'm assuming you consider individual commits to be the basic unit of change? This isn't always the case. Some products are not amenable to adding features fractionally > communicates clearly to future self about why changes were made You can do that with a squash-merge too! I've noticed people who work on an evergreen deployment can afford to work on a very granular, commit-level. However, if you have to support multiple production branches concurrently and often have to cherry-pick features and fixes across them, features will naturally become the basic unit of change you will find yourself gravitating towards, and will liberally use squash-merging just to keep your sanity.

u801e 3y ago

Another good reason is that having a small commit that changes just one thing is a lot easier to revert without encountering conflicts, even after other features have been committed to the main/master branch.

toast0 3y ago

> * enables use of git bisect to locate bugs This is really only viable if each intermediate commit on a development branch is intended to be bug free. If that's the standard you and your team work with, that's fine, but it's not usually my standard; in a development branch, I may commit things that don't even compile, let alone work, if it's a good point to commit.

bonzini 3y ago
The point of the parent comment is exactly that you should clean up the history before merging to a public branch, so that you can use bisect, even if so far you had wip wip doh wip as the commit messages. The way to get there is to have a mix of proper and wip commits.
- WorldMaker 3y ago
  If merge points are your "known good" points anyway you can just use the powers of the git dag and `git bisect --first-parent` in your main branch to just bisect the merge points. There's no need for rebase/squash and you still get useful git bisect results.
  - bonzini 3y ago
    All commits are good points and potentially useful points. Was the bug in the refactoring? In the feature itself? In the resolution of merge conflicts? You can only answer if you don't squash, and it becomes easier to fix the bug if you know the answer.
    - WorldMaker 3y ago
      Sure, but also no one particularly wants to CI every commit inside a PR, so there is a usefulness in `git bisect --first-parent` as the "first pass" of known CI points (merge commits presumably from PRs) to find the "PR that introduced the problem" and then drill down into every smaller commit to see if you can get additional bisect information (from commits that may or may not have passed CI in the first place in development work-in-progress).
      - bonzini 3y ago
        I don't do full CI for every commit but I do run the relevant unit tests (or all of them depending on the change and the project) and ensure that they pass.
      - dkubb 3y ago
        I think the point the GP message is making is that, prior to review/merge you extract atomic commits from your WIP that tell a clear, concise story of how the change was made. The reviewer has less built up context so by chunking it like this they can step through each commit one at a time. IMHO the expectation is that each commit would 100% pass CI, so if you decided to extract some commits and merge that early you can. This is especially useful when a 6 commit PR is reviewed, and the first 3 commits are fine but there is more feedback on the last three. The reviewer can split the first 3 good ones out, get them merged and whittle down the PR to the remaining three. The subsequent follow up will be less. IME team velocity goes up with this too, and it encourages small and easy to review commits like a Remove to be extracted and merged early. Since PRs are always as large or larger than commits, I would much rather have a specific commit flagged than have to wade through the whole PR diff. If the PR is not familiar to me, I want to increase my effectiveness narrowing down the cause, so I can fix it faster.
- watwut 3y ago
  Frankly, people lately spend more time managing commit history then using it. Like, commit history is useful once in a year little bit, maybe, but we spend absurd amount of time trying to make it look nice.
  - gwright 3y ago
    Not my experience, nor my team's experience over almost 10 years of using this approach.
    - Chris_Newton 3y ago
      I’m firmly in your camp on this one, but I’ve noticed that advocating a tidy history gets a lot of push-back online. I think there is an element of self-fulfilling prophecy here. If a team habitually leaves a messy history behind, that history is rarely going to be useful, so naturally the team has low expectations and sees little value in doing anything to curate it. And if a team isn’t used to making an effort to curate its history, they may assume that doing so is expensive because `git rebase -i` is scary and not something they use on auto-pilot for a few seconds at a time. In other news, our developers also create several small PRs every day but each is for an incomplete change that doesn’t stand alone so we’re never quite sure which features are finished in any given build, everyone keeps complaining about being interrupted to do code reviews all the time when the code reviews have no value anyway because they always just say LGTM :+1:, and we have targets that no more than 15% of commits should break production when CI/CD deploys them and that we recover fully within an hour each time that happens. If only there were something we could do to improve all this…
      - watwut 3y ago
        I worked in a team where tech lead insisted on nice history. It was a lot of effort all the time and very little to no benefit. He was lead and could influence salaries, his opinion mattered. So, in real life, people rarely pushed back. That is not the same as us sharing the same opinions tho. I became more verbal about history not being useful online.
      - 3y ago
- Terr_ 3y ago
  > even if so far you had wip wip doh wip as the commit messages Aside, `git commit --fixup HEAD` is often better than `git commit -m "oops, one more thing"`, since it means you can easily `git rebase -i --autosquash`.
seba_dos1 3y ago
Of course I commit a lot of garbage commits that don't work, it's super useful to do so. Those never get pushed out into branches that I share to others though - why would I waste their time having them look at those? What I push out are atomic commits that make sense logically, not an external undo log of my text editor; squashing those on merge provides no benefit and only loses useful information. Squashing should happen before push, not on merge, and there's no reason to have buggy "intermediate" commits recorded in your central remote branch at all.
u801e 3y ago
> > * enables use of git bisect to locate bugs > This is really only viable if each intermediate commit on a development branch is intended to be bug free. git rebase has an --exec option that allows you to run a command or set of commands for each commit in the branch. You could rebase your development branch before pushing it up for review and ensure each commit passes coffee linting and tests.

bogeholm 3y ago

> facilitates much better code review discussions Hmm, I usually mark PR’s as draft until ready for review, and then I expect the discussion to be about the current state, not a previous intermediate state. Easiest with small PR’s. > enables use of git bisect to locate bugs Interesting. I know _of_ git bisect, but haven’t used it as part of my workflow. Have you found it useful to bisect commits on a feature branch (which, presumably, represents unfinished work)? > allows for informative commit messages associated with the changes I find using the PR title and accompanying info in GitHub or similar to be quite informative - that should convey the purpose of the change. > communicates clearly to future self about why changes were made See above. Perhaps we work differently, but I find it clearer to read a git history where each commit represents a single, complete feature/fix/refactor instead of intermediate steps.

sangnoir 3y ago

There are a multitude of Git workflows, and opinions on what the basic unit of change is: for some, a feature is atomic, so squash-merging feature branches is perfectly natural. > facilitates much better code review discussions This can be done while adding code to the feature branch > allows for informative commit messages associated with the changes I'm assuming you consider individual commits to be the basic unit of change? This isn't always the case. Some products are not amenable to adding features fractionally > communicates clearly to future self about why changes were made You can do that with a squash-merge too! I've noticed people who work on an evergreen deployment can afford to work on a very granular, commit-level. However, if you have to support multiple production branches concurrently and often have to cherry-pick features and fixes across them, features will naturally become the basic unit of change you will find yourself gravitating towards, and will liberally use squash-merging just to keep your sanity.