On 1/23/16 1:20 PM, Gregory Szorc wrote:
While MozReview defaults to submitting all pushed commits for review, you can override these defaults to pick a) any single commit b) a range of commits at the bottom c) middle or d) top of the series.
OK, but you said people shouldn't be pushing cherry-picked commits like this to MozReview...
It's worth noting that I also want to put less emphasis on bugs as our unit of work.
Bugs are not the relevant thing here. The relevant thing is multiple conceptually independent lines of work that happen to touch the same code.
It is a generally accepted best practice to use multiple topic branches.
If switching topic branches, or even from a topic branch back to tip so you can start a new topic branch didn't typically involve long rebuilds, I'd consider doing this....
It feels wrong to me that we currently have to practice sub-optimal code writing and review workflows to accommodate the relationship between bugs. This changes as code review is extracted from Bugzilla. See also http://gregoryszorc.com/blog/2015/01/16/bugzilla-and-the-future-of-firefox-development/
I've read this, yes. I even agree with a lot of it, but it has nothing to do with the issue we're discussing here.
We would eventually like most Firefox landings to go through Autoland.
Yes, would be good.
1) Whatever you have done up to this point 2) Rebase / rewrite history / cherry-pick / etc your commits on top of central 3) Push to MozReview 4) Autoland
OK. So review effectively happens in step 1 here, yes?
Related to this, I always found it a bit surprising we perform so much activity on the patch author side before submission. Part of me thinks reviewers should take one quick glance at the interdiff before the final version lands and should be gate keepers.
With my reviewer hat on, I often do just that. As in, I'll explicitly say "r+ with these changes, but I want to see the interdiff before you land" or "r+ with these changes, and feel free to land, but I want to see the interdiff". Of course sometimes I'll also just say "r+ with these changes", or "You need to make these changes and request another review". It all depends on the scope of the changes and my past experience (or lack thereof) with the patch author...
I think this is the correct model: if a reviewer absolutely wants to see the final version before landing, they can withhold r+ until all issues are addressed.
Sure. And the "r+ with these changes, and feel free to land, but I want to see the interdiff" mode is supported with Autoland because the interdiff would be available in mozreview post-facto, as you note.
I hope this addressed your concerns.
If we assume that review happens in step 1 of your 4-item list above, then yes.
-Boris _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
