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>
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4