[Python-Dev] Revised 1.6 jobs list

[Greg Stein]

On Mon, 5 Jun 2000, Jeremy Hylton wrote:
> >>>>> "GS" == Greg Stein <gstein@lyra.org> writes:
>...
>   GS> 1) trust: will the person make sure it is Right And Proper to do
>   GS> the checkin? (have they reviewed the code? is a a Good Change?
>   GS> etc) The counter here is that we don't want people that will
>   GS> simply take stuff arriving at patches@ and checking them in.
> 
> Many of the people who ultimately have checkin privileges should limit
> themselves to their particular domains.  There are a bunch of modules
> that are owned by other people, e.g. Eric's netrc module, your new
> httplib, MAL for Unicode, etc.

Agreed. Each person with commit privs will definitely need to operate
under the premise of "<this> is the area that I'm allowed to commit
changes for." For example, I believe that Guido made it very clear, at
some point in the past, that *nobody* but Guido will have commit access to
the Parser/ area.

Note that when I say "commit access" here, this is the same as "areas that
I can commit for." In fact, it is really just an extension of the Basic
Python Principle, "we're all adults here, don't do what you know you
shouldn't be doing." :-)

>...
> Ultimately, I think I agree with Mark's suggestion that we be a little
> more liberal with changes and risk backing out the occasional
> changes.  (For some definition of "a little more" :-).

This only works if you have reasonably assurance of review via the
-checkins alias. Otherwise, you simply need to accept that instabilities
will creep in. (and that broader alpha/beta/etc cycles may be desirable)

In the Apache group, we have a very good set of eyeballs watching the
checkins. I'd say that each checkin is reviewed by two or three people in
detail. Invariably, there are several checkins each week that result in a
thread discussing: (a) you broke it, (b) why was it done that way, (c)
couldn't it be done this way, (d) this appears to be missing, (e) etc.

IMO, for that to happen several (or more!) times every week means that we
have a good set of eyeballs. Maybe it also means the committers just suck
lollipops, but I'd like to think otherwise :-)

>   GS> 2) more people reviewing changes on the -checkins mailing
>   GS> list. Assuming that you want more than one pair of eyeballs
>   GS> looking at patches/code, that more people with commit privs
>   GS> increases the rate of commits, then you need more reviewers to
>   GS> keep up (because the reviewers probably are not going to review
>   GS> ALL checkins)
> 
> You're doing a great job so far.  We'll just have to get you to spend
> more time on it <0.8 wink>.

hehe...

well, I just completed a big-ass wave of mod_dav work. In fact, I don't
know what else to do to the thing. I made a snapshot and am waiting for
feedback, if any, before declaring it 1.0. Maybe I'll add some comments to
the code :-)

Anyways... that's why I've been working on httplib. Free time! :-)

>   GS> 3) increasing dependence on -checkins means that patches MUST be
>   GS> smaller chunks. they MUST be single-purpose patches. practically
>   GS> nobody will review a 200k patch, or patches that fix N different
>   GS> things at once. A small, focused patch is easily reviewed.
> 
>   GS>    For example: Trent has recently mailed a bunch of patches to
>   GS> the patches list. This is Goodness: each one is focused and can
>   GS> be individually reviewed. Since they are not a single, giant
>   GS> blob, it also keeps them short and reviewable... each can have a
>   GS> +1/-1 independent of the others.
> 
> I agree in principle, but there are times when it will make more sense
> to commit a set of changes as one big patch.  The GC patches are going
> to affect a bunch of files, but probably ought to be done as one big
> commit. 

Actually, I will disagree here. I've reviewed the GC patch a couple times.
There are a number of changes in there that can/should be done separately
from the "real" GC patch.

For example, there were a number of changes to use PyMem_Allco() instead
of PyMEM_Alloc(). Or whatever. Point being, that they had *nothing* to do
with GC -- they could be checked in *independent* of the actual GC work.
The changes to the PyTypeObject declaration (adding tp_clean) can also be
done without doing the "real work".

After these "zero impact" changes are completed, then the GC patch will be
greatly reduced in size and complexity. It makes it much more reviewable.

The point being: even big patches can usually be broken down into layers.
One layer after another, adding "platform" functionality until you get to
the meat of the matter. The GC patch was a definite case of this.

Really huge patches are okay if they have a *seriously* narrow focus. For
example, Skip's patch to use PyArg_ParseTuple() with the ":method" stuff.
That was pretty brain-dead to review. The hardest part was that the
context-diff wasn't contextual enough to verify that the name in the
ParseTuple matched the method that it occurred in.
[ I found a case where they did not match. not easy... ]

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/