review stop-energy (was 24hour review)

[Taras Glek]

Hi,
Browsers are a competitive field. We need to move faster. Eliminating
review lag is an obvious step in the right direction.

I believe good code review is essential for shipping a good browser.

Conversely, poor code review practices hold us back. I am really
frustrated with how many excellent developers are held back by poor
review practices. IMHO the single worst practice is not communicating
with patch author as to when the patch will get reviewed.

Anecdotal evidence suggests that we do best at reviews where the patch
in question lines up with reviewer's current project The worst thing
that happens there is rubber-stamping (eg reviewing non-trivial 60KB+
patches in 30min).

Anecdotally, latency correlates inversely with how close the reviewer is
to patch author, eg:

project > same team > same part of organization > org-wide > random
community member

I think we need change a couple things*:

a) Realize that reviewing code is more valuable than writing code as it
results in higher overall project activity. If you find you can't write
code anymore due to prioritizing reviews over coding, grow more reviewers.

b) Communicate better. If you are an active contributor, you should not
leave r? patches sitting in your queue without feedback. "I will review
this next week because I'm (busy reviewing ___ this week|away at
conference). I think bugzilla could use some improvements there. If you
think a patch is lower priority than your other work communicate that.

c) If you think saying nothing is better than admitting than you wont
get to the patch for a while**, that's passive aggressiveness
(https://en.wikipedia.org/wiki/Passive-aggressive_behavior). This is not
a good way to build a happy coding community. Managers, look for
instances of this on your team.

In my experience the main cause of review stop-energy is lack of will to
inconvenience own projects by switching gears to go through another
person's work.

I've seen too many amazing, productive people get discouraged by poor
review throughput. Most of these people would rather not create even
more tension by complaining about this...that's what managers are for :)

Does anyone disagree with my 3 points above? Can we make some derivative
of these rules into a formal policy(some sort of code of developer conduct)?

Taras

* There obvious exceptions to above guidelines (eg deadlines).
** Holding back bad code is a feature, not a bug, do it politely.

[Boris Zbarsky]

On 7/9/13 3:14 PM, Taras Glek wrote:
> Conversely, poor code review practices hold us back.

Agreed. At the same time, poor patch practices make reviews _much_
harder. We should generally expect good patch practices from
established contributors; obviously expecting them from new contributors
is not reasonable.

I believe that getting fast review turnaround is a collaboration between
the reviewer and review requester; it shouldn't be solely on the
reviewer to do a fast review no matter how hard the requester makes it.
More on this below.

> a) Realize that reviewing code is more valuable than writing code as it
> results in higher overall project activity.

I agree that it results in higher activity. Whether it results in
overall higher value depends on the activity. But as a first cut, I
agree with this claim.

> If you find you can't write
> code anymore due to prioritizing reviews over coding, grow more reviewers.

Easier said than done, of course. ;) Especially for people who get
flagged to review abandoned code because there is no one else who is
willing to learn it.

> b) Communicate better.

Yes, absolutely.

The flip side of this is "don't request review from people who are
labeled as being away in Bugzilla and expect fast turnaround". I really
wish Bugzilla could let me flag myself as not available for reviews when
I'm on vacation, say. Expecting people to comment about being on
vacation while on vacation is, imo, not reasonable.

> Does anyone disagree with my 3 points above?

Modulo the caveats above, no, but I would like to add some points about
what makes a patch easier to review. A lot of our contributors are not
very good on these points:

* Split mass-changes or mechanical changes into a separate patch from
the substantive changes.

* If possible, separate patches into conceptually-separate pieces for
review purposes (even if you then later collapse them into a single
changeset to push). Any time you're requesting review from multiple
people on a single huge diff, chance are splitting it might have been a
good idea.

* Actually address the review comments before requesting another review.
It's very common to just ignore (miss?) some of the review comments,
which means the reviewer then needs to triple-check that all the things
they pointed out got fixed.

* When requesting a second review on a patch, provide an interdiff so
that the reviewer can just verify that the changes you made match what
they asked for. Bugzilla's interdiff is totally unsuitable for this
purpose, unfortunately, because it fails so often.

* When requesting review or feedback on just part of the patch, make
that very clear.

All of the above are a consequence of a simple observation: time to
review, except for simple mass-changes, is nonlinear in patch size. For
a single review pass, I would say it's probably quadratic in most cases.
Since the number of review passes is itself typically non-constant in
patch size, this means that the common modus operandi of posting a huge
diff with a bunch of issues, then addressing some of the review comments
and posting another huge diff, etc, takes up a huge amount of reviewer
time, most of it basically wasted.

-Boris

[Chris Peterson]

> I really
> wish Bugzilla could let me flag myself as not available for reviews when
> I'm on vacation, say. Expecting people to comment about being on
> vacation while on vacation is, imo, not reasonable.

I've seen people change their Bugzilla name to include a comment about
being on PTO. We should promote this practice. We could also add a
Bugzilla feature (just a simple check box or a PTO date range) that
appends some vacation message to your Bugzilla name.

> * Actually address the review comments before requesting another review.
> It's very common to just ignore (miss?) some of the review comments,
> which means the reviewer then needs to triple-check that all the things
> they pointed out got fixed.

Some review tools track review comments, so checking subsequent patches
is easier. A couple months, I heard discussion about an investigation of
Review Board for Bugzilla. Is anyone still investigating Review Board?

> Bugzilla's interdiff is totally unsuitable for this
> purpose, unfortunately, because it fails so often.

Can we fix Bugzilla's interdiff?


chris

[Boris Zbarsky]

On 7/9/13 4:29 PM, Chris Peterson wrote:
> I've seen people change their Bugzilla name to include a comment about
> being on PTO.

Sure. As a simple example, I did that on June 20th. I got about 20
review requests over the course of the following week and a half, and
that's with most of the people who would normally ask me for review
_not_ doing so because they knew I was away.

>> Bugzilla's interdiff is totally unsuitable for this
>> purpose, unfortunately, because it fails so often.
>
> Can we fix Bugzilla's interdiff?

Not easily, because it does not have access to the original code... The
most common failure mode here is something like this:

1) Author posts patch.
2) Review happens.
3) Author rebases patch to tip, makes edits to address review
comments, re-requests review.

At this point, bugzilla interdiff between the new patch and the old one
will only work if the rebase was exceedingly trivial. Further, what's
really desired in most cases is the diff between the rebased patch and
the patch that addresses comments, not the diff between the old patch
and the new patch.

-Boris

[therealbr...@gmail.com]

Good news is bugzilla is getting attention now, both back-end and front-end. More on that separately, because it's not the main point of Taras's post.

The main point is that review is mandatory and must be prompt or the whole peer review potlatch system breaks down.

I've said this before, not sure it's written in wiki-stone, maybe it should be: if you get a review request, respond same-day either with the actual review, or an ETA or promise to review by a certain date.

People may need reminding or nagging but that should be the exception. It sounds like it isn't exceptional, or rare enough.

/be

[Boris Zbarsky]

On 7/9/13 6:11 PM, therealbr...@gmail.com wrote:
> I've said this before, not sure it's written in wiki-stone, maybe it should be: if you get a review request, respond same-day either with the actual review, or an ETA or promise to review by a certain date.

Again, this is not viable during vacations/weekends... Unless by "get"
you mean "get the mail" as opposed to "have it appear in your queue".

> People may need reminding or nagging but that should be the exception. It sounds like it isn't exceptional, or rare enough.

Indeed.

-Boris

[therealbr...@gmail.com]

On Tuesday, July 9, 2013 6:49:20 PM UTC-7, Boris Zbarsky wrote:

> On 7/9/13 6:11 PM, brendan wrote:
>
> > I've said this before, not sure it's written in wiki-stone, maybe it should be: if you get a review request, respond same-day either with the actual review, or an ETA or promise to review by a certain date.
>
>
>
> Again, this is not viable during vacations/weekends... Unless by "get"
>
> you mean "get the mail" as opposed to "have it appear in your queue".

Yes, that's what I meant. How else could one respond within a day?

> > People may need reminding or nagging but that should be the exception. It sounds like it isn't exceptional, or rare enough.
>
>
>
> Indeed.

Ok. Does this need to go on wiki.m.o or MDN somewhere (not that it would help those who don't read the updated pages)? Should I use a bigger bully pulpit?

/be

[Boris Zbarsky]

On 7/9/13 9:59 PM, therealbr...@gmail.com wrote:
> Yes, that's what I meant. How else could one respond within a day?

Some of the "within a day" proposals have suggested that it include
weekends, fwiw.

> Ok. Does this need to go on wiki.m.o or MDN somewhere (not that it would help those who don't read the updated pages)? Should I use a bigger bully pulpit?

Having something on wikimo for both requester and requestee best
practices would be a good start.

-Boris

[Joshua Cranmer 🐧]

On 7/9/2013 5:11 PM, therealbr...@gmail.com wrote:
> Good news is bugzilla is getting attention now, both back-end and front-end. More on that separately, because it's not the main point of Taras's post.
>
> The main point is that review is mandatory and must be prompt or the whole peer review potlatch system breaks down.
>
> I've said this before, not sure it's written in wiki-stone, maybe it should be: if you get a review request, respond same-day either with the actual review, or an ETA or promise to review by a certain date.

For reviewers who are not Mozilla employees but rather do this only in
spare time, responding within 24 hours can be difficult. I typically end
up doing most reviews in a weekly-ish batch on the weekends, for example.

--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

[Mark Banner]

On 09/07/2013 21:29, Chris Peterson wrote:
>> I really
>> wish Bugzilla could let me flag myself as not available for reviews when
>> I'm on vacation, say. Expecting people to comment about being on
>> vacation while on vacation is, imo, not reasonable.
>
> I've seen people change their Bugzilla name to include a comment about
> being on PTO. We should promote this practice. We could also add a
> Bugzilla feature (just a simple check box or a PTO date range) that
> appends some vacation message to your Bugzilla name.

The problem is, that doesn't work on the patch submission forms. If
bugzilla does decide to autocomplete, you can't necessarily see all the
info on the name, because the field isn't wide enough.

If you don't need/look at the autocomplete, then you get zero indication
of the bugzilla name.

The only real chance you get to see that name change is if you're on a
bug where they have already commented or filed.

I wouldn't want something just for PTOs (e.g. meetups can take up review
time for projects I'm not meeting up for), but I think it would be good
to have the option to provide a warning with a little bit of text *and*
have that a) displayed on the submission forms, and b)
prompted/confirmed if the user decides to request anyway.

Mark.

[smaug]

In general, +1 to all the 3 points. For b) it would be nice if bugzilla would let also the patch author to indicate if some patch isn't anything
urgent. (or perhaps the last sentence of b) means that. Not sure whether 'you' refers to the reviewer or the patch author :) )


One thing, which has often brought up, would be to have other automatic coding style checker than just Ms2ger.
At least in the DOM land we try to follow the coding style rules rather strictly and it would ease reviewers work if
there was some good tool which does the coding style check automatically.



Curious, do we have some recent statistics how long it takes to get a review? Hopefully per module.



On 07/09/2013 03:46 PM, Boris Zbarsky wrote:
....

> * Split mass-changes or mechanical changes into a separate patch from the substantive changes.
>
> * If possible, separate patches into conceptually-separate pieces for review purposes (even if you then later collapse them into a single changeset to
> push). Any time you're requesting review from multiple people on a single huge diff, chance are splitting it might have been a good idea.

...

Splitting patches is usually useful, but having a patch containing all the changes can be also good.
If you have a set of 20-30 patches, but not a patch which contains all the changes, it is often hard to see the big picture.
Again, perhaps some tool could help here. Something which can generate the full patch from the smaller ones.




-Olli

[Boris Zbarsky]

On 7/10/13 8:31 AM, Mark Banner wrote:
> The problem is, that doesn't work on the patch submission forms.

Or on bzexport. I can't recall the last time I used the Bugzilla UI for
requesting review...

> but I think it would be good
> to have the option to provide a warning with a little bit of text *and*
> have that a) displayed on the submission forms, and b)
> prompted/confirmed if the user decides to request anyway.

And communicated via bzapi so bzexport can also warn.

-Boris

[Anthony Ricaud]

On 10/07/13 15:09, smaug wrote:
> One thing, which has often brought up, would be to have other automatic
> coding style checker than just Ms2ger.
> At least in the DOM land we try to follow the coding style rules rather
> strictly and it would ease reviewers work if
> there was some good tool which does the coding style check automatically.

In Gaia, we have a Git pre-commit hook that runs our linter for every
commit (if the committer has installed the linter).

You can also see that we only run it on specific directories.

(And in case you know what you're doing, you can bypass it with |git
commit --no-verify| )

https://github.com/mozilla-b2g/gaia/blob/master/tools/pre-commit

[msrec...@mozilla.com]

On Tuesday, 9 July 2013 15:46:31 UTC-4, Boris Zbarsky wrote:
> On 7/9/13 3:14 PM, Taras Glek wrote:
>
> > Conversely, poor code review practices hold us back.
>
>
>
> Agreed. At the same time, poor patch practices make reviews _much_
>
> harder. We should generally expect good patch practices from
>
> established contributors; obviously expecting them from new contributors
>
> is not reasonable.
>

Why not? If there is a list of good patch practices, there is no reason we can't ask people to complete a checklist and comment on it. It would probably also let us train more reviewers, but letting them know what to start with checking.

>
>
> I believe that getting fast review turnaround is a collaboration between
>
> the reviewer and review requester; it shouldn't be solely on the
>
> reviewer to do a fast review no matter how hard the requester makes it.

+1. "It's so easy to review your code" should be one of the highest praises for a developer (code quality being equal).

--
- Milan

[Boris Zbarsky]

On 7/10/13 12:56 PM, msrec...@mozilla.com wrote:
> Why not?

Because submitting a first patch is scary enough as it is that we should
try to minimize the roadblocks involved in it.

This is also why the reviewer in cases like that should handle setting
the checkin-needed keyword (or just land the patch), push it to try as
needed, etc.

> If there is a list of good patch practices, there is no reason we can't ask people to complete a checklist and comment on it.

I think that's a fine thing to do for people who are planning to do more
than one patch, and is definitely something we should do when someone
starts contributing somewhat consistently. But I'm not convinced that
it's reasonable to require it for a first patch...

-Boris

[Mark Côté]

On 2013-07-09 4:48 PM, Boris Zbarsky wrote:> On 7/9/13 4:29 PM, Chris

Peterson wrote:
>>> Bugzilla's interdiff is totally unsuitable for this
>>> purpose, unfortunately, because it fails so often.
>>
>> Can we fix Bugzilla's interdiff?
>
> Not easily, because it does not have access to the original code...

The BMO team is again considering switching to ReviewBoard, which should
fix this problem, at least for our most-used repos. When we last
evaluated this option, a few years ago, there actually wasn't a BMO
team, so we went with the easier option (Splinter), since it is
nontrivial to do proper Bugzilla-ReviewBoard integration. We have more
resources now, and ReviewBoard seems to answer a lot of user complaints
about code reviews, so we're working on this anew.

Mark

[msrec...@mozilla.com]

On Wednesday, 10 July 2013 13:06:04 UTC-4, Boris Zbarsky wrote:

Fair enough. We don't want to scare people off.

[Boris Zbarsky]

On 7/10/13 1:58 PM, Mark Côté wrote:
> The BMO team is again considering switching to ReviewBoard, which should
> fix this problem

How does ReviewBoard address this?

Again, what we have in the bug is diff 1 against changeset A and diff 2
against changeset B that incorporates the review changes. What the
reviewer would like to see is a diff from 1 rebased against B to 2.

I guess that's possible if the system tries to automatically rebase diff
1 against changeset B, but if the automatic rebase fails you still fail...

-Boris

[Jeff Walden]

On 07/09/2013 07:17 PM, Joshua Cranmer 🐧 wrote:
>> I've said this before, not sure it's written in wiki-stone, maybe it should be: if you get a review request, respond same-day either with the actual review, or an ETA or promise to review by a certain date.
>
> For reviewers who are not Mozilla employees but rather do this only in spare time, responding within 24 hours can be difficult. I typically end up doing most reviews in a weekly-ish batch on the weekends, for example.

I find myself wondering: why 24h, exactly? Why is a day the limit on the time to wait?

Reviewer-side: I've lately tried to minimize my review turnaround time such that I get things done, roughly FIFO, before I get a review-nag mail. I can approximately do that, while keeping up with most of my coding. Establishing one-day turnaround time on reviews, or on requests, would require a lot more polling on my review queue. And I don't think that's a good thing, context-switching from coding tasks being pretty expensive for any developer.

Reviewee-side: maybe I'm just weird, or maybe JS is just different, but I always have half a dozen to a dozen different things I'm doing, or could be doing, at any time. If I find myself waiting for forward progress one one -- waiting for try results, waiting for a review, waiting for my clock to overlap with that of someone in Europe, or anything else -- I can work on something else, and not feel obligated to interrupt whoever/whatever I'm waiting on to ask for prioritization. Outside of must-fix-for-security-uplift-this-week situations, I can't think of a case where 24h turnaround (even to provide expectations) is something I've ever wanted so strongly that I'd force every reviewer to drop enough things to provide it.

Or maybe I'm just weird. But 24h, for much other than new-reviewer requests, seems way shorter than I think the maximum turnaround time should be. If it's a new contributor, okay, 24h or so (modulo weekends, vacations, etc.) is a bit short, but perhaps doable. But for regular contributors, with many things to do while waiting for any particular review, I don't see why the delay shouldn't be a few days, to a week, to maybe slightly over a week.

Jeff

[Taras Glek]

I tried to capture feedback from this thread in
https://wiki.mozilla.org/Code_Review


Taras

[Justin Lebar]

One definition of insanity is doing the same thing twice and expecting
different results.

I recall that Taras has written basically this same e-mail before. We
seem to have this conversation every six months or so. Why do we
expect different results this time?

If I can propose something that's perhaps different:

1) Write software to figure out who's "slow with reviews".
2) We talk to those people.

This is basically the same approach as we had with the tryserver
highscore board. It seems to have helped (?).

I think Bugzilla archaeology was intended to help with [1], but that
site is not useful for this purpose, IMO. But I don't see anything
stopping us from actually writing this software.

[1] https://metrics.mozilla.com/bugzilla-analysis/ReviewHistory.html

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Mark Côté]

True, sorry, I meant only the part about it having access to source
control. Apologies for the diversion.

Mark

[L. David Baron]

[ responding to the two months worth flood of email that just
resulted from https://bugzilla.mozilla.org/show_bug.cgi?id=891906 ]

On Tuesday 2013-07-09 12:14 -0700, Taras Glek wrote:
> a) Realize that reviewing code is more valuable than writing code as
> it results in higher overall project activity. If you find you can't
> write code anymore due to prioritizing reviews over coding, grow
> more reviewers.

Agreed, as long as the reviews are for things that we actually agree
are important.

> b) Communicate better. If you are an active contributor, you should
> not leave r? patches sitting in your queue without feedback. "I will
> review this next week because I'm (busy reviewing ___ this week|away
> at conference). I think bugzilla could use some improvements there.
> If you think a patch is lower priority than your other work
> communicate that.
>
> c) If you think saying nothing is better than admitting than you
> wont get to the patch for a while**, that's passive aggressiveness
> (https://en.wikipedia.org/wiki/Passive-aggressive_behavior). This is
> not a good way to build a happy coding community. Managers, look for
> instances of this on your team.

I think there's a distinction between review requests: some of the
review requests I recieve are assertions "I believe this code is
right, could you check?". Some of them aren't; they're "this seems
to work, but I really have no idea if it's correct; is it?".

I think we should perhaps be able to have an expectation of fast
response on the first set of review requests, but I don't think we
should have an expectation of fast response on the second half,
since many of them require the reviewer to do more work than the
patch author. (I think I get a pretty substantial number of this
form of review requests, at least when counting percentage of time
rather than percentage of requests.)

But sometimes it's also not clear which category the review request
is in, or sometimes it's somewhere in-between. (Maybe we should ask
people to distinguish the types? Should people then be embarrassed
to get a review- on a patch of the first type where they're told to
go back to the drawing board?)


Or maybe I should just summarily minus review requests that appear
to be of the second form, perhaps with pointers as to how the patch
author should learn what's needed to figure out the necessary
information?


I also agree with Boris's comments about things that patch authors
should do to make patches easier to review. I should probably be
better about using review- when patch authors don't do these things,
though I often feel bad about doing that when I've been away for a
week and spent a few days catching up, and it's a patch that's
already been sitting there for ten days. I guess I should just do
it anyway.

My list of things would be:

* make the summary of the bug reflect the problem so that there's a
clear description of what the patch is trying to fix

* split things into small, logical, patches

* write good commit messages that describe what's changing between
old and new code (which, if it can't be summarized in less than
about 100-150 characters, should have a short summary on the
first line and a longer description on later lines)

* write good code comments that describe the state of the new code,
and if the patch is of nontrivial size, point to the important
comments in the non-first lines of the commit message

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla http://www.mozilla.org/ 𝄂

[Taras Glek]

L. David Baron wrote:
> [ responding to the two months worth flood of email that just
> resulted from https://bugzilla.mozilla.org/show_bug.cgi?id=891906 ]
>
> On Tuesday 2013-07-09 12:14 -0700, Taras Glek wrote:

>> a) Realize that reviewing code is more valuable than writing code as
>> it results in higher overall project activity. If you find you can't
>> write code anymore due to prioritizing reviews over coding, grow
>> more reviewers.
>

> Agreed, as long as the reviews are for things that we actually agree
> are important.
>

>> b) Communicate better. If you are an active contributor, you should
>> not leave r? patches sitting in your queue without feedback. "I will
>> review this next week because I'm (busy reviewing ___ this week|away
>> at conference). I think bugzilla could use some improvements there.
>> If you think a patch is lower priority than your other work
>> communicate that.
>>
>> c) If you think saying nothing is better than admitting than you
>> wont get to the patch for a while**, that's passive aggressiveness
>> (https://en.wikipedia.org/wiki/Passive-aggressive_behavior). This is
>> not a good way to build a happy coding community. Managers, look for
>> instances of this on your team.
>

> I think there's a distinction between review requests: some of the
> review requests I recieve are assertions "I believe this code is
> right, could you check?". Some of them aren't; they're "this seems
> to work, but I really have no idea if it's correct; is it?".

I added a low-priority flag request to
https://wiki.mozilla.org/Code_Review#Bugzilla_Improvements

I agree that we should focus on clear rules for good patches. If a patch
author doesn't quite know how to accomplish something, that's more of f?
or a low-priority flag.

Would you mind reworking above wiki with your points?


Taras


>
> -David
>

[L. David Baron]

On Tuesday 2013-07-09 15:46 -0400, Boris Zbarsky wrote:
> * When requesting a second review on a patch, provide an interdiff
> so that the reviewer can just verify that the changes you made match
> what they asked for. Bugzilla's interdiff is totally unsuitable for
> this purpose, unfortunately, because it fails so often.

For what it's worth, I've learned to read diffs of diffs, and I
don't consider it that hard a skill to learn. I read diffs of diffs
for nearly all re-reviews that I do (even ignoring any interdiff in
the bug).

Given one of:
alias vs="wget -q -O -"
alias vs="curl -s"
alias vs="lynx -source"
I just use (in bash):
diff -u <(vs 'https://bugzilla.mozilla.org/attachment.cgi?id=688611') <(vs 'https://bugzilla.mozilla.org/attachment.cgi?id=696690') | less -S

Speaking of re-reviews, I wish bugzilla had a way to indicate that,
since I often get re-reviews long enough after the original review
that I've forgotten it's a re-review. And I prefer to prioritize
re-reviews highest because it helps me get through the reviews
faster, since it will be faster if I still remember what I was
thinking during the first review.

[Neil]

smaug wrote:

> One thing, which has often brought up, would be to have other
> automatic coding style checker than just Ms2ger. At least in the DOM
> land we try to follow the coding style rules rather strictly and it
> would ease reviewers work if there was some good tool which does the
> coding style check automatically.

Or failing that, is this the sort of thing that mentors should be
checking for before contributors/interns apply for a full review?

--
Warning: May contain traces of nuts.

[Robert O'Callahan]

On Wed, Jul 10, 2013 at 6:09 AM, smaug <sm...@welho.com> wrote:

> One thing, which has often brought up, would be to have other automatic
> coding style checker than just Ms2ger.
>

See https://bugzilla.mozilla.org/show_bug.cgi?id=875605, which is,
ironically, waiting for review.

Rob
--
Jtehsauts tshaei dS,o n" Wohfy Mdaon yhoaus eanuttehrotraiitny eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o Whhei csha iids teoa
stiheer :p atroa lsyazye,d 'mYaonu,r "sGients uapr,e tfaokreg iyvoeunr,
'm aotr atnod sgaoy ,h o'mGee.t" uTph eann dt hwea lmka'n? gBoutt uIp
waanndt wyeonut thoo mken.o w *
*

[Robert O'Callahan]

On Wed, Jul 10, 2013 at 3:26 PM, Justin Lebar <justin...@gmail.com>wrote:

> If I can propose something that's perhaps different:
>
> 1) Write software to figure out who's "slow with reviews".
> 2) We talk to those people.
>

We've done this before too.

But we should just do it again --- the "definition of insanity" aphorism is
nonsense. Repetition helps people learn.

[Robert O'Callahan]

We can't have a rigid rule about 24 hours. Someone requesting a review from
me on Thursday PDT probably won't get a response until Monday if neither of
us work during the weekend.

But I think it's reasonable to expect developers to process outstanding
review requests (and needinfos) at least once every regular work day.
Processing includes leaving a comment with an ETA.

[Robert O'Callahan]

On Wed, Jul 10, 2013 at 2:48 PM, Jeff Walden <jwald...@mit.edu> wrote:

> Reviewer-side: I've lately tried to minimize my review turnaround time
> such that I get things done, roughly FIFO, before I get a review-nag mail.
> I can approximately do that, while keeping up with most of my coding.
> Establishing one-day turnaround time on reviews, or on requests, would
> require a lot more polling on my review queue. And I don't think that's a
> good thing, context-switching from coding tasks being pretty expensive for
> any developer.
>

I think a reasonable expectation is that paid staff process outstanding
review requests at least once every regular work day. If you do it at the
beginning of the day, before diving into your other work, it won't
interrupt that other work.

[Nicholas Nethercote]

On Thu, Jul 11, 2013 at 5:06 PM, Robert O'Callahan <rob...@ocallahan.org> wrote:
> On Wed, Jul 10, 2013 at 6:09 AM, smaug <sm...@welho.com> wrote:
>

>> One thing, which has often brought up, would be to have other automatic
>> coding style checker than just Ms2ger.
>

> See https://bugzilla.mozilla.org/show_bug.cgi?id=875605, which is,
> ironically, waiting for review.

https://bugzilla.mozilla.org/show_bug.cgi?id=880088 is similar (albeit
SpiderMonkey-only), and is also waiting for review!

Nick

[Nicholas Nethercote]

On Thu, Jul 11, 2013 at 7:48 AM, Jeff Walden <jwald...@mit.edu> wrote:
>
> Establishing one-day turnaround time on reviews, or on requests, would require a lot more polling on my review queue.

You poll your review queue? Like, by visiting your Bugzilla
dashboard, or something like that? That's *awful*.

I personally use a push notification system called "email with
filters". Well, strictly speaking it's poll-like because I have to
check my "high priority bugs" folder, but I do that anyway multiple
times per day so I'm unlikely to take more than an hour or two (while
working) to notice a review request.

Seriously, if your bug management system causes you to not notice
review requests very quickly, it is broken.

Nick

[Gijs Kruitbosch]

I find this fascinating. I am like Nick in this respect (although I
don't need to filter out review requests from other bugmail, I do use
filters for bugmail and will notice them quickly enough, generally
speaking). My email client is always open and I generally notice bugmail
more or less as it happens.

However, contemporary wisdom seems to be that people get more done if
they use pull rather than push systems. I'm not sure we should force
people to use a push system here (although I do think it'd be good to
have reviews go faster / get ETAs if they can't go fast).

~ Gijs

[Gijs Kruitbosch]

On 11/07/13 09:08 , Robert O'Callahan wrote:
> On Wed, Jul 10, 2013 at 3:26 PM, Justin Lebar <justin...@gmail.com>wrote:
>
>> If I can propose something that's perhaps different:
>>
>> 1) Write software to figure out who's "slow with reviews".
>> 2) We talk to those people.
>>
>
> We've done this before too.
>
> But we should just do it again --- the "definition of insanity" aphorism is
> nonsense. Repetition helps people learn.
>
> Rob

I am confused regarding "figure out who's 'slow with reviews'". Taras'
original proposal explicitly outlined the option of giving an ETA
because it's not always possible to be "super fast with reviews", to put
it that way. How do we intend to software-solve this ETA giving rather
than r+/-/_ setting? Or am I making perfect the enemy of the good here? :-)

~ Gijs

[Gervase Markham]

On 09/07/13 21:29, Chris Peterson wrote:
> I've seen people change their Bugzilla name to include a comment about
> being on PTO. We should promote this practice. We could also add a
> Bugzilla feature (just a simple check box or a PTO date range) that
> appends some vacation message to your Bugzilla name.

Hey, if we had a PTO app that tracked all absences, we could integrate
with it...
<sigh>

> Some review tools track review comments, so checking subsequent patches
> is easier. A couple months, I heard discussion about an investigation of
> Review Board for Bugzilla. Is anyone still investigating Review Board?

Yes; it's on the BMO roadmap to be integrated; I believe its even being
worked on now. CCing glob.

Gerv

[Gervase Markham]

On 10/07/13 23:14, Taras Glek wrote:
> I tried to capture feedback from this thread in
> https://wiki.mozilla.org/Code_Review

I just did a pass over that page to highlight the key points.

Gerv

[Gervase Markham]

On 10/07/13 15:09, Boris Zbarsky wrote:
> And communicated via bzapi so bzexport can also warn.

BzAPI could add a flag based on a parsing of the name - but then, if
there was an accurate algorithm for parsing a name to extract absence
information, bzexport could use it directly.

Perhaps we could enhance bzexport to request the username of the user of
whom you are requesting review, and display it to you at the time of
submission? You could then notice manually if they were away.

Gerv

[Boris Zbarsky]

On 7/11/13 7:59 AM, Gervase Markham wrote:
> Hey, if we had a PTO app that tracked all absences, we could integrate
> with it...
> <sigh>

Just in case you were talking about the moco PTO app, it doesn't track
absences for non-MoCo employees, and even for employees it does not
track non-PTO absences (being a PTO app and all).

-Boris

[Justin Lebar]

One thing I've been thinking about is /why/ people are slow at reviews.

Someone who usually has a long review queue has told me that he "hates"
reviewing code. I realized that we don't really have a place at Mozilla for
experienced hackers who don't want to do reviews. Should we? Could we do this
without violating people's sense of fairness?

As another data point, someone told me recently that he's trying not to become
a peer of the modules he's working on, because he doesn't want to review code.
Maybe we're not incentivizing people enough to be reviewers.

Roc said that we've spoken with people who are slow at reviews. Did we learn
anything from that? I'd expect that people are slow for different reasons.

It seems backward to me to focus on a solution without first understanding the
causes.

-Justin

On Thu, Jul 11, 2013 at 3:08 AM, Robert O'Callahan <rob...@ocallahan.org> wrote:
> On Wed, Jul 10, 2013 at 3:26 PM, Justin Lebar <justin...@gmail.com>
> wrote:
>>
>> If I can propose something that's perhaps different:
>>
>> 1) Write software to figure out who's "slow with reviews".
>> 2) We talk to those people.
>
>
> We've done this before too.
>
> But we should just do it again --- the "definition of insanity" aphorism is
> nonsense. Repetition helps people learn.
>
> Rob

[Mark Banner]

On 11/07/2013 12:59, Gervase Markham wrote:
> On 09/07/13 21:29, Chris Peterson wrote:
>> I've seen people change their Bugzilla name to include a comment about
>> being on PTO. We should promote this practice. We could also add a
>> Bugzilla feature (just a simple check box or a PTO date range) that
>> appends some vacation message to your Bugzilla name.
>
> Hey, if we had a PTO app that tracked all absences, we could integrate
> with it...
> <sigh>

I'd rather have a custom field anyway. Sometimes I'm highly focussed on
a project for particular reason, or doing meet-up weeks. During those
times I rarely even check bugmail for my other projects.

Mark.

[Ehsan Akhgari]

On 2013-07-11 3:06 AM, Robert O'Callahan wrote:
> On Wed, Jul 10, 2013 at 6:09 AM, smaug <sm...@welho.com> wrote:
>
>> One thing, which has often brought up, would be to have other automatic
>> coding style checker than just Ms2ger.
>>
>
> See https://bugzilla.mozilla.org/show_bug.cgi?id=875605, which is,
> ironically, waiting for review.

Anybody who knows python can help with reviews there, FWIW.

Cheers,
Ehsan

[Ted Mielczarek]

On 7/11/2013 9:23 AM, Justin Lebar wrote:
> One thing I've been thinking about is /why/ people are slow at reviews.
>
> Someone who usually has a long review queue has told me that he "hates"
> reviewing code. I realized that we don't really have a place at Mozilla for
> experienced hackers who don't want to do reviews. Should we? Could we do this
> without violating people's sense of fairness?
>
> As another data point, someone told me recently that he's trying not to become
> a peer of the modules he's working on, because he doesn't want to review code.
> Maybe we're not incentivizing people enough to be reviewers.
>
> Roc said that we've spoken with people who are slow at reviews. Did we learn
> anything from that? I'd expect that people are slow for different reasons.
>
> It seems backward to me to focus on a solution without first understanding the
> causes.

I can definitely sympathize with this, as the build system owner (now
peer), and test harness owner I get a lot of reviews thrown my way. I
have been able to pretty successfully grow the peer list of both modules
over the past few years, and it's worked out pretty well. One nice
benefit that people shouldn't overlook is that by becoming a peer in a
module you work in you help spread out the review load, which means that
the owner and other peers have more time to review *your* patches. This
has been really great for me in the build system, where I prioritize
reviews requests from the other peers, and in return also get quick
turnaround on patches I need review for. It's a little "tit-for-tat",
but I think it's a healthy positive feedback loop.

-Ted

[Boris Zbarsky]

On 7/11/13 9:23 AM, Justin Lebar wrote:
> One thing I've been thinking about is /why/ people are slow at reviews.

Here are some possible reasons I've encountered myself:

1) Feeling overwhelmed because you have too many reviews pending and
therefore just staying away from your list of reviews. Note that this
can be self-perpetuating.

2) Feeling like you have to do your reviews in some sort of FIFO order,
so that a review for a large patch (which takes a long time because
that's just how large patches work) will block possibly-quick reviews
for smaller patches.

3) Feeling like you want to do your reviews in some sort of
most-expeditious order, so that a constant stream of small patches
delays review of a big patch for weeks or months.

4) Feeling that you need multiple hours of uninterrupted time, probably
over several days for review of a complicated patch and not being able
to find it due to the scattering of meetings, people asking you
questions, smaller reviews, etc that you're also doing. Note that
anyone who has no time to code because of reviews probably also has no
time to do big complicated reviews, for the same reasons!

5) Simply having too high a review load. Anecdotally, it typically
takes me until sometime Tuesday afternoon at best to catch up on all the
review requests that come in between end-of-work Friday and Monday morning.

6) Just disliking doing reviews (but more on this below).

7) Wanting to completely focus on some other complicated task for a few
days ... or weeks.

It would be interesting to see what other reasons there are and what the
relative frequencies are.

> Someone who usually has a long review queue has told me that he "hates"
> reviewing code.

Reviewing code is just not all that fun. You have to tell people they
did something wrong, often with some fairly arbitrary (in the grand
scheme of things; we do it so we have consistent coding style) nitpicks,
there is not that much of a sense of accomplishment at the end. It
feels very adversarial at times. Sometimes you learn new things in the
process, but more often you try hard to politely tell people that
they've screwed something up... possibly after you already told them
that about that same thing in the previous iteration of the patch. At
least for me, it can get downright depressing at times.

But debugging leaks is not that fun either. Or trying to read through
flat profiles and speed things up. Or hunting down a dangling pointer
bug. Or trying to convince people on a standards mailing list to not do
something insane, for that matter.

Fundamentally, we feel that reviews are useful and that means they have
to get done; the doing is not all that fun, but it's a sort of giving
back to the community, in my view.

> I realized that we don't really have a place at Mozilla for
> experienced hackers who don't want to do reviews. Should we? Could we do this
> without violating people's sense of fairness?

How do we handle this with the other unpalatable tasks above?

Informally, what I see is that they get shunted to people who are either
not as unhappy doing them or have a stronger sense of "I don't like it,
but it needs doing, and no one else is stepping up". And it sure as
heck violates people's sense of fairness. Of course we do that with
reviews too (a well-known technique for doing fewer reviews is just
being slow about it so people stop asking, whereas someone who does
reviews quickly is "rewarded" by people piling on reviews), and it
likewise violates people's sense of fairness.

Of course the other thing I see happen with the unpalatable tasks I list
above is that they just don't get done in many cases because no one
steps up. That's a bit rarer with reviews, because we don't consider
those as optional. But it has happened: I've seen patches die because no
one stepped up to review.

Here's another way to look at this: do these hackers want _others_ to
review their patches? If so, how do they expect that to happen?

Obviously everyone would prefer to work on the things they _want_ to
work on, not necessarily the things that _need_ to be worked on. To the
extent that someone focuses more on the former than on the latter,
they're shafting others whose sense of duty won't let them do that. The
result is that you get people who are unhappy because they never get to
work on the things they want to or who are unhappy because they're doing
all their "I would like to get this done" work after hours. Or both, at
times: only working on things that need to happen _and_ having to do it
after hours...

And as a further note, the more reviews pile on just a small subset of
people, the more problems 1-5 listed above manifest. As Taras said, the
right solution to review load is to grow more reviewers, but that's hard
to do if people are basically shirking the responsibility.

> As another data point, someone told me recently that he's trying not to become
> a peer of the modules he's working on, because he doesn't want to review code.

But he wants his own code reviewed, right? ;)

> Maybe we're not incentivizing people enough to be reviewers.

That's very much possible, yes. It's not clear to me how to best
incentivize this without creating incentives for rubber-stamping code.

-Boris

[Ehsan Akhgari]

I've talked to the BMO folks about this before. ReviewBoard doesn't
know what the base revision of patches are, so it will not be helpful.
It's just a fancier splinter. To do this properly we need to push
commits/changesets somewhere instead of attaching textual diffs, and
unfortunately that doesn't seem to be a problem that the ReviewBoard
switch is trying to solve.

Ehsan

[Robert O'Callahan]

While I think your observations are useful, I think (hope!) you are a
massive outlier and I don't think we should design a policy based on the
assumption that your review commitments are in any way normal.

I would be totally OK with a policy that explicitly applies to everyone
except you. Or, we could parameterize it with a dynamic list of exceptions
who are known to be overloaded with reviews. In fact, making that list
explicit and publishing it would let people know not to request reviews
from people on that list except as a last resort (which is, in fact, how I
treat you and dbaron).

Rob
--
Jtehsauts tshaei dS,o n" Wohfy Mdaon yhoaus eanuttehrotraiitny eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o Whhei csha iids teoa
stiheer :p atroa lsyazye,d 'mYaonu,r "sGients uapr,e tfaokreg iyvoeunr,
'm aotr atnod sgaoy ,h o'mGee.t" uTph eann dt hwea lmka'n? gBoutt uIp

waanndt wyeonut thoo mken.o w *
*

[Robert O'Callahan]

On Thu, Jul 11, 2013 at 6:23 AM, Justin Lebar <justin...@gmail.com>wrote:

> Someone who usually has a long review queue has told me that he "hates"

> reviewing code. I realized that we don't really have a place at Mozilla

> for
> experienced hackers who don't want to do reviews. Should we?
>

I think someone can "hate doing reviews" and also "want to do reviews"
because they're important. I don't know anyone who loves doing reviews.

As another data point, someone told me recently that he's trying not to
> become
> a peer of the modules he's working on, because he doesn't want to review
> code.
>

If someone who can do reviews is avoiding them, they're pushing workload
onto their peers who probably don't enjoy it much more. That's antisocial
and unacceptable IMHO.

[Robert O'Callahan]

The way I work is that review and needinfo requests go to my inbox and I
process them with high priority. I can and do ignore them there temporarily
if I need to. Given I use my inbox as a to-do list, that approach seems
perfect for me.

[Boris Zbarsky]

On 7/11/13 11:37 AM, Robert O'Callahan wrote:
> While I think your observations are useful, I think (hope!) you are a
> massive outlier

Somewhat. My list was based on not only what makes my reviews slow but
what I've heard people mention as making reviews slow.

I do agree we shouldn't design a policy based on my review queue. ;)

> I would be totally OK with a policy that explicitly applies to everyone
> except you. Or, we could parameterize it with a dynamic list of exceptions
> who are known to be overloaded with reviews.

So the thing is, I'm not _always_ overloaded with reviews. I generally
keep on top of them unless I take time off or several large patches end
up in my review queue at once.

And I think that applies to most people who are trying to do their
reviews... The problem is that right now there is no way to mark
yourself as "temporarily less responsive on reviews than normal",
whether it's because you're on vacation or because you have a 500KB
patch to review that will take all week. And then reviews pile on.

-Boris

[L. David Baron]

On Thursday 2013-07-11 00:14 -0700, Robert O'Callahan wrote:
> We can't have a rigid rule about 24 hours. Someone requesting a review from
> me on Thursday PDT probably won't get a response until Monday if neither of
> us work during the weekend.
>
> But I think it's reasonable to expect developers to process outstanding
> review requests (and needinfos) at least once every regular work day.
> Processing includes leaving a comment with an ETA.

So, partly, I'm really bad at figuring out ETAs for small tasks,
since I find the priority order of small tasks to be relatively
dynamic, given how often small things come up. For example, reading
and responding to this thread (something I've spent at least 15
minutes on so far this morning, and it'll probably end up being more
than 30, which is probably 10% of my non-meeting working day today).
Should I have prioritized that above doing code reviews, or should I
come back to this thread and give you my thoughts in 3 weeks (as I'm
currently doing on the prefixing policy thread, which I feel
requires more thought)?

I spend a pretty big portion of my time on things that come up at
the last minute: questions from colleagues, discussions on lists
(Mozilla lists and standards lists, etc.). (Another interesting
question: should I prioritize questions / needinfos from people in
the *middle* of writing a patch over code reviews which are at the
*end* of writing a patch? Right now I think I sometimes do, and
sometimes treat them equally.)

Like Boris, I feel guilty about not getting to reviews, and I feel
like I'm bad at figuring out how to prioritize them.

I suppose what leaving an ETA would do is force me to try to stick
to what I've promised, which in turn means doing code reviews rather
than doing things like reading email or responding to this thread.

[Mark Côté]

To be clear and to correct something I said earlier: we are standing up
a somewhat independent RB service this quarter, which will be connected
to Bugzilla's auth and will know about some common repos but nothing
more. RB seems great but I don't think we've conclusively decided that
it's the best solution; we probably need to experiment with it a bit to
make sure it works best for us.

Mark

[Milan Sreckovic]

That last thing was another item I found useful in the previous life. When requesting a review from somebody, people could see "this person currently has X items in their review queue". You can ignore that information, but it's there and it may help. It's still probably simpler for the reviewer to know they're too busy, notify the author, and help them find somebody else if they can't agree on the timeframe.

On 2013-07-11, at 11:37 , Robert O'Callahan <rob...@ocallahan.org> wrote:

> While I think your observations are useful, I think (hope!) you are a

> massive outlier and I don't think we should design a policy based on the
> assumption that your review commitments are in any way normal.
>

> I would be totally OK with a policy that explicitly applies to everyone
> except you. Or, we could parameterize it with a dynamic list of exceptions

> who are known to be overloaded with reviews. In fact, making that list
> explicit and publishing it would let people know not to request reviews
> from people on that list except as a last resort (which is, in fact, how I
> treat you and dbaron).
>

> Rob
> --
> Jtehsauts tshaei dS,o n" Wohfy Mdaon yhoaus eanuttehrotraiitny eovni
> le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o Whhei csha iids teoa
> stiheer :p atroa lsyazye,d 'mYaonu,r "sGients uapr,e tfaokreg iyvoeunr,
> 'm aotr atnod sgaoy ,h o'mGee.t" uTph eann dt hwea lmka'n? gBoutt uIp

> waanndt wyeonut thoo mken.o w *
> *
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Jet Villegas]

I may have a skewed view of things, but I personally have not had problems getting prompt code reviews. I also don't have a problem talking to my reviewers about what I'm hacking on. I'm hesitant to throw a bunch of technology at this problem, if it's a social issue. Code reviews are a conversation and more code has to come with more conversations.

Talk to each other, people!

We'll cover this topic at the next Rendering Meeting. Let's have a look at the bugs sitting in review and reassign reviewers as needed. We need to grow more reviewers, and the only way I know to do that is to trust new people with reviewing code (and check their work.)

--Jet

[Jeff Walden]

On 07/11/2013 03:09 AM, Nicholas Nethercote wrote:
> On Thu, Jul 11, 2013 at 7:48 AM, Jeff Walden <jwald...@mit.edu> wrote:
>>
>> Establishing one-day turnaround time on reviews, or on requests, would require a lot more polling on my review queue.
>
> You poll your review queue? Like, by visiting your Bugzilla
> dashboard, or something like that? That's *awful*.
>
> I personally use a push notification system called "email with
> filters". Well, strictly speaking it's poll-like because I have to
> check my "high priority bugs" folder, but I do that anyway multiple
> times per day so I'm unlikely to take more than an hour or two (while
> working) to notice a review request.

I have https://bugzilla.mozilla.org/request.cgi?requestee=jwalden%2Bbmo%40mit.edu&do_union=1&group=type&action=queue open in a browser tab and check it from time to time. I don't see how that's any different from a mail-filtering folder except in terms of the UI. They're both polling, as you note. :-)

Jeff

[Chris Pearce]

On 7/11/2013 8:55 AM, Boris Zbarsky wrote:
> On 7/11/13 11:37 AM, Robert O'Callahan wrote:
>> While I think your observations are useful, I think (hope!) you are a
>> massive outlier
>
> Somewhat. My list was based on not only what makes my reviews slow
> but what I've heard people mention as making reviews slow.
>
> I do agree we shouldn't design a policy based on my review queue. ;)

Maybe you need to start farming out reviews to other/newer members of
the teams you work on? We've done that in media; giving reviews to the
newer members of the team has been a great way to spread the load, and
upskill the other team members.

Obviously some patches need the attention of an experienced hacker like
yourself, but style nitsand mechanical errors etc can be picked up a
less experienced reviewer. You could always look for the big issues,
feedback+/- and then reassign the review request to another reviewer.


Chris P.

[Axel Hecht]

I wish I could watch more than one requestee in one page. I actually
have a tab with a history of three bugzilla accounts' requests page, and
go back and forth and reload.

Axel

[Boris Zbarsky]

On 7/11/13 2:41 PM, Chris Pearce wrote:
> Maybe you need to start farming out reviews to other/newer members of
> the teams you work on?

Oh, that's been happening. A lot.

-Boris

[Neil]

Milan Sreckovic wrote:

>That last thing was another item I found useful in the previous life. When requesting a review from somebody, people could see "this person currently has X items in their review queue".
>

Even better would be if Bugzilla could compute their median review
turnaround for you.

--
Warning: May contain traces of nuts.

[Mounir Lamouri]

On 11/07/13 16:43, Neil wrote:
> Milan Sreckovic wrote:
>
>> That last thing was another item I found useful in the previous life.
>> When requesting a review from somebody, people could see "this person
>> currently has X items in their review queue".
>>
> Even better would be if Bugzilla could compute their median review
> turnaround for you.

Those two metrics are pretty bad because they will move most review
requests to a set of reviewers that have a fast turnaround. I'm not sure
that is a good idea even though experienced developers tend to already
know who are the fast reviewers.

--
Mounir

[Milan Sreckovic]

On 2013-07-12, at 11:46 , Mounir Lamouri <mou...@lamouri.fr> wrote:

> On 11/07/13 16:43, Neil wrote:

>> Milan Sreckovic wrote:
>>
>>> That last thing was another item I found useful in the previous life.
>>> When requesting a review from somebody, people could see "this person
>>> currently has X items in their review queue".
>>>
>> Even better would be if Bugzilla could compute their median review
>> turnaround for you.
>

> Those two metrics are pretty bad because they will move most review
> requests to a set of reviewers that have a fast turnaround. I'm not sure
> that is a good idea even though experienced developers tend to already
> know who are the fast reviewers.

I would definitely not use this information as an automatic way of selecting a reviewer, but the information may help.


>
> --
> Mounir

[Hubert Figuière]

On 10/07/13 10:17 AM, Anthony Ricaud wrote:

> In Gaia, we have a Git pre-commit hook that runs our linter for every
> commit (if the committer has installed the linter).
>
> You can also see that we only run it on specific directories.
>
> (And in case you know what you're doing, you can bypass it with |git
> commit --no-verify| )

Good to know. (I did it differently to work around)

As the other day I encountered this:

https://bugzilla.mozilla.org/show_bug.cgi?id=892329

Note: the review problems also apply to bug triage. Sometime you file a
bug and it is lost, ie nobody comments on it seems to view it.

That itself was a stop for a contributor.

:-/

Hub

[Gervase Markham]

I was talking about a possible future app which did this. Hence the
<sigh> that we don't have it. (We do have a new PTO app, but it's not
been deployed, ostensibly due to legal reasons because it tracked
non-PTO absences!)

Gerv

[Honza Bambas]

On 7/10/2013 3:09 PM, smaug wrote:
> Splitting patches is usually useful, but having a patch containing all
> the changes can be also good.
> If you have a set of 20-30 patches, but not a patch which contains all
> the changes, it is often hard to see the big picture.
> Again, perhaps some tool could help here. Something which can generate
> the full patch from the smaller ones.
>
>
Since I know how hard reviewing is, when I'm submitting a patch for a
review (usually a larger one) I always do:

- explanation of what the patch is doing in few or more points ;
anything that could be perceived unexpected in the patch is also
explained very well
- providing patch split to logically separated parts with numbers like
"part 1 of 6"
- and also a complete (folded) patch for reference
- strictly versioning the patch among review rounds
- when submitting a new version of a patch after r- always explain what
has changed and provide an interdiff
- before that I take great care to address all r comments or discuss
them well

This should be a standard IMO.

-hb-

>
>
> -Olli

[Mike Hoye]

On 2013-07-11 2:14 PM, Jet Villegas wrote:
> I may have a skewed view of things, but I personally have not had problems getting prompt code reviews. I also don't have a problem talking to my reviewers about what I'm hacking on. I'm hesitant to throw a bunch of technology at this problem, if it's a social issue. Code reviews are a conversation and more code has to come with more conversations.

For what it's worth, there's some data on the subject available (inside
the firewall only, unfortunately) here:

https://metrics.mozilla.com/bugzilla-analysis/ReviewIntensity.html#sampleInterval=day&sampleMax=2013-07-15&sampleMin=2013-01-15&requestee=

You can drill into that data on a per-project or per-team basis, but in
aggregate you can see that in the last six months more than two-thirds
of reviews already happen in less than a day, and it tails off pretty
sharply after that.

That's not to say there's not room for improvement there; the ~2200
reviews that take two or three days, and ~1700 that take four to six, vs
the ~12500 one-days, still seems like a lot, and I agree with Jet that
some communication - particularly with new contributors or first-timers
- would go a long way to relieving the stop-energy problem even if you
can't get to the review in the next eight hours.

There's a blip at the right hand side of the graph there, but a casual
inspection of the "over 10 days" pile seems to indicate that those are
mostly hard problems, not neglect.

- mhoye

[Chris Peterson]

On 7/15/13 7:10 AM, Honza Bambas wrote:
> - providing patch split to logically separated parts with numbers like
> "part 1 of 6"
> - and also a complete (folded) patch for reference
> - strictly versioning the patch among review rounds
> - when submitting a new version of a patch after r- always explain what
> has changed and provide an interdiff

If reviewee submits a new version of (say) patch 1 of 6, should they:

* attach patch 1 version 2
* an interdiff between patch 1 version 1 and 2
* and a new complete/folded patch (of patches 1-6)?

Which file would be r+'d for review?


chris

[Boris Zbarsky]

On 7/15/13 2:36 PM, Chris Peterson wrote:
> If reviewee submits a new version of (say) patch 1 of 6, should they:
>
> * attach patch 1 version 2
> * an interdiff between patch 1 version 1 and 2

Yes, to both.

> * and a new complete/folded patch (of patches 1-6)?

Usually not needed.

> Which file would be r+'d for review?

Generally whatever will actually get checked in.

-Boris

[Steve Fink]

On Mon 15 Jul 2013 11:43:05 AM PDT, Boris Zbarsky wrote:
> On 7/15/13 2:36 PM, Chris Peterson wrote:
>> If reviewee submits a new version of (say) patch 1 of 6, should they:
>>
>> * attach patch 1 version 2
>> * an interdiff between patch 1 version 1 and 2
>
> Yes, to both.

Bleagh. All of this points to an annoying gap in our tooling. Can't we
get a multiheaded repo of some sort that we can push this stuff to, to
avoid the need for these contortions? I guess it would probably suffer
the same perf death as the try repo, unless we prune out heads from
resolved (or shipped?) bugs.

>> Which file would be r+'d for review?
>
> Generally whatever will actually get checked in.

Even for the case of dependent patches (ones with separate parts that
cannot be landed together, but are usefully split up for review)? In
that case, I'd expect only the individual patches to carry the r+. I
have occasionally uploaded a new rollup patch and marked it r+ myself
in these situations, but usually I just ignore or obsolete the rollup
when it's no longer needed for reviewers or fuzzers. Even when it's the
rollup that I land. Perhaps that's wrong of me, but it seems like the
patches attached to bugs and the patches that actually land aren't very
well correlated in our current setup anyway. I regard bug attachments
as totally review-focused, not commit-focused. The info about what was
committed is communicated (accurately) via the landing revision urls.

[Boris Zbarsky]

On 7/15/13 2:58 PM, Steve Fink wrote:
> Even for the case of dependent patches (ones with separate parts that
> cannot be landed together, but are usefully split up for review)?

Assuming s/cannot/must/, that's why I said "generally", not "always". ;)

> Perhaps that's wrong of me, but it seems like the
> patches attached to bugs and the patches that actually land aren't very
> well correlated in our current setup anyway.

They should be for anyone using checkin-needed... But yes, in other
cases maybe not so much.

-Boris

[Benjamin Smedberg]

On 7/15/2013 2:58 PM, Steve Fink wrote:
> On Mon 15 Jul 2013 11:43:05 AM PDT, Boris Zbarsky wrote:

>> On 7/15/13 2:36 PM, Chris Peterson wrote:
>>> If reviewee submits a new version of (say) patch 1 of 6, should they:
>>>
>>> * attach patch 1 version 2
>>> * an interdiff between patch 1 version 1 and 2
>>
>> Yes, to both.
>

> Bleagh. All of this points to an annoying gap in our tooling. Can't we
> get a multiheaded repo of some sort that we can push this stuff to, to
> avoid the need for these contortions? I guess it would probably suffer
> the same perf death as the try repo, unless we prune out heads from
> resolved (or shipped?) bugs.

What I'd love to have is a per-bug repo that you could push to. You
could push new trees after rebasing/review nits and all would be
awesome. But alas, that is not yet a reality.

--BDS

[Honza Bambas]

On 7/15/2013 8:36 PM, Chris Peterson wrote:
> On 7/15/13 7:10 AM, Honza Bambas wrote:
>> - providing patch split to logically separated parts with numbers like
>> "part 1 of 6"
>> - and also a complete (folded) patch for reference
>> - strictly versioning the patch among review rounds
>> - when submitting a new version of a patch after r- always explain what
>> has changed and provide an interdiff
>
> If reviewee submits a new version of (say) patch 1 of 6, should they:
>
> * attach patch 1 version 2

yes, with title "Patch 1 of 6, whatever description, v2"

> * an interdiff between patch 1 version 1 and 2

depends on complexity (I always ask), but idiff of a new full patch
might be enough

> * and a new complete/folded patch (of patches 1-6)?

for sure, but also depends on if you ask more then one reviewer. I
consider it a bit impolite to obsolete a patch that somebody is just
reviewing. communication!

> Which file would be r+'d for review?

all parts all the complete one, depends also on how many reviewers you ask

-hb-
>
>
> chris

[Randell Jesup]

>On 10/07/13 23:14, Taras Glek wrote:
>> I tried to capture feedback from this thread in
>> https://wiki.mozilla.org/Code_Review
>
>I just did a pass over that page to highlight the key points.

I added a Tips and Tricks section, with some tips on practices to make
interdiff creation easier.

--
Randell Jesup, Mozilla Corp
remove ".news" for personal email