This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: Silence does not imply consensus.
On 7 March 2013 22:52, Carlos O'Donell <carlos@redhat.com> wrote:
> 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.
You're being too hard on yourself :) The way I see it, there was no
response to the updated patch for 3 weeks, after which you stepped
forward and did the review. I agree that your viewpoint was biased as
you wanted this feature as well with a timeline, but whatever
opposition to the patch there was, it was a good two revisions earlier
and a lack of comment through all that has in the past been
(correctly) perceived as a 'no objection'. In that context, the call
was justified.
> 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.
Right, all I'm saying is that I wouldn't want to wait around for weeks
to get someone who had objected to the first version of the patch, to
tell me that the latest version is fine despite having had a positive
review from another reviewer. I'm fine discussing my changes for
months if needed, but all I ask for is that if someone has a problem
with a change, they ought to be interested enough to persist with the
argument. I would do that myself if I had an opinion regarding any
change.
I also don't see this as a slippery slope, especially because we have
a workflow requiring another developer to do a review before commit.
In fact, I see this as incentive for everyone involved to be *more*
involved and not assume that the development process will stop for
them.
> 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.
I've never had a problem pinging patches and soliciting reviews, as
you will see from a brief search in the archives. I'm not suggesting
that we change that either. Getting a comprehensive review of a patch
is absolutely necessary for check in unless the change is trivial.
> Don't fall into the habit of thinking that silence implies consensus.
As a general rule, Ack. I don't plan to push patches without a
satisfactory review anytime soon ;)
> 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.
I will continue to disagree on this, but I guess we achieved the
desired outcome from that exchange - we'll hopefully have a much more
active discussion about the feature I proposed and the usage of
environment variables in general.
Siddhesh
--
http://siddhesh.in