11 ms·

What a waste of time and effort. Squash when merging to master and be done with it. Every PR/commit merged to master should be a clean logical unit. That means

by _hao 5y ago

What a waste of time and effort. Squash when merging to master and be done with it. Every PR/commit merged to master should be a clean logical unit. That means not fixing a bug in a branch where I'm doing something else. Those will be two separate PR's. The second problem shouldn't exist IMO.

mttjj 5y ago
Bingo! This strategy has worked well for my org (75+ engineers) for years. If an engineer makes non-related bug fixes in the same branch we require them to revert the change and make a new branch. We also have each GitHub repo configured so the "Squash and Merge" option is the ONLY option available when merging a PR. I don't care one lick what someone's branch history looks like. If they want to commit every day, or every hour, or after every keystroke - I don't care. All I know is that once the PR is merged, it's all going to be squashed into a logical unit so the `main` commit history will look just fine.
Supermancho 5y ago
aka Premature process optimization. Dont make work upfront, that is rarely important. When it is important, find the commit and split it up (as necessary) at that singular point (instead of across all PRs). You have now cut down how much time it takes to make PRs while retaining the same end result.
jcranmer 5y ago
The problem comes when you have a related set of changes where you both want to see how everything eventually fits together and where you still want to keep small "clean logical units." You can see this kind of thing play out frequently in, for example, Linux changesets, where you might have a 24-patch series of changes that need to go in for a feature.
- laserbeam 5y ago
  There's no such thing as "clean logical units". There's a product you work on. There are bugs. The prodct needs some features, good UX, performance requirements. Spending effort on managing git is mental effort you don't spend on solving your actual problems. By far the best experience I've ever hadeith git was: everyone works straight on the dev branch, just rebase, fix your stuff, test often, and if you're doing some multi-day work then sure, branch and think it over then merge. That's it. That's all you need. I've had a million more problems with every attempt at making this process "clean", or "smart". Dumb was by far more efficient, more enjoyable, helped us find and fix bugs faster, and had the shortest time to market ever.
  - ninkendo 5y ago
    If you're in a large organization, you're expecting other engineers to make sense of the work you're submitting. Anything that helps them here is a good thing (although it's always a tradeoff.) The advice I try to live by, is that however messy my work was leading up to a PR, I make sure the end result is something somebody can review without additional context. Commits should have lengthy descriptions of changes that describe the "why" and "how" of a particular change, in a way that makes it easy to digest for a reviewer. Sometimes multiple commits make sense (like if you're renaming a module/class, put that in a single commit, then put the actual code change in the next one), sometimes they're not necessary. But it's worth it to put in the effort here if it means it helps a reviewer, IMO.
  - yencabulator 5y ago
    A clean history of small atomic commits is great for software archeology. That is going to be needed for debugging anything complex enough.
garyrule 5y ago
This is what we're doing as well. No merged commits to master
ninkendo 5y ago
I think you underestimate just how much stupid garbage I have in my commit history. It's embarrassing. Even if you squash when merging the PR, the squashed commit message is gonna have a ton of messages that look like "Why the hell isn't this compiling?", "wtf?", "FINALLY PASSES!" etc etc etc. I could avoid creating those commits in the first place, but asking me to only commit my local changes when I have something intelligent to say about them, is a damn near impossibility for me. I basically use git to "save my work" before I try an approach to something. I reset back if it doesn't work out. Sometimes I reset back again, if the approach that didn't work out turned out to be the least worst option. I create commits to experiment with something, so that I can quickly compare back and forth between two approaches. etc. etc. etc. I would instead say, that if you're only making commits when you have something logical to say, you're probably not using git to its fullest potential. You should really go nuts with it IMO, and only bother to sound intelligent in your commit messages once you're ready to do so, and (most importantly) you should still make commits before that happens. Git's decentralized for a reason, take advantage!
- kristaps 5y ago
  It's true that messy commits happen, that's why squash is there. Nothing forces you to keep the messy commit messages either - keep the short commit message and make sure it's good, then delete the combined individual commit messages from the long message field, done.
  - ninkendo 5y ago
    My point is that I don't want anyone to see my WIP commits in the first place. Squashing only in the end when merging to master means the code reviewers get to see my messy commit history, which is what I'm trying to avoid. (Yes, my commit history is that bad that it's embarrassing. But at least I commit early and often, which has saved my ass more times than I can count.)
    - digisign 5y ago
      Not in gitlab at least. Click on squash and you see nothing but the total of the branch difference, the merge req title, and a few lines of boilerplate it adds. About as easy as it gets. As titles are issue numbers changes are documented automatically.
cerved 5y ago
What's the benefit of pretending everything happened in one discrete commit? I'm not sure I see the value, only information destroyed
- digisign 5y ago
  There's much less value in commits where the tests are broken.
  - cerved 5y ago
    not every single commit needs to pass every single test besides CI only needs to test the merge commit
    - yencabulator 5y ago
      If every commit passes tests, tools like `git bisect` are much more useful.
      - cerved 5y ago
        how do you mean?
        yencabulator 5y ago
        If you find a new bug, git bisect can very quickly find which commit introduced the bug, and understanding the origin makes fixing the bug easier. That mechanism does not work nearly as well if some commits are not in a usable state.
        cerved 5y ago
        but if that is the case why not just git bisect skip $(git merge-base main branch) ? to me, squashing the tree to simplify an edge case seems needlessly radical
Double_a_92 5y ago
This does not work well if you have long-lived branches (i.e. weeks) for more substantial features. Completely squashing it would lose all the granular commits and especially their commmit messages, which might be useful for debugging later on.
- benbruscella 5y ago
  Cherry pick those commits into another set of PRs
  - Double_a_92 5y ago
    I don't quite follow. E.g. at work with have feature branches with dozens of meaningful commits that we worked like 2-3 weeks on before merging to master. My point was that I don't want to squash the information contained in all those commits. I assume people here have different workflows, where they work alone on small features for maybe 1-2 days and then just squash all their tiny WIP commits into one?
- _hao 5y ago
  You are implying the original branch holding that history will be deleted when squashed into a single commit for a PR. That's not the case (granted, that's based on settings on GitHub, Azure DevOps whatever you're using, but the option is there) because in the PR you still see which branch the changes come from. If you're not deleting the branches you're good. Even if you delete the branch when merging I think most PR views nowadays will keep that info (the commits) in perpetuity. Again, I don't think this is a problem.
  - Double_a_92 5y ago
    Are you sure? My common usecase when debugging something is "Why is this line of code like this, and who might know something about it?". So then I git blame that file and see when exactly that line was modified. If that line is inside some huge blob of changes (from weeks of work) inside one commit, that is less useful to me. Even if the original branch is still floating around somewhere, that would still be an extra hassle.
nhaehnle 5y ago
> Every PR/commit merged to master should be a clean logical unit. The issue is one of review scaling. I wrote a blog post about this a while ago[0], but the gist of it is that those clean logical units are often too small for meaningful high-level reviews of more complex work. With complex features or refactorings, you're often in a situation where those clean logical units allow reviewers to do a good low-level review (do a check for logic corner cases, style issues, etc.) but they don´t allow a high-level review of how all the pieces of the feature work together. IMHO the most open-source process friendly solution to the issue is to review patch series, where you can review the series as a whole for the big picture, but also dig into individual commits for the details. Building such a patch series requires an approach as described in the article. (In closed source environments, you may get a good enough approximation of the result with a separate, disciplined software design process.) [0] http://nhaehnle.blogspot.com/2020/06/they-want-to-be-small-they-want-to-be.html http://nhaehnle.blogspot.com/2020/06/they-want-to-be-small-t...
rpowers 5y ago
Yeah I'm also confused why making multiple unrelated changes is celebrated with a special gitflow. Two changes == Two PRs.

by _hao 5y ago

mttjj 5y ago

Bingo! This strategy has worked well for my org (75+ engineers) for years. If an engineer makes non-related bug fixes in the same branch we require them to revert the change and make a new branch. We also have each GitHub repo configured so the "Squash and Merge" option is the ONLY option available when merging a PR. I don't care one lick what someone's branch history looks like. If they want to commit every day, or every hour, or after every keystroke - I don't care. All I know is that once the PR is merged, it's all going to be squashed into a logical unit so the `main` commit history will look just fine.

Supermancho 5y ago

aka Premature process optimization. Dont make work upfront, that is rarely important. When it is important, find the commit and split it up (as necessary) at that singular point (instead of across all PRs). You have now cut down how much time it takes to make PRs while retaining the same end result.

jcranmer 5y ago

The problem comes when you have a related set of changes where you both want to see how everything eventually fits together and where you still want to keep small "clean logical units." You can see this kind of thing play out frequently in, for example, Linux changesets, where you might have a 24-patch series of changes that need to go in for a feature.

laserbeam 5y ago
There's no such thing as "clean logical units". There's a product you work on. There are bugs. The prodct needs some features, good UX, performance requirements. Spending effort on managing git is mental effort you don't spend on solving your actual problems. By far the best experience I've ever hadeith git was: everyone works straight on the dev branch, just rebase, fix your stuff, test often, and if you're doing some multi-day work then sure, branch and think it over then merge. That's it. That's all you need. I've had a million more problems with every attempt at making this process "clean", or "smart". Dumb was by far more efficient, more enjoyable, helped us find and fix bugs faster, and had the shortest time to market ever.
- ninkendo 5y ago
  If you're in a large organization, you're expecting other engineers to make sense of the work you're submitting. Anything that helps them here is a good thing (although it's always a tradeoff.) The advice I try to live by, is that however messy my work was leading up to a PR, I make sure the end result is something somebody can review without additional context. Commits should have lengthy descriptions of changes that describe the "why" and "how" of a particular change, in a way that makes it easy to digest for a reviewer. Sometimes multiple commits make sense (like if you're renaming a module/class, put that in a single commit, then put the actual code change in the next one), sometimes they're not necessary. But it's worth it to put in the effort here if it means it helps a reviewer, IMO.
- yencabulator 5y ago
  A clean history of small atomic commits is great for software archeology. That is going to be needed for debugging anything complex enough.

garyrule 5y ago

This is what we're doing as well. No merged commits to master

ninkendo 5y ago

I think you underestimate just how much stupid garbage I have in my commit history. It's embarrassing. Even if you squash when merging the PR, the squashed commit message is gonna have a ton of messages that look like "Why the hell isn't this compiling?", "wtf?", "FINALLY PASSES!" etc etc etc. I could avoid creating those commits in the first place, but asking me to only commit my local changes when I have something intelligent to say about them, is a damn near impossibility for me. I basically use git to "save my work" before I try an approach to something. I reset back if it doesn't work out. Sometimes I reset back again, if the approach that didn't work out turned out to be the least worst option. I create commits to experiment with something, so that I can quickly compare back and forth between two approaches. etc. etc. etc. I would instead say, that if you're only making commits when you have something logical to say, you're probably not using git to its fullest potential. You should really go nuts with it IMO, and only bother to sound intelligent in your commit messages once you're ready to do so, and (most importantly) you should still make commits before that happens. Git's decentralized for a reason, take advantage!

kristaps 5y ago
It's true that messy commits happen, that's why squash is there. Nothing forces you to keep the messy commit messages either - keep the short commit message and make sure it's good, then delete the combined individual commit messages from the long message field, done.
- ninkendo 5y ago
  My point is that I don't want anyone to see my WIP commits in the first place. Squashing only in the end when merging to master means the code reviewers get to see my messy commit history, which is what I'm trying to avoid. (Yes, my commit history is that bad that it's embarrassing. But at least I commit early and often, which has saved my ass more times than I can count.)
  - digisign 5y ago
    Not in gitlab at least. Click on squash and you see nothing but the total of the branch difference, the merge req title, and a few lines of boilerplate it adds. About as easy as it gets. As titles are issue numbers changes are documented automatically.

cerved 5y ago

What's the benefit of pretending everything happened in one discrete commit? I'm not sure I see the value, only information destroyed

digisign 5y ago
There's much less value in commits where the tests are broken.
- cerved 5y ago
  not every single commit needs to pass every single test besides CI only needs to test the merge commit
  - yencabulator 5y ago
    If every commit passes tests, tools like `git bisect` are much more useful.
    - cerved 5y ago
      how do you mean?
      - yencabulator 5y ago
        If you find a new bug, git bisect can very quickly find which commit introduced the bug, and understanding the origin makes fixing the bug easier. That mechanism does not work nearly as well if some commits are not in a usable state.
        cerved 5y ago
        but if that is the case why not just git bisect skip $(git merge-base main branch) ? to me, squashing the tree to simplify an edge case seems needlessly radical

Double_a_92 5y ago

This does not work well if you have long-lived branches (i.e. weeks) for more substantial features. Completely squashing it would lose all the granular commits and especially their commmit messages, which might be useful for debugging later on.

benbruscella 5y ago
Cherry pick those commits into another set of PRs
- Double_a_92 5y ago
  I don't quite follow. E.g. at work with have feature branches with dozens of meaningful commits that we worked like 2-3 weeks on before merging to master. My point was that I don't want to squash the information contained in all those commits. I assume people here have different workflows, where they work alone on small features for maybe 1-2 days and then just squash all their tiny WIP commits into one?
_hao 5y ago
You are implying the original branch holding that history will be deleted when squashed into a single commit for a PR. That's not the case (granted, that's based on settings on GitHub, Azure DevOps whatever you're using, but the option is there) because in the PR you still see which branch the changes come from. If you're not deleting the branches you're good. Even if you delete the branch when merging I think most PR views nowadays will keep that info (the commits) in perpetuity. Again, I don't think this is a problem.
- Double_a_92 5y ago
  Are you sure? My common usecase when debugging something is "Why is this line of code like this, and who might know something about it?". So then I git blame that file and see when exactly that line was modified. If that line is inside some huge blob of changes (from weeks of work) inside one commit, that is less useful to me. Even if the original branch is still floating around somewhere, that would still be an extra hassle.

nhaehnle 5y ago

> Every PR/commit merged to master should be a clean logical unit. The issue is one of review scaling. I wrote a blog post about this a while ago[0], but the gist of it is that those clean logical units are often too small for meaningful high-level reviews of more complex work. With complex features or refactorings, you're often in a situation where those clean logical units allow reviewers to do a good low-level review (do a check for logic corner cases, style issues, etc.) but they don´t allow a high-level review of how all the pieces of the feature work together. IMHO the most open-source process friendly solution to the issue is to review patch series, where you can review the series as a whole for the big picture, but also dig into individual commits for the details. Building such a patch series requires an approach as described in the article. (In closed source environments, you may get a good enough approximation of the result with a separate, disciplined software design process.) [0] http://nhaehnle.blogspot.com/2020/06/they-want-to-be-small-they-want-to-be.html http://nhaehnle.blogspot.com/2020/06/they-want-to-be-small-t...

rpowers 5y ago

Yeah I'm also confused why making multiple unrelated changes is celebrated with a special gitflow. Two changes == Two PRs.