When you say "I almost never want to review individual commits and instead want to review the changeset as a single diff," I'm confused because a commit is a changeset (in Mercurial terms at least) and this statement is contradictory. You seem to be saying that you want to look at a series of changesets/commits as a single diff? (I'm guessing your definition of "changeset" doesn't match mine.) Anyway, you seem to be advocating for the GitHub model where there are N commits on a pull/review request but most people typically only look at the aggregate/final diff.
The Firefox model of code review has reviewers looking at individual diffs/patches/commits which then land as-is (or close to as-is) [after a rebase]. MozReview's suggested workflow (and the messaging you are seeing) is based on this model. This model is definitely at odds with what is practiced by a lot of projects (especially on GitHub), where how-the-sausage-was-made commits/history are merged into the final repo history. Firefox and large parts of Mozilla have largely rejected this model and instead try to ensure that final history is clean (this can also be accomplished by squashing on land - a feature that GitHub finally added yesterday). The widely practiced Firefox code review model is to try to ensure that commits that reviewers see - whether on Bugzilla or MozReview - are as close to their final, landing state as possible. In my opinion, the model of submitting all the original, intermediate, to-be-thrown-away commits adds all kinds of UI/UX challenges, overhead, and dissonance to the code review process. It is much simpler for reviewers and the tooling to require history rewriting occur on the client side and for the proposed, final commits - and only the proposed, final commits - to be the thing exposed to MozReview. In theory, MozReview could be made to support both models. But in the interest of simplicity, we chose to support 1 in MozReview. It happens to be the same model as what was practiced in Bugzilla before (you rarely saw how-the-sausage-was-made commits in Bugzilla), so it hasn't been a very controversial decision. People have wanted to create "append at the end" / fixup commits to address review feedback. But as I said, there are all kinds of UI challenges and complexities, especially once you start having 2 flavors of commits - a commit that should land as is and another that should be folded. I believe there are areas where we could do a better job on the UI for people that want how-the-sausage-was-made-commits. e.g. we could have a submission mode that squashes commits before submission to MozReview instead of requiring explicit rewriting before review submission. This is similar to how arcanist/Phabricator and a bunch of other review tools work (but not GitHub - it renders what is pushed). We could also have the ability to squash commits in the MozReview web UI if we really wanted to. However, the policy for Firefox repo history is that each commit (not a series/merge/push) should stand on its own and final repo history should be linear and commit level bisectable (ignore merge commits from integration repositories - they will go away eventually if dougt and I have our way). It's easy to argue that this practice should be practiced by developers on their local clones, that tools should encourage this, and that how-the-sausage-is-made commits should not be seen by anyone other than the original author. I recognize this makes patch authors' lives more difficult (because history rewriting is harder than throwing commits over a wall). But we've made the decision to prioritize reviewer's time over patch author's (reviewers are a more limited resource). If people really hate this model, we can consider supporting sausage-making commits. But from my perspective, most people are OK with the existing model: they understand that spending a little extra effort to craft "cleaner" commits is justified in order to improve reviewer's lives, keep review tooling simpler, make it easier to land things (including autoland), and produce a clean final repository history. tl;dr supporting how-the-sausage-is-made commits is a departure from how Firefox code review is done and would introduce a lot of complexities into processes and tooling. At this juncture, there are far more important things to do work on in MozReview first. On Sat, Apr 2, 2016 at 9:35 AM, Eric Rescorla <e...@rtfm.com> wrote: > "This is a squashed review request, containing the sum of all commits in > the series. It is intended only to provide an overview of a series of > commits. At the moment, you *can*leave review comments here, which will be > mirrored to Bugzilla, but they will not affect the review status of > individual commits, nor will they result in any changes to review flags in > Bugzilla. Since the commits will land separately, please review them > individually by using the links in the "Diff" and "Reviews" columns in the > table above. > > It is likely that squashed review requests will either be made read only or > removed entirely at some point." > > > As I'm sure I have mentioned in a previous email, I almost never want to > review individual commits and instead want to review the changeset as a > single diff. Is this just historical nagging or is there actually some plan > to remove the ability to review a changeset as a single unit? Hint: that > would not be OK. > > -Ekr > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
