4 ms·

If you want to preserve separate commits you can just make them separate PRs? Small PRs are better since they're easier to review.

by klipt 3y ago

tazjin 3y ago
The problem there is that toy systems like Github's review tool don't have good flows for stacking dependent changes, and then people end up not bothering. For this reason I don't bother with Github and the like and just use Gerrit ;)
imiric 3y ago
There are many reasons for having several commits in the same PR. PRs often have a lot of overhead. They need a separate branch, CI jobs need to run, there are more notifications for everyone, separate approvals, etc. Sometimes there's a need for keeping separate commits if they're all related to a single change. Proposing them all as part of the same PR helps maintaining that context while reviewing as well. Reviewers can always choose to focus on individual commits, and see the progression easily. Sometimes it does makes sense to squash a PR if the work is part of the same change, but the golden rule of atomic commits always applies. Never just blindly squash PRs. In fact, if the PR is messy and contains fixes and changes from the review, and the PR should have more than one commit, I go back and clean up the history by squashing each change to its appropriate commit. `git commit --fixup` helps with this, so I also prefer addressing comments locally rather than via GitHub's UI. Then it's a simple matter of running `git rebase --autosquash`.
- burntsushi 3y ago
  I do the same. Given that you didn't mention git-absorb[1], I wonder if you've tried it. If not, you might be very happy to discover it. [1]: https://github.com/tummychow/git-absorb https://github.com/tummychow/git-absorb
  - imiric 3y ago
    Interesting. I'll give it a try, thanks. At first glance it reads like too much magic that I'd have to undo manually, but if it works reliably, it would save some time. Though usually it's not much work to figure out which commit to reference in `--fixup`. In short histories, I usually just use `HEAD~<n>`, instead of hashes. BTW, thanks for all your open source work. <3 You're an inspiration!
    - burntsushi 3y ago
      It is a very nice bit of magic. I've been using it for years, and I don't think it has ever given me a false positive. Plenty of false negatives though, but it tells you. And then you just fall back to what you've been doing anyway. It is literally a tool that has removed a fair amount of tedium from my workflow.
- IshKebab 3y ago
  > making each step revertible ... > CI jobs need to run A step is only revertible if the previous state has passed CI, and as you noted that only happens if it is a separate PR. The only reason to split a PR into separate commits it to make it easier to review and understand. But if it's so big you need to do that, it should be separate PRs anyway really. IMO the only time you should ever preserve branches when merging them is if they're long-lived ones that multiple people have worked on and the commits in them have passed CI.
  - atq2119 3y ago
    > But if it's so big you need to do that, it should be separate PRs anyway really. The problem with that in practice is that the commits often depend on each other and then you have a choice of: - serializing the PRs, i.e. only have one PR outstanding at a time, which increases your development latency; or - no native UI for tracking how the PRs relate to each other Yes, there are people trying to hack around the second problem. Still, the story there is far from great. The CI thing is a real issue, I'll give you that. Though it is obviously also solvable, if the will for it were to exist.
    - IshKebab 3y ago
      > serializing the PRs, i.e. only have one PR outstanding at a time, which increases your development latency I don't see a significant downside to this. It doesn't affect my development latency - if the commits depend on each serially other then you have to wait for them to be reviewed in order whether or not they are separate PRs. I do agree that GitHub/Gitlab don't support dependent PRs very well. You pretty much have to wait for one to be merged before submitting the next. Not a big problem in practice but it could definitely be improved.
      - TheCleric 3y ago
        Once you start doing serialized PRs then doing squash merges increases your chance of a nightmare. For example you have PR 1 and PR 2 (based on 1). Squash merge PR 1 into main. But while 2 is under review someone merges in 3 into main and you need to bring it into 2 to resolve some conflicts. You’re now hosed because the changes done in 1 are now present TWICE. First in 1’s commit, and also in the squash commit. Now you’re resolving all of 1’s changes as merge conflicts. This may sound like a contrived examples but it has happened to me EVERY time I’ve worked somewhere that demands squash commits. And this is one of the reasons I hate squash commits. “Makes the history prettier” vs “Rewriting the actual commit history”. Telling the truth > pretty.
mostlylurks 3y ago
If you're doing proper atomic commits, as most people critiquing squashing probably are, the overhead of making separate PRs for each would be ludicrous. It depends heavily on the situation, but in favorable conditions (greenfield development of some simple CRUD app, for instance) you can easily produce dozens of clean, atomic commits a day. As part of the same PR, those take almost no time at all to review. Put into separate PRs, you'd be wasting a lot of time and effort both on the reviewer's and reviewee's side.
burntsushi 3y ago
You can "just" do anything. The problem is that things don't "just" work that smoothly. What happens when the feature or bug fix I'm working on demands a refactor or a name change or something of that sort. I could put the refactor in a separate PR, but what if I'm not sure of the changes until I get far enough along with implementing the feature or bug fix? I might want to go back and tweak the refactor I did. So if I put up a PR as soon as the refactor was done, I'll then need to put up another PR with tweaks to it and then a third PR with the actual feature or bug fix. Or I could just put them all up in one PR together broken down by commit. Reviewers can review commit-by-commit. Or I could wait until I've finished everything, and then I'm left with submitting a single PR or splitting them into multiple PRs are submitting them simultaneously. (And dealing with stacking them appropriately.) This is of course a balancing act. Which is my point. Sometimes it makes sense to split things up into multiple PRs. But sometimes it makes sense to fatten a PR a little bit with multiple commits. You can't just say "small PRs are better." Size is but one dimension of what makes a good PR. This is why I personally use both "squash & merge" and "rebase & merge." If a PR has a bunch of commits but is really just one logical change, then I'll squash it. But if a PR has thoughtful commits, then I'll rebase it and do a fast-forward merge. My bottom line is that I try to treat the source history as well as I treat the source. The source history is a tool for communicating changes to other humans, both for review and for looking back on. Squash & merge has a place in that worldview, but so does rebase & merge.

If you want to preserve separate commits you can just make them separate PRs? Small PRs are better since they're easier to review.

by klipt 3y ago

If you want to preserve separate commits you can just make them separate PRs? Small PRs are better since they're easier to review.

tazjin 3y ago

The problem there is that toy systems like Github's review tool don't have good flows for stacking dependent changes, and then people end up not bothering. For this reason I don't bother with Github and the like and just use Gerrit ;)

imiric 3y ago

There are many reasons for having several commits in the same PR. PRs often have a lot of overhead. They need a separate branch, CI jobs need to run, there are more notifications for everyone, separate approvals, etc. Sometimes there's a need for keeping separate commits if they're all related to a single change. Proposing them all as part of the same PR helps maintaining that context while reviewing as well. Reviewers can always choose to focus on individual commits, and see the progression easily. Sometimes it does makes sense to squash a PR if the work is part of the same change, but the golden rule of atomic commits always applies. Never just blindly squash PRs. In fact, if the PR is messy and contains fixes and changes from the review, and the PR should have more than one commit, I go back and clean up the history by squashing each change to its appropriate commit. `git commit --fixup` helps with this, so I also prefer addressing comments locally rather than via GitHub's UI. Then it's a simple matter of running `git rebase --autosquash`.

burntsushi 3y ago
I do the same. Given that you didn't mention git-absorb[1], I wonder if you've tried it. If not, you might be very happy to discover it. [1]: https://github.com/tummychow/git-absorb https://github.com/tummychow/git-absorb
- imiric 3y ago
  Interesting. I'll give it a try, thanks. At first glance it reads like too much magic that I'd have to undo manually, but if it works reliably, it would save some time. Though usually it's not much work to figure out which commit to reference in `--fixup`. In short histories, I usually just use `HEAD~<n>`, instead of hashes. BTW, thanks for all your open source work. <3 You're an inspiration!
  - burntsushi 3y ago
    It is a very nice bit of magic. I've been using it for years, and I don't think it has ever given me a false positive. Plenty of false negatives though, but it tells you. And then you just fall back to what you've been doing anyway. It is literally a tool that has removed a fair amount of tedium from my workflow.
IshKebab 3y ago
> making each step revertible ... > CI jobs need to run A step is only revertible if the previous state has passed CI, and as you noted that only happens if it is a separate PR. The only reason to split a PR into separate commits it to make it easier to review and understand. But if it's so big you need to do that, it should be separate PRs anyway really. IMO the only time you should ever preserve branches when merging them is if they're long-lived ones that multiple people have worked on and the commits in them have passed CI.
- atq2119 3y ago
  > But if it's so big you need to do that, it should be separate PRs anyway really. The problem with that in practice is that the commits often depend on each other and then you have a choice of: - serializing the PRs, i.e. only have one PR outstanding at a time, which increases your development latency; or - no native UI for tracking how the PRs relate to each other Yes, there are people trying to hack around the second problem. Still, the story there is far from great. The CI thing is a real issue, I'll give you that. Though it is obviously also solvable, if the will for it were to exist.
  - IshKebab 3y ago
    > serializing the PRs, i.e. only have one PR outstanding at a time, which increases your development latency I don't see a significant downside to this. It doesn't affect my development latency - if the commits depend on each serially other then you have to wait for them to be reviewed in order whether or not they are separate PRs. I do agree that GitHub/Gitlab don't support dependent PRs very well. You pretty much have to wait for one to be merged before submitting the next. Not a big problem in practice but it could definitely be improved.
    - TheCleric 3y ago
      Once you start doing serialized PRs then doing squash merges increases your chance of a nightmare. For example you have PR 1 and PR 2 (based on 1). Squash merge PR 1 into main. But while 2 is under review someone merges in 3 into main and you need to bring it into 2 to resolve some conflicts. You’re now hosed because the changes done in 1 are now present TWICE. First in 1’s commit, and also in the squash commit. Now you’re resolving all of 1’s changes as merge conflicts. This may sound like a contrived examples but it has happened to me EVERY time I’ve worked somewhere that demands squash commits. And this is one of the reasons I hate squash commits. “Makes the history prettier” vs “Rewriting the actual commit history”. Telling the truth > pretty.

mostlylurks 3y ago

If you're doing proper atomic commits, as most people critiquing squashing probably are, the overhead of making separate PRs for each would be ludicrous. It depends heavily on the situation, but in favorable conditions (greenfield development of some simple CRUD app, for instance) you can easily produce dozens of clean, atomic commits a day. As part of the same PR, those take almost no time at all to review. Put into separate PRs, you'd be wasting a lot of time and effort both on the reviewer's and reviewee's side.

burntsushi 3y ago

You can "just" do anything. The problem is that things don't "just" work that smoothly. What happens when the feature or bug fix I'm working on demands a refactor or a name change or something of that sort. I could put the refactor in a separate PR, but what if I'm not sure of the changes until I get far enough along with implementing the feature or bug fix? I might want to go back and tweak the refactor I did. So if I put up a PR as soon as the refactor was done, I'll then need to put up another PR with tweaks to it and then a third PR with the actual feature or bug fix. Or I could just put them all up in one PR together broken down by commit. Reviewers can review commit-by-commit. Or I could wait until I've finished everything, and then I'm left with submitting a single PR or splitting them into multiple PRs are submitting them simultaneously. (And dealing with stacking them appropriately.) This is of course a balancing act. Which is my point. Sometimes it makes sense to split things up into multiple PRs. But sometimes it makes sense to fatten a PR a little bit with multiple commits. You can't just say "small PRs are better." Size is but one dimension of what makes a good PR. This is why I personally use both "squash & merge" and "rebase & merge." If a PR has a bunch of commits but is really just one logical change, then I'll squash it. But if a PR has thoughtful commits, then I'll rebase it and do a fast-forward merge. My bottom line is that I try to treat the source history as well as I treat the source. The source history is a tool for communicating changes to other humans, both for review and for looking back on. Squash & merge has a place in that worldview, but so does rebase & merge.