137483 2014-10-07 04:27:16 -0700 [webkitpy] CQ- should mean not valid patch for commit queue 2017-06-20 02:16:50 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 1 ossy webkit-unassigned ap commit-queue dbates glenn jake.nielsen.webkit ossy rniwa oldest_to_newest 1039999 0 ossy 2014-10-07 04:27:16 -0700 [webkitpy] CQ- should mean not valid patch for commit queue 1040000 1 239406 ossy 2014-10-07 04:33:06 -0700 Created attachment 239406 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. 1040038 2 239406 ap 2014-10-07 09:44:32 -0700 Comment on attachment 239406 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? 1040040 3 239406 ossy 2014-10-07 09:55:20 -0700 Comment on attachment 239406 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. 1040043 4 239414 ossy 2014-10-07 10:03:03 -0700 Created attachment 239414 Patch 1040049 5 ap 2014-10-07 10:35:57 -0700 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. 1040079 6 ap 2014-10-07 12:08:07 -0700 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. 1040109 7 ap 2014-10-07 13:17:06 -0700 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. 1040110 8 239414 rniwa 2014-10-07 13:20:12 -0700 Comment on attachment 239414 Patch Oops, cq-ing this per ap's comment. Please resolve the remaining issue with the patch as commented by ap before landing this. 1040125 9 239414 ap 2014-10-07 13:48:02 -0700 Comment on attachment 239414 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. 1040128 10 ossy 2014-10-07 14:07:29 -0700 (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. 1040131 11 ap 2014-10-07 14:33:54 -0700 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. 239406 2014-10-07 04:33:06 -0700 2014-10-07 10:02:55 -0700 Patch bug-137483-20141007133307.patch text/plain 2123 ossy U3VidmVyc2lvbiBSZXZpc2lvbjogMTc0Mzg5CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggMTE0NjcwMmQxMzE4Y2NlYWM2ZTY5MTc0Yjg0NGE1ZWQ2 M2MyZGMxNS4uZjhkYWZmOTA1OWQxYmE2NWMxMDFkNmY0MDhjOTlhMmNjZjYzODUwZiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1 IEBACisyMDE0LTEwLTA3ICBDc2FiYSBPc3p0cm9nb27DoWMgIDxvc3N5QHdlYmtpdC5vcmc+CisK KyAgICAgICAgW3dlYmtpdHB5XSBDUS0gc2hvdWxkIG1lYW4gbm90IHZhbGlkIHBhdGNoIGZvciBj b21taXQgcXVldWUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTEzNzQ4MworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgICogU2NyaXB0cy93ZWJraXRweS90b29sL2JvdC9jb21taXRxdWV1ZXRhc2sucHk6CisgICAg ICAgIChDb21taXRRdWV1ZVRhc2sudmFsaWRhdGUpOgorICAgICAgICAqIFNjcmlwdHMvd2Via2l0 cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrX3VuaXR0ZXN0LnB5OgorICAgICAgICAodGVzdF92 YWxpZGF0ZSk6CisKIDIwMTQtMTAtMDYgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+ CiAKICAgICAgICAgTWFrZSB3ZWJraXQtcGF0Y2ggZmluZC11c2VycyB1c2VmdWwKZGlmZiAtLWdp dCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrLnB5IGIv VG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL2JvdC9jb21taXRxdWV1ZXRhc2sucHkKaW5kZXgg YTk1YzdiMTAzYTE1YWU4ZjdmMjc5NmIyZGVmZmI0M2MyNGZmNzY5Zi4uMTIwMzdmYWNjYjg5NTAw OTg2OTZjZTU1N2FkY2E0ZjZmZDVjOTFiMCAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJr aXRweS90b29sL2JvdC9jb21taXRxdWV1ZXRhc2sucHkKKysrIGIvVG9vbHMvU2NyaXB0cy93ZWJr aXRweS90b29sL2JvdC9jb21taXRxdWV1ZXRhc2sucHkKQEAgLTUwLDYgKzUwLDggQEAgY2xhc3Mg Q29tbWl0UXVldWVUYXNrKFBhdGNoQW5hbHlzaXNUYXNrKToKICAgICAgICAgICAgIHJldHVybiBG YWxzZQogICAgICAgICBpZiBzZWxmLl9wYXRjaC5yZXZpZXcoKSA9PSAiLSI6CiAgICAgICAgICAg ICByZXR1cm4gRmFsc2UKKyAgICAgICAgaWYgc2VsZi5fcGF0Y2guY29tbWl0X3F1ZXVlKCkgPT0g Ii0iOgorICAgICAgICAgICAgcmV0dXJuIEZhbHNlCiAgICAgICAgIHJldHVybiBUcnVlCiAKICAg ICBkZWYgX3ZhbGlkYXRlX2NoYW5nZWxvZyhzZWxmKToKZGlmZiAtLWdpdCBhL1Rvb2xzL1Njcmlw dHMvd2Via2l0cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrX3VuaXR0ZXN0LnB5IGIvVG9vbHMv U2NyaXB0cy93ZWJraXRweS90b29sL2JvdC9jb21taXRxdWV1ZXRhc2tfdW5pdHRlc3QucHkKaW5k ZXggZGM0ZTNkM2VmNTg4MWVkODNjNjEzODcwYzc4YjM3ZGFmZDY4ODI3Ny4uN2VjMzk5MDA5NmQ0 MmVlZmVhOTE4OWUwNjVmYTVjNDI3MTNkZGM4MyAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93 ZWJraXRweS90b29sL2JvdC9jb21taXRxdWV1ZXRhc2tfdW5pdHRlc3QucHkKKysrIGIvVG9vbHMv U2NyaXB0cy93ZWJraXRweS90b29sL2JvdC9jb21taXRxdWV1ZXRhc2tfdW5pdHRlc3QucHkKQEAg LTYyMiwzICs2MjIsNCBAQCBjb21tYW5kX2ZhaWxlZDogZmFpbHVyZV9tZXNzYWdlPSdVbmFibGUg dG8gbGFuZCBwYXRjaCcgc2NyaXB0X2Vycm9yPSdNT0NLIGxhbmQgZgogICAgICAgICBzZWxmLl9l eHBlY3RfdmFsaWRhdGUoc2VsZi5fbW9ja19wYXRjaChidWdfZGljdD17J2J1Z19zdGF0dXMnOiAn Q0xPU0VEJ30pLCBGYWxzZSkKICAgICAgICAgc2VsZi5fZXhwZWN0X3ZhbGlkYXRlKHNlbGYuX21v Y2tfcGF0Y2goY29tbWl0dGVyPU5vbmUpLCBGYWxzZSkKICAgICAgICAgc2VsZi5fZXhwZWN0X3Zh bGlkYXRlKHNlbGYuX21vY2tfcGF0Y2goeydyZXZpZXcnOiAnLSd9KSwgRmFsc2UpCisgICAgICAg IHNlbGYuX2V4cGVjdF92YWxpZGF0ZShzZWxmLl9tb2NrX3BhdGNoKHsnY29tbWl0LXF1ZXVlJzog Jy0nfSksIEZhbHNlKQo= 239414 2014-10-07 10:03:03 -0700 2014-10-07 14:05:03 -0700 Patch bug-137483-20141007190304.patch text/plain 2533 ossy U3VidmVyc2lvbiBSZXZpc2lvbjogMTc0MjMwCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggYzAyMTU5MGFjZDE3ZWQzYzc0YWNjNDA4MDBlMDFhMTM1 NmJmZjU1ZC4uZWIxMzhkODNjZGZlZTYyZWJiYzBhMDc5NjUyNDRlOGQ2YTBlZDJjMSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1 IEBACisyMDE0LTEwLTA3ICBDc2FiYSBPc3p0cm9nb27DoWMgIDxvc3N5QHdlYmtpdC5vcmc+CisK KyAgICAgICAgW3dlYmtpdHB5XSBDUS0gc2hvdWxkIG1lYW4gbm90IHZhbGlkIHBhdGNoIGZvciBj b21taXQgcXVldWUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTEzNzQ4MworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgICogU2NyaXB0cy93ZWJraXRweS90b29sL2JvdC9jb21taXRxdWV1ZXRhc2sucHk6CisgICAg ICAgIChDb21taXRRdWV1ZVRhc2sudmFsaWRhdGUpOgorICAgICAgICAqIFNjcmlwdHMvd2Via2l0 cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrX3VuaXR0ZXN0LnB5OgorICAgICAgICAodGVzdF92 YWxpZGF0ZSk6CisKIDIwMTQtMTAtMDIgIENvbW1pdCBRdWV1ZSAgPGNvbW1pdC1xdWV1ZUB3ZWJr aXQub3JnPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJvbGxpbmcgb3V0IHIxNzQxMjAuCmRpZmYg LS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3Rvb2wvYm90L2NvbW1pdHF1ZXVldGFzay5w eSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrLnB5Cmlu ZGV4IGE5NWM3YjEwM2ExNWFlOGY3ZjI3OTZiMmRlZmZiNDNjMjRmZjc2OWYuLmJjNjZhYTFkNWNm YmI0YzVhNjZkMWRiZWU4MmJmZWYwYTY1YTlmNWYgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1NjcmlwdHMv d2Via2l0cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMv d2Via2l0cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrLnB5CkBAIC01MCw2ICs1MCw4IEBAIGNs YXNzIENvbW1pdFF1ZXVlVGFzayhQYXRjaEFuYWx5c2lzVGFzayk6CiAgICAgICAgICAgICByZXR1 cm4gRmFsc2UKICAgICAgICAgaWYgc2VsZi5fcGF0Y2gucmV2aWV3KCkgPT0gIi0iOgogICAgICAg ICAgICAgcmV0dXJuIEZhbHNlCisgICAgICAgIGlmIHNlbGYuX3BhdGNoLmNvbW1pdF9xdWV1ZSgp ICE9ICIrIjoKKyAgICAgICAgICAgIHJldHVybiBGYWxzZQogICAgICAgICByZXR1cm4gVHJ1ZQog CiAgICAgZGVmIF92YWxpZGF0ZV9jaGFuZ2Vsb2coc2VsZik6CmRpZmYgLS1naXQgYS9Ub29scy9T Y3JpcHRzL3dlYmtpdHB5L3Rvb2wvYm90L2NvbW1pdHF1ZXVldGFza191bml0dGVzdC5weSBiL1Rv b2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrX3VuaXR0ZXN0LnB5 CmluZGV4IGRjNGUzZDNlZjU4ODFlZDgzYzYxMzg3MGM3OGIzN2RhZmQ2ODgyNzcuLjBhMTNmYTk0 YWMwNzllYzNlYmE5ZjdkMzI0NTJjMGQ5MTY0ZWIzYzEgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1Njcmlw dHMvd2Via2l0cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrX3VuaXR0ZXN0LnB5CisrKyBiL1Rv b2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9ib3QvY29tbWl0cXVldWV0YXNrX3VuaXR0ZXN0LnB5 CkBAIC02MTcsOCArNjE3LDExIEBAIGNvbW1hbmRfZmFpbGVkOiBmYWlsdXJlX21lc3NhZ2U9J1Vu YWJsZSB0byBsYW5kIHBhdGNoJyBzY3JpcHRfZXJyb3I9J01PQ0sgbGFuZCBmCiAgICAgICAgIHJl dHVybiBwYXRjaAogCiAgICAgZGVmIHRlc3RfdmFsaWRhdGUoc2VsZik6Ci0gICAgICAgIHNlbGYu X2V4cGVjdF92YWxpZGF0ZShzZWxmLl9tb2NrX3BhdGNoKCksIFRydWUpCisgICAgICAgIHNlbGYu X2V4cGVjdF92YWxpZGF0ZShzZWxmLl9tb2NrX3BhdGNoKCksIEZhbHNlKQogICAgICAgICBzZWxm Ll9leHBlY3RfdmFsaWRhdGUoc2VsZi5fbW9ja19wYXRjaCh7J2lzX29ic29sZXRlJzogVHJ1ZX0p LCBGYWxzZSkKICAgICAgICAgc2VsZi5fZXhwZWN0X3ZhbGlkYXRlKHNlbGYuX21vY2tfcGF0Y2go YnVnX2RpY3Q9eydidWdfc3RhdHVzJzogJ0NMT1NFRCd9KSwgRmFsc2UpCiAgICAgICAgIHNlbGYu X2V4cGVjdF92YWxpZGF0ZShzZWxmLl9tb2NrX3BhdGNoKGNvbW1pdHRlcj1Ob25lKSwgRmFsc2Up CiAgICAgICAgIHNlbGYuX2V4cGVjdF92YWxpZGF0ZShzZWxmLl9tb2NrX3BhdGNoKHsncmV2aWV3 JzogJy0nfSksIEZhbHNlKQorICAgICAgICBzZWxmLl9leHBlY3RfdmFsaWRhdGUoc2VsZi5fbW9j a19wYXRjaCh7J2NvbW1pdC1xdWV1ZSc6ICctJ30pLCBGYWxzZSkKKyAgICAgICAgc2VsZi5fZXhw ZWN0X3ZhbGlkYXRlKHNlbGYuX21vY2tfcGF0Y2goeydjb21taXQtcXVldWUnOiAnPyd9KSwgRmFs c2UpCisgICAgICAgIHNlbGYuX2V4cGVjdF92YWxpZGF0ZShzZWxmLl9tb2NrX3BhdGNoKHsnY29t bWl0LXF1ZXVlJzogJysnfSksIFRydWUpCg==