Code review process (was: Status of more regular code deployments)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Code review process (was: Status of more regular code deployments)

bawolff
> The problem isn't reverting commits that are bad, it's reverting
> commits solely because they haven't been reviewed.  In a pre-commit
> review system, the equivalent to the proposed 72-hour rule would be
> letting unreviewed code languish without comment.  Both of these are
> bad things.
>
> The point is, people's code should only be rejected with some specific
> reason that either a) lets them fix it and resubmit, or b) tells them
> definitively that it's not wanted and they shouldn't waste further
> effort or hope on the project.  Whether you're using
> review-then-commit or commit-then-review, one of the most demoralizing
> things you can do to a volunteer's contributions is say "nobody cares
> enough to provide any feedback on your code, so we're just going to
> reject it by default".

+1

As a volunteer person, I'm fine if code I commit is reverted based on
it sucking, being too complicated, being too ugly, etc provided there
is actually some objection to the code. However, I'd be rather
offended if it was reverted on the basis of no one got around to
looking at in the last 3 days, since that is something essentially out
of my control.

As it stands, trivial one line changes aren't even reviewed in 72 hours.

-bawolff

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code review process (was: Status of more regular code deployments)

Brion Vibber
On Wed, Jun 1, 2011 at 1:53 PM, bawolff <[hidden email]> wrote:

> As a volunteer person, I'm fine if code I commit is reverted based on
> it sucking, being too complicated, being too ugly, etc provided there
> is actually some objection to the code. However, I'd be rather
> offended if it was reverted on the basis of no one got around to
> looking at in the last 3 days, since that is something essentially out
> of my control.
>

When someone looks at your commit within ~72 hours and reverts it because
nobody's yet managed to figure out whether it works or not and it needs more
research and investigation... what was the reason for the revert?

Because 'no one reviewed it'? Or because someone looked at it and decided it
needed more careful review than they could give yet?

-- brion
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code review process (was: Status of more regular code deployments)

bawolff
On Wed, Jun 1, 2011 at 3:02 PM, Brion Vibber <[hidden email]> wrote:

> On Wed, Jun 1, 2011 at 1:53 PM, bawolff <[hidden email]> wrote:
>>
>> As a volunteer person, I'm fine if code I commit is reverted based on
>> it sucking, being too complicated, being too ugly, etc provided there
>> is actually some objection to the code. However, I'd be rather
>> offended if it was reverted on the basis of no one got around to
>> looking at in the last 3 days, since that is something essentially out
>> of my control.
>
> When someone looks at your commit within ~72 hours and reverts it because
> nobody's yet managed to figure out whether it works or not and it needs more
> research and investigation... what was the reason for the revert?
>
> Because 'no one reviewed it'? Or because someone looked at it and decided it
> needed more careful review than they could give yet?
>
> -- brion
>

That's fine. I'm only concerned with the possibility of no one looks
at the commit, and it gets summarily reverted (Which is what I thought
was being proposed).

--bawolff

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code review process (was: Status of more regular code deployments)

Aryeh Gregor
In reply to this post by Brion Vibber
On Wed, Jun 1, 2011 at 5:02 PM, Brion Vibber <[hidden email]> wrote:
> When someone looks at your commit within ~72 hours and reverts it because
> nobody's yet managed to figure out whether it works or not and it needs more
> research and investigation... what was the reason for the revert?
>
> Because 'no one reviewed it'? Or because someone looked at it and decided it
> needed more careful review than they could give yet?

"Yet" is the keyword here.  Currently the only guarantee anyone has
that their code will be reviewed at all is that it's in trunk, and
historically releases and Wikimedia deployments have always been based
on trunk, so someone's got to either review or revert it at some
point, and it's eventually easier to review than revert once enough
changes have accumulated on top of it.  If a 72-hour revert rule were
instituted with no additional policy changes, there's no reason to
believe any of the reverted commits would ever be reviewed.  And if it
is reviewed weeks or months later, you've got to hope it hasn't
bitrotted, which is a nonissue if it was on trunk the whole time.
Plus it gets testing, if it's on trunk, so the longer it's been there,
the more certain the reviewer can be that it doesn't cause serious
issues.

If the policy worked out to be "95% of volunteer commits get reviewed
within three days, and the rest get temporarily reverted but put on a
list where someone gets to them within a week or two" -- that would be
fine.  Heck, if 95% of volunteer commits got reviewed in three days
and the rest got reverted never to see the light of day again, that
would probably be a big improvement over the status quo.  But
*currently*, what percentage of volunteer commits get reviewed within
three days?

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code review process

Thomas Gries
In reply to this post by bawolff
Hello,

the big problem I have with Code Review and the discussion is:

- merely testing and confirming newly committed code for correctness (if
it works, that it does not break anything else)
seems to be possible or at least not too difficult for the majority of
us including me

- mere checks if the code fulfils code style guides and coding convention
seems also be possible for the mass

- the amount of bugs requires daily work to fix
current priority appears to be fixing bugs (not reviewing fixes)

- but to check against security holes, flaws and potentially dangerous
side effects
seems to be difficult for the average coding person and reviewer

Perhaps some of you (as the "profs") could hold a kind of a "(virtual)
teaching class" to teach newbies in improving their CR skills?
and to have at the end more entrusted code reviewers? "Real teaching
classes" could be also done during next Hackathons, or conferences, as
lessons and as workshops!

Well, it takes efforts and costs at least time: I admit, and we all have
other task to do.

Looking forward to your answers,

CU
Tom




_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Code review process

K. Peachey-2
On Thu, Jun 2, 2011 at 3:00 PM, Thomas Gries <[hidden email]> wrote:
> Perhaps some of you (as the "profs") could hold a kind of a "(virtual)
> teaching class" to teach newbies in improving their CR skills?
Basically its:
* Make sure the commit does what the <del>tin</del><ins>commit
summary</ins> says.
* Doesn't cause any "major" breakages.
* Doesn't introduced any "bad" code or security vulnerability code
(eg: XSS/CSRF)
* Code style guidelines only play a little part (and they can easily
be fixed by anyone, if people submit stuff that is slightly off the
guidelines)

And if people don't feel comfortable reviewing the code or don't have
the experience, they can use the sign off feature to indicated that
they have inspected/tested it and that it seems ok,

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code review process (was: Status of more regular code deployments)

Ariel Glenn WMF
In reply to this post by bawolff
Στις 01-06-2011, ημέρα Τετ, και ώρα 15:58 -0600, ο/η bawolff έγραψε:

> On Wed, Jun 1, 2011 at 3:02 PM, Brion Vibber <[hidden email]> wrote:
> > On Wed, Jun 1, 2011 at 1:53 PM, bawolff <[hidden email]> wrote:
> >>
> >> As a volunteer person, I'm fine if code I commit is reverted based on
> >> it sucking, being too complicated, being too ugly, etc provided there
> >> is actually some objection to the code. However, I'd be rather
> >> offended if it was reverted on the basis of no one got around to
> >> looking at in the last 3 days, since that is something essentially out
> >> of my control.
> >
> > When someone looks at your commit within ~72 hours and reverts it because
> > nobody's yet managed to figure out whether it works or not and it needs more
> > research and investigation... what was the reason for the revert?
> >
> > Because 'no one reviewed it'? Or because someone looked at it and decided it
> > needed more careful review than they could give yet?
> >
> > -- brion
> >
>
> That's fine. I'm only concerned with the possibility of no one looks
> at the commit, and it gets summarily reverted (Which is what I thought
> was being proposed).
>
> --bawolff

One question is how thorough is thorough: if the code is reviewed for
security issues and doesn't apparently break things, does it need to get
gone over with a fine toothed comb before we accept it for deployment?
I would say no, and that we could live with "the commit adheres to our
basic coding guidelines, looks like it will generally do what it says
(without necessarily having tested every detail personally), and follows
general usage patterns (appropriate use of hooks, cacheing, i18n, etc.
etc.)".

Can we ensure that folks have (now and ongoing) enough available time to
review commits to trunk at that level within 72 hours?  This means
buy-in from reviewers *and from their managers* that they have X time
for this and that time will not get reassigned to other priorities.  

If we can't ensure this, we need to decide on the minimum level of
review we *can* get done, or extend the time period, or both.

Asking all committers to trunk to guess whether their reviewer can or
cannot make the 72-hour window for their commit seems like a recipe for
pissing people off.

Just my two cents.

Ariel

P.S. Oh and for the record I would like 4 releases a year ideally, I
could live with 3, and I would be pretty unhappy with two.  Deployment
from trunk should happen a lot more often.  I don't know if we can
manage weekly, but it's a good target.



_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l