Mehdi AMINI via llvm-dev
2020-Jul-21 18:29 UTC
[llvm-dev] Phabricator sending spurious "This revision was not accepted when it landed" emails
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>
David Blaikie via llvm-dev
2020-Jul-21 18:41 UTC
[llvm-dev] Phabricator sending spurious "This revision was not accepted when it landed" emails
On Tue, Jul 21, 2020 at 11:29 AM Mehdi AMINI <joker.eph at gmail.com> 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 >In my inbox I have two emails for that review. (though, also, on the commits list, I do see this: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808735.html - though my mail client (gmail) is rendering this one as in the same thread/doesn't seem to show the "[Differential]" prefix in the subject - not sure what's going on there, usually gmail is /too/ ready to group mails by actual subject text, rather than by thread ids in the email hedaers... )> 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? >Similarly, the separate "[Differential]" email seems to be unthreaded and contains the problematic "not accepted when it landed" text: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808779.html> > > >> >> 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/3e065caf/attachment.html>
Jordan Rupprecht via llvm-dev
2020-Jul-21 20:03 UTC
[llvm-dev] Phabricator sending spurious "This revision was not accepted when it landed" emails
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>
David Blaikie via llvm-dev
2020-Jul-21 20:09 UTC
[llvm-dev] Phabricator sending spurious "This revision was not accepted when it landed" emails
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
Mehdi AMINI via llvm-dev
2020-Jul-21 22:41 UTC
[llvm-dev] Phabricator sending spurious "This revision was not accepted when it landed" emails
On Tue, Jul 21, 2020 at 11:42 AM David Blaikie <dblaikie at gmail.com> wrote:> > > On Tue, Jul 21, 2020 at 11:29 AM Mehdi AMINI <joker.eph at gmail.com> 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 >> > > In my inbox I have two emails for that review. > (though, also, on the commits list, I do see this: > http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808735.html - > though my mail client (gmail) is rendering this one as in the same > thread/doesn't seem to show the "[Differential]" prefix in the subject - > not sure what's going on there, usually gmail is /too/ ready to group mails > by actual subject text, rather than by thread ids in the email hedaers... ) >Thanks! I dug a bit and found out that these came from my test instance, I ran an upgraded Phabricator instance and didn't disable polling the repository: so it closed the revision without having any approval. -- Mehdi> > >> 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? >> > > Similarly, the separate "[Differential]" email seems to be unthreaded and > contains the problematic "not accepted when it landed" text: > http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808779.html > > >> >> >> >>> >>> 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/c742c232/attachment.html>