Apology, and the patch review process

Apology, and the patch review process

[Jim Blandy]

I'm sorry to have wasted people's time with this mess.  If I lose my
temper with someone, that doesn't need airing here.

I understand that the patch review process is losing --- perhaps
symbol table patches have fared worse than patches to other areas.
I'd honestly welcome constructive suggestions for how to improve this.

In theory, I have time set aside on my schedule for reviewing patches.
In reality, I tend to be more responsive to deadline pressure than
quotidian responsibilities.  The people who can tend their patch queue
properly while still finishing contracts on time really impress me.

Re: Apology, and the patch review process

[Eli Zaretskii]

On Wed, 13 Jun 2001, Jim Blandy wrote:

> I understand that the patch review process is losing --- perhaps
> symbol table patches have fared worse than patches to other areas.
> I'd honestly welcome constructive suggestions for how to improve this.

I don't have any magic wand to offer, but one way to improve things is to 
(1) reply to posted patches quickly, even if the reply just says ``noted, 
will get to it soon''; and (2) review large patches in small chunks and 
publish any requests to modify the reviewed parts as soon as you can say 
something useful.  That still leaves the more general design issues that 
cannot be reviewed without allocating significant time, but at least it 
gets the minor boring issues such as formatting, ChangeLog entries, etc. 
out of your way.

Patch review process

[Andrew Cagney]

> I don't have any magic wand to offer, but one way to improve things is to 
> (1) reply to posted patches quickly, even if the reply just says ``noted, 
> will get to it soon''; and (2) review large patches in small chunks and 
> publish any requests to modify the reviewed parts as soon as you can say 
> something useful.  That still leaves the more general design issues that 
> cannot be reviewed without allocating significant time, but at least it 
> gets the minor boring issues such as formatting, ChangeLog entries, etc. 
> out of your way.


It should be possible to take some of these factors out of the equation. 
  For instance, both tracking patches and needing to comment on 
stylistic issues.

Aegis, for instance, handles the adminstrative side of a patch 
submition.  Before a change even surfaces it has been put through some 
basic checking criteria ex: does it compile; does it meet certain 
criteria of the coding standard; if it fixes a bug does it include a 
test case that pass/fails dependant on the change; ... (you could 
quickly get out of control here :-).  Once this basic criteria have been 
met, it is passed on to the relevant maintainer for approval.  Once 
approved, much of the commit phase is also handled automatically.  The 
system always knows what patches are where.

Aegis, in our environment, however won't fly - at a technical level it 
isn't very good at being distributed distributed.

While trying to build an equivalent system on top of CVS might be 
useful, I think we can take a few more basic steps. We could, for 
instance, make:

	o	-Werror a requirement for patches?

	o	a gdbstyle.sh script (a bit like the ari)
		script I have that checks things like
		indentation and stuff like (free vs xfree)

The other one is a way of better tracking patches.  At present, in the 
end, it is still me using my mailbox for manual processing.

	Andrew

Re: Patch review process

[Christopher Faylor]

On Wed, Jun 13, 2001 at 01:14:38PM -0400, Andrew Cagney wrote:
>The other one is a way of better tracking patches.  At present, in the
>end, it is still me using my mailbox for manual processing.

Didn't we discuss tracking patches in the PR database at one point?

cgf

Re: Patch review process

[Andrew Cagney]

> On Wed, Jun 13, 2001 at 01:14:38PM -0400, Andrew Cagney wrote:
> 
>>The other one is a way of better tracking patches.  At present, in the
>>end, it is still me using my mailbox for manual processing.
> 
> 
> Didn't we discuss tracking patches in the PR database at one point?


It tripped up when testing revealed that you couldn't reliably extract a 
patch.  I think it is also a hack, it lacks the tight integration that 
some other systems have.

To smoke something bad, I'd really like:

	$ gdb-patches submit
	$
	BIFF: from gdb-patches
	Patch rejected:
	Error 101: Sorry Dave, I can't do that, you've used basename() instead of 
basename()
	Error 406: This doesn't compile with -Werror
	Error 555: This causes a testsuite regression
	$
	$ gdb-patches ls
	1: Chris Faylor foo.diff
	2: Cagney bar.diff

	$ gdb-patches cat 1 | more
	.
	,
	,

	$ gdb-patches approve 1
	Patch foo.diff applied and committeed.

other systems do let me do this sort of thing.

I guess, the question is.  Is it less of a hack than we have now?

	Andrew

Re: Patch review process

[Christopher Faylor]

On Wed, Jun 13, 2001 at 04:02:00PM -0400, Andrew Cagney wrote:
>> On Wed, Jun 13, 2001 at 01:14:38PM -0400, Andrew Cagney wrote:
>> 
>>>The other one is a way of better tracking patches.  At present, in the
>>>end, it is still me using my mailbox for manual processing.
>> 
>> 
>> Didn't we discuss tracking patches in the PR database at one point?
>
>
>It tripped up when testing revealed that you couldn't reliably extract a 
>patch.  I think it is also a hack, it lacks the tight integration that 
>some other systems have.

We're using a new version of GNATS on sourceware these days.  I don't
know if it is any better or not for extracting patches, though.

cgf

Re: Patch review process

[Christopher Faylor]

On Wed, Jun 13, 2001 at 04:14:37PM -0400, Christopher Faylor wrote:
>On Wed, Jun 13, 2001 at 04:02:00PM -0400, Andrew Cagney wrote:
>>> On Wed, Jun 13, 2001 at 01:14:38PM -0400, Andrew Cagney wrote:
>>> 
>>>>The other one is a way of better tracking patches.  At present, in the
>>>>end, it is still me using my mailbox for manual processing.
>>> 
>>> 
>>> Didn't we discuss tracking patches in the PR database at one point?
>>
>>
>>It tripped up when testing revealed that you couldn't reliably extract a 
>>patch.  I think it is also a hack, it lacks the tight integration that 
>>some other systems have.
>
>We're using a new version of GNATS on sourceware these days.  I don't
>know if it is any better or not for extracting patches, though.

Oops.  Make that a new version of gnatsweb, not gnats.

cgf

Re: Patch review process

[Daniel Berlin]

Andrew Cagney <ac131313@cygnus.com> writes:

>> I don't have any magic wand to offer, but one way to improve things
>> is to (1) reply to posted patches quickly, even if the reply just
>> says ``noted, will get to it soon''; and (2) review large patches in
>> small chunks and publish any requests to modify the reviewed parts
>> as soon as you can say something useful.  That still leaves the more
>> general design issues that cannot be reviewed without allocating
>> significant time, but at least it gets the minor boring issues such
>> as formatting, ChangeLog entries, etc. out of your way.
> 
> 
> 
> It should be possible to take some of these factors out of the
> equation. For instance, both tracking patches and needing to comment
> on stylistic issues.
> 
> 
> Aegis, for instance, handles the adminstrative side of a patch
> submition.  Before a change even surfaces it has been put through some
> basic checking criteria ex: does it compile; does it meet certain
> criteria of the coding standard; if it fixes a bug does it include a
> test case that pass/fails dependant on the change; ... (you could
> quickly get out of control here :-).  Once this basic criteria have
> been met, it is passed on to the relevant maintainer for approval.
> Once approved, much of the commit phase is also handled automatically.
> The system always knows what patches are where.
> 
> 
> Aegis, in our environment, however won't fly - at a technical level it
> isn't very good at being distributed distributed.
> 
> 
> While trying to build an equivalent system on top of CVS might be
> useful, I think we can take a few more basic steps. We could, for
> instance, make:
> 
> 
> 	o	-Werror a requirement for patches?
> 

Ahh, -Werror has been my god in rewriting the typesystem.

I use it to pinpoint exactly which code needs to be updated to deal
with a given new type code's structure.  Usually, if i just remove the
routines (create_set_type became make_set_type, etc. There was a mix
before, i just made them consistent), or remove a macro, i just get
various *warnings* about unreferenced this, or that. 
Since the lookup_pointer_type now returns a struct pointer_type *, for
instance, i pinpointed all code that would need to change by compiling
-Werror, since you'll get a conversion warning.
Things like that.
It's easier than trying to grep.

The only problem with requiring -Werror free, or so i've heard,  is
you might end up doing things like what i've done
temporarily in some places during the typesystem rewrite.  I add
explicit cases, particularly where a routine declares a
struct type * at the top somewhere, and uses it to store 18 different
kinds of types in it.  Since the various routines now return the
correct structure for their type (lookup_pointer_type returns a struct
pointer_type), and struct type isn't yet the base of all the types,
i'd get incompatible type assignment, when i know it's okay.

Of course, i'm doing it on purpose, and mark them all with a /*TYPEFIX
Explicit cast check*/ comment, to remember to check them later. Some of them
are correct, of course, since struct type will eventually be the base
for all the types. It's not now so i can still compile gdb while i'm
making type changes, changing over one type code at a time to the new
type structures, etc.

But like I said, I've heard that -Werror promotes this type of behavior elsewhere.


> 	o	a gdbstyle.sh script (a bit like the ari)
> 		script I have that checks things like
> 		indentation and stuff like (free vs xfree)
> 
> The other one is a way of better tracking patches.  At present, in the
> end, it is still me using my mailbox for manual processing.
> 
> 
> 	Andrew

-- 
"I don't like the sound of my phone ringing so I put my phone
inside my fish tank.  I can't hear it, but every time I get a
call I see the fish go like this <<<>>><<>><<<<.  I go down to
the pet store -- "Gimme another ten guppies, I got a lotta calls
yesterday."
"-Steven Wright