[llvm-dev] Phabricator sending spurious "This revision was not accepted when it landed" emails

[David Blaikie via llvm-dev]

On Tue, Jul 21, 2020 at 1:04 PM Jordan Rupprecht <rupprecht at google.com> wrote:
>
>
>
> On Tue, Jul 21, 2020 at 11:29 AM Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org> wrote:
>>
>>
>>
>> On Tue, Jul 21, 2020 at 11:07 AM David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>> +Mehdi AMINI who's taking some (shared?) ownership of Phabricator these days.
>>>
>>> Mehdi - was Phab updated recently (such that we might've picked up new semantics)?
>>
>>
>> No: I upgraded the hardware and the OS, but not Phab itself yet.
>>
>> 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.
>>
>>>
>>>
>>> On Tue, Jul 21, 2020 at 4:25 AM Jay Foad via llvm-dev <llvm-dev at lists.llvm.org> wrote:
>>>>
>>>> Has anyone else noticed Phabricator sending emails saying:
>>>>   This revision was not accepted when it landed; it landed in state
>>>> "Needs Review".
>>>> when the review clearly has been accepted by someone?
>>>>
>>>> Some recent examples:
>>>> https://reviews.llvm.org/D83952
>>
>>
>> Seems like this one closed as expected without the message? http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808734.html
>>
>>
>>>>
>>>>
>>>> https://reviews.llvm.org/D80116
>>
>>
>> Same here: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808778.html
>>
>> Can you forward me the email you received for these revisions?
>>
>>
>>>
>>>
>>> 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".
>>>
>>> Do you have any examples that didn't have post-approval-pre-commit changes that still got this annotation about being committed without review?
>>>
>>>> https://reviews.llvm.org/D81267
>>>
>>>
>>> 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
>>
>>
>> Indeed this one shows the message: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200713/807554.html
>
> 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.
>
> 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).

I could go either way there - sometimes if multiple people have
provided feedback/requested changes, we do try to wait to check that
those who requested them sign off that they've been addressed,
otherwise some reviewers can (unintentionally or otherwise) undercut
others.

- Dave

>>
>>
>> --
>> Mehdi
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev