Re: Silence does not imply consensus.

["Carlos O'Donell" <carlos at redhat dot com>]

On 03/07/2013 11:08 AM, Siddhesh Poyarekar wrote:
> On 7 March 2013 21:12, Carlos O'Donell <carlos@redhat.com> wrote:
>> I've recently fallen into this trap myself, and I
>> thank Roland for pulling me out of it. I can't just
>> check things in because nobody complains. That's not
>> consensus. It's easy to fall into this trap,
>> particularly when you're busy and need to get that
>> patch in before you reach your deadline.
> 
> I did not see anything inherently wrong with the discussion on the
> default stack size patch, which is the example I think you're talking
> about here.  I had posted the patch quite a long time ago (first draft
> was 2 months ago), pinged and discussed it multiple times and after
> those iterations, got an ack and checked it in.  Roland noticed it and
> raised his objection, at which point the patch was reverted.  *All* of
> that, including having to revert the patch was IMO a perfectly healthy
> exchange.  I'd rather check in the patch after an approval and then
> have someone point out a flaw/protest and revert it, than wait/ping
> endlessly waiting to check it in.  In some cases, it actually halts
> progress when it shouldn't.

There was nothing wrong with the process we followed. It was OK to
check it in, and it was the correct thing to do to check it out when
someone pointed out that we should have had more discussion.

However, I failed you as a reviewer. I should have noticed that 
we really didn't have much consensus regarding the addition of
new environment variables to change the behaviour of the 
program.

The patch timeline is as follows:

- Initial post January 15th.
- Kosaki Motohiro does a review, no negative revie.
- Roland requests a split of the function and a lot more
  justification and cosideration for the env var addition.
- Andreas Schwab mildly objects.
- I don't really count as having an opinion because it presents
  a conflict of interests since I'm strongly involved in helping
  with the patch.
- Second post February 5th.
- Ping second post February 12th.
- February 28th I do a thorough review and it is checked in.

I can't look at this list and say that I as a reviewer did a good
job of ensuring there was consensus or discussion about the change.
I should have engaged more people to comment on the issue.
Roland asked for more discussion and we didn't do it.

>> Consensus needs to be worked on, and involves reaching
>> out to people in the community and getting them to
>> comment on your change.
>>
>> Is it hard work? Yes.
> 
> It is not like once a patch is checked in, it cannot be reverted or
> that reverting it is a big hindrance.  I'm all for discussions, but I
> don't like the idea of waiting for someone to break silence on a
> project that is silent by default.

I disagree strongly, there is a very very slippery slope here.
Patch checkin is the *final* step in the process and we should
not get into the habit of reverting patches. Reverting patches
messes up work that has gone on after the commit.

We are a conservative project and we should make sure that what
we do is correct and that we have consensus *before* checkin.

I know that you'll be around to fix your problems, but I do not
want to set the precedent that you check things in and then
revert them if someone later complains.

Lastly, you should not wait silently, you should ping, and in
addition specifically call people out to help with your review.
Ask Andreas, Joseph, or others for review on specific patches.

>> This community does not adhere to implicit approval.
> 
> I do agree that checking in without *any* approval or if someone
> objects to the patch in its current form, is wrong.  At least one
> other maintainer ought to approve it and if someone has a problem with
> a change, it should be their responsibility to bring it up and stay
> with it and not the patch author's responsibility to ping everyone in
> sight to gather consensus.  There's plenty of time to object - right
> up to the point that a release is frozen.

That's correct, consensus is a judgement call.

I'm saying that my judgement call as a reviewer was wrong.

Roland asked for broader discussion.

We didn't do that, and he objected to the patch.

There is nothing wrong with the process, we have reverted the patch,
and we'll have a broader discussion on environment variable use for
tuning runtime behaviour.

>> Reach out and gather consensus, and then make
>> the change, your code will be better, the quality
>> of the project will be better, and you'll grow
>> as a developer.
> 
> I guess the crux of what I'm trying to say is that while I agree that
> gathering consensus is necessary, please let us not make it into a
> needlessly bureaucratic process.

I agree.

I wanted to make one point with this email:

Don't fall into the habit of thinking that silence implies consensus.

If you judge that you have gathered enough consensus then you are
justified in committing with approval.

In the particular case of the pthread default stack size patch I feel
that I as a reviewer failed you in that I did not judge well, and
that we need more discussion.

Cheers,
Carlos.