4 ms·

I would ask such changes to be split into multiple independent PRs

by Seb-C 5y ago

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.

I would ask such changes to be split into multiple independent PRs

by Seb-C 5y ago

I would ask such changes to be split into multiple independent PRs

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.