4 ms·

>If you create and submit small PRs and git-squash those PRs into the main branch, you'll get the best of both worlds. I definitely advocate for this. But I've

>If you create and submit small PRs and git-squash those PRs into the main branch, you'll get the best of both worlds. I definitely advocate for this. But I've worked some at some spots with "never squashers" that deliver every PR as 119 commits and tell me this yarn about "the time having all those commits totally saved their bacon". I've just never regretted squashing my commits into something manageable.

fishywang 6y ago
The problem is not on squash. The problem is on "PR" itself. The pull request workflow popularized by Github and adopted by others (like Gitlab) is just bad for code reviews. They force you to choose between squash and history. Making several, smaller PRs are not the universal solution either. In a lot of cases those PRs have a dependency over each other and dependent PRs is also a pain in the ass in Github's PR workflow. In a better code review system you don't have to make that choice. For example, in Gerrit (I _think_ this is also similar in phabricator, but I'm less familiar with that), every code review will end up a single commit when it's merged (if you configured it so, and that's the default configuration), and you don't lose the history during the workflow, all the intermediate states are stored as git refs on the server (you can't use bisect on them though, but they are there and you could write your own custom script similar to bisect to loop through them). Code reviews with dependencies are also handled naturally.
- jen20 6y ago
  GitHub could _drastically_ improve this workflow by allowing a collaborative commit message to be part of the review. That way the PR can use small commits, and the main history can use atomic commits with an agreed-upon message useful for future code archaeology. Gerrit gets this correct. I and several others I'm aware of spoke with a product team at GitHub about this over a year ago, but nothing seems to have come of it. In the meantime, my default is for any repository I maintain to have atomic commits with fully descriptive commit messages only. The kind of use case described in the article is not, to my mind, a good reason for the purpose of a commit message - describing for future maintainers _why_ a change has been made - to be defeated.
deleted 6y ago
[deleted]

fishywang 6y ago

The problem is not on squash. The problem is on "PR" itself. The pull request workflow popularized by Github and adopted by others (like Gitlab) is just bad for code reviews. They force you to choose between squash and history. Making several, smaller PRs are not the universal solution either. In a lot of cases those PRs have a dependency over each other and dependent PRs is also a pain in the ass in Github's PR workflow. In a better code review system you don't have to make that choice. For example, in Gerrit (I _think_ this is also similar in phabricator, but I'm less familiar with that), every code review will end up a single commit when it's merged (if you configured it so, and that's the default configuration), and you don't lose the history during the workflow, all the intermediate states are stored as git refs on the server (you can't use bisect on them though, but they are there and you could write your own custom script similar to bisect to loop through them). Code reviews with dependencies are also handled naturally.

jen20 6y ago
GitHub could _drastically_ improve this workflow by allowing a collaborative commit message to be part of the review. That way the PR can use small commits, and the main history can use atomic commits with an agreed-upon message useful for future code archaeology. Gerrit gets this correct. I and several others I'm aware of spoke with a product team at GitHub about this over a year ago, but nothing seems to have come of it. In the meantime, my default is for any repository I maintain to have atomic commits with fully descriptive commit messages only. The kind of use case described in the article is not, to my mind, a good reason for the purpose of a commit message - describing for future maintainers _why_ a change has been made - to be defeated.

deleted 6y ago

[deleted]