Making It Easier To Start Working On Bugzilla

Tue Dec 20 18:05:44 UTC 2005

Executive summary:

More documentation is required to help new developers and reviewers see
the entire development, review, approval and release processes.

Reviewer and developer mentoring needs to be a greater emphasis.

Reviewers and developers both need encouragement.

Details:

I would reply in-line, but I think this makes more sense for this case.
At any rate...

I agree - I think documentation showing users how to get to patches so
they can see just what kinds of things have been approved in the past is
a great way to help others gain confidence in the process.  Small
patches are the best to start with because they 1) demonstrate what kind
of patches we'll accept, 2) demonstrate what the patch should look like,
and 3) give them an opportunity to review the patch like a reviewer
would.  I think it would also be helpful for us to point people to some
samples of patches that were rejected and discuss why those patches were
rejected.  That's jumping a bit ahead of ourselves, however.  I think
the documentation should...

*) Document the entire patch submission process from conception /
initial bug report through approval, check-in and release.
*) Document how outsiders can look at the code via LXR and Bonsai to
find out who to contact regarding specific updates.
*) Document Tinderbox use so developers understand how they can see if
their patch submission worked properly, or if not, what they need to do
to begin the repair process.
*) Document branching and how we use it - do the people doing a check-in
create a branch to test the submission on then merge that back into the
trunk if it works properly?  If not, why?  If so, why?  Who has check-in
authorization?  How does one get it?

Many others have made good points in the discussion as well.  While it's
true that at times, it would be quicker to let the reviewer make small
mods, I think the point is well taken that the whole point of review is
letting reviewers review and not ask them to make code changes.  At the
same time, reviewers have a responsibility to let developers know what
issues they see as soon as possible.  A challenge I've had in the review
process in the past was I was asked to change one thing on one review,
then on another subsequent review, I was asked to change more stuff,
unrelated to what the original review discussed.  It seems that it would
be in our best interest to make sure reviews were adequately complete so
that developers get a good sense of what needs to be fixed up front.
This is especially true for new developers because the more times they
go through r-, the less likely they are to continue submitting.  At some
point, a developer says to him/herself that submission was good enough.
Take it or leave it.  If the reviewer does another r-, development stops
and we loose interest from an otherwise qualified developer.  If the r-
was over nits (I've seen a lot of that), we're shooting ourselves in the
foot.

I would hope that the review process would be more of a mentoring
process than just a gate-keeping process.  I've seen a lot more
mentoring of late and that's good.  As we developers get more and more
r-'s, I think it becomes more critical for the reviewer (when possible)
to make sure that the developer doesn't loose sight of what will change
that r- to an r+.  That means that each review needs to be complete and
at times, reviewers must decide when something is "good enough."  That
is not to say we should drop our quality standards, but there are times
when nits really do get in the way of good submissions.

As a new reviewer, I really appreciate it a bunch when I'm lead to easy
reviews as well so I can learn more about the review process.  At times,
I think it's really helpful for an experienced reviewer to mentor a new
reviewer through the process, helping them see how they would do the
review for a particular patch, discussing what are the plusses and the
minuses of the patch.  If we as reviewers can do that, we'll be doing
ourselves a much better service - we'll be recruiting and training
qualified reviewers.

LpSolit (IRC) asked me if I would do a quick/easy review for him
yesterday.  I gladly accepted.  I asked him some questions about his
code, checked it against the CVS tip, found that it did what it promised
to do, didn't interfere with Bugzilla operationally and gave him the r+.
If I had not understood the implications of his changes, I would have
discussed with him how he would have approached the review and asked him
more about how he would research the patch.  If at that point, I had
felt it was okay to r+, I would have given it the r+ but asked for
another review.  I didn't need to do that for his patch.  In the
meantime, I got another review under my belt and he got his patch one
step closer to inclusion in the Bugzilla trunk.  This is a special case,
of course...  LpSolit is both a developer and reviewer so it was a great
opportunity for mentoring.

---
Kevin Benton
Perl/Bugzilla Developer/Administrator, Perforce SCM Administrator
Personal Computing Systems Group
Advanced Micro Devices

The opinions stated in this communication do not necessarily reflect the
view of Advanced Micro Devices and have not been reviewed by management.
This communication may contain sensitive and/or confidential and/or
proprietary information.  Distribution of such information is strictly
prohibited without prior consent of Advanced Micro Devices.  This
communication is for the intended recipient(s) only.  If you have
received this communication in error, please notify the sender, then
destroy any remaining copies of this communication.

> -----Original Message-----
> From: developers-owner at bugzilla.org
[mailto:developers-owner at bugzilla.org]
> On Behalf Of Jochen Wiedmann
> Sent: Tuesday, December 20, 2005 12:36 AM
> To: developers at bugzilla.org
> Subject: Re: Making It Easier To Start Working On Bugzilla
> 
> Max Kanat-Alexander wrote:
> 
> > 	Any other ideas? Any other barriers that new developers out
there
> have
> > run into? I'd really like to have a *lot* of developers, which would
in
> > turn lead to a lot of reviewers.
> 
> IMO, the Bugzilla developers have a clearly different attitude when
> handling contributed patches than other Open Source projects. There
may
> be reasons for certain policies (quality, in particular), but to me,
the
> policies are sometimes overemphasized.
> 
> When I, as the committer of another Open Source project, receive a
> patch, then it is in my interest to encourage the contributor. Of
> course, if the patch is clearly wrong, overcomplicated, or whatever,
> then I have to reject the patch. But that is rarely the case.
Typically,
>   there are some oddities. Does the developer obeye style guides? Is
the
> indentation right? Is the wording right (in particular, is the
> contributor using proper english)?
> 
> At that point, I have two choices: First, I can tell the contributor
> what I dislike. Second, I can fix the problems for myself and pull the
> patch in. (Or, if a review is required, I can attach a modified
version
> of the patch.)
> 
>  From my experience, Bugzilla developers will *always* choose the
first
> alternative. I do not know, what the reasons are. It may be lack of
> time, but my personal impression is, that accepting the patch will
> sometimes be less work than reviewing a modified patch again.
> 
> Do you think, it is encouraging to provide a patch the next time, if
my
> suggestion is rejected simply because the reviewer does not like my
> wording? Do you think it is likely that I take the work the next time?
> 
> But even when rejecting a patch, I do believe, that developers can be
> more encouraging to the contributor. For example, I once contributed a
> patch to a file, which was removed some weeks later. The review
occurred
> even more weeks later and the developer took that for a reason to
reject
> my patch. Okay, that's understandable. But I would expect to receive
at
> least a hint, where I've got to look now. Being not as deep in the
> current happenings, it took me almost half an hour to find the new
place.
> 
> 
> Jochen
> -
> To view or change your list settings, click here:
> <http://bugzilla.org/cgi-bin/mj_wwwusr?user=kevin.benton@amd.com>