On Fri, Jan 22, 2016 at 1:45 PM, Boris Zbarsky <bzbar...@mit.edu> wrote:
> On 1/22/16 3:52 PM, Gregory Szorc wrote: > >> I would say that pushing cherry-picked commits for review that depend on >> other commits not in the commit's ancestry is just wrong. If you pushed >> this to Try, it would fail. So why are you pushing a "bad" commit/tree for >> review? If your commits depend on something else, that something else >> should be in the ancestry [and available to MozReview to inspect]. >> > > I'm going to assume this is a non-rhetorical question. > > Here's a concrete example of a reason to do this. Say I have two patches > for two separate bugs. They're totally unrelated to each other in general, > but they happen to both need to touch the same spot of code, possibly in > the same way, but possibly in slightly different ways). So the patches > conflict with each other, effectively. > > Now I have several options: > > 1) Develop the patches on separate branches (or on separate repos), > request separate reviews on them, whichever one loses the review/land race > needs to get updated in mozreview before getting pushed. > > 2) Develop the patches on the same branch, request reviews on them, if > the one that's "second" gets review first, reorder the commits, making the > minor changes needed to handle that one conflict, land the thing that has > review, update the other one in mozreview. > > 3) Develop the patches on the same branch, request reviews on them, wait > until the "first" one has reviews before landing either one. > > In practice, I believe option 3 is the worst one for the situation I > described, because it gates an unrelated bug on another bug. And this > situation happens all the time. I'd estimate that at least 10% of my > patches have self-conflicts of the type I described above. > > Option 2 is nice in needing a bit less overhead in terms of working trees > or rebuilds on branch-switches. It also just happens automatically quite > often, unless you're using one branch per bug always. I believe you're > saying this option is unsupported by MozReview if the actual branch is > pushed (because it's two separate bugs), so the natural way to use > MozReview here would be to cherry-pick. > 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. > > Option 1 just happens if you use one branch per bug, or just happen to > work in a different local repo and don't realize you have the > self-conflict. Happens all the time, again. In practice it will lead to > precisely the "cherrypicked" setup Ehsan described: as soon as one patch > lands, the other will behave as if it had been cherrypicked. > In general, I don't want MozReview to dictate client-side branching workflows. It does that today courtesy of not handling multi-bug series and advanced commit tracking that well. Support will improve over time. It's worth noting that I also want to put less emphasis on bugs as our unit of work. Modern version control workflows revolve around lines of work, or topic branches. It is a generally accepted best practice to use multiple topic branches. But some people just lump everything together because it can be easier than wrestling with endless history editing and merge conflicts in your version control tool. Anyway. sometimes a feature branch is related to a single bug. Sometimes multiple bugs. Sometimes you start work on something that has no bug yet! I'd like our code development workflow to flow more around the code than bugs. Bugs may be the driver for the work and the ultimate mechanism to track that work. But the code/commits - not the bugs - should be driving the workflow. 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/ > > The ultimate goal is to >> push things as they need to land >> > > This is a ... pretty vague concept. In practice, the way it works is you > write a bunch of code for nominally unrelated bugs, some of it possibly > with non-obvious dependencies or self-conflicts or both, possibly on one > branch, possibly on several branches, possibly on several branches with > multiple bugs per branch. Then you ask for reviews on all of the unrelated > things in parallel and wait hours to months depending on the reviewers. > Then in the order you get the reviews you cherrypick things onto that day's > tip, fix merge issues as needed, run a try push if you're not confident > enough that things are still OK, and then push. We would eventually like most Firefox landings to go through Autoland. This tentatively means going through MozReview as a means to get into Autoland because the systems are tightly coupled right now. So, the workflow would be: 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 (3 & 4 could be streamlined into one step so you don't need to leave the command line.) 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. To not do this is somewhat undermining the review process. Another part of me realizes that we're generally smart, well-intentioned people that get the rewriting correct 95% of the time and that requiring a cursory re-review is much unwanted process overhead and delay. MozReview does carry forward r+ if it was granted, effectively trusting author-side rewrites and allowing Autoland to occur. 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. A benefit to re-publishing the final commit state to MozReview before landing is that this shows up as an interdiff, which kind of closes the loop on the patch's lifecycle (although we could have Autoland publish landed commits to MozReview if we wanted to). I hope this addressed your concerns. If you want more detail, perhaps a centralized document would be a better medium than a mailing list. _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
