Commit Access: SR must not have reviewed?
Benjamin Smedberg
access. Since I don't know the person and have never reviewed any of his
patches, I declined to grant the sr. He then informed me that he was
required by policy to ask somebody who had never reviewed his patches.
After some discussion on IRC, I learned that this policy was a result of one
of the previous discussions here, in order to prevent "groupthink". But I
think this policy is seriously misguided.
The purpose of vouching for commit access is to make sure the person is
responsible: that he will watch the tree after committing, check in patches
only after the necessary reviews/approvals have been obtained, etc. Unless
you've actively worked with somebody, there's really no way to know whether
they are a responsible person. I could read their patches all day and it
wouldn't help me to make an educated decision about commit access.
Given the current policy, I don't think I can accept any sr requests for
commit access. I propose that SRs who have reviewed patches or otherwise
worked with the person requesting commit access are the only people who can
make an educated decision about commit access, and the "must not have
reviewed" policy should be dropped.
--BDS
Mike Connor
stuff for a few weeks.
On 9-Sep-09, at 9:08 AM, Benjamin Smedberg wrote:
> The purpose of vouching for commit access is to make sure the person
> is
> responsible: that he will watch the tree after committing, check in
> patches
> only after the necessary reviews/approvals have been obtained, etc.
> Unless
> you've actively worked with somebody, there's really no way to know
> whether
> they are a responsible person. I could read their patches all day
> and it
> wouldn't help me to make an educated decision about commit access.
I think the disconnect is whether SR approval is "vouching for a
person" or a check and balance. I've done a couple of SR approvals
under this policy, and I don't think it's extremely onerous. Even
back in the voucher + three SRs days, it was always the case that SRs
were validating that the requester had a solid track record of
patches, interactions in the community, etc. (i.e. I'm pretty sure
jst was one of my SRs, even though we hadn't worked together at all,
but I'm writing this on a plane so I can't prove it)
Historically the division seems to have been: the voucher is vouching
for the person, the SR is granting an approval based on track record.
I don't think this is any different from what we had before (before we
dropped the employer restriction, biesi was the SR for a great many
requests where he hadn't worked closely with someone). None of this
is new, I think people have just conflated vouching and SR (perhaps
for the best) but that isn't the concept the policy requires.
> Given the current policy, I don't think I can accept any sr requests
> for
> commit access. I propose that SRs who have reviewed patches or
> otherwise
> worked with the person requesting commit access are the only people
> who can
> make an educated decision about commit access, and the "must not have
> reviewed" policy should be dropped.
Ultimately, the discussion came down to whether access to mozilla-
central and friends is something that should be protected against the
perils of "groupthink" and the like. I actually believe that if
groupthink is a problem with the vouchers, they already _can_ abuse
the system regardless of who types "hg push" so the transitive trust
is not very different at all in reality. However, I was unable to
drive that view to consensus, so we got to this as the least
restrictive option that offered some protection against groupthink.
-- Mike
Boris Zbarsky
> Ultimately, the discussion came down to whether access to
> against the perils of "groupthink" and the like.
Is it just me, or is this concern.... slightly overwrought? I think we
should revisit this decision.
Whoever had these groupthink concerns, can you please speak up and make
a clear case for your viewpoint?
-Boris
Simon Paquet
As far as I know, the policy was only changed a few months ago. I guess
all the major arguments are still available in the thread discussing
this.
So I'd say that Google Groups is your friend here.
Simon
--
Thunderbird/Calendar Localisation (L10n) Coordinator
Thunderbird l10n blog: http://thunderbird-l10n.blogspot.com
Calendar website maintainer: http://www.mozilla.org/projects/calendar
Calendar developer blog: http://weblogs.mozillazine.org/calendar
Boris Zbarsky
> As far as I know, the policy was only changed a few months ago. I guess
> all the major arguments are still available in the thread discussing this.
> So I'd say that Google Groups is your friend here.
I read that thread; I don't recall any compelling arguments for the
policy we ended up with...
-Boris
Mitchell Baker
I have this concern.
I'm acutely aware that almost all of the module owners, SRs and peers
are employees of Mozilla. We tend towards independent thinkers who
speak their mind, but even so having a shared perspective means the
risks of not asking the "unthinkable" questions go up.
Add to that the idea that a small group of people working under the same
pressures, with the same goals. I've been concerned that a small group
of people, with a lot of shared history, goals, interactions, etc. is
structurally at risk of challenging shared perspectives less -- thus the
"group-think" concern.
I'm not positive I'm right. Or more precisely, I'm reasonably sure that
it's a structural risk; less sure re the right way to address it.
I am adverse to decreeing that X% of module owners, etc must not be
employees -- that sets up all sort of negative incentives.
Mitchell
Boris Zbarsky
> Add to that the idea that a small group of people working under the same
> pressures, with the same goals. I've been concerned that a small group
> of people, with a lot of shared history, goals, interactions, etc. is
> structurally at risk of challenging shared perspectives less -- thus the
> "group-think" concern.
This seems like a quite valid concern to me.
What I'm questioning is the specific "commit access requires sr from
someone who has not reviewed any of the candidates patches before" rule.
I don't quite see how that rule addresses the concern you raise above...
-Boris
Mitchell Baker
The thinking is that requiring an SR who hasn't reviewed patches will
get outside the small group that has been working together, and thus
will be more likely to get someone fresh who will will realize when an
issue needs more attention.
mitchell
Mike Shaver
Yeah, I'm in the same position here. I share the concern, mostly[*],
but don't think that the restriction mitigates it.
It used to be that having commit privileges gave you the ability to
review patches, which meant that getting your buddy to be a committer
meant that he could review your patch when it might not have been
acceptable to the pre-existing committers. Thus, super-review and
various employment-related restrictions were needed and born.
Since we have moved to a peer-and-owner review model, I don't think
that having commit access gives anyone additional influence over the
direction of the project (unless we're worried about people with
commit access pushing things that don't have owner-or-peer approval,
perhaps; I am not).
[*] I think there's something to be said for coherence and shared
values, but I may not be modelling the problem the same way Mitchell
is.
Mike
Benjamin Smedberg
> The thinking is that requiring an SR who hasn't reviewed patches will
> get outside the small group that has been working together, and thus
> will be more likely to get someone fresh who will will realize when an
> issue needs more attention.
But how would the SR actually do this? mconnor has said that he does these
by reading the patches. I have refused to do them because I don't know the
person and therefore wouldn't be able to detect potential issues anyway. In
neither case do you have any evidence other than what's already in bugzilla,
which hardly helps stop groupthink, no?
--BDS
Boris Zbarsky
> get outside the small group that has been working together, and thus
> will be more likely to get someone fresh who will will realize when an
> issue needs more attention.
OK, so I think I'm just missing something...
To make sure I'm not, it seems to me that we're hypothesizing the
following situation:
1) We have someone (call them X) who has been writing patches
and wants commit access.
2) There is some valid reason why X should not have commit access.
3) Two people have vouched for X (presumably they worked with X)
and didn't notice said valid reason.
In this situation, is the claim is that an sr who has never reviewed any
of X's patches has a better chance of noticing said valid reason than
someone who has never reviewed any of X's patches? Or that the sr who
has not worked with X is more likely to act on noticing the reason? Or
some combination? Something else?
What process do we envision an sr who has not reviewed any of X's
patches using to form a decision on the matter? How much time are we
expecting said sr to spend on this?
-Boris
Mike Shaver
> The thinking is that requiring an SR who hasn't reviewed patches will get
> outside the small group that has been working together, and thus will be
> more likely to get someone fresh who will will realize when an issue needs
> more attention.
Oh!
Are you worried about group-think _during_ the evaluation process for
the commit access? I thought the concern was about group-think in the
resulting similarity-by-selection committer pool, sorry! That's
different for sure.
Mike
Mike Connor
Not by reading the patches, but by reading bugs they`ve patched.
Getting a sense for how they interact with reviewers and others is
possible, and a much better way of establishing someone`s credibility
than patches...
-- Mike
Mitchell Baker
> On Tue, Oct 27, 2009 at 4:38 PM, Mitchell Baker<mitc...@mozilla.com> wrote:
>
>> The thinking is that requiring an SR who hasn't reviewed patches will get
>> outside the small group that has been working together, and thus will be
>> more likely to get someone fresh who will will realize when an issue needs
>> more attention.
>>
>
> Are you worried about group-think _during_ the evaluation process for
> the commit access? I thought the concern was about group-think in the
> resulting similarity-by-selection committer pool, sorry! That's
> different for sure.
>
> Mike
>
struggling with undefined virus). I think the answer may be both. But
I'm not sure I had focused on the latter yet. Should I?
ml
Mike Connor
On 27-Oct-09, at 5:29 PM, Mitchell Baker wrote:
> On 10/27/09 1:56 PM, Mike Shaver wrote:
>> On Tue, Oct 27, 2009 at 4:38 PM, Mitchell
>> Baker<mitc...@mozilla.com> wrote:
>>
>>> The thinking is that requiring an SR who hasn't reviewed patches
>>> will get
>>> outside the small group that has been working together, and thus
>>> will be
>>> more likely to get someone fresh who will will realize when an
>>> issue needs
>>> more attention.
>>>
>> Oh!
>>
>> Are you worried about group-think _during_ the evaluation process for
>> the commit access? I thought the concern was about group-think in
>> the
>> resulting similarity-by-selection committer pool, sorry! That's
>> different for sure.
>>
>> Mike
>>
> OK, your mighty powers of abstraction are taxing my tired brain
> (still struggling with undefined virus). I think the answer may be
> both. But I'm not sure I had focused on the latter yet. Should I?
I think that's separate, and shouldn't be controlled via commit
access, but through keeping a reasonable oversight of module ownership
issues. "are we making the right technical decisions?" is the
question that needs answering to address the latter concern. Right
now, I feel quite confident that we are, and that the right people
will be watching for that not being the case.
-- Mike
Boris Zbarsky
> I think there's a reason between "not noticing" and "missing
> intentionally" which is what I would bring in relation to "group-think".
If you mean that the vouchers know X shouldn't get access but vouch
anyway.... then I think we have bigger problems already, since the
vouchers are supposed to be module owners or peers, no?
-Boris