Bit late to the party, but I think there are a couple of core issues causing this debate:
There are actually two kinds of trust involved. First is the trust to not botnet the CI, basically everyone here (at least everyone with try) has that. But that's not the actual one being discussed here. The second is not really a form of "trust", rather it's the level of familiarity involved in the codebase to know the areas of code where small changes may cause big problems. For example, I would probably ask for re-review on rebases of my own changes to layout code or bindings code. Often a PR is to an area of code where stuff is straightforward and there's very little chances for things to mess up, which is where delegate+ comes in. Other times it may be in an area which some things need to be done precisely, and a rebase needs review -- but it's not always possible to tell these areas apart at first glance. Reviewers have the "trust" that they know when review-carry is appropriate. The debate boils down to whether or not everyone has this kind of "trust". delegate+ lets reviewers mark a PR as being in an area of code where there's not much that can be inadvertently broken. (This sort of addresses bholley's point about "a contributor is likely to find themselves with carry privileges in some situations but not others") The second issue is that reviewers themselves shouldn't be review-carrying often in the first place. We do it, but I don't think everyone agrees that we should, not as often at least. In the past it wasn't so necessary, either -- I don't recall seeing as many merge conflicts in the past as I do now. Most of the conflicts are due to import sorting, however, now that we enforce stronger sorting (i.e. within use declarations). It's also a workflow thing: most of us don't seem to structure our workflow to depend on existing PRs (if it hasn't landed yet, base your work off the existing changes, and rebase+PR when the first one lands). Folks like bholley actively working cross- crate can't use this workflow. Path overrides are useful in those cases, but aren't as ergonomic. Given that we've got a lot more merge conflicts and stuff happening these days, I think we might need to start being more liberal with review-carry, both as reviewers and non-reviewers. As jdm mentioned in the meeting, this codifies existing practice to not really review rebases thoroughly. FWIW I'm okay with being less restrictive with r+ privs and maintaining a distinction between reviewers and those-with-r+ (committers). Those-with-r+ should use it in case of import rebases, when a reviewer has said "r=me after rebase" or "r=me after nits", or in other cases where it's obvious that the rebase can't break anything. -Manish _______________________________________________ dev-servo mailing list dev-servo@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-servo
