On Sat, Jan 23, 2016 at 10:20 AM, Gregory Szorc <g...@mozilla.com> wrote:
> 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/ I'm not sure why you're focusing on "Bugs" versus "Commits" as the issue. It's not. Any situation where you have two parallel streams of work that tend to impact the same code can run into each other like this. To take a trivial example, I'm presently working on the implementation of TLS 1.3 in NSS. Along the way, we discovered the need to change the way that the GTests are linked. Jed Davis has a patch for this which hasn't landed yet. So now we have two: things in flight: - Jed's patch to the build system (smallish) - My enormous TLS 1.3 patch So, I cherry-picked Jed's patch into my branch, kept developing and am having my patch reviewed (note: we use Rietveld for NSS, which handles this very cleanly). If Mozreview can't cleanly handle this case, that's bad. -Ekr _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
