<div dir="ltr">Based purely on UI clues etc, I think the intent in Phabricator is that a "Request for Changes" can only be overridden by the person who requested them marking the revision as accepted (note how an updated review following such a request has the red cross still coloured, unlike the ticks for an acceptance, implying somehow that it's still important).<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 21 Jul 2020 at 21:09, David Blaikie via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">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">On Tue, Jul 21, 2020 at 1:04 PM Jordan Rupprecht <<a href="mailto:rupprecht@google.com" target="_blank">rupprecht@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Jul 21, 2020 at 11:29 AM Mehdi AMINI via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>><br>
>><br>
>><br>
>> On Tue, Jul 21, 2020 at 11:07 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>> +Mehdi AMINI who's taking some (shared?) ownership of Phabricator these days.<br>
>>><br>
>>> Mehdi - was Phab updated recently (such that we might've picked up new semantics)?<br>
>><br>
>><br>
>> No: I upgraded the hardware and the OS, but not Phab itself yet.<br>
>><br>
>> I have a test instance running with an upgraded Phab though, it may have been sending duplicate emails in the last day or two when I didn't notice I had the email daemon running.<br>
>><br>
>>><br>
>>><br>
>>> On Tue, Jul 21, 2020 at 4:25 AM Jay Foad via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>>>><br>
>>>> Has anyone else noticed Phabricator sending emails saying:<br>
>>>>   This revision was not accepted when it landed; it landed in state<br>
>>>> "Needs Review".<br>
>>>> when the review clearly has been accepted by someone?<br>
>>>><br>
>>>> Some recent examples:<br>
>>>> <a href="https://reviews.llvm.org/D83952" rel="noreferrer" target="_blank">https://reviews.llvm.org/D83952</a><br>
>><br>
>><br>
>> Seems like this one closed as expected without the message? <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808734.html" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808734.html</a><br>
>><br>
>><br>
>>>><br>
>>>><br>
>>>> <a href="https://reviews.llvm.org/D80116" rel="noreferrer" target="_blank">https://reviews.llvm.org/D80116</a><br>
>><br>
>><br>
>> Same here: <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808778.html" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808778.html</a><br>
>><br>
>> Can you forward me the email you received for these revisions?<br>
>><br>
>><br>
>>><br>
>>><br>
>>> Hard for me to tell what happened here. I wonder if it's related to making changes after review/before committing. While that's common in LLVM, I could imagine a review tool (especially if we picked up a newer version - as I don't think it's always had this behavior) might get fussy about that - perhaps it'd be configurable, so it'd say "this was committed with extra changes" but not "This was committed without review".<br>
>>><br>
>>> Do you have any examples that didn't have post-approval-pre-commit changes that still got this annotation about being committed without review?<br>
>>><br>
>>>> <a href="https://reviews.llvm.org/D81267" rel="noreferrer" target="_blank">https://reviews.llvm.org/D81267</a><br>
>>><br>
>>><br>
>>> Last one seems more clear - one of the reviewers (rupprecht) still had the review marked "requires changes", so it was committed without closure on that<br>
>><br>
>><br>
>> Indeed this one shows the message: <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200713/807554.html" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200713/807554.html</a><br>
><br>
> dmgreen accepted it after I requested changes. Shouldn't that override my earlier "requires changes" request? It seems like a bad SPOF of failure to require *my* LGTM.<br>
><br>
> FWIW, the reland of the patch is good with me because it includes a variant of the crash repro I provided. I just didn't LGTM it -- I'm not familiar with the patch at all beyond that it caused a crash -- which is why I assumed someone else would be able to approve and take it out of the "requires changes" state (e.g. make sure it fixes the crash in the right way).<br>
<br>
I could go either way there - sometimes if multiple people have<br>
provided feedback/requested changes, we do try to wait to check that<br>
those who requested them sign off that they've been addressed,<br>
otherwise some reviewers can (unintentionally or otherwise) undercut<br>
others.<br>
<br>
- Dave<br>
<br>
>><br>
>><br>
>> --<br>
>> Mehdi<br>
>><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>