Bug 137483 – [webkitpy] CQ- should mean not valid patch for commit queue

• Component: New Bugs
• Status: NEW

[Csaba Osztrogonác]

[webkitpy] CQ- should mean not valid patch for commit queue

[Csaba Osztrogonác]

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.

[Alexey Proskuryakov]

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?

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 239414 [details]
Patch

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

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.

[Alexey Proskuryakov]

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.

[Csaba Osztrogonác]

(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.

[Alexey Proskuryakov]

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.