On Sat, Jan 23, 2016 at 4:42 PM, Gregory Szorc <g...@mozilla.com> wrote:
> On Sat, Jan 23, 2016 at 4:01 PM, Eric Rescorla <e...@rtfm.com> wrote: > >> >> >> 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. >> > > I run into this scenario all the time. MozReview currently doesn't handle > it well if the 2 patches belong to different bugs. We need to fix that. > > But to go back to my point about bugs versus commits, I assume in this > scenario you gave that there are 2 bugs: 1 for the TLS 1.3 patch and 1 for > the build system. > Yes, but that's completely not relevant. The issue isn't the number of bugs but that they issues are bring worked on by two different people in parallel. In my future ideal world, we wouldn't need multiple bugs: if you discover > an unrelated issue as part of working on feature/bug X, just write the > commit, push it, and review will get sorted out. In this scenario, as the > build system module owner, I couldn't care less if this GTests link issue > were tracked in a Core :: Build Config: I care that a build peer reviewed > the change to the linking behavior. > This kind of seems like a digression, as NSS internal build changes don't require changes from build system peers but rather from NSS peers (in this case Wan-Teh Chang). I'm making a big deal about bugs versus commits because we have all this > overhead around Bugzilla/bugs. > That may be the case in some situations, but it's not the case here because we are working in Rietveld and filing Bugzilla bugs solely for bookkeeping reasons. The issue is purely about having separate review streams, and that issue will persist with Mozreview, regardless of the Bugzilla integration. -Ekr _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
