HEAD is open for 8.5 development

Let the breakage begin ...

regards, tom lane

On Wed, Jul 1, 2009 at 7:30 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Let the breakage begin ...
>
>                        regards, tom lane

Tom - and all other committers -

In order to avoid duplication of effort, we should avoid assigning
round-robin reviewers to any patches which the committers feel that
they already have adequately covered. In order to do that, we need to
make sure we know which ones they are. If you are taking a look at
any of the patches submitted for this CommitFest, please put your name
next to them on the wiki. Please ALSO add a comment saying something
like "tgl says: I am reviewing this, no need to assign a round-robin
reviewer".

During the last CommitFest, we had occasional shenanigans where
someone would stick the name of a committer or other community member
next to a patch just because they had made some comments on it. But
the person whose name got stuck there didn't necessarily know about it
or feel any responsibility to conduct a complete review. So it would
be helpful if everyone could make sure to add a comment when claiming
patches.

Thanks,

...Robert

On Thursday 02 July 2009 05:29:58 Robert Haas wrote:
> If you are taking a look at
> any of the patches submitted for this CommitFest, please put your name
> next to them on the wiki. Please ALSO add a comment saying something
> like "tgl says: I am reviewing this, no need to assign a round-robin
> reviewer".

> During the last CommitFest, we had occasional shenanigans where
> someone would stick the name of a committer or other community member
> next to a patch just because they had made some comments on it. But
> the person whose name got stuck there didn't necessarily know about it
> or feel any responsibility to conduct a complete review. So it would
> be helpful if everyone could make sure to add a comment when claiming
> patches.

Um, how about no one sticks no one's name to any patch unless it is themselves
or they are the commit fest management team and the reviewer has explicitly
OK'ed it.

Let's not begin to use several levels of "Reviewer: X", "X says: I will really
review it.", "RH says: This X is legit.", "Reconfirmed-by: X". The wiki is
weird enough to use that we shouldn't need to edit it in more than one place
for each logical step.

On Thu, Jul 2, 2009 at 2:42 AM, Peter Eisentraut<peter_e(at)gmx(dot)net> wrote:
> On Thursday 02 July 2009 05:29:58 Robert Haas wrote:
>> If you are taking a look at
>> any of the patches submitted for this CommitFest, please put your name
>> next to them on the wiki.  Please ALSO add a comment saying something
>> like "tgl says: I am reviewing this, no need to assign a round-robin
>> reviewer".
>
>> During the last CommitFest, we had occasional shenanigans where
>> someone would stick the name of a committer or other community member
>> next to a patch just because they had made some comments on it.  But
>> the person whose name got stuck there didn't necessarily know about it
>> or feel any responsibility to conduct a complete review.  So it would
>> be helpful if everyone could make sure to add a comment when claiming
>> patches.
>
> Um, how about no one sticks no one's name to any patch unless it is themselves
> or they are the commit fest management team and the reviewer has explicitly
> OK'ed it.

I'm all in favor of that.

> Let's not begin to use several levels of "Reviewer: X", "X says: I will really
> review it.", "RH says: This X is legit.", "Reconfirmed-by: X".  The wiki is
> weird enough to use that we shouldn't need to edit it in more than one place
> for each logical step.

Well, IIRC, Tom was very insistent when we first set this up that the
last column of the chart should be labelled "Reviewers" rather than
"Reviewer", so as not to project the idea that if there already is a
reviewer, then no other reviewers need apply. And in fact we do
sometimes assign multiple reviewers, if the first one gets busy and
can't continue reviewing or if the first one flakes out and never
posts a review at all or if the patch needs review from somebody else
with a different skillset or if someone who has not volunteered to be
a rrreviewer takes an interest in a patch and volunteers to review
that specific thing or if (ha ha ha) we have enough rrreviewers to
assign extras to certain patches.

So I think it's important to clarify the distinction between when a
committer is planning to look at the patch but maybe somebody else
should too, and when the committer is planning to take exclusive
responsibility for the patch and nobody else should bother touching
it. Unless, of course, we want to make the rule that if a committer's
name is next to a patch, then that automatically means that no help
from anyone else is needed. But that seems like a policy that would
inevitably give rise to exceptions.

Also, in general, I have found that when trying to recall the status
of a patch, the comment trail is a lot more useful than the main
summary line. It's a lot easier to look at the comment trail and try
to remember whether anything has happened "since then" than it is to
look at the at the summary line and try to remember whether it matches
current reality. Also, if people date-stamp their comments (which
will happen automatically if we get over to my new system), it makes
it possible to get a sense of how long it's been since a certain state
change took place. Right now, I can look at the Wiki page and say,
"Oh, Peter is looking at a bunch of patches, great". And I know that
it's recent because your name wasn't there a couple of days ago, and I
don't really care whether you're going to commit them soon because the
CommitFest hasn't even started yet. But by the first of August I
suspect there will be so much activity that without a comment trail it
will be impossible to reconstruct what is going on. So I'd like to
encourage not just committers claiming patches but ANYONE who updates
ANYTHING to leave a comment with the details.

....Robert

On Thu, 2 Jul 2009, Robert Haas wrote:

> I think it's important to clarify the distinction between when a
> committer is planning to look at the patch but maybe somebody else
> should too, and when the committer is planning to take exclusive
> responsibility for the patch and nobody else should bother touching it.

Suggesting additional work for the committers is never a good idea, and
the tagging scheme you suggested seems pretty heavy for something that's
not particularly common. How about you add an additional "Committer"
field to the table, to be filled in when it's appropriate to do so and
defaulting to "nobody" as usual. That would be handy in a lot of cases to
track who is working on that part of patch application anyway. And in the
special case you're trying to handle, I'd think listing their name under
both reviewer/committer fields would be sufficient to deflect wasted
review effort.

> Also, if people date-stamp their comments (which will happen
> automatically if we get over to my new system), it makes it possible to
> get a sense of how long it's been since a certain state change took
> place.

The Mediawiki standard here is that you can use the various tilde macros
to fill these in: http://www.mediawiki.org/wiki/Help:Signatures

I wonder if it's possible to include that expansion in the CommitFest
templates? Even if it's not, we could strongly encourage its use. When
you're using the page editor, one of the little buttons at the top is
"Your signature with timestamp", and it expands to "--~~~~"; that makes it
easy to remember this particular bit.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD