On Sat, Apr 2, 2016 at 5:51 PM, Eric Rescorla <e...@rtfm.com> wrote: > 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. >
(sorry for the delayed response - fire drill last week and traveling/meetings this week) I think a "squash on submit" feature request is reasonable and would encourage you to file a bug if you haven't already. https://bugzilla.mozilla.org/enter_bug.cgi?product=MozReview&component=Integration%3A%20Git . I'll warn you in advance that there are some trade-offs involved depending on how the feature is implemented. So expect a bit of bikeshedding [on the bug] before any code is written. Also, the focus of MozReview development lately is on the web side of things, specifically bits that make the reviewer experience better. So it might be some time before this client-side improvement is acted upon by one of the regular MozReview hackers. I hope you understand. > > >> >> >> 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
