<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 21, 2020 at 11:29 AM Mehdi AMINI via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 21, 2020 at 11:07 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="cremed">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><a class="gmail_plusreply cremed" id="gmail-m_2950305667630111462gmail-m_-5989698225186102693plusReplyChip-0" href="mailto:joker.eph@gmail.com" target="_blank">+Mehdi AMINI</a> who's taking some (shared?) ownership of Phabricator these days.<br><br>Mehdi - was Phab updated recently (such that we might've picked up new semantics)?</div></div></blockquote><div><br></div><div>No: I upgraded the hardware and the OS, but not Phab itself yet.</div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 21, 2020 at 4:25 AM Jay Foad via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="cremed">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Has anyone else noticed Phabricator sending emails saying:<br>
  This revision was not accepted when it landed; it landed in state<br>
"Needs Review".<br>
when the review clearly has been accepted by someone?<br>
<br>
Some recent examples:<br>
<a href="https://reviews.llvm.org/D83952" rel="noreferrer" target="_blank" class="cremed">https://reviews.llvm.org/D83952</a></blockquote></div></div></blockquote><div><br></div><div>Seems like this one closed as expected without the message? <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808734.html" target="_blank" class="cremed">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808734.html</a><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<a href="https://reviews.llvm.org/D80116" rel="noreferrer" target="_blank" class="cremed">https://reviews.llvm.org/D80116</a></blockquote></div></div></blockquote><div><br></div><div>Same here: <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808778.html" target="_blank" class="cremed">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200720/808778.html</a></div><div><br></div><div>Can you forward me the email you received for these revisions?</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>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".<br><br>Do you have any examples that didn't have post-approval-pre-commit changes that still got this annotation about being committed without review?<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<a href="https://reviews.llvm.org/D81267" rel="noreferrer" target="_blank" class="cremed">https://reviews.llvm.org/D81267</a></blockquote><div><br></div><div>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</div></div></div></blockquote><div><br></div><div>Indeed this one shows the message: <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200713/807554.html" target="_blank" class="cremed">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200713/807554.html</a></div></div></div></div></div></div></blockquote><div>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.</div><div><br></div><div>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).</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div></div><div><br></div><div>-- </div><div>Mehdi</div><div> </div></div></div></div></div></div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="cremed">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank" class="cremed">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>