108983 2013-02-05 14:34:09 -0800 webkit-patch upload regenerates the WebCore ChangeLog every time it's called 2013-02-11 23:59:45 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 jchaffraix webkit-unassigned abarth dpranke eric rniwa timloh webkit.review.bot oldest_to_newest 825826 0 jchaffraix 2013-02-05 14:34:09 -0800 How to reproduce: * WebKit revision r141929 (git checkout FWIW) * Apply https://bug-108975-attachments.webkit.org/attachment.cgi?id=186706 * Remove the ChangeLogs diffs (git checkout LayoutTests/ChangeLog Source/WebCore/ChangeLog) * Call prepare-ChangeLog -b 108975 * Edit the WebCore ChangeLog and paste back the one from the attachment. * Enjoy calling webkit-patch upload and set it regenerate the ChangeLog every time it's called. 825902 1 timloh 2013-02-05 15:51:30 -0800 (In reply to comment #0) > How to reproduce: > > * WebKit revision r141929 (git checkout FWIW) > * Apply https://bug-108975-attachments.webkit.org/attachment.cgi?id=186706 > * Remove the ChangeLogs diffs (git checkout LayoutTests/ChangeLog Source/WebCore/ChangeLog) > * Call prepare-ChangeLog -b 108975 > * Edit the WebCore ChangeLog and paste back the one from the attachment. > * Enjoy calling webkit-patch upload and set it regenerate the ChangeLog every time it's called. Did you manually remove "(WebCore):" and "(RenderBox):" from the list, because if so, this is *technically* a feature (albeit an annoying one), and we can work out some heuristics to allow this behaviour to pass (maybe allow users to delete (\w*): from .cpp and .h files). 825932 2 rniwa 2013-02-05 16:33:16 -0800 (In reply to comment #1) > Did you manually remove "(WebCore):" and "(RenderBox):" from the list, because if so, this is *technically* a feature (albeit an annoying one), and we can work out some heuristics to allow this behaviour to pass (maybe allow users to delete (\w*): from .cpp and .h files). I would have preferred for features like this one to run only when developers told the tool to do so. Automatically adding things back into change log seems overly aggressive. 826101 3 timloh 2013-02-05 23:11:51 -0800 (In reply to comment #2) > (In reply to comment #1) > > Did you manually remove "(WebCore):" and "(RenderBox):" from the list, because if so, this is *technically* a feature (albeit an annoying one), and we can work out some heuristics to allow this behaviour to pass (maybe allow users to delete (\w*): from .cpp and .h files). > > I would have preferred for features like this one to run only when developers told the tool to do so. Automatically adding things back into change log seems overly aggressive. The main point of the feature is to add to the change log entry new touched files/functions. At the moment there's really no way to tell the difference between what's been just added, or what's been removed. The way I see it is that adding functionality to see what the user has removed is unnecessary since the simplest solution is to just leave in the lines like (WebCore) in the change log entries (which I think most people would be doing anyway), which makes this is a non-issue. Another option is to revert to the previous behaviour (i.e. don't update the change logs with the new list) by default, and have the new behaviour behind a flag, but I'm not sure that this would be preferred by other developers. 826103 4 rniwa 2013-02-05 23:16:44 -0800 (In reply to comment #3) > The main point of the feature is to add to the change log entry new touched files/functions. At the moment there's really no way to tell the difference between what's been just added, or what's been removed. That's why I'm suggesting to make this an opt-in feature. It's annoying for a tool to enforce a particular change when that change is not necessarily desirable. >The way I see it is that adding functionality to see what the user has removed is unnecessary since the simplest solution is to just leave in the lines like (WebCore) in the change log entries (which I think most people would be doing anyway), which makes this is a non-issue. That's not an option. The reason a developer deletes such an entry is because it's incorrect or unnecessary. > Another option is to revert to the previous behaviour (i.e. don't update the change logs with the new list) by default, and have the new behaviour behind a flag, but I'm not sure that this would be preferred by other developers. I think that's a much better behavior. The the patch made webkit-patch useless in many situations until we added --no-prepare-changelogs. 826106 5 rniwa 2013-02-05 23:22:15 -0800 In particular, prepare-ChangeLogs is terrible at generating change logs in non-C++ languages. e.g. function Class() { this.method = function () {...} } results in: (Class): (this.method) While I appreciate your contribution to the project, prepare-ChangeLogs does not generate the most appropriate change logs at the moment, and replacing change logs humans manually edited is not going to help it. 826435 6 jchaffraix 2013-02-06 07:36:56 -0800 > Did you manually remove "(WebCore):" and "(RenderBox):" from the list, because if so, this is *technically* a feature (albeit an annoying one), and we can work out some heuristics to allow this behaviour to pass (maybe allow users to delete (\w*): from .cpp and .h files). Good point, I indeed removed the namespace / empty class entries as I find that they don't add anything to the ChangeLog. I also usually advise people in reviews to do the same. If we fix this, this would probably solve my pain point with dealing with webkit-patch upload. Though there is still the possibility of shooting yourself in the foot that would need to be solved. A (really bad) idea would be to add some OOPS so that the commit hook can detect duplicated entries. 827942 7 timloh 2013-02-07 15:42:48 -0800 (In reply to comment #4) > (In reply to comment #3) > > The main point of the feature is to add to the change log entry new touched files/functions. At the moment there's really no way to tell the difference between what's been just added, or what's been removed. > > That's why I'm suggesting to make this an opt-in feature. It's annoying for a tool to enforce a particular change when that change is not necessarily desirable. OK. I'll put a patch to revert the default behaviour to pre-r140511 (i.e. Bug 74358), with the new behaviour available behind --update-changelogs. 827951 8 187190 timloh 2013-02-07 15:51:07 -0800 Created attachment 187190 Patch 830612 9 rniwa 2013-02-11 23:53:30 -0800 I'm hitting the bug where webkit-patch upload duplicates change log entries when I manually edit them :( 830615 10 187190 webkit.review.bot 2013-02-11 23:59:41 -0800 Comment on attachment 187190 Patch Clearing flags on attachment: 187190 Committed r142589: <http://trac.webkit.org/changeset/142589> 830616 11 webkit.review.bot 2013-02-11 23:59:45 -0800 All reviewed patches have been landed. Closing bug. 187190 2013-02-07 15:51:07 -0800 2013-02-11 23:59:40 -0800 Patch bug-108983-20130208104742.patch text/plain 5264 timloh U3VidmVyc2lvbiBSZXZpc2lvbjogMTQyMTg4CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggMGU3ZjMxYjgyM2RiOTVlMDdlMWM5MzM2NWQ1ZDQ4OTY5 MDQ2MzE5Yi4uOWQ1ZDE4NDQxZGU0YWIyYjVmYzA3ZTQ2ODI0M2JjNzQyZTcyYTdhMyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIz IEBACisyMDEzLTAyLTA3ICBUaW1vdGh5IExvaCAgPHRpbWxvaEBjaHJvbWl1bS5jb20+CisKKyAg ICAgICAgd2Via2l0LXBhdGNoIHVwbG9hZCByZWdlbmVyYXRlcyB0aGUgV2ViQ29yZSBDaGFuZ2VM b2cgZXZlcnkgdGltZSBpdCdzIGNhbGxlZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MTA4OTgzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgVGhpcyBwYXRjaCBwdXRzIHRoZSBiZWhhdmlvdXIgZnJvbSBCdWcg NzQzNTggYmVoaW5kIHRoZSBmbGFnIChkZWZhdWx0PU9GRikKKyAgICAgICAgYC0tdXBkYXRlLWNo YW5nZWxvZ3MnLCBhbmQgcmVtb3ZlcyB0aGUgZmxhZyBgLS1uby1wcmVwYXJlLWNoYW5nZWxvZ3Mn LgorICAgICAgICBUaGUgZmxhZyBuYW1lIGNoYW5nZSBmcm9tIHByZXBhcmUgdG8gdXBkYXRlIGlz IHNpbmNlIHdlIHN0aWxsIHdhbnQgdG8KKyAgICAgICAgcHJlcGFyZSBjaGFuZ2Vsb2dzIGluIHRo ZSBkZWZhdWx0IGNhc2Ugd2hlbiBub25lIGN1cnJlbnRseSBleGlzdC4KKworICAgICAgICAqIFNj cmlwdHMvd2Via2l0cHkvdG9vbC9jb21tYW5kcy9jb21tYW5kdGVzdC5weToKKyAgICAgICAgKENv bW1hbmRzVGVzdC5hc3NlcnRfZXhlY3V0ZV9vdXRwdXRzKToKKyAgICAgICAgKiBTY3JpcHRzL3dl YmtpdHB5L3Rvb2wvc3RlcHMvb3B0aW9ucy5weToKKyAgICAgICAgKE9wdGlvbnMpOgorICAgICAg ICAqIFNjcmlwdHMvd2Via2l0cHkvdG9vbC9zdGVwcy9wcmVwYXJlY2hhbmdlbG9nLnB5OgorICAg ICAgICAoUHJlcGFyZUNoYW5nZUxvZy5vcHRpb25zKToKKyAgICAgICAgKFByZXBhcmVDaGFuZ2VM b2cucnVuKToKKwogMjAxMy0wMi0wNyAgUm9nZXIgRm9uZyAgPHJvZ2VyX2ZvbmdAYXBwbGUuY29t PgogCiAgICAgICAgIFVucmV2aWV3ZWQuIEFub3RoZXIgdGVtcG9yYXJ5IEVXUyBib3QgZml4LiBJ dCdsbCB0b3RhbGx5IHdvcmsgdGhpcyB0aW1lLgpkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93 ZWJraXRweS90b29sL2NvbW1hbmRzL2NvbW1hbmR0ZXN0LnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJr aXRweS90b29sL2NvbW1hbmRzL2NvbW1hbmR0ZXN0LnB5CmluZGV4IDZkMDA5YmNjYTlmZjAyZThm ZWE2OTMyYmFjOTMzMWJlZDI3NTc1NTIuLjY1NWMzM2ZkYWM0MWRjNDExYjk1NzhkNTE0YTJkOThi MTZlMGVhOTEgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9jb21tYW5k cy9jb21tYW5kdGVzdC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3Rvb2wvY29tbWFu ZHMvY29tbWFuZHRlc3QucHkKQEAgLTQyLDcgKzQyLDcgQEAgY2xhc3MgQ29tbWFuZHNUZXN0KFRl c3RDYXNlKToKICAgICAgICAgb3B0aW9ucy5vYnNvbGV0ZV9wYXRjaGVzID0gVHJ1ZQogICAgICAg ICBvcHRpb25zLm9wZW5fYnVnID0gVHJ1ZQogICAgICAgICBvcHRpb25zLnBvcnQgPSAnTU9DSyBw b3J0JwotICAgICAgICBvcHRpb25zLnByZXBhcmVfY2hhbmdlbG9ncyA9IFRydWUKKyAgICAgICAg b3B0aW9ucy51cGRhdGVfY2hhbmdlbG9ncyA9IEZhbHNlCiAgICAgICAgIG9wdGlvbnMucXVpZXQg PSBUcnVlCiAgICAgICAgIG9wdGlvbnMucmV2aWV3ZXIgPSAnTU9DSyByZXZpZXdlcicKICAgICAg ICAgY29tbWFuZC5iaW5kX3RvX3Rvb2wodG9vbCkKZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMv d2Via2l0cHkvdG9vbC9zdGVwcy9vcHRpb25zLnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90 b29sL3N0ZXBzL29wdGlvbnMucHkKaW5kZXggMDUyMDRmYTE2NmE4NTM3OWExYzUzNTg3ZDNjOTA5 ZDUxMDMxY2FiNC4uN2VkYTYxNDU5MDFiZjUyZDhiODljNzVkNGRjYTY1ZmM1MzQ1MzIyZiAxMDA2 NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL3N0ZXBzL29wdGlvbnMucHkKKysr IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL3N0ZXBzL29wdGlvbnMucHkKQEAgLTQ5LDcg KzQ5LDYgQEAgY2xhc3MgT3B0aW9ucyhvYmplY3QpOgogICAgIG9ic29sZXRlX3BhdGNoZXMgPSBt YWtlX29wdGlvbigiLS1uby1vYnNvbGV0ZSIsIGFjdGlvbj0ic3RvcmVfZmFsc2UiLCBkZXN0PSJv YnNvbGV0ZV9wYXRjaGVzIiwgZGVmYXVsdD1UcnVlLCBoZWxwPSJEbyBub3Qgb2Jzb2xldGUgb2xk IHBhdGNoZXMgYmVmb3JlIHBvc3RpbmcgdGhpcyBvbmUuIikKICAgICBvcGVuX2J1ZyA9IG1ha2Vf b3B0aW9uKCItLW9wZW4tYnVnIiwgYWN0aW9uPSJzdG9yZV90cnVlIiwgZGVzdD0ib3Blbl9idWci LCBkZWZhdWx0PUZhbHNlLCBoZWxwPSJPcGVucyB0aGUgYXNzb2NpYXRlZCBidWcgaW4gYSBicm93 c2VyLiIpCiAgICAgcGFyZW50X2NvbW1hbmQgPSBtYWtlX29wdGlvbigiLS1wYXJlbnQtY29tbWFu ZCIsIGFjdGlvbj0ic3RvcmUiLCBkZXN0PSJwYXJlbnRfY29tbWFuZCIsIGRlZmF1bHQ9Tm9uZSwg aGVscD0iKEludGVybmFsKSBUaGUgY29tbWFuZCB0aGF0IHNwYXduZWQgdGhpcyBpbnN0YW5jZS4i KQotICAgIHByZXBhcmVfY2hhbmdlbG9ncyA9IG1ha2Vfb3B0aW9uKCItLW5vLXByZXBhcmUtY2hh bmdlbG9ncyIsIGFjdGlvbj0ic3RvcmVfZmFsc2UiLCBkZXN0PSJwcmVwYXJlX2NoYW5nZWxvZ3Mi LCBkZWZhdWx0PVRydWUsIGhlbHA9IkRvbid0IHByZXBhcmUgKGNyZWF0ZSBhbmQvb3IgdXBkYXRl KSBDaGFuZ2VMb2dzLiIpCiAgICAgcXVpZXQgPSBtYWtlX29wdGlvbigiLS1xdWlldCIsIGFjdGlv bj0ic3RvcmVfdHJ1ZSIsIGRlc3Q9InF1aWV0IiwgZGVmYXVsdD1GYWxzZSwgaGVscD0iUHJvZHVj ZSBsZXNzIGNvbnNvbGUgb3V0cHV0LiIpCiAgICAgcmVxdWVzdF9jb21taXQgPSBtYWtlX29wdGlv bigiLS1yZXF1ZXN0LWNvbW1pdCIsIGFjdGlvbj0ic3RvcmVfdHJ1ZSIsIGRlc3Q9InJlcXVlc3Rf Y29tbWl0IiwgZGVmYXVsdD1GYWxzZSwgaGVscD0iTWFyayB0aGUgcGF0Y2ggYXMgbmVlZGluZyBh dXRvLWNvbW1pdCBhZnRlciByZXZpZXcuIikKICAgICByZXZpZXcgPSBtYWtlX29wdGlvbigiLS1u by1yZXZpZXciLCBhY3Rpb249InN0b3JlX2ZhbHNlIiwgZGVzdD0icmV2aWV3IiwgZGVmYXVsdD1U cnVlLCBoZWxwPSJEbyBub3QgbWFyayB0aGUgcGF0Y2ggZm9yIHJldmlldy4iKQpAQCAtNTcsNCAr NTYsNSBAQCBjbGFzcyBPcHRpb25zKG9iamVjdCk6CiAgICAgc3VnZ2VzdF9yZXZpZXdlcnMgPSBt YWtlX29wdGlvbigiLS1zdWdnZXN0LXJldmlld2VycyIsIGFjdGlvbj0ic3RvcmVfdHJ1ZSIsIGRl ZmF1bHQ9RmFsc2UsIGhlbHA9Ik9mZmVyIHRvIENDIGFwcHJvcHJpYXRlIHJldmlld2Vycy4iKQog ICAgIHRlc3QgPSBtYWtlX29wdGlvbigiLS10ZXN0IiwgYWN0aW9uPSJzdG9yZV90cnVlIiwgZGVz dD0idGVzdCIsIGRlZmF1bHQ9RmFsc2UsIGhlbHA9IlJ1biBydW4td2Via2l0LXRlc3RzIGJlZm9y ZSBjb21taXR0aW5nLiIpCiAgICAgdXBkYXRlID0gbWFrZV9vcHRpb24oIi0tbm8tdXBkYXRlIiwg YWN0aW9uPSJzdG9yZV9mYWxzZSIsIGRlc3Q9InVwZGF0ZSIsIGRlZmF1bHQ9VHJ1ZSwgaGVscD0i RG9uJ3QgdXBkYXRlIHRoZSB3b3JraW5nIGRpcmVjdG9yeS4iKQorICAgIHVwZGF0ZV9jaGFuZ2Vs b2dzID0gbWFrZV9vcHRpb24oIi0tdXBkYXRlLWNoYW5nZWxvZ3MiLCBhY3Rpb249InN0b3JlX3Ry dWUiLCBkZXN0PSJ1cGRhdGVfY2hhbmdlbG9ncyIsIGRlZmF1bHQ9RmFsc2UsIGhlbHA9IlVwZGF0 ZSBleGlzdGluZyBDaGFuZ2VMb2cgZW50cmllcyB3aXRoIG5ldyBkYXRlLCBidWcgZGVzY3JpcHRp b24sIGFuZCB0b3VjaGVkIGZpbGVzL2Z1bmN0aW9ucy4iKQogICAgIGNoYW5nZWxvZ19jb3VudCA9 IG1ha2Vfb3B0aW9uKCItLWNoYW5nZWxvZy1jb3VudCIsIGFjdGlvbj0ic3RvcmUiLCB0eXBlPSJp bnQiLCBkZXN0PSJjaGFuZ2Vsb2dfY291bnQiLCBoZWxwPSJOdW1iZXIgb2YgY2hhbmdlbG9ncyB0 byBwYXJzZS4iKQpkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL3N0ZXBz L3ByZXBhcmVjaGFuZ2Vsb2cucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3Rvb2wvc3RlcHMv cHJlcGFyZWNoYW5nZWxvZy5weQppbmRleCA5MjYyOTAzYmFjNDRjMmNlMWVkN2I1OTRlYTJmNjhh OTczN2RmYzVjLi43MTZhYjgyNmQ1YmZhMDdhMWJlODU1MzEzNTMxNGM2NWQwNWY4MjJhIDEwMDY0 NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3Rvb2wvc3RlcHMvcHJlcGFyZWNoYW5nZWxv Zy5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3Rvb2wvc3RlcHMvcHJlcGFyZWNoYW5n ZWxvZy5weQpAQCAtNDUsNyArNDUsNyBAQCBjbGFzcyBQcmVwYXJlQ2hhbmdlTG9nKEFic3RyYWN0 U3RlcCk6CiAgICAgICAgICAgICBPcHRpb25zLnF1aWV0LAogICAgICAgICAgICAgT3B0aW9ucy5l bWFpbCwKICAgICAgICAgICAgIE9wdGlvbnMuZ2l0X2NvbW1pdCwKLSAgICAgICAgICAgIE9wdGlv bnMucHJlcGFyZV9jaGFuZ2Vsb2dzLAorICAgICAgICAgICAgT3B0aW9ucy51cGRhdGVfY2hhbmdl bG9ncywKICAgICAgICAgXQogCiAgICAgZGVmIF9lbnN1cmVfYnVnX3VybChzZWxmLCBzdGF0ZSk6 CkBAIC0xMDIsMTEgKzEwMiwxMCBAQCBjbGFzcyBQcmVwYXJlQ2hhbmdlTG9nKEFic3RyYWN0U3Rl cCk6CiAgICAgICAgIHJldHVybiBmaW5hbF9lbnRyeSArICJcbiIKIAogICAgIGRlZiBydW4oc2Vs Ziwgc3RhdGUpOgotICAgICAgICBpZiBub3Qgc2VsZi5fb3B0aW9ucy5wcmVwYXJlX2NoYW5nZWxv Z3M6Ci0gICAgICAgICAgICByZXR1cm4KLQogICAgICAgICBpZiBzZWxmLmNhY2hlZF9sb29rdXAo c3RhdGUsICJjaGFuZ2Vsb2dzIik6CiAgICAgICAgICAgICBzZWxmLl9lbnN1cmVfYnVnX3VybChz dGF0ZSkKKyAgICAgICAgICAgIGlmIG5vdCBzZWxmLl9vcHRpb25zLnVwZGF0ZV9jaGFuZ2Vsb2dz OgorICAgICAgICAgICAgICAgIHJldHVybgogCiAgICAgICAgIGFyZ3MgPSBzZWxmLl90b29sLmRl cHJlY2F0ZWRfcG9ydCgpLnByZXBhcmVfY2hhbmdlbG9nX2NvbW1hbmQoKQogICAgICAgICBpZiBz dGF0ZS5nZXQoImJ1Z19pZCIpOgo=