On 22/01/2016 20:52, 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].
There are two reasons I disagree with this, both of which I believe to be distinct from Boris' reasons:
* "depends" is fuzzy and does not always lead to try or build failure. For an example, I now have review for 80% of my patches on bug 1219215, but I need to wait to land that until bug 1241275 gets a patch, lands, etc., otherwise I will regress window dragging on Windows. To the best of my knowledege we do not have try coverage for this, in part (I think) because doing "native" aero snap dragging within mochitest isn't possible. The code I'm touching won't conflict with the patch in bug 1241275, and anyway, a finished patch wasn't available when I wrote patches for 1219215.
* Historically, mozreview has been... less than amazing... about pushing draft commits with multiple different bug numbers and treating them "as I would expect" (ie, separate bug numbers go on separate bugs, not all in one parent review. If it doesn't know what to do, it should ask with a warning, or do nothing, not blunder ahead and mess things up.). I have developed the reflex to always re-'up' to fx-team tip in my local repo before starting a new patch - even if it's related to the other patch I'm writing - simply to avoid accidentally pushing reviews onto the wrong bugzilla bug, which is impossible to undo anyway, and sometimes messes with other bugs' state in ways that are painful to manually recover.
~ Gijs _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
