Responses inline. >> Last thursday, we a had a short discussion about possibly changing the >> merge process to allow unsquashed commits >> > > I don't think is the best way. > The history of a pull request may have several minor commits and IMO it is > better to hide such conversation in the official git history of the project > If we think that the resulting commit is too big maybe we should split the > patch into smaller parts. Sam isn't suggesting that we keep all the little updates in the commit log. Rather, he is suggesting that PRs get split into self contained.
Large diffs are just bad. [1] gives a good summary of why, but I'll add that in a globally distributed open source project, the problem of large diffs being hard to review is magnified tenfold. We don't all live in the same part of the world, so the turn around between posting the PR, getting feedback, fixing feedback and getting more feedback is long. For the most part we don't work for the same organizations, so code reviews are often bumped for other tasks that have higher priority in our jobs. Again, this lengthens turn around time. And the longer turn around time is, the harder it is to retain any context on what you were reviewing. Swapping back into a context takes time and effort, leads to "review fatigue" and ultimately to reviewers just skimming the patch and giving it a LGTM, rather than doing a proper review. This is a real problem. I recall seeing a research paper from google(I think?), where they correlated diff size to bug generation rates (I can't find the reference now, but will keep looking). We need to reduce the amount of context that needs to be swapped back in when coming back to a review, and the largest part of this is reducing the size of patches. [2] has a good primer on how to keep PRs and patches small. If anyone wants help breaking up a patch before code review, I would be more than happy to lend a hand. [1] https://medium.com/@kurtisnusbaum/large-diffs-are-hurting-your-ability-to-ship-e0b2b41e8acf [2] https://graysonkoonce.com/stacked-pull-requests-keeping-github-diffs-small/ > | and the use of the github merge button. > > I don't like the GitHub button, it is easy to use it even with the > smartphone and it is very dangerous ! I would like to disable it at all. > I am actually running very well with the bk-merge-script as it does all the > validation we want. I'm neither yay nor nay on the button, but if we do require the merge script, we should disable the button completely. > The checks from jenkins/travis/coveralls.io...could be flaky and it would > be very annoying to have to wait for a full green light for a pull request > to be merged This is a problem in itself, and needs to be addressed. -Ivan
