On Fri, Jan 22, 2016 at 11:24 AM, Ehsan Akhgari <ehsan.akhg...@gmail.com> wrote:
> On 2016-01-22 2:10 PM, Gregory Szorc wrote: > >> On Fri, Jan 22, 2016 at 7:30 AM, Boris Zbarsky <bzbar...@mit.edu> wrote: >> >> On 1/22/16 10:08 AM, Andrew Halberstadt wrote: >>> >>> In the end, it's on the reviewer. If the patch is complicated, has open >>>> dependencies or looks like it might cause problems, as a reviewer I'm >>>> not going to land it no matter what. If it's simple and self-contained, >>>> why not? >>>> >>>> >>> There is no obvious indication in mozreview whether a patch is >>> self-contained or has dependencies, is there? >>> >>> Sometimes one can make a good guess, of course. >>> >> >> >> Not currently, no. >> >> However, we know from the repository DAG what the "dependencies" are and >> we >> done want to eventually surface these in MozReview by moving the review >> request grouping in MozReview to something that is more DAG based instead >> of bug-based. >> > > What about the case where the information doesn't exist in the repository > because the author, for example, cherry-picked a specific commit on a > throw-away branch because the rest of the dependencies are still being > worked on? Or, as another example, what if the patch needs to be checked > in only after another patch (perhaps done by a different author) that is > not connected to the review at hand? > > (Both of these examples are from my real workflow in the past couple of > weeks or so.) > > 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]. MozReview today is very simple about how it handles incoming commits. It defaults to reviewing all pushed commits (although you can control this). It assumes one bug per submitted series. It assumes one author per series. This is done because it was simple to implement. The ultimate goal is to push things as they need to land and MozReview will sort it out. This includes pushing commits from multiple bugs and commits belonging to other people. I don't think it is possible for this to be perfect for all workflows. But we should be able to handle common workflows or at least nudge people towards well-supported workflows. _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
