4 ms·

I think I'm missing something here. How valuable is to have 20 commits of "fix this error" , "fix the fix of the error", "revert all fixes", "real fix".... etc?

I think I'm missing something here. How valuable is to have 20 commits of "fix this error" , "fix the fix of the error", "revert all fixes", "real fix".... etc? I'd argue a PR with many commits such as these, conveys little no no useful information, when the actual change is 1-3 LOC. what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. Sure, it can take a lot of time, but it's done once, by you, after all you're supplying the changes. And if it takes a really long time, you're doing it wrong in the first place by submitting PRs with many LOC. Have you tried rebasing branches with dozens of short useless commits? That's a real waste of time, and everyone needs to do this if you want to update your main branch. So you've now multiplied the amount of wasted time for everyone.

darknavi 5y ago
> what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. So... squashing?
- phone8675309 5y ago
  Yes, but the author of the code, who would presumably know their intent, gets to choose how many commits to squash to instead of just one. This means that they can preserve the development process in a logical way.
- u801e 5y ago
  What if the change really should be split into multiple commits. Having a mega commit affecting hundreds of lines across tens of files doesn't make reviewing the change easy, and reverting it will result in conflicts.
  - Seb-C 5y ago
    I would ask such changes to be split into multiple independent PRs
    - u801e 5y ago
      Then you essentially double the number of commits for a particular change, since it then becomes: 1. first related change 2. first merge commit 3. second related change 4. second merge commit 5. third related change 6. ... Instead of just having all related changes in several commits with a merge commit at the end that groups those related changes.
      - Seb-C 5y ago
        The number of commits does not matter. What matters is the quality of it. My opinion is that each PR should be an independent and working unit. If you have a too-huge PR that can be subdivided, it almost always means you have multiple units of work (feature/fixes) into one branch/PR. The reason I agree with the article and use squash is that it allows us to have a different history between master and the local branches. Developers can freely subdivide each feature into how many commits are useful to their mindset and workflow, while the master branch will remain clean. In my experience, the individual scope of each commit on the master branch is almost always correlated to the scope of one code review, and thus one PR.
        u801e 5y ago
        Then there really is no need to even have merge commits. But, as far as I know, there no way to disable the creation of a merge commit when using the merge button on the github PR page. The one thing you lose with just making one PR per commit is the relationship between a set of commits used to implement a feature. One commit involves some retractoring to make it easier to cleanly implement the feature. The other commit would be the feature and tests, and the last commit would be to add calls/references to that feature. If these commits were kept in the same branch and merged in a single PR, then it would be easy to see the relation between them. If they were merged as 3 separate PRs, then it would be more difficult to see why the refactoring was done, especially if PRs for unrelated changes were merged in the interim.
pudmaidai 5y ago
It's not that those commits are in any way useful, it's that this "fix the error" commit might be 3 commits after the commit it belongs to. If there are no conflicts, `!fixup` will handle it for you; But since this is the real world, you're likely going to waste huge amounts of time solving conflicts that help no one. My solution is: - small-scoped PRs - attempt to keep sub-commits readable, but don't waste time on them Squashed commits also link to the PR where not only do you get the original commit list, but also get the whole discussion around the code.
calpaterson 5y ago
> How valuable is to have 20 commits of "fix this error" , "fix the fix of the error", "revert all fixes", "real fix".... etc? I'd argue a PR with many commits such as these, conveys little no no useful information, when the actual change is 1-3 LOC. I'm not saying that I wouldn't broadly discourage the worst of that but most people do not commit every changed line so it is rarely a problem in practice. What does become a problem are people who insist on "clean history", "re-order these commits", etc etc at PR stage when you both agree the functional problem is done/fixed and now the negotiation has moved on to your git history. I do contract work (read: move around a lot) and this social anti-pattern comes up very often. Usually one guy who's into it throwing his weight around. Would it surprise you to learn there is a large crossover because people who do this and people who go forensic on other cosmetic stuff: whitespace, syntax, etc?! :)
rendaw 5y ago
I've used these commits to identify the correct fix for bugs before. An early commit changed something to do X, but did it wrong. A "more fixes" commit tried to fix it but made it do Y. It was obvious that the later commit was wrong and I could fix it to do X. If it were squashed I would have thought the original intent was to do Y and spent way more time trying to figure out how to fix it. Obviously it's not guaranteed but throwing away that information is a bad idea if it's just for some arbitrary aesthetics. Same goes for rebasing to clean up the commit history, which would ostensibly do the same.

darknavi 5y ago

> what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. So... squashing?

phone8675309 5y ago
Yes, but the author of the code, who would presumably know their intent, gets to choose how many commits to squash to instead of just one. This means that they can preserve the development process in a logical way.
u801e 5y ago
What if the change really should be split into multiple commits. Having a mega commit affecting hundreds of lines across tens of files doesn't make reviewing the change easy, and reverting it will result in conflicts.
- Seb-C 5y ago
  I would ask such changes to be split into multiple independent PRs
  - u801e 5y ago
    Then you essentially double the number of commits for a particular change, since it then becomes: 1. first related change 2. first merge commit 3. second related change 4. second merge commit 5. third related change 6. ... Instead of just having all related changes in several commits with a merge commit at the end that groups those related changes.
    - Seb-C 5y ago
      The number of commits does not matter. What matters is the quality of it. My opinion is that each PR should be an independent and working unit. If you have a too-huge PR that can be subdivided, it almost always means you have multiple units of work (feature/fixes) into one branch/PR. The reason I agree with the article and use squash is that it allows us to have a different history between master and the local branches. Developers can freely subdivide each feature into how many commits are useful to their mindset and workflow, while the master branch will remain clean. In my experience, the individual scope of each commit on the master branch is almost always correlated to the scope of one code review, and thus one PR.
      - u801e 5y ago
        Then there really is no need to even have merge commits. But, as far as I know, there no way to disable the creation of a merge commit when using the merge button on the github PR page. The one thing you lose with just making one PR per commit is the relationship between a set of commits used to implement a feature. One commit involves some retractoring to make it easier to cleanly implement the feature. The other commit would be the feature and tests, and the last commit would be to add calls/references to that feature. If these commits were kept in the same branch and merged in a single PR, then it would be easy to see the relation between them. If they were merged as 3 separate PRs, then it would be more difficult to see why the refactoring was done, especially if PRs for unrelated changes were merged in the interim.

pudmaidai 5y ago

It's not that those commits are in any way useful, it's that this "fix the error" commit might be 3 commits after the commit it belongs to. If there are no conflicts, `!fixup` will handle it for you; But since this is the real world, you're likely going to waste huge amounts of time solving conflicts that help no one. My solution is: - small-scoped PRs - attempt to keep sub-commits readable, but don't waste time on them Squashed commits also link to the PR where not only do you get the original commit list, but also get the whole discussion around the code.

calpaterson 5y ago

> How valuable is to have 20 commits of "fix this error" , "fix the fix of the error", "revert all fixes", "real fix".... etc? I'd argue a PR with many commits such as these, conveys little no no useful information, when the actual change is 1-3 LOC. I'm not saying that I wouldn't broadly discourage the worst of that but most people do not commit every changed line so it is rarely a problem in practice. What does become a problem are people who insist on "clean history", "re-order these commits", etc etc at PR stage when you both agree the functional problem is done/fixed and now the negotiation has moved on to your git history. I do contract work (read: move around a lot) and this social anti-pattern comes up very often. Usually one guy who's into it throwing his weight around. Would it surprise you to learn there is a large crossover because people who do this and people who go forensic on other cosmetic stuff: whitespace, syntax, etc?! :)

rendaw 5y ago

I've used these commits to identify the correct fix for bugs before. An early commit changed something to do X, but did it wrong. A "more fixes" commit tried to fix it but made it do Y. It was obvious that the later commit was wrong and I could fix it to do X. If it were squashed I would have thought the original intent was to do Y and spent way more time trying to figure out how to fix it. Obviously it's not guaranteed but throwing away that information is a bad idea if it's just for some arbitrary aesthetics. Same goes for rebasing to clean up the commit history, which would ostensibly do the same.