104032 2012-12-04 13:27:39 -0800 commit-queue can get stuck with a local commit 2013-01-09 21:06:45 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 abarth abarth cgarcia dpranke eric ojan oldest_to_newest 782852 0 abarth 2012-12-04 13:27:39 -0800 commit-queue can get stuck with a local commit 782854 1 177536 abarth 2012-12-04 13:29:37 -0800 Created attachment 177536 Patch 782878 2 177536 dpranke 2012-12-04 13:46:57 -0800 Comment on attachment 177536 Patch As discussed over IRC, I think this patch is okay, but "clean_working_directory" means something different than "reset to merge base" to me ... maybe we should rename this command/method to reset_to_master() or something? 782911 3 177536 ojan 2012-12-04 14:04:17 -0800 Comment on attachment 177536 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177536&action=review > Tools/Scripts/webkitpy/tool/steps/commit.py:-53 > - working_directory_message = "" if error.working_directory_is_clean else " and working copy changes" > - return ('There are %s local commits%s. Everything will be committed as a single commit. ' > + return ('There are %s local commits (and possibly changes in the working directory. ' > + 'Everything will be committed as a single commit. ' > 'To avoid this prompt, set "git config webkit-patch.commit-should-always-squash true".' % ( > - error.num_local_commits, working_directory_message)) I'm confused. We can still hit this if someone is committing locally. Why change this? 782986 4 abarth 2012-12-04 14:59:29 -0800 > I'm confused. We can still hit this if someone is committing locally. Why change this? Just because the semantics of working_directory_is_clean have changed slightly. It now tells you whether there are any changes relative to the merge base, so we don't know if there are uncommitted changed in the working copy. 782990 5 abarth 2012-12-04 15:01:07 -0800 > As discussed over IRC, I think this patch is okay, but "clean_working_directory" means something different than "reset to merge base" to me ... maybe we should rename this command/method to reset_to_master() or something? Interesting idea. The trickiness is that we need a name that makes sense for SVN too. 782991 6 177536 abarth 2012-12-04 15:02:16 -0800 Comment on attachment 177536 Patch Clearing flags on attachment: 177536 Committed r136583: <http://trac.webkit.org/changeset/136583> 782993 7 abarth 2012-12-04 15:02:19 -0800 All reviewed patches have been landed. Closing bug. 783013 8 dpranke 2012-12-04 15:13:13 -0800 (In reply to comment #5) > > As discussed over IRC, I think this patch is okay, but "clean_working_directory" means something different than "reset to merge base" to me ... maybe we should rename this command/method to reset_to_master() or something? > > Interesting idea. The trickiness is that we need a name that makes sense for SVN too. Yeah, dunno ... If we can't come up with a better name we should at least add a comment or a docstring describing the intent of the method (to prepare the checkout so that we can update cleanly) ... maybe clean_for_update() or prepare_for_update() or something? 783514 9 cgarcia 2012-12-05 01:33:05 -0800 This makes webkit-patch land -g HEAD to fail complaining that working copy is not clean, even though it's clean. The problem is that working_directory_is_clean() not gets the diff between HEAD and remote_merge_base(), which is the commit you want to land. 784048 10 ojan 2012-12-05 13:15:50 -0800 (In reply to comment #4) > > I'm confused. We can still hit this if someone is committing locally. Why change this? > > Just because the semantics of working_directory_is_clean have changed slightly. It now tells you whether there are any changes relative to the merge base, so we don't know if there are uncommitted changed in the working copy. I see. It's unfortunate losing this extra information about why the commit failed. I've hit this before and it's saved me debugging time. Also, really we should probably rename working_directory_is_clean to checkout_is_clean or something like that and clean_working_directory to clean_checkout maybe? I agree w Dirk. The code is now confusing. 784395 11 abarth 2012-12-05 18:04:16 -0800 I filed bug 104198 about this issue. Hopefully I'll get a patch written tomorrow, but it might need to wait until Jan. Sorry for making this code more confusing. :( 177536 2012-12-04 13:29:37 -0800 2012-12-04 15:02:16 -0800 Patch bug-104032-20121204132702.patch text/plain 5530 abarth U3VidmVyc2lvbiBSZXZpc2lvbjogMTM2NTI3CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggZjMxMTE4ZThiMTFkYzZiMTZlNjcxMzQ1YmM0ZDdkMmJk MjY5Mzg5Ni4uZjYzNzFmOTM4NDdjZDE1MTc2YzcxNDU4MWY1MTM1YTU2YmQxNmYzYSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDI5 IEBACiAyMDEyLTEyLTA0ICBBZGFtIEJhcnRoICA8YWJhcnRoQHdlYmtpdC5vcmc+CiAKKyAgICAg ICAgY29tbWl0LXF1ZXVlIGNhbiBnZXQgc3R1Y2sgd2l0aCBhIGxvY2FsIGNvbW1pdAorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA0MDMyCisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUHJldmlvdXNseSB0aGUg Y29tbWl0LXF1ZXVlIHdvdWxkIG9ubHkgY2xlYW4gZG93biB0byBIRUFELiBJZiB0aGUKKyAgICAg ICAgY29tbWl0LXF1ZXVlIGdldHMgaW50ZXJydXB0ZWQgaW4gdGhlIG1pZGRsZSBvZiBhIGNvbW1p dCAoZS5nLiwgYWZ0ZXIKKyAgICAgICAgdGhlIGxvY2FsIGNvbW1pdCBidXQgYmVmb3JlIHRoZSBk Y29tbWl0KSwgdGhlbiBpdCBjYW4gZW5kIHVwIHdpdGggYQorICAgICAgICBwYXRjaCBsb2NhbGx5 IGNvbW1pdHRlZC4gVGhhdCBwYXRjaCB3aWxsIHRoZW4gcHJldmVudCB0aGUgY29tbWl0LXF1ZXVl CisgICAgICAgIGZyb20gZnVuY3Rpb25pbmcuCisKKyAgICAgICAgVGhpcyBwYXRjaCB0ZWFjaGVz IHRoZSBjb21taXQtcXVldWUgdG8gY2xlYW4gZG93biB0byB0aGUgbWVyZ2UgYmFzZSwKKyAgICAg ICAgd2hpY2ggd2lsbCByZW1vdmUgYW55IGxvY2FsIGNvbW1pdHMuCisKKyAgICAgICAgKiBTY3Jp cHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9zY20vZ2l0LnB5OgorICAgICAgICAoR2l0Lndv cmtpbmdfZGlyZWN0b3J5X2lzX2NsZWFuKToKKyAgICAgICAgKEdpdC5jbGVhbl93b3JraW5nX2Rp cmVjdG9yeSk6CisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS90b29sL2NvbW1hbmRzL3JvbGxf dW5pdHRlc3QucHk6CisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS90b29sL3N0ZXBzL2NvbW1p dC5weToKKyAgICAgICAgKENvbW1pdC5fY29tbWl0X3dhcm5pbmcpOgorICAgICAgICAoQ29tbWl0 LnJ1bik6CisKKzIwMTItMTItMDQgIEFkYW0gQmFydGggIDxhYmFydGhAd2Via2l0Lm9yZz4KKwog ICAgICAgICBSZW1vdmUgb2Jzb2xldGUgY29kZSBmcm9tIHdlYmtpdHB5CiAgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDQwMjkKIApkaWZmIC0tZ2l0IGEv VG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvc2NtL2dpdC5weSBiL1Rvb2xz L1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHkKaW5kZXggNjMxMzI1 NmQ4YzQ1YWI3ZjdhN2ExYjk4Y2Q3MjA2NjExNzFmNjY4ZS4uZThmMTRkZTJjNmVkMmJhZDE0M2Y3 NmM1ODAzMGZlYjVjMTlmYjRlMCAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9j b21tb24vY2hlY2tvdXQvc2NtL2dpdC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2Nv bW1vbi9jaGVja291dC9zY20vZ2l0LnB5CkBAIC0xNDcsMTEgKzE0NywxMSBAQCBjbGFzcyBHaXQo U0NNLCBTVk5SZXBvc2l0b3J5KToKICAgICAgICAgcmV0dXJuIHNlbGYuX2ZpbGVzeXN0ZW0uZXhp c3RzKHNlbGYuYWJzb2x1dGVfcGF0aChzZWxmLl9maWxlc3lzdGVtLmpvaW4oJy5naXQnLCAncmVi YXNlLWFwcGx5JykpKQogCiAgICAgZGVmIHdvcmtpbmdfZGlyZWN0b3J5X2lzX2NsZWFuKHNlbGYp OgotICAgICAgICByZXR1cm4gc2VsZi5fcnVuX2dpdChbJ2RpZmYnLCAnSEVBRCcsICctLW5vLXJl bmFtZXMnLCAnLS1uYW1lLW9ubHknXSkgPT0gIiIKKyAgICAgICAgcmV0dXJuIHNlbGYuX3J1bl9n aXQoWydkaWZmJywgc2VsZi5yZW1vdGVfbWVyZ2VfYmFzZSgpLCAnLS1uby1yZW5hbWVzJywgJy0t bmFtZS1vbmx5J10pID09ICIiCiAKICAgICBkZWYgY2xlYW5fd29ya2luZ19kaXJlY3Rvcnkoc2Vs Zik6CiAgICAgICAgICMgQ291bGQgcnVuIGdpdCBjbGVhbiBoZXJlIHRvbywgYnV0IHRoYXQgd291 bGRuJ3QgbWF0Y2ggd29ya2luZ19kaXJlY3RvcnlfaXNfY2xlYW4KLSAgICAgICAgc2VsZi5fcnVu X2dpdChbJ3Jlc2V0JywgJy0taGFyZCcsICdIRUFEJ10pCisgICAgICAgIHNlbGYuX3J1bl9naXQo WydyZXNldCcsICctLWhhcmQnLCBzZWxmLnJlbW90ZV9tZXJnZV9iYXNlKCldKQogICAgICAgICAj IEFib3J0aW5nIHJlYmFzZSBldmVuIHRob3VnaCB0aGlzIGRvZXMgbm90IG1hdGNoIHdvcmtpbmdf ZGlyZWN0b3J5X2lzX2NsZWFuCiAgICAgICAgIGlmIHNlbGYucmViYXNlX2luX3Byb2dyZXNzKCk6 CiAgICAgICAgICAgICBzZWxmLl9ydW5fZ2l0KFsncmViYXNlJywgJy0tYWJvcnQnXSkKZGlmZiAt LWdpdCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9jb21tYW5kcy9yb2xsX3VuaXR0ZXN0 LnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL2NvbW1hbmRzL3JvbGxfdW5pdHRlc3Qu cHkKaW5kZXggMWRhZTQ5N2JjM2ZhZmE1NGIxMWIyZDI0MDFhN2Q1Y2U0ZmRhNTA5NS4uNzkwMDAz OGU0NWIzZGU3MzhmZmQ2Yzg2OTE2ZjVkZTY2NWQzYjc4YiAxMDA2NDQKLS0tIGEvVG9vbHMvU2Ny aXB0cy93ZWJraXRweS90b29sL2NvbW1hbmRzL3JvbGxfdW5pdHRlc3QucHkKKysrIGIvVG9vbHMv U2NyaXB0cy93ZWJraXRweS90b29sL2NvbW1hbmRzL3JvbGxfdW5pdHRlc3QucHkKQEAgLTQwLDcg KzQwLDggQEAgTU9DSzogdXNlci5vcGVuX3VybDogZmlsZTovLy4uLgogV2FzIHRoYXQgZGlmZiBj b3JyZWN0PwogQ29tbWl0dGVkIHI0OTgyNDogPGh0dHA6Ly90cmFjLndlYmtpdC5vcmcvY2hhbmdl c2V0LzQ5ODI0PgogIiIiCi0gICAgICAgIHNlbGYuYXNzZXJ0X2V4ZWN1dGVfb3V0cHV0cyhSb2xs Q2hyb21pdW1ERVBTKCksIFs2NzY0XSwgZXhwZWN0ZWRfbG9ncz1leHBlY3RlZF9sb2dzKQorICAg ICAgICBvcHRpb25zID0gTW9ja09wdGlvbnMobm9uX2ludGVyYWN0aXZlPUZhbHNlKQorICAgICAg ICBzZWxmLmFzc2VydF9leGVjdXRlX291dHB1dHMoUm9sbENocm9taXVtREVQUygpLCBbNjc2NF0s IG9wdGlvbnM9b3B0aW9ucywgZXhwZWN0ZWRfbG9ncz1leHBlY3RlZF9sb2dzKQogCiAgICAgZGVm IHRlc3RfdXBkYXRlX2Nocm9taXVtX2RlcHNfb2xkZXJfcmV2aXNpb24oc2VsZik6CiAgICAgICAg IG9wdGlvbnMgPSBNb2NrT3B0aW9ucyhub25faW50ZXJhY3RpdmU9RmFsc2UpCmRpZmYgLS1naXQg YS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3Rvb2wvc3RlcHMvY29tbWl0LnB5IGIvVG9vbHMvU2Ny aXB0cy93ZWJraXRweS90b29sL3N0ZXBzL2NvbW1pdC5weQppbmRleCA5NDc1NDZjY2I0YmQ3MGFk YTRjZGFmYjA2ZDljNzY2ZTBjMmUwZThhLi5jNTJhNjNhOTU1NjE0Njc1YWY4OTcyMWM2ZTNjYzMz OGZiYmZhNWUxIDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3Rvb2wvc3RlcHMv Y29tbWl0LnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9zdGVwcy9jb21taXQu cHkKQEAgLTQ3LDEwICs0NywxMCBAQCBjbGFzcyBDb21taXQoQWJzdHJhY3RTdGVwKToKICAgICAg ICAgXQogCiAgICAgZGVmIF9jb21taXRfd2FybmluZyhzZWxmLCBlcnJvcik6Ci0gICAgICAgIHdv cmtpbmdfZGlyZWN0b3J5X21lc3NhZ2UgPSAiIiBpZiBlcnJvci53b3JraW5nX2RpcmVjdG9yeV9p c19jbGVhbiBlbHNlICIgYW5kIHdvcmtpbmcgY29weSBjaGFuZ2VzIgotICAgICAgICByZXR1cm4g KCdUaGVyZSBhcmUgJXMgbG9jYWwgY29tbWl0cyVzLiBFdmVyeXRoaW5nIHdpbGwgYmUgY29tbWl0 dGVkIGFzIGEgc2luZ2xlIGNvbW1pdC4gJworICAgICAgICByZXR1cm4gKCdUaGVyZSBhcmUgJXMg bG9jYWwgY29tbWl0cyAoYW5kIHBvc3NpYmx5IGNoYW5nZXMgaW4gdGhlIHdvcmtpbmcgZGlyZWN0 b3J5LiAnCisgICAgICAgICAgICAgICAgJ0V2ZXJ5dGhpbmcgd2lsbCBiZSBjb21taXR0ZWQgYXMg YSBzaW5nbGUgY29tbWl0LiAnCiAgICAgICAgICAgICAgICAgJ1RvIGF2b2lkIHRoaXMgcHJvbXB0 LCBzZXQgImdpdCBjb25maWcgd2Via2l0LXBhdGNoLmNvbW1pdC1zaG91bGQtYWx3YXlzLXNxdWFz aCB0cnVlIi4nICUgKAotICAgICAgICAgICAgICAgIGVycm9yLm51bV9sb2NhbF9jb21taXRzLCB3 b3JraW5nX2RpcmVjdG9yeV9tZXNzYWdlKSkKKyAgICAgICAgICAgICAgICBlcnJvci5udW1fbG9j YWxfY29tbWl0cykpCiAKICAgICBkZWYgX2NoZWNrX3Rlc3RfZXhwZWN0YXRpb25zKHNlbGYsIGNo YW5nZWRfZmlsZXMpOgogICAgICAgICB0ZXN0X2V4cGVjdGF0aW9uc19maWxlcyA9IFtmaWxlbmFt ZSBmb3IgZmlsZW5hbWUgaW4gY2hhbmdlZF9maWxlcyBpZiBmaWxlbmFtZS5lbmRzd2l0aCgnVGVz dEV4cGVjdGF0aW9ucycpXQpAQCAtNzMsMTIgKzczLDExIEBAIGNsYXNzIENvbW1pdChBYnN0cmFj dFN0ZXApOgogICAgICAgICAgICAgcmFpc2UgRXhjZXB0aW9uKCJBdHRlbXB0ZWQgdG8gY29tbWl0 IHdpdGggYSBjb21taXQgbWVzc2FnZSBzaG9ydGVyIHRoYW4gMTAgY2hhcmFjdGVycy4gIEVpdGhl ciB5b3VyIHBhdGNoIGlzIG1pc3NpbmcgYSBDaGFuZ2VMb2cgb3Igd2Via2l0LXBhdGNoIG1heSBo YXZlIGEgYnVnLiIpCiAKICAgICAgICAgc2VsZi5fY2hlY2tfdGVzdF9leHBlY3RhdGlvbnMoc2Vs Zi5fY2hhbmdlZF9maWxlcyhzdGF0ZSkpCi0KICAgICAgICAgc2VsZi5fc3RhdGUgPSBzdGF0ZQog CiAgICAgICAgIHVzZXJuYW1lID0gTm9uZQogICAgICAgICBwYXNzd29yZCA9IE5vbmUKLSAgICAg ICAgZm9yY2Vfc3F1YXNoID0gRmFsc2UKKyAgICAgICAgZm9yY2Vfc3F1YXNoID0gc2VsZi5fb3B0 aW9ucy5ub25faW50ZXJhY3RpdmUKIAogICAgICAgICBudW1fdHJpZXMgPSAwCiAgICAgICAgIHdo aWxlIG51bV90cmllcyA8IDM6CkBAIC05Miw3ICs5MSw3IEBAIGNsYXNzIENvbW1pdChBYnN0cmFj dFN0ZXApOgogICAgICAgICAgICAgICAgIHNlbGYuX3N0YXRlWyJjb21taXRfdGV4dCJdID0gY29t bWl0X3RleHQKICAgICAgICAgICAgICAgICBicmVhazsKICAgICAgICAgICAgIGV4Y2VwdCBBbWJp Z3VvdXNDb21taXRFcnJvciwgZToKLSAgICAgICAgICAgICAgICBpZiBzZWxmLl9vcHRpb25zLm5v bl9pbnRlcmFjdGl2ZSBvciBzZWxmLl90b29sLnVzZXIuY29uZmlybShzZWxmLl9jb21taXRfd2Fy bmluZyhlKSk6CisgICAgICAgICAgICAgICAgaWYgc2VsZi5fdG9vbC51c2VyLmNvbmZpcm0oc2Vs Zi5fY29tbWl0X3dhcm5pbmcoZSkpOgogICAgICAgICAgICAgICAgICAgICBmb3JjZV9zcXVhc2gg PSBUcnVlCiAgICAgICAgICAgICAgICAgZWxzZToKICAgICAgICAgICAgICAgICAgICAgIyBUaGlz IHdpbGwgY29ycmVjdGx5IGludGVycnVwdCB0aGUgcmVzdCBvZiB0aGUgY29tbWl0IHByb2Nlc3Mu Cg==