<div dir="ltr"><div>Thanks for the comments. If nothing else, they'll prove useful if and when this happens in the future.</div><div><br></div><div>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.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 26 Apr 2021 at 17:45, Renato Golin <<a href="mailto:rengolin@gmail.com">rengolin@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>I agree with David.</div><div><br></div>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.<div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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)?</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>cheers,</div><div>--renato</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 26 Apr 2021 at 16:52, David Blaikie via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+Konstantin (patch author) and Matt (patch approver) for visibility.<br>
<br>
Sometimes folks miss the signal in the noise of the -commits lists,<br>
unfortunately - so probably an email here on the list (at least if the<br>
breakage isn't totally obvious/failing buildbots/etc and noticed<br>
relatively quickly (the longer a patch has been in, the higher the bar<br>
to revert it, generally)), such as this one is probably a good start.<br>
I'd give this thread a day or two to seek if the original author and<br>
reviewer chime in.<br>
<br>
Has the bug in the patch been fixed? Are there outstanding functional<br>
issues with the patch as-is?<br>
<br>
If the known bug is fixed and there aren't known functional issues<br>
outstanding or fundamental design issues (see, for instance, the<br>
CfgTraits ("RFC: CfgTraits/CfgInterface design discussion") episode<br>
last year), it may not meet the bar for a revert at this point -<br>
though some fairly firm requests for better test coverage could be<br>
warranted. If the author continues not to respond to even this email,<br>
I could see this maybe escalating to a revert - though that's super<br>
rare and I don't think there's a general policy guideline that you<br>
should take away from this thread for that situation - basically in<br>
that situation, having this sort of thread is probably necessary to<br>
gut-check/seek some agreement that it's enough of a problem to revert.<br>
<br>
- Dave<br>
<br>
On Mon, Apr 26, 2021 at 5:58 AM James Henderson via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
><br>
> Hi all,<br>
><br>
> 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?<br>
><br>
> Background:<br>
><br>
> I was not too long ago involved in reviewing a patch on Phabricator (<a href="https://reviews.llvm.org/D95638" rel="noreferrer" target="_blank">https://reviews.llvm.org/D95638</a>). 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.<br>
><br>
> 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.<br>
><br>
> Thanks in advance for advice.<br>
><br>
> James<br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>
</blockquote></div>