[llvm-dev] What to do when a developer ignores post-commit review comments

Tue Apr 27 00:47:10 PDT 2021

Thanks for the comments. If nothing else, they'll prove useful if and when
this happens in the future.

As it turns out, it was a month-long absence in this case, and as Renato
mentioned it is now being sorted out now that Konstantin is back.

On Mon, 26 Apr 2021 at 17:45, Renato Golin <rengolin at gmail.com> wrote:

> I agree with David.
>
> My *personal* take on this is that we shouldn't try to encode this
> situation into policy because there are a number of ways in which it can
> happen, and they're sometimes very different from each other. In this
> particular instance it looks like classic miscommunication on multiple
> levels, which is perfectly natural and shouldn't need additional guidelines.
>
> After a number of reviews, it's hard to know which questions you have
> answered and which you haven't. It can also be hard to know what is a
> request for change, a comment, a question or just a reminder. People
> usually read my replies with a lot more emphasis than I intended because of
> how I write, and I can take comments a lot more lightly than the original
> authors intended by the reverse process.
>
> The lack of response, while long (one month), can be due to holidays or
> any number of personal reasons, all of which we strive to not force
> response times upon people. The approval was also common, one of the
> reviewers interpreted that all questions were answered and approved after a
> number of pings.
>
> We can still learn a few things from this, though. For example, in the
> absence of the original author, some co-worker could have been reached? Or
> Matt (who approved the patch) could work with you to understand the problem
> and try to fix without reverting (after such a long time)?
>
> Another thing that I've learnt over the years is to not rely on Phab to
> close the gap between post-commit review and action. Not many people look
> at replies after the patch has been committed, and it's common for people
> to disconnect notifications or leave the email altogether if they're
> one-off contributors. I wouldn't have waited a month for the author to
> reply, I'd have gone on IRC, mailing list, personal emails etc until the
> matter was resolved. I'd be personally very sad if a patch I committed
> broken someone else's build for a month and I didn't know.
>
> In the end, there seems to be a new patch to resolve the problem
> (D101304), so that's good news. Hopefully that'll have enough to fix all
> the problems without having to revert anything.
>
> cheers,
> --renato
>
> On Mon, 26 Apr 2021 at 16:52, David Blaikie via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> +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
>> _______________________________________________
>> 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/750df064/attachment-0001.html>