Speeding up reviews
Smaug
while running around Helsinki yesterday I was thinking about
ways to speed up review process. Or thinking about
two experiments.
- Try to review all the new patches really really fast.
As soon as possible, always. Like in a day or two.
I know, it is not always possible but this would be
just an experiment. The idea would be to get experience
whether this kind of mode slows down the
"real work". It very well might do that. Or might not, if
getting reviews becomes faster for all the patches, also the
ones reviewer writes himself.
This mode would probably require that reviewers could easily
move reviews to someone else, if the review queue becomes too
long. Or perhaps bugzilla could prevent asking review from
someone who's queue is too long.
Large patches would be problematic, but perhaps in those cases
either the patches should be split up to smaller pieces or reviewing
could be done in several steps.
- Another idea was to show patches in some kind of pool (sorted by
bugzilla component?) if the patch has been waiting for review
more than x days. (perhaps x==5). This way other people familiar
with the code and who might have more time could review the patches.
-Olli
Justin Dolske
> - Try to review all the new patches really really fast.
> As soon as possible, always. Like in a day or two.
That specific speed seems a little aggressive, but fast turnaround does
seem like a good goal... It reduces the "oh, I'm already behind so
what's another week" tendency, and encourages a goal that reviewees
should expect. I think it also helps if broad-but-incomplete comments
can be given early (affirmative or negative),
But, really, for reviewers who are actually over burdened (or can't
spend more time reviewing for whatever reason), moving reviews to other
people is the only thing likely to really help.
[And of course there will always be exceptions for complex/risky
patches, and areas where there are few qualified reviewers.]
> - Another idea was to show patches in some kind of pool (sorted by
> bugzilla component?) if the patch has been waiting for review
> more than x days. (perhaps x==5). This way other people familiar
> with the code and who might have more time could review the patches.
It's been mentioned before; one risk is that people are likely to feel
less responsible for taking care of pool-owned bugs vs.
personally-assigned things. Still, having Bugzilla at least suggest an
initial reviewer (from a list showing review load) would be a huge step
from where we are today!
Justin
Mike Kristoffersen
> On 4/15/10 12:45 PM, Smaug wrote:
>
>> - Try to review all the new patches really really fast.
>> As soon as possible, always. Like in a day or two.
>
> That specific speed seems a little aggressive, but fast turnaround does
> seem like a good goal... It reduces the "oh, I'm already behind so
> what's another week" tendency, and encourages a goal that reviewees
> should expect. I think it also helps if broad-but-incomplete comments
> can be given early (affirmative or negative),
Would it be an idea, that the reviewer if he can't review it quickly, in
a day or two, should give a guestimate on when he expects the review to
be complete, so the person who requested the review can try to find
another reviewer, try to accelerate the review, or at least be able to
plan when the next merge to trunk should happen :)
>> - Another idea was to show patches in some kind of pool (sorted by
>> bugzilla component?) if the patch has been waiting for review
>> more than x days. (perhaps x==5). This way other people familiar
>> with the code and who might have more time could review the patches.
>
> It's been mentioned before; one risk is that people are likely to feel
> less responsible for taking care of pool-owned bugs vs.
> personally-assigned things. Still, having Bugzilla at least suggest an
> initial reviewer (from a list showing review load) would be a huge step
> from where we are today!
I think you are right in the "someone else will take care of it" fear,
but it might be good to have more visibility on "ignored" review
requests? - maybe a list generated for the manger/module owner (who we
can expect to handle it, by pushing the reviewer or finding a less
overloaded reviewer).
^__^
MikeK
Marco Bonardo
> Hi all,
>
> while running around Helsinki yesterday I was thinking about
> ways to speed up review process.
Apart your suggestions (about that, i like the idea of a dashboard
showing reviews that are taking more time splitted by component), i must
underline "feedback?" flag has also been setup to help reviewers, they
should at least check review requests as soon as possible, or even
immediately, and if their queue is too long, forward a feedback request
to other peers they know could help the requester while he waits for
final review. This way final review can take less time and the requester
is not let alone for weeks without knowing if he should do changes to
the patch. Fixing code style, APIs misuse, unreadable code paths and
things like that before actually getting a review, could help.
-m
Smaug
> It's been mentioned before; one risk is that people are likely to feel
> less responsible for taking care of pool-owned bugs vs.
> personally-assigned things.
My idea was that all the bugs would be still personally-assigned.
The pool would just show patches which haven't been reviewed in
x days.
-Olli
L. David Baron
> - Try to review all the new patches really really fast.
> As soon as possible, always. Like in a day or two.
I agree this is a good idea; roc's been pretty good at it for a
while, and has been encouraging me to do the same. I caught up with
my review queue about a month ago (I think), and I've been pretty
much keeping up since, although a few patches have slipped to 2-3
days (until this past week, when I was on vacation for the whole
week).
> - Another idea was to show patches in some kind of pool (sorted by
> bugzilla component?) if the patch has been waiting for review
> more than x days. (perhaps x==5). This way other people familiar
> with the code and who might have more time could review the patches.
If we wanted to do this sort of thing, we'd probably need to get
better about noting if there's a reason a particular person was
asked for review.
I'm more enthusiastic about the first idea: trying to move towards
a culture where < 24 hour review turnaround is considered the norm,
except for unusually large patches.
-David
--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/
Nicholas Nethercote
>
> I'm more enthusiastic about the first idea: trying to move towards
> a culture where < 24 hour review turnaround is considered the norm,
> except for unusually large patches.
There are some people I know who will usually review within a day, and
some I know who are likely to take a week or more. Guess who I'm more
likely to ask for a review? (No good deed goes unpunished, as they
say.) Because fast reviews are so helpful I try to make my own
reviews fast.
Even the fastest reviewers sometimes have cases that fall through the
cracks. If Bugzilla emailed you if you hadn't responded (with either
a comment or a review result)
within 3 days, say, that might help. Maybe this could be an optional
thing, so that those who aim for fast turnarounds can get some
automated help.
Nick
Mike Kristoffersen
> Even the fastest reviewers sometimes have cases that fall through the
> cracks. If Bugzilla emailed you if you hadn't responded (with either
> a comment or a review result)
> within 3 days, say, that might help. Maybe this could be an optional
> thing, so that those who aim for fast turnarounds can get some
> automated help.
I like the idea, and even my proposal was shut down the last time, i
would like to extend the idea again:)
Would it be an idea to be able to configure Bugzilla, so you could get a
weekly/x-days e-mail reminder, with your pending tasks?
Alternatively could it be the "welcome" page in bugzilla?
Something along the lines of:
Hi,
this is Bugzilla with your weekly status report:
You have 2 greens, 1 yellow and 1 red this week.
---------
(GREEN)
You are active on 5 bugs, and are doing a great job!
bug1111: Bug descrpition 1
bug2222: Bug description 2
bug3333: Bug description 3
bug4444: Bug description 4
bug5555: Bug description 5
---------
(YELLOW)
Could you have a look at the following 2 bugs that you are assigned to,
but haven't touched in more than a month. Please verify that you still
should be assigned to these and that they have enough information in
them. If you are not going to work on them in the near future, please
move them back into the pool of unassigned bugs, or pass them on to
someone else.
bug6666: Bug description 6
bug7777: Bug description 7
---------
(GREEN)
You have been asked to review the following bug since last time - please
review it or estimate when you have time to review it (for the benefit
of the project and the poor soul who worked hard on creating a patch).
bug8888: Bug description 8
---------
(RED)
You have one review request that is more than a month old, please don't
forget about it.
bug9999: Bug description 9
---------
Statistics:
You closed 3 bugs this week (total 103)
You opened 1 bug this week (total 57)
You are assigned to 5 bugs
Your average time for processing a bug is 20 days
Mike Kristoffersen
>
> You could set up whines in order to do that, right?
>
> --
> Ehsan
> <http://ehsanakhgari.org/>
What is whines?
^__^
MikeK
Tanner M. Young
Yes, I would recommend having the reviewers receive a daily list of the
patches they need to review. Someone with whining privileges would need to
set that up, but it's a very simple and immediate solution.
> Alternatively could it be the "welcome" page in bugzilla?
The Bugzilla project has Bug 130835 (
https://bugzilla.mozilla.org/show_bug.cgi?id=130835 ) open for changing the
homepage. I haven't really followed what they are doing in it, but it might
be good to see if we can work proposal like the one from the Mike into what
they are already changing about the homepage. We would also probably need
to wait on a later version of Bugzilla for BMO, which could be some time
away, since the patch hasn't been committed yet.
Tanner
Tanner M. Young
mkristo...@mozilla.com> wrote:
> What is whines?
>
http://www.bugzilla.org/docs/tip/en/html/whining.html
Essentially, it is a tool that emails the results of a Bugzilla search to
users at a specified interval (daily, hourly, weekly, etc.).
Tanner
Ehsan Akhgari
Whines are queries which are run on a specific schedule, and Bugzilla
will mail you the results once they're run. You can add one in this
page:
https://bugzilla.mozilla.org/editwhines.cgi
I'm not sure if you require a permission for whines to be enabled on
your account, though.
--
Ehsan
<http://ehsanakhgari.org/>
Ehsan Akhgari
> On Mon, Apr 19, 2010 at 1:03 PM, Mike Kristoffersen
> <mkristo...@mozilla.com> wrote:
> will mail you the results once they're run. You can add one in this
> page:
>
> https://bugzilla.mozilla.org/editwhines.cgi
>
> I'm not sure if you require a permission for whines to be enabled on
> your account, though.
Hmm, seems like you need the bz_canusewhines permission on bmo in
order to be able to use whines.
--
Ehsan
<http://ehsanakhgari.org/>
Mike Kristoffersen
> On Mon, Apr 19, 2010 at 12:25 PM, Mike Kristoffersen
> <mkristo...@mozilla.com> wrote:
>> Nicholas Nethercote wrote:
>>> Even the fastest reviewers sometimes have cases that fall through the
>>> cracks. If Bugzilla emailed you if you hadn't responded (with either
>>> a comment or a review result)
>>> within 3 days, say, that might help. Maybe this could be an optional
>>> thing, so that those who aim for fast turnarounds can get some
>>> automated help.
>> I like the idea, and even my proposal was shut down the last time, i would
>> like to extend the idea again:)
>>
>> Would it be an idea to be able to configure Bugzilla, so you could get a
>> weekly/x-days e-mail reminder, with your pending tasks?
>
Knowing what whines is, the question is:
Can the search be more advanced than just - bugs where I/X have been
asked to review - like, "give me the bugs, I have been asked to review,
but I haven't commented on for the last week, or since the review was
requested" ?
I doesn't seem obvious to me how to do such a search - and I guess what
we want here is to flag the review requests that are falling through,
not the normal ones?
btw - do we have any indication what the cause is for reviews taking a
long time?
It would be interesting to know why (thou maybe not an easy question to
answer for the people involved) - like: if the main reason is that some
review requests are forgotten, if they are not forgotten but so
uninspiring to do that they are pushed to the last minute, or if they
are moved down on the todo list because there is more important work to do?
I'm sure it could be a bit of all of the above, but the root cause might
tell us how to really solve it - and even I personally would like
reminder mails, it might not solve the root cause in the majority of the
cases.
^__^
MikeK
Ehsan Akhgari
<mkristo...@mozilla.com> wrote:
> Ehsan Akhgari wrote:
>>
>> On Mon, Apr 19, 2010 at 12:25 PM, Mike Kristoffersen
>> <mkristo...@mozilla.com> wrote:
>>>
>>> Nicholas Nethercote wrote:
>>>>
>>>> Even the fastest reviewers sometimes have cases that fall through the
>>>> cracks. If Bugzilla emailed you if you hadn't responded (with either
>>>> a comment or a review result)
>>>> within 3 days, say, that might help. Maybe this could be an optional
>>>> thing, so that those who aim for fast turnarounds can get some
>>>> automated help.
>>>
>>> I like the idea, and even my proposal was shut down the last time, i
>>> would
>>> like to extend the idea again:)
>>>
>>> Would it be an idea to be able to configure Bugzilla, so you could get a
>>> weekly/x-days e-mail reminder, with your pending tasks?
>>
>> You could set up whines in order to do that, right?
>>
>> --
>> Ehsan
>> <http://ehsanakhgari.org/>
>
> Knowing what whines is, the question is:
>
> Can the search be more advanced than just - bugs where I/X have been asked
> to review - like, "give me the bugs, I have been asked to review, but I
> haven't commented on for the last week, or since the review was requested" ?
The queries are just saved searches in your account, which means that
you have the full power of the bugzilla search system at your
fingertips. But that full power is very hard to formulate, and it's
not always possible to express what you want. For example, I'm not
sure if you can specify the query you just described in a way that
bugzilla understands it.
--
Ehsan
<http://ehsanakhgari.org/>
Mike Kristoffersen
My exact point :) so in order to use whines, we would need to extend
the bugzilla-search system or not use whines and create some kind of
search script... hmm... could the two be combined???
^__^
MikeK
Benjamin Smedberg
> Can the search be more advanced than just - bugs where I/X have been
> asked to review - like, "give me the bugs, I have been asked to review,
> but I haven't commented on for the last week, or since the review was
> requested" ?
Yes. Using the charts you can do things like
Chart 1:
Flag is review
Flag requesteee is X
Flag changed after 7d
Chart 2 "not":
Comment changed by X
Comment changed after 7d
--BDS
Mike Kristoffersen
Oh great - thanks :) I'll try to setup some for myself now.
What I fear is that some people will now think that the problem that
started this thread has been solved...
^__^
MikeK
Benjamin Smedberg
> You seem to be one of them... Maybe I should have been more explicit:
One of "them" what?
> The problem is not solved by finding a way to get notified, it's a step
> in the process, but unless the root cause is that reviewers simply
> forget about the review requests, and we get them to sign up to these
> notifications, then the problem is not solved.
>
> I'm not convinced that all reviewers read this thread, nor that the once
> who read it will all sign up, nor that the only cause for slow reviews
> are because the reviewers forget that they have pending reviews.
Yes. The problem is solved by somebody writing a reporting tool which can
send out an email each day/week/whatever. And then getting that tool
deployed on bugzilla.mozilla.org. Can you do that?
--BDS
Mike Kristoffersen
> On 4/19/10 2:30 PM, Mike Kristoffersen wrote:
>
>> What I fear is that some people will now think that the problem that
>> started this thread has been solved...
>
> probably have to either implement it yourself or convince somebody who
> knows bugzilla internals to do it for you. Doing it yourself is probably
> the faster path to success.
>
> --BDS
You seem to be one of them... Maybe I should have been more explicit:
The problem is not solved by finding a way to get notified, it's a step
in the process, but unless the root cause is that reviewers simply
forget about the review requests, and we get them to sign up to these
notifications, then the problem is not solved.
I'm not convinced that all reviewers read this thread, nor that the once
who read it will all sign up, nor that the only cause for slow reviews
are because the reviewers forget that they have pending reviews.
^__^
MikeK
Benjamin Smedberg
> What I fear is that some people will now think that the problem that
> started this thread has been solved...
Which people? If you want a nice daily or weekly email report, you'll
Mike Kristoffersen
> On 4/19/10 3:22 PM, Mike Kristoffersen wrote:
>
>> You seem to be one of them... Maybe I should have been more explicit:
>
> One of "them" what?
I was assuming that since you asked who might think the problem was
solved, you yourself saw the problem as solved, which, now that I read
it again, absolutely makes no sense, sorry :)
> Yes. The problem is solved by somebody writing a reporting tool which
> can send out an email each day/week/whatever. And then getting that tool
> deployed on bugzilla.mozilla.org. Can you do that?
>
Personally I would have no idea where to start :)
If we agree that we need such a tool/extension to Bugzilla, then I guess
the right way would be to open a bug on it, and let the normal process
handle the execution and prioritisation of it? which is one thing
within the reach of my abilities ;)
^__^
MikeK
Benjamin Smedberg
> Personally I would have no idea where to start :)
>
> If we agree that we need such a tool/extension to Bugzilla, then I guess
> the right way would be to open a bug on it, and let the normal process
> handle the execution and prioritisation of it? which is one thing within
> the reach of my abilities ;)
You can do that, and it will be a step in the right direction. I suspect the
next step will be "nobody who knows how to do it has time to even consider
it". Which is why people such as yourself who have good suggestions might
want to spend a day or two learning the query/reporting APIs to see if you
can take on the task.
--BDS
Mike Kristoffersen
Which I assume means that there is more important stuff that has higher
priority that needs to be done first?
Anyway I have opened https://bugzilla.mozilla.org/show_bug.cgi?id=560329
with the suggestion.
^__^
MikeK
David Mandelin
> Hi all,
>
> while running around Helsinki yesterday I was thinking about
> ways to speed up review process. Or thinking about
> two experiments.
>
> - Try to review all the new patches really really fast.
> As soon as possible, always. Like in a day or two.
I try to do this too, on the assumption that the review will block
someone. Now that I think about it, when I request reviews, sometimes it
blocks me, so I want a fast review, but other times it doesn't, and I
don't mind waiting. I'm not sure we would really want to a add a review
priority flag, but it's a thought.
I definitely support the idea that reviewers should within a day or so
do one of: review the patch, hand off the review, or leave a note with
delay reason and ETA.
Robert O'Callahan
> - Try to review all the new patches really really fast.
> As soon as possible, always. Like in a day or two.
I try to do this and mostly succeed.
Now, possibly it's easier for me because I put less effort into my
reviews than other people. On the other hand, maybe it's better to give
a quick, slightly-less-good review than a very thorough review very
late. Well, that's my excuse :-).
No matter how much effort you put in, I don't think quick reviews can be
a lose in general. Delaying reviews creates opportunities for people to
forget things, for patches to rot, and for people to find and report
duplicate bugs.
Obviously there can be overriding tactical considerations like
super-high-priority emergencies, but those are exceptional.
Rob
Tanner M. Young
> Obviously there can be overriding tactical considerations like
> super-high-priority emergencies, but those are exceptional.
>
And in cases like that, one should pass the review to someone else or note
that there will be a delay with an ETA depending on the problem.
I see all of this as a very good plan: reviews happen when the patch author
and reviewer communicate. All of this facilitates that, particularly for
simpler patches.
I am concerned about very large code changes with 1000+ line patches which
require several reviews, super-reviews, and ui-reviews: these tend to be a
real mess. I know of one almost 4000 line patch in particular that has gone
through at least 7 different reviewers and super-reviewers because of its
complexity, and there still isn't a clear indication of whether the patch
should be pursued or not. I have postponed work waiting for this patch
since its bug was filed last year, but it still hasn't been properly
reviewed or fixed. Is there any policy we could implement that would
specifically help these larger patches that can sit for weeks or months
without valuable feedback or direction?
Perhaps the solution with the above problem is to define a clear plan for
resolving the problem in the wiki or the bug--with the help of the future
reviewers/super-reviewers--and then implement it in small pieces of < 100
lines of code that can be reviewed in reasonable bite-sized patches.
Tanner
timeless
> I am concerned about very large code changes with 1000+ line patches which
> require several reviews, super-reviews, and ui-reviews: these tend to be a
> real mess. I know of one almost 4000 line patch in particular that has gone
> through at least 7 different reviewers and super-reviewers because of its
> complexity, and there still isn't a clear indication of whether the patch
> should be pursued or not. I have postponed work waiting for this patch
> since its bug was filed last year, but it still hasn't been properly
> reviewed or fixed. Is there any policy we could implement that would
> specifically help these larger patches that can sit for weeks or months
> without valuable feedback or direction?
>
> Perhaps the solution with the above problem is to define a clear plan for
> resolving the problem in the wiki or the bug--with the help of the future
> reviewers/super-reviewers--and then implement it in small pieces of < 100
> lines of code that can be reviewed in reasonable bite-sized patches.
personally, I recently resurrected a fairly large patch. it's actually
reviewed now, but at this point, my current excitement is diving tree
state to try to push stuff.
That said, my outbound request list is roughly 11 printed pages long.
Gervase Markham
> The queries are just saved searches in your account, which means that
> you have the full power of the bugzilla search system at your
> fingertips. But that full power is very hard to formulate, and it's
> not always possible to express what you want. For example, I'm not
> sure if you can specify the query you just described in a way that
> bugzilla understands it.
Well, you could always do a BzAPI search which is guaranteed to return a
superset of the bugs you want, and then filter it using whatever
additional criteria you like to get the set you are interested in, turn
that into a buglist.cgi URL with the bug numbers explicitly specified,
and email it to yourself.
But granted, that's harder work. :-)
Gerv
Gervase Markham
> while running around Helsinki yesterday I was thinking about
> ways to speed up review process. Or thinking about
> two experiments.
See also:
https://wiki.mozilla.org/GovernanceIssues#Triage_Stale_Reviews
Gerv
Mike Shaver
> I am concerned about very large code changes with 1000+ line patches which
> require several reviews, super-reviews, and ui-reviews: these tend to be a
> real mess.
Yes, and we should be trying to avoid such patches. Decomposing into
stages (mq helps a lot here) and doing piecemeal landings is
worthwhile. I don't know very many people who can actually review a
non-mechanical 1000-line patch across modules with high confidence
anyway.
> Perhaps the solution with the above problem is to define a clear plan for
> resolving the problem in the wiki or the bug--with the help of the future
> reviewers/super-reviewers--and then implement it in small pieces of < 100
> lines of code that can be reviewed in reasonable bite-sized patches.
Yes. I would recommend that the author of the 4K-line patch identify
some partitions in the patch, file sub-bugs, and raise the issue at
the development call to have a high-bandwidth conversation about how
to proceed.
Mike
L. David Baron
> I am concerned about very large code changes with 1000+ line patches which
> require several reviews, super-reviews, and ui-reviews: these tend to be a
> through at least 7 different reviewers and super-reviewers because of its
> complexity, and there still isn't a clear indication of whether the patch
To look at this from the other side: I think generating an
*unexpected* review request for a multi-thousand line patch is a bad
idea. Authors of large patches should probably be discussing them,
at least with the expected reviewers if not more widely, before
writing a multi-thousand line patch.
I find it much easier to review a patch where I've discussed the
basic ideas (the plan) behind the patch with the author beforehand;
I generally find it much easier to review at the conceptual level
than the code level. When I get a big chunk of code with no
conceptual description, I essentially have to generate that
conceptual description for myself. So even if I didn't see it
before the author wrote it, it's still useful if they write in in
prose in the bug when attaching the patch.
-David
--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/
Mike Kristoffersen
> To look at this from the other side: I think generating an
> *unexpected* review request for a multi-thousand line patch is a bad
> idea. Authors of large patches should probably be discussing them,
> at least with the expected reviewers if not more widely, before
> writing a multi-thousand line patch.
Yeah, it's sometimes next to impossible to implement every new feature
in the tiny steps where a code review is actually meaningful (as it
would take days or weeks to do a proper review of a multi-thousand line
patch, which is close to inhumane asking anyone to do).
> I find it much easier to review a patch where I've discussed the
> basic ideas (the plan) behind the patch with the author beforehand;
> I generally find it much easier to review at the conceptual level
> than the code level. When I get a big chunk of code with no
> conceptual description, I essentially have to generate that
> conceptual description for myself. So even if I didn't see it
> before the author wrote it, it's still useful if they write in in
> prose in the bug when attaching the patch.
By conceptual description, do you mean (high level) design documentation?
^__^
MikeK
Boris Zbarsky
> Yeah, it's sometimes next to impossible to implement every new feature
> in the tiny steps where a code review is actually meaningful (as it
> would take days or weeks to do a proper review of a multi-thousand line
> patch, which is close to inhumane asking anyone to do).
And yet this sort of patch is exactly where bugs are likely to creep in
due to the difficulty of catching them via self-review...
The right way to do this is to break up if possible, else to very
clearly describe what the patch is trying to do so the reviewer can
compare the code they see to that description.
-Boris
L. David Baron
> L. David Baron wrote:
> >I find it much easier to review a patch where I've discussed the
> >basic ideas (the plan) behind the patch with the author beforehand;
> >I generally find it much easier to review at the conceptual level
> >than the code level. When I get a big chunk of code with no
> >conceptual description, I essentially have to generate that
> >conceptual description for myself. So even if I didn't see it
> >before the author wrote it, it's still useful if they write in in
> >prose in the bug when attaching the patch.
>
> By conceptual description, do you mean (high level) design documentation?
I don't think design documentation is really the right word. What I
want is the following:
Assume you were going to write a patch to fix/implement X, but
wanted to check with the module owner first that the approach you're
planning to take would be accepted. So you'd say / write something
like:
Would you expect to accept a patch fixing X in which I changed ... ?
What I want is the "..." part. It might be a sentence; it might be
a few decent-length paragraphs.
Blake Kaplan
> Yes, and we should be trying to avoid such patches. Decomposing into
> stages (mq helps a lot here) and doing piecemeal landings is
> worthwhile. I don't know very many people who can actually review a
> non-mechanical 1000-line patch across modules with high confidence
> anyway.
For what it's worth, Ben Newman, over in bug 559962 asked for review on a 54kb
patch. Fortunately, he had broken the patch up into easily reviewable mq
patches, each of which I could review in a matter of minutes with a high
degree of confidence. The overall patch in the bug would have taken me
significantly longer to review and I would have trusted my own review a lot
less.
If more people did this, I believe the world would be a much better place.
--
Blake Kaplan
Nicholas Nethercote
>
> For what it's worth, Ben Newman, over in bug 559962 asked for review on a 54kb
> patch. Fortunately, he had broken the patch up into easily reviewable mq
> patches, each of which I could review in a matter of minutes with a high
> degree of confidence. The overall patch in the bug would have taken me
> significantly longer to review and I would have trusted my own review a lot
> less.
>
> If more people did this, I believe the world would be a much better place.
+1
This particularly helps separate refactoring changes from "real"
changes, IYSWIM.
Nick
Gervase Markham
> To look at this from the other side: I think generating an
> *unexpected* review request for a multi-thousand line patch is a bad
> idea. Authors of large patches should probably be discussing them,
> at least with the expected reviewers if not more widely, before
> writing a multi-thousand line patch.
Note that also we have a new "feedback" flag for attachments in
Bugzilla, which patch authors can use to get early input from their
reviewers (which may include things like "write a high-level description
of what this does" or "split this up into smaller pieces").
Gerv