3 ms·

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

by 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.

by imiric 3y ago

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.