Responding to an old thread... (had saved as draft but didn't realize I hadn't sent). Only replying to .platform here since last time I cross-post-replied it didn't show up.
>I want to explicitly state that we're moving towards >N=~10 people having raw push access to the Firefox repos with the majority >of landings occurring via autoland (via Conduit via MozReview/Bugzilla). >This reduces our attack surface area (less exposure to compromised SSH >keys), Reducing our attack surface is (on the surface) good... but... >establishes better auditing (history of change will be on >Bugzilla/MozReview and not on a random local machine), I presume you mean rebases and nit-handling. I see this as unimportant. >eliminates potential >for bugs or variance with incorrect rebasing done on local machines, Ok, but I don't see this as critical, and rarely if ever see issues with this. >and allows us to do extra things as part of the landing/rebase, such as >automatically reformat code to match unified code styles, Oh, that is something that would be a *massive* annoyance and likely involve destroying a bunch of history info (or rather hiding it from easy access). >do binding generation, etc. They say all problems in computer science >can be solved by another level of indirection. "Can be" and "Is a good idea" are two very separate things... :-) :-) >Deferred landing (autoland) is such an >indirection and it solves a lot of problems. "A lot of problems" - please, this needs very concrete enumeration and discussion. I do see one (SSH key compromise), though this isn't the only way to deal with that. The rest listed above I don't see as compelling when weighed against the costs (direct and indirect) of this proposal. I also agree with a lot of when Ehsan said. Also, the SSH issue - compromised SSH keys are certainly an avenue for compromise of our binaries, but there are a whole bunch more ways that could still happen, ways that don't leave the forensic trails checkins would (as ekr mentioned). How many times (that we know of) has SSH compromise led to a rogue checkin that got to release? (feel free to answer that privately, for anyone who knows.) >It will be happening. Most of >us will lose access to push directly to the Firefox repos. We won't be >losing ability to initiate a push, however. For the record, I'm not in >favor of making this change until the tooling is such that it won't be a >significant workflow/productivity regression. This is a reason why it >hasn't been made yet. I don't see how this proposal will achieve the goal you state as a blocker. (E.g. ehsan's and other's comments.) >A handful have commented on whether a rebase invalidates a previous r+. >This is a difficult topic. Strictly speaking, a rebase invalidates >everything because a change in a commit being rebased over could invalidate >assumptions. Same goes for a merge commit. In reality, most rebases are >harmless and we shouldn't be concerned with their existence. This speaks against some of the benefits mentioned above. >In the cases where it does matter, we can help prevent things from >falling through the cracks by having the review tool detect when >in-flight reviews touch the same files / topics and route them to the >same reviewer(s) and/or notify the different reviewer(s) so people are >aware of potential for conflict. This feature was conceived before >MozReview launched but (sadly) hasn't been implemented. I can't imagine how this will work (and be effective) in practice. >There have been a few comments on interdiffs when rebases are in play. I >want to explicitly state that there is no single "correct" way to display a >diff much less an interdiff much less an interdiff when a rebase is >involved. This is why tools like Git have multiple diffing algorithms >(minimal, patience, histogram, Myers). This is why there are different ways >of rendering a diff (unified, side-by-side, 3-way). Rendering a simple diff >is hard. Rendering an interdiff is harder. Unfortunately, central review >tools force a specific rendering on users. ReviewBoard does allow some >tweaking of diff behavior (such as ignore whitespace). But no matter what >options you use, someone will be unhappy with it. An example is MozReview's >handling of interdiffs. It used to hide lines changed by a rebase that >weren't changed in the commit up for review. But that was slightly buggy in >some situations and some people wanted to actually see those changes, so >the behavior was changed to show all file changes in the interdiff. This >made a different set of people unhappy because the changes were spurious >and contributed noise. In summary, you can't please all the people all the >time. So you have to pick a reasonable default then debate about adding >complexity to placate the long tail or handle corner cases. Standard >product design challenges. On top of that, technical users are the 1% and >are a very difficult crowd to please. I've dealt with these by handling rebases myself (i.e. reviewers don't get involved), with the rare exception that a rebase requires non-trivial rewrite, in which case I just ask for re-review or re-review on that part, or write the rebase as a new patch on top of the reject. This is rarely needed, however. Interdiffs for responding to review comments - sometimes (trivial patches) I just replace and r? the entire patch. More complex ones I'll set up as a separate patch (interdiff) and ask them to review that, then fold/merge before checkin (yes, I use mq and splinter still - but the general idea would apply regardless). And yes, mozreview does a MUCH better (and more automatic) job of dealing with interdiffs automatically. But that exposes you to the rebase problem. As you say, there isn't an easy answer. Or just "never rebase until the patch is "done", then deal with rebasing all at the end. (And, of course, for some long-in-review patch queues, this won't be possible.) >I'm sensitive to concerns that removal of "r+ with nits" will provide a net >productivity regression. We should be thinking of ways to make landing code >easier, not harder. This is a larger discussion spanning several topics, of >course. But the point I want to make is we have major, systemic problems >with code review latency today. Doing away with "r+ with nits" exacerbates >them, which is part of the reason I think people feel so passionately about >it. I feel like there needs to be a major cultural shift that emphasizes >low review latency, especially for new and high volume contributors. Tools >can help some. But tools only encourage behavior, not enforce it. I feel >like some kind of forcing function (either social/cultural or formal in >policy) is needed. What exactly, I'm not sure. At this point (where I see >very senior engineers with massive review queues), I fear the problem may >have to be addressed by management to adequately correct. (Yes, I hated >having to type the past few sentences.) We have greatly improved review latency in the last year or two (an extreme case was certain people left reviews active for 9 months or a year, because they disagreed with the patch direction). It's much better now, but it isn't perfect, and trying to drive it a lot lower can have negative impacts that may be hard to measure. C.f. Peopleware, and the impact of interruptions on productivity (and quality). Many people don't read email for stretches because they're busy working on something (or meetings, etc). Some even disconnect from the internet to focus (roc did). People have PTO, they have doctor's visits, they have p1 quantum bugs that are blocking other people - not all reviews are equal. >I think Steven MacLeod's response was highly insightful and is worth >re-reading multiple times. Pursuant to his lines of thought, I think there >is room to have more flexible policies for different components. For >example, high-value code like anything related to cryptography or security >could be barred from having "r+ with nits" but low-impact, non-shipping >files like in-tree documentation could have a very low barrier to >change. Having tighter requirements for select portions of the tree would be ok. We effectively have that for things like NSS since it's imported periodically. Such a limited tightening would be very different than what's proposed. >Of course, this implies flexible tools to accommodate flexible workflows >which may imply Mozilla's continued investment in those tools (which is in >contrast to a mindset held by some that we should use tools in an >off-the-shelf configuration instead of highly tailoring them to our >workflows). I like the simplicity of one policy for everything but am not >convinced it is best. I'd like more details on what other large companies >do here. > >I hope this post doesn't revive this thread and apologize in advance if it >does. Please consider replying privately instead of poking the bear. I did consider it. But I can't be quiet publicly with a posting stating "this *will* happen" (emphasis added) on the table. I certainly know of the vulnerabilities here.... but I also see how much friction for our development cycle might be caused, with little real-world benefit (IMHO). The last thing we need is to move a lot slower. -- Randell Jesup, Mozilla Corp remove "news" for personal email _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
