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

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
https://reviews.llvm.org/D80116
https://reviews.llvm.org/D81267

Thanks,
Jay.

Yes, I've seen them occasionally, although not always as far as I've
noticed.

James

On Tue, 21 Jul 2020 at 12:25, 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
> https://reviews.llvm.org/D80116
> https://reviews.llvm.org/D81267
>
> Thanks,
> Jay.
> _______________________________________________
> 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/52b99c31/attachment.html>

I believe that can happen when someone says LGTM in a reply, but doesn’t do the
drop-down to Accept Revision.
The flip side is if you do the drop-down and don’t put in a reply, there’s no
notice to the mailing list.
--paulr

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of James
Henderson via llvm-dev
Sent: Tuesday, July 21, 2020 7:46 AM
To: Jay Foad <jay.foad at gmail.com>
Cc: LLVM Developers Mailing List <llvm-dev at lists.llvm.org>
Subject: Re: [llvm-dev] Phabricator sending spurious "This revision was not
accepted when it landed" emails

Yes, I've seen them occasionally, although not always as far as I've
noticed.

James

On Tue, 21 Jul 2020 at 12:25, Jay Foad via llvm-dev <llvm-dev at
lists.llvm.org<mailto: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
https://reviews.llvm.org/D80116
https://reviews.llvm.org/D81267

Thanks,
Jay.
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto: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/d93d26fe/attachment.html>

+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)?

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
> https://reviews.llvm.org/D80116

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.

>
>
> Thanks,
> Jay.
> _______________________________________________
> 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/f5208356/attachment-0001.html>

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

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200721/4e13aeea/attachment.html>