On Sat, Apr 2, 2016 at 5:59 PM, Gregory Szorc <g...@mozilla.com> wrote:
> 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.) > Yes. When I say "changeset" I mean a logical unit, not the particular series of commits that went into that unit, because I expect those to be squashed down before landing. > 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. > As I think I've mentioned before on this list, with other review systems (e.g., Rietveld) I work in the following way: 1. I write a bunch of code, committing along the way, so I have a lot of commits named "Checkpoint" and "Fix bug" and the like. 2. When it works, I push the code up to the review system for review. 3. In response to review comments, I add a bunch more changes as new commits and push them up the review system for review. 4. Repeat 2 and 3 until I get r+ 5. Squash everything into one commit and land it. Every time I do #3, it creates a new review request, but as you can see, this doesn't have any meaningful connection to my local commits, which is a good thing because while I want to keep my local history, I don't want it to show up either in review or in the tree. This is also the way I want to see patches because I want to review the whole thing at once. > 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. > I largely agree with this, provided that Mozreview automatically takes all my local commits and squashes them before pushing them up to the server (I'm also happy to have some flag that I provide when I do that). > 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. > Yes, I think this is what is needed. I note below that you talk about prioritizing reviewer time over patch author time, but this wouldn't consume any more reviewer time (because people who work the way I do would otherwise have to manually squash and because people who want to review the way I do will just force them to). 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. > I don't believe I am asking for this, just auto-squash on submit. I certainly understand if it's your position that you have higher priorities, that's fine, but it's not fine to remove the ability to do squashed reviews before something like that lands. -Ekr > > > 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
