5 ms·

Every PR we have in GitHub is merged with a squash, so I'm kinda missing the value proposition here. Is it really crucial for each commit to be a nice clean uni

by dbbk 4y ago

Every PR we have in GitHub is merged with a squash, so I'm kinda missing the value proposition here. Is it really crucial for each commit to be a nice clean unit of work?

agumonkey 4y ago
you keep the feat branch but only add single merged commit to master ?
giancarlostoro 4y ago
I think it really depends on whose doing the commits, I've mostly worked with people who wont commit things that don't work so you see a lot less commits than you would expect. I prefer commits to be iterative and I prefer to see that if possible. I rather see 100 commits across dozens of files per commit vs 1 commit for 500 files.
- warmwaffles 4y ago
  Depends on the situation entirely. If I see a ton of commits like "fixing formatting" or "oops", I want those squashed away into proper commits. If the changes you make are atomic in their own right and I can check out that commit, compile it, then run tests, and it passes. That's perfect. It works good for git-bisect, but the trick is to get everyone on board in the project to do that. For public libraries I maintain, it's squash merges all the way. I like a clean history and the ability to check out that commit, compile, test, and run cleanly is perfect.
  - giancarlostoro 4y ago
    > If I see a ton of commits like "fixing formatting" or "oops", I want those squashed away into proper commits. Every team I'm in I strongly advise against such commits honestly. They don't help anyone, I emphasize being able to find things you changed if you need to find them. You can even enforce a format for commits. Funnily enough, I saw a wave of "fixes bug" "now really fixes bug" that got auto rejected by some commitcop utility at a former job, the guy was freaking out cause it wouldnt take all his changes. I guess they wanted to force him to stop doing such awful commit messages. Commit messages should be useful and historically descriptive.
    - warmwaffles 4y ago
      > Every team I'm in I strongly advise against such commits honestly. They don't help anyone, I emphasize being able to find things you changed if you need to find them. You can even enforce a format for commits. Same with me, but people still ignore it.
- P5fRxh5kUvp2th 4y ago
  plus, you can tell who doesn't use git bisect because they prefer a single large commit across 500 files to keep history clean. As opposed to a series of small commits, that would make git bisect actually useful.
  - dmatech 4y ago
    Why can't we just have some kind of "virtual squash" where the commits are preserved, but they're grouped together so you could view it as a single squashed commit if that's preferable?
    - lixtra 4y ago
      Why not keep commits as the atomic element and use (release) tags if you want a coarse overview what happened?
    - wnoise 4y ago
      That's a semi-linear history merge.
  - mixmastamyk 4y ago
    Your features are not broken down enough if one ticket requires hundreds of changes.
    - giancarlostoro 4y ago
      This is a solid argument. Except in cases where you're upgrading everything in a small to mid sized codebase that needs a major dependency updated that affects everything from syntax to other things, and cannot be done discretely.
      - mixmastamyk 4y ago
        For a Python 2 to 3 migration a few years back we did it in a branch with many commits. We didn't obsess about telling a story we just got the work done and the tests passing. Before this there was some work to bring the codebase as close to 3.x paradigms as possible. As long as the two points before and after the work are copacetic we were satisfied. In that sense it was discreet.
- deleted 4y ago
  [deleted]
stinos 4y ago
In my opinion, yes, but like everything else it's not a hard rule. But especially for refactoring I really really want to read a comprehensive story of granular enough (but not too much) commits which logically follow each other, instead of a code dump with commit message 'bugfix'. That, plus just looking at the default graphical representation most git tools out there produce for merges vs simple rebased history: yes I'll take the latter.
Nagyman 4y ago
As always, it depends. Especially for large PRs, I will go through the effort of rebasing to help the code reviewer so they can view key commits rather than a mile long scroll-fest on the GH "Files Changed" tab. It's about being a good co-worker and facilitating faster reviews.
dnsmichi 4y ago
GitLab team member here, putting my personal hat on - from my experience in using different Git workflows since 2009, a smaller clean unit of work can help with debugging and troubleshooting. It also provides a way to new team members and contributors to understand the thought process and ideation to implement a new architecture, apply performance fixes, add documentation, work with tests, additional fixes, until its final release. Most of this can be tracked within a MR/PR and the history of code reviews, etc. - even after the merge and squash and Git branch delete, not trying to argue with this functionality. :) From the Git CLI, without any reference to Git* platforms, it is not so obvious when searching for a commit that introduced a bug, e.g. using "git bisect" for binary search. Reading a 10,000 lines git diff can be harder than a smaller commit that also explains the reasoning in the commit message. Speaking from own experience and programming mistakes in a small team, focussing on clean commits and a good history tremendously helped in stressful debug situations. Until you hit a compiler regression bug, but that's a different story then ;) I'm personally still very fast on the Git CLI, but I also know that there are a variety of CLI and UI tools out there that can help with analysing large Git commits. Potentially in the future also AI assisted that tell us which change a diff caused a performance regression in a release 5 months later. Or we don't need it at all because Observability driven development enabled to see these problems before merging and code reviews, e.g. the memory leak but only when DNS fails. True story from ~2016, more in my KubeCon EU talk at https://www.youtube.com/watch?v=BkREMg8adaI https://www.youtube.com/watch?v=BkREMg8adaI and project at https://gitlab.com/everyonecancontribute/observability/cpp-dns-leaker https://gitlab.com/everyonecancontribute/observability/cpp-d...
- mixmastamyk 4y ago
  If your merge request is gigantic, that’s the problem. Easier to keep those focused than obsess about every commit.
  - dnsmichi 4y ago
    True, thanks. Some workflows can require larger merge requests, having the platforms and tools that enable smaller iterations help reduce (or eliminate) them though.
throwaway_au_1 4y ago
Never understood the appeal of squash commits at merge time, assuming the PR contains atomic, logical commits (all bets are off if your team's PR process accepts ad hoc commits..). You lose the utility of git bisect, conventional commits, etc, and also have larger, noisier commits forming your history/documentation. Is there a benefit to squash commits other than allowing developers to forget about that as they work? I may be biased against squash commits as I have spent enough time diving through garbage commit history to figure out bugs/Chesterton's fence that good commits as documentation appeals to me.
- phailhaus 4y ago
  > assuming the PR contains atomic, logical commits This is impossible to enforce or guarantee at scale. Squashing PRs, though, is practically fool-proof: PRs already represent a single, atomic unit of work that passes all CI checks and is safe to merge. No such thing is true (or should be!) of individual commits within that PR. Whether we like it or not, a branch commit really only represents a "save point" for a developer.
  - kbr- 4y ago
    What you say is impossible, we pretty successfully apply at ScyllaDB (see https://github.com/scylladb/scylladb/commits/master https://github.com/scylladb/scylladb/commits/master). I'm not sure 100% of the commits compile & pass all tests - there may be some mistakes - but generally we're in a pretty good state, and the clean git log is being successfully used for bisecting. If you want even larger scale - if I understand correctly, the Linux kernel practices a similar thing, which is where we got this practice from (ScyllaDB founders came from kernel development). And since Git was originally created to help developing Linux - that's where you want to look for good practices.
    - throwaway_au_1 4y ago
      I also found the comment you replied to a little unconvincing. The remark concerning scale in particular did not hit home, as I would guess the vast majority of teams are <10 devs which I would hardly call 'scale'. I left my previous role for several reasons but one was the constant "Microsoft does x".. Microsoft has 100k devs, we had 5. Not the same.
blktiger 4y ago
I sometimes prefer a rebase to a merge when pulling in changes from some other branch. It can be easier to deal with several smaller merges than one big one which a rebase accomplishes. This is not always true of course, so I usually start with a regular merge and if it's sufficiently complex or hard to untangle then I give a rebase a try to see if things get easier.

by dbbk 4y ago

Every PR we have in GitHub is merged with a squash, so I'm kinda missing the value proposition here. Is it really crucial for each commit to be a nice clean unit of work?

agumonkey 4y ago

you keep the feat branch but only add single merged commit to master ?

giancarlostoro 4y ago

I think it really depends on whose doing the commits, I've mostly worked with people who wont commit things that don't work so you see a lot less commits than you would expect. I prefer commits to be iterative and I prefer to see that if possible. I rather see 100 commits across dozens of files per commit vs 1 commit for 500 files.

warmwaffles 4y ago
Depends on the situation entirely. If I see a ton of commits like "fixing formatting" or "oops", I want those squashed away into proper commits. If the changes you make are atomic in their own right and I can check out that commit, compile it, then run tests, and it passes. That's perfect. It works good for git-bisect, but the trick is to get everyone on board in the project to do that. For public libraries I maintain, it's squash merges all the way. I like a clean history and the ability to check out that commit, compile, test, and run cleanly is perfect.
- giancarlostoro 4y ago
  > If I see a ton of commits like "fixing formatting" or "oops", I want those squashed away into proper commits. Every team I'm in I strongly advise against such commits honestly. They don't help anyone, I emphasize being able to find things you changed if you need to find them. You can even enforce a format for commits. Funnily enough, I saw a wave of "fixes bug" "now really fixes bug" that got auto rejected by some commitcop utility at a former job, the guy was freaking out cause it wouldnt take all his changes. I guess they wanted to force him to stop doing such awful commit messages. Commit messages should be useful and historically descriptive.
  - warmwaffles 4y ago
    > Every team I'm in I strongly advise against such commits honestly. They don't help anyone, I emphasize being able to find things you changed if you need to find them. You can even enforce a format for commits. Same with me, but people still ignore it.
P5fRxh5kUvp2th 4y ago
plus, you can tell who doesn't use git bisect because they prefer a single large commit across 500 files to keep history clean. As opposed to a series of small commits, that would make git bisect actually useful.
- dmatech 4y ago
  Why can't we just have some kind of "virtual squash" where the commits are preserved, but they're grouped together so you could view it as a single squashed commit if that's preferable?
  - lixtra 4y ago
    Why not keep commits as the atomic element and use (release) tags if you want a coarse overview what happened?
  - wnoise 4y ago
    That's a semi-linear history merge.
- mixmastamyk 4y ago
  Your features are not broken down enough if one ticket requires hundreds of changes.
  - giancarlostoro 4y ago
    This is a solid argument. Except in cases where you're upgrading everything in a small to mid sized codebase that needs a major dependency updated that affects everything from syntax to other things, and cannot be done discretely.
    - mixmastamyk 4y ago
      For a Python 2 to 3 migration a few years back we did it in a branch with many commits. We didn't obsess about telling a story we just got the work done and the tests passing. Before this there was some work to bring the codebase as close to 3.x paradigms as possible. As long as the two points before and after the work are copacetic we were satisfied. In that sense it was discreet.
deleted 4y ago
[deleted]

stinos 4y ago

In my opinion, yes, but like everything else it's not a hard rule. But especially for refactoring I really really want to read a comprehensive story of granular enough (but not too much) commits which logically follow each other, instead of a code dump with commit message 'bugfix'. That, plus just looking at the default graphical representation most git tools out there produce for merges vs simple rebased history: yes I'll take the latter.

Nagyman 4y ago

As always, it depends. Especially for large PRs, I will go through the effort of rebasing to help the code reviewer so they can view key commits rather than a mile long scroll-fest on the GH "Files Changed" tab. It's about being a good co-worker and facilitating faster reviews.

dnsmichi 4y ago

GitLab team member here, putting my personal hat on - from my experience in using different Git workflows since 2009, a smaller clean unit of work can help with debugging and troubleshooting. It also provides a way to new team members and contributors to understand the thought process and ideation to implement a new architecture, apply performance fixes, add documentation, work with tests, additional fixes, until its final release. Most of this can be tracked within a MR/PR and the history of code reviews, etc. - even after the merge and squash and Git branch delete, not trying to argue with this functionality. :) From the Git CLI, without any reference to Git* platforms, it is not so obvious when searching for a commit that introduced a bug, e.g. using "git bisect" for binary search. Reading a 10,000 lines git diff can be harder than a smaller commit that also explains the reasoning in the commit message. Speaking from own experience and programming mistakes in a small team, focussing on clean commits and a good history tremendously helped in stressful debug situations. Until you hit a compiler regression bug, but that's a different story then ;) I'm personally still very fast on the Git CLI, but I also know that there are a variety of CLI and UI tools out there that can help with analysing large Git commits. Potentially in the future also AI assisted that tell us which change a diff caused a performance regression in a release 5 months later. Or we don't need it at all because Observability driven development enabled to see these problems before merging and code reviews, e.g. the memory leak but only when DNS fails. True story from ~2016, more in my KubeCon EU talk at https://www.youtube.com/watch?v=BkREMg8adaI https://www.youtube.com/watch?v=BkREMg8adaI and project at https://gitlab.com/everyonecancontribute/observability/cpp-dns-leaker https://gitlab.com/everyonecancontribute/observability/cpp-d...

mixmastamyk 4y ago
If your merge request is gigantic, that’s the problem. Easier to keep those focused than obsess about every commit.
- dnsmichi 4y ago
  True, thanks. Some workflows can require larger merge requests, having the platforms and tools that enable smaller iterations help reduce (or eliminate) them though.

throwaway_au_1 4y ago

Never understood the appeal of squash commits at merge time, assuming the PR contains atomic, logical commits (all bets are off if your team's PR process accepts ad hoc commits..). You lose the utility of git bisect, conventional commits, etc, and also have larger, noisier commits forming your history/documentation. Is there a benefit to squash commits other than allowing developers to forget about that as they work? I may be biased against squash commits as I have spent enough time diving through garbage commit history to figure out bugs/Chesterton's fence that good commits as documentation appeals to me.

phailhaus 4y ago
> assuming the PR contains atomic, logical commits This is impossible to enforce or guarantee at scale. Squashing PRs, though, is practically fool-proof: PRs already represent a single, atomic unit of work that passes all CI checks and is safe to merge. No such thing is true (or should be!) of individual commits within that PR. Whether we like it or not, a branch commit really only represents a "save point" for a developer.
- kbr- 4y ago
  What you say is impossible, we pretty successfully apply at ScyllaDB (see https://github.com/scylladb/scylladb/commits/master https://github.com/scylladb/scylladb/commits/master). I'm not sure 100% of the commits compile & pass all tests - there may be some mistakes - but generally we're in a pretty good state, and the clean git log is being successfully used for bisecting. If you want even larger scale - if I understand correctly, the Linux kernel practices a similar thing, which is where we got this practice from (ScyllaDB founders came from kernel development). And since Git was originally created to help developing Linux - that's where you want to look for good practices.
  - throwaway_au_1 4y ago
    I also found the comment you replied to a little unconvincing. The remark concerning scale in particular did not hit home, as I would guess the vast majority of teams are <10 devs which I would hardly call 'scale'. I left my previous role for several reasons but one was the constant "Microsoft does x".. Microsoft has 100k devs, we had 5. Not the same.

blktiger 4y ago

I sometimes prefer a rebase to a merge when pulling in changes from some other branch. It can be easier to deal with several smaller merges than one big one which a rebase accomplishes. This is not always true of course, so I usually start with a regular merge and if it's sufficiently complex or hard to untangle then I give a rebase a try to see if things get easier.