Bug 137483 - [webkitpy] CQ- should mean not valid patch for commit queue
Summary: [webkitpy] CQ- should mean not valid patch for commit queue
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-07 04:27 PDT by Csaba Osztrogonác
Modified: 2017-06-20 02:16 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.07 KB, patch)
2014-10-07 04:33 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (2.47 KB, patch)
2014-10-07 10:03 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2014-10-07 04:27:16 PDT
[webkitpy] CQ- should mean not valid patch for commit queue
Comment 1 Csaba Osztrogonác 2014-10-07 04:33:06 PDT
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 2 Alexey Proskuryakov 2014-10-07 09:44:32 PDT
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 3 Csaba Osztrogonác 2014-10-07 09:55:20 PDT
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.
Comment 4 Csaba Osztrogonác 2014-10-07 10:03:03 PDT
Created attachment 239414 [details]
Patch
Comment 5 Alexey Proskuryakov 2014-10-07 10:35:57 PDT
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.
Comment 6 Alexey Proskuryakov 2014-10-07 12:08:07 PDT
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.
Comment 7 Alexey Proskuryakov 2014-10-07 13:17:06 PDT
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 8 Ryosuke Niwa 2014-10-07 13:20:12 PDT
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 9 Alexey Proskuryakov 2014-10-07 13:48:02 PDT
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.
Comment 10 Csaba Osztrogonác 2014-10-07 14:07:29 PDT
(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.
Comment 11 Alexey Proskuryakov 2014-10-07 14:33:54 PDT
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.