llvm dev - Apr 2021 - What to do when a developer ignores post-commit review comments

Hi all,

TL;DR - what should you do if a patch author fails to respond to
post-commit review comments? I'm guessing the answer is revert after
sufficient prompting, but how much is sufficient prompting?

Background:

I was not too long ago involved in reviewing a patch on Phabricator (
https://reviews.llvm.org/D95638). I made several comments, and some of
these were addressed. Unfortunately, I didn't have the time to come back to
the review at that time, and one or two others made a couple of review
comments themselves. Eventually, after a couple of pings by the patch
author, one of the people on the review approved the patch, and it was
committed. About a week later, one of my colleagues noticed a bug in the
patch, which prompted me to take another look at it. At this point, I
noticed that the patch had landed without several of my comments being
addressed; the patch in particular had (in my opinion) insufficient
testing. I posted these comments on the Phabricator review page for the
patch, asking for them to be addressed. After taking time off for a couple
of weeks, I noticed that the patch author hadn't responded to my comments,
so I prodded them again. A week later and there's still been no response.

What should I do at this point? I can of course revert the patch (and any
subsequent patch that relied on it). Before taking that action though, I
wanted to make sure this was the appropriate approach.

Thanks in advance for advice.

James
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210426/2360d2f7/attachment.html>

+Konstantin (patch author) and Matt (patch approver) for visibility.

Sometimes folks miss the signal in the noise of the -commits lists,
unfortunately - so probably an email here on the list (at least if the
breakage isn't totally obvious/failing buildbots/etc and noticed
relatively quickly (the longer a patch has been in, the higher the bar
to revert it, generally)), such as this one is probably a good start.
I'd give this thread a day or two to seek if the original author and
reviewer chime in.

Has the bug in the patch been fixed? Are there outstanding functional
issues with the patch as-is?

If the known bug is fixed and there aren't known functional issues
outstanding or fundamental design issues (see, for instance, the
CfgTraits ("RFC: CfgTraits/CfgInterface design discussion") episode
last year), it may not meet the bar for a revert at this point -
though some fairly firm requests for better test coverage could be
warranted. If the author continues not to respond to even this email,
I could see this maybe escalating to a revert - though that's super
rare and I don't think there's a general policy guideline that you
should take away from this thread for that situation - basically in
that situation, having this sort of thread is probably necessary to
gut-check/seek some agreement that it's enough of a problem to revert.

- Dave

On Mon, Apr 26, 2021 at 5:58 AM James Henderson via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Hi all,
>
> TL;DR - what should you do if a patch author fails to respond to
post-commit review comments? I'm guessing the answer is revert after
sufficient prompting, but how much is sufficient prompting?
>
> Background:
>
> I was not too long ago involved in reviewing a patch on Phabricator
(https://reviews.llvm.org/D95638). I made several comments, and some of these
were addressed. Unfortunately, I didn't have the time to come back to the
review at that time, and one or two others made a couple of review comments
themselves. Eventually, after a couple of pings by the patch author, one of the
people on the review approved the patch, and it was committed. About a week
later, one of my colleagues noticed a bug in the patch, which prompted me to
take another look at it. At this point, I noticed that the patch had landed
without several of my comments being addressed; the patch in particular had (in
my opinion) insufficient testing. I posted these comments on the Phabricator
review page for the patch, asking for them to be addressed. After taking time
off for a couple of weeks, I noticed that the patch author hadn't responded
to my comments, so I prodded them again. A week later and there's still been
no response.
>
> What should I do at this point? I can of course revert the patch (and any
subsequent patch that relied on it). Before taking that action though, I wanted
to make sure this was the appropriate approach.
>
> Thanks in advance for advice.
>
> James
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Hi,
If i can drop my five cents, max assume good will. If it taints release in
deadly way keep it in purgatory. If its not fatal, let it get released but
announce bounty for fixing it. The commms might be failure either
phabricator or email process might fail as its unclear which to use etc as
it was revealed recently.

Id say make sure your change request hits author of patch. If theres doubt
or difference of opinions discuss etc but id guess, opensource is based on
kindness and volunteering etc so id work on it in aforementioned way above.

Hope that helps.

Best regards and best wishes for llvm,
Pawel kunio

pon., 26.04.2021, 14:58 użytkownik James Henderson via llvm-dev <
llvm-dev at lists.llvm.org> napisał:
> Hi all,
>
> TL;DR - what should you do if a patch author fails to respond to
> post-commit review comments? I'm guessing the answer is revert after
> sufficient prompting, but how much is sufficient prompting?
>
> Background:
>
> I was not too long ago involved in reviewing a patch on Phabricator (
> https://reviews.llvm.org/D95638). I made several comments, and some of
> these were addressed. Unfortunately, I didn't have the time to come
back to
> the review at that time, and one or two others made a couple of review
> comments themselves. Eventually, after a couple of pings by the patch
> author, one of the people on the review approved the patch, and it was
> committed. About a week later, one of my colleagues noticed a bug in the
> patch, which prompted me to take another look at it. At this point, I
> noticed that the patch had landed without several of my comments being
> addressed; the patch in particular had (in my opinion) insufficient
> testing. I posted these comments on the Phabricator review page for the
> patch, asking for them to be addressed. After taking time off for a couple
> of weeks, I noticed that the patch author hadn't responded to my
comments,
> so I prodded them again. A week later and there's still been no
response.
>
> What should I do at this point? I can of course revert the patch (and any
> subsequent patch that relied on it). Before taking that action though, I
> wanted to make sure this was the appropriate approach.
>
> Thanks in advance for advice.
>
> James
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210427/9bbf160d/attachment.html>