[LLVMdev] code-owner sporks

[Sean Silva]

> And now I have another 6 patches at ping^6
> [1/6] Hexagon TC: Update toolchain to add appropriate include paths

:( I'm really sorry. To be honest, looking at those patches, I'm not
entirely surprised though. Consider the following a "meta"-review:

First off, is chandlerc *really* the only person who can review these
changes? It seems unwise to bottleneck a patch on one of our busiest
developers. git-blame, or straight-up ask on IRC who else knows about
that part of the code.

Second, those patches are not optimized for review: they are basically
code dumps. You know your reviewer's time is limited, but the commit
message in your patch is a oneliner? Seriously? Also, there is no
context for *why* those changes are necessary in the body of the
message; phrases like "this patch is a step towards our main goal of
..." really help the reviewer get oriented and speeds the review,
along with letting other list-readers know what's up and maybe get
them pitch in to the review (maybe for just style review, but that
helps). Also, it may help to try to complete the phrase "this patch is
completely obvious except for X", and then specifically ask the
reviewer to look at X so they don't waste their time wading through
obvious stuff to find the stuff that needs to be reviewed. Better yet,
directly commit the obvious changes. Another thing to take advantage
of is that usually when people start reading text, they continue to
read it, so if you have a nice, well-written description of the patch,
and that by the end the reviewer knows what to expect from the review,
what they need to be looking for, etc., then when they finish it is
likely that they will simply open the patch, look for the things you
asked, and give you the LGTM. I could go on, but I think I have
already communicated the main idea: optimize for code review.

Third, my impression is that if you want a patch set to be treated as
a whole, attach the patches to the same message. On the LKML people's
mail setups allows for the [PATCH I/N] to work really well, but my
impression is that the mail setup of most people here doesn't work
well with it.

Fourth, you need to be more adaptive about the whole situation. It
seems like the people pushing those patches basically are running a
mechanical loop:

while (!Patch.isCommitted()) {
  sleep(several days);
  Patch.ping();
}

You aren't a machine! Clearly what you are doing is not working, and
as a human you should realize that and correct it! After 2 or 3 pings,
I would express an understanding of the reviewer's busy-ness and
explicitly ask the reviewer if they can suggest someone else to do the
review, so that the patch can be committed in a timely fashion. If
that doesn't work, then you will probably want to reach out on IRC.
Ultimately, the highest level of escalation I can see is going to be a
thread to llvmdev discussing your situation and the systemic problem
you are running into (not a one-liner "hey, wtf guys we have a bunch
of patches that aren't getting reviewed", but a real, thought-out
discussion of the situation, indicating the forces at play as you see
it, the nature of the patches, their importance, what you have tried,
and possible directions for the community to address the situation)
and asking the community for advice: such a thread will not go
unanswered and will is practically guaranteed to lead to the issues
being swiftly addressed.

Sorry for the lengthy message. Hopefully it will help get that patch
set committed :)

-- Sean Silva

On Fri, Nov 16, 2012 at 12:29 PM, Sebastian Pop <spop at codeaurora.org> wrote:
> Sean Silva wrote:
>> I think the most pings I've seen before an answer is Ping^4
>> ("Fix cmake for Hexagon cross compilers"), and the reviewers were very
>
> And now I have another 6 patches at ping^6
> [1/6] Hexagon TC: Update toolchain to add appropriate include paths
>
> Sebastian
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation