[webkitpy] CQ- should mean not valid patch for commit queue
Created attachment 239406 [details] Patch I found this bug during working on bug137460. Now the commit queue doesn't refuse the patch marked with cq- during processing, but not obsoleted or closed the bug. cq- should mean not valid too to make cq refuse the patch during the the second validation phase just before landing.
Comment on attachment 239406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239406&action=review > Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py:54 > + if self._patch.commit_queue() == "-": > + return False What code checks that flag hasn't been changed back to "cq?"? Perhaps there is a separate check somewhere that cq+ is still set?
Comment on attachment 239406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239406&action=review >> Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py:54 >> + return False > > What code checks that flag hasn't been changed back to "cq?"? Perhaps there is a separate check somewhere that cq+ is still set? Good point, there isn't a separate check for it.
Created attachment 239414 [details] Patch
Yesterday evening, I saw a patch that was endlessly spinning in exactly this situation - it was marked cq+ at first, then back cq?. Commit queue started, then bailed out, then started to go in circles very quickly. This tells me that there is already a check somewhere that was (temporarily) rejecting patches with cq? form commit queue.
I tried to find any such checks, and couldn't. Maybe I'm confused, and commit queue happily lands anything with cq-? And we didn't notice that over all those years? Started an experiment in bug 137492.
So looks like cq- already prevents landing. Looking through the code again, it appears that the check is under here: if not self._patch.committer(): return False committer() is only present when cq+ is set, see _parse_attachment_flag in bugzilla.py: def _parse_attachment_flag(self, element, flag_name, attachment, result_key): flag = element.find('flag', attrs={'name': flag_name}) if flag: attachment[flag_name] = flag['status'] if flag['status'] == '+': attachment[result_key] = flag['setter'] We should probably add a comment explaining that.
Comment on attachment 239414 [details] Patch Oops, cq-ing this per ap's comment. Please resolve the remaining issue with the patch as commented by ap before landing this.
Comment on attachment 239414 [details] Patch I think that the patch is good, except that we need to replace the code change with a comment. And maybe fix the mock implementation to more closely match the actual one, so that it tests the right thing.
(In reply to comment #7) > So looks like cq- already prevents landing. > > Looking through the code again, it appears that the check is under here: > > if not self._patch.committer(): > return False > > committer() is only present when cq+ is set, see _parse_attachment_flag in bugzilla.py: > > def _parse_attachment_flag(self, > element, > flag_name, > attachment, > result_key): > flag = element.find('flag', attrs={'name': flag_name}) > if flag: > attachment[flag_name] = flag['status'] > if flag['status'] == '+': > attachment[result_key] = flag['setter'] > > We should probably add a comment explaining that. Good catch, it is so hidden check. :) I'll add a comment and fix the mock too.
Jake suggested that maybe we can replace the mysterious check with your one. With a cursory look over the code, it seems like it may just work.