3 ms·

> nobody has absolutely any interest in what happened on your development branch I absolutely do. Every lead I've ever worked with absolutely does. If you merg

> nobody has absolutely any interest in what happened on your development branch I absolutely do. Every lead I've ever worked with absolutely does. If you merge an unnecessarily big commit to master, you are potentially making life very difficult in the future. If you make a meandering series of commits during development, squash them for the PR. But please, please do not squash the entire feature upon merge. Also, I'm not sure how rebase could be confusing except in the case where there are multiple commits with big conflicts, but that's rare, and you can make exceptions if the developer is really that unsure with git in those rare cases (or ask for help).

CamouflagedKiwi 3y ago
I disagree. It's a nice property that every commit in master is (conceptually) fully buildable & all tests passed. I've not found much benefit from having heaps of tiny commits and trying to work out their state; also cherry-picking (if needed) is simple with squash commits, but extremely difficult without.
- nemetroid 3y ago
  Of course all commits should be buildable and pass tests. There’s a scale between ”heaps of tiny commits” and ”squash everything”.
  - dllthomas 3y ago
    All commits that will be shared. There's nothing wrong with a local WIP commit when you want to plant a flag to compare your next changes against.
- lolinder 3y ago
  That's why they said this: > If you make a meandering series of commits during development, squash them for the PR. But please, please do not squash the entire feature upon merge. I typically turn my PRs into a series of buildable and testable commits. I also put a lot of work into making those commits tell a useful story to the reviewer and to anyone doing git blame later, and squashing them all into one commit undoes that work.
  - dllthomas 3y ago
    Buildable and testable, but notably sometimes not formatted or linted, when (atypically but not rarely) the semantic change is much easier to understand separated from the superficial changes it motivates in the context of the formatting/linting rules of the project.
    - lolinder 3y ago
      Yep. And in a similar vein, not squashing allows you to separate a file move from changes to it so that git can track the move.
63stack 3y ago
Why are you interested in looking at individual commits? What can you possibly learn from that that you can't get from looking at the entire diff the PR is introducing? Does your CI test each individual commit? Afaik most of them only test the top commit. How do you know/enforce that all the inbetween commits also build/pass tests? How do you know how many commits to revert, if you need to revert the feature? Instead of reverting 1, now you have to revert N where N is not recorded anywhere.
- Snild 3y ago
  > Why are you interested in looking at individual commits? Because they can explain their individual rationales, while still making the most sense to merge all together. > What can you possibly learn from that that you can't get from looking at the entire diff the PR is introducing? Ease of review (both before merge, and in the future when wondering why something was done a certain way). Saves me as a reviewer from having to guess which parts of the commit are meant to do what. This, of course, means that every commit needs to be a reasonable change in itself -- fixup commits done while developing should be squashed into the original change with a local rebase (these are the "meandering" commits your parent post mentioned). > How do you know/enforce that all the inbetween commits also build/pass tests? I'm no CI expert, but I would hope that most systems allow this as an option. > How do you know how many commits to revert, if you need to revert the feature? Instead of reverting 1, now you have to revert N where N is not recorded anywhere. It's recorded in the merge, assuming you always make a merge commit. Otherwise, since each commit is actually its own logical change, you figure it out the same way as you would figure it out in the "squash PR" model -- bisect to find it, then see if reverting it helps.

CamouflagedKiwi 3y ago

I disagree. It's a nice property that every commit in master is (conceptually) fully buildable & all tests passed. I've not found much benefit from having heaps of tiny commits and trying to work out their state; also cherry-picking (if needed) is simple with squash commits, but extremely difficult without.

nemetroid 3y ago
Of course all commits should be buildable and pass tests. There’s a scale between ”heaps of tiny commits” and ”squash everything”.
- dllthomas 3y ago
  All commits that will be shared. There's nothing wrong with a local WIP commit when you want to plant a flag to compare your next changes against.
lolinder 3y ago
That's why they said this: > If you make a meandering series of commits during development, squash them for the PR. But please, please do not squash the entire feature upon merge. I typically turn my PRs into a series of buildable and testable commits. I also put a lot of work into making those commits tell a useful story to the reviewer and to anyone doing git blame later, and squashing them all into one commit undoes that work.
- dllthomas 3y ago
  Buildable and testable, but notably sometimes not formatted or linted, when (atypically but not rarely) the semantic change is much easier to understand separated from the superficial changes it motivates in the context of the formatting/linting rules of the project.
  - lolinder 3y ago
    Yep. And in a similar vein, not squashing allows you to separate a file move from changes to it so that git can track the move.

63stack 3y ago

Why are you interested in looking at individual commits? What can you possibly learn from that that you can't get from looking at the entire diff the PR is introducing? Does your CI test each individual commit? Afaik most of them only test the top commit. How do you know/enforce that all the inbetween commits also build/pass tests? How do you know how many commits to revert, if you need to revert the feature? Instead of reverting 1, now you have to revert N where N is not recorded anywhere.

Snild 3y ago
> Why are you interested in looking at individual commits? Because they can explain their individual rationales, while still making the most sense to merge all together. > What can you possibly learn from that that you can't get from looking at the entire diff the PR is introducing? Ease of review (both before merge, and in the future when wondering why something was done a certain way). Saves me as a reviewer from having to guess which parts of the commit are meant to do what. This, of course, means that every commit needs to be a reasonable change in itself -- fixup commits done while developing should be squashed into the original change with a local rebase (these are the "meandering" commits your parent post mentioned). > How do you know/enforce that all the inbetween commits also build/pass tests? I'm no CI expert, but I would hope that most systems allow this as an option. > How do you know how many commits to revert, if you need to revert the feature? Instead of reverting 1, now you have to revert N where N is not recorded anywhere. It's recorded in the merge, assuming you always make a merge commit. Otherwise, since each commit is actually its own logical change, you figure it out the same way as you would figure it out in the "squash PR" model -- bisect to find it, then see if reverting it helps.