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

[James Henderson via llvm-dev]

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).

On Tue, 21 Jul 2020 at 21:09, David Blaikie via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> 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
> _______________________________________________
> 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/20200722/113127d0/attachment.html>