2017-10-11 18:58 GMT+02:00 Ivan Kelly <iv...@apache.org>: > 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/
I really think that most of the problem is related to having huge diffs as Ivan is claiming I am really sorry that we broke GitHub with patch for BP-15, with toom any commits and comments. Maybe we used the PR dashboard is a bad way. We did a discussion and we continued the conversation on the PR even if we changed opinion several times. In the original document for PB15 there is a small subset of the changes that will be in with that patch. That patch introduces a new framework for mocking BookKeeper client artifacts for instance :-) It could be done in another patch. I will experiment with BP-14 for stacked PRs ! Enrico > > > > | 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. > +1 I would like to do this very soon > > > 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. > Yup, we are working on flaky tests and we are on the way > -Ivan >
