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

[Jordan Rupprecht via llvm-dev]

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 <joker.eph at gmail.com> 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).


>
> --
> Mehdi
>
> _______________________________________________
> 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/20200721/3bdc64ad/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3856 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200721/3bdc64ad/attachment.bin>