27664 2009-07-24 12:46:56 -0700 commit-log-editor should allow git commit --amend to regenerate the commit log based on the modifed ChangeLog 2009-07-27 21:27:10 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 pdherbemont pdherbemont oldest_to_newest 134632 0 pdherbemont 2009-07-24 12:46:56 -0700 commit-log-editor should allow git commit --amend to regenerate the commit log based on the modifed ChangeLog. It would be especially useful after being reviewed, and when editing the ChangeLogs of an already existing commit. 134635 1 33465 pdherbemont 2009-07-24 12:53:32 -0700 Created attachment 33465 2009-07-24 Pierre d'Herbemont <pdherbemont@apple.com> Reviewed by NOBODY (OOPS!). commit-log-editor should allow git commit --amend to regenerate the commit log based on the modifed ChangeLog https://bugs.webkit.org/show_bug.cgi?id=27664 * Scripts/commit-log-editor: Add --allow-commit-log-suppression option. The user is asked if he wants to suppress previous ChangeLog and regenerate it, if this option is enabled. --- 2 files changed, 32 insertions(+), 2 deletions(-) 135057 2 33465 ddkilzer 2009-07-27 16:13:04 -0700 Comment on attachment 33465 2009-07-24 Pierre d'Herbemont <pdherbemont@apple.com> > + * Scripts/commit-log-editor: Add --allow-commit-log-suppression option. > + The user is asked if he wants to suppress previous ChangeLog and regenerate it, > + if this option is enabled. I think "--allow-commit-log-suppression" should have a better name. Maybe --regenerate-log? > +my $previousLogMessage = ""; [...] > - $logContents .= $_; > + if(!isGit() || (/^#/)) { > + $logContents .= $_; > + } else { > + $previousLogMessage .= $_; > + } > $existingLog = isGit() && !(/^#/ || /^\s*$/) unless $existingLog; The $previousLogMessage contents don't seem to be used anywhere else. What happens if you want to use the previousLogMessage? Should $previousLogMessage be assigned (or appended) to $logContents? > +my $keepExistingLog = 1; > +if ($allowPreviousCommitLogSuppression && $existingLog) { > + print "Existing log message detected, enter 'd' to delete, and regenerate log message from ChangeLogs. Press Return to continue.\n"; > + my $key = getc(STDIN); > + $keepExistingLog = 0 if ($key eq "d"); > +} My reading of the code indicates that *any* key besides 'd' will cause the existing log to be kept. I'd change the message to something like: print "Existing log message detected. Use 'r' to regenerate from ChangeLogs, or any other key to keep the existing message.\n"; r- to address the above issues. 135060 3 pdherbemont 2009-07-27 16:51:57 -0700 (In reply to comment #2) > (From update of attachment 33465 [details]) > > + * Scripts/commit-log-editor: Add --allow-commit-log-suppression option. > > + The user is asked if he wants to suppress previous ChangeLog and regenerate it, > > + if this option is enabled. > > I think "--allow-commit-log-suppression" should have a better name. Maybe > --regenerate-log? Seems like a better choice. > > > +my $previousLogMessage = ""; > [...] > > - $logContents .= $_; > > + if(!isGit() || (/^#/)) { > > + $logContents .= $_; > > + } else { > > + $previousLogMessage .= $_; > > + } > > $existingLog = isGit() && !(/^#/ || /^\s*$/) unless $existingLog; > > The $previousLogMessage contents don't seem to be used anywhere else. What > happens if you want to use the previousLogMessage? Should $previousLogMessage > be assigned (or appended) to $logContents? Yes, it's not being used here. It's here just because it's easier to read IMO. At second thought, it mades little sense, and a comment will probably be better. We could add it back as a comment in the new changelog. I didn't like this very much because it most of the time clutters the view of the modified files etc. > > > +my $keepExistingLog = 1; > > +if ($allowPreviousCommitLogSuppression && $existingLog) { > > + print "Existing log message detected, enter 'd' to delete, and regenerate log message from ChangeLogs. Press Return to continue.\n"; > > + my $key = getc(STDIN); > > + $keepExistingLog = 0 if ($key eq "d"); > > +} > > My reading of the code indicates that *any* key besides 'd' will cause the > existing log to be kept. I'd change the message to something like: > > print "Existing log message detected. Use 'r' to regenerate from ChangeLogs, > or any other key to keep the existing message.\n"; Actually, it's waiting for a return. I'll switch so that it uses Term::ReadKey package and actually just wait for one key. And change the comment. Thanks for the review! Pierre. 135080 4 33585 pdherbemont 2009-07-27 17:59:12 -0700 Created attachment 33585 2009-07-24 Pierre d'Herbemont <pdherbemont@apple.com> Reviewed by NOBODY (OOPS!). commit-log-editor should allow git commit --amend to regenerate the commit log based on the modifed ChangeLog https://bugs.webkit.org/show_bug.cgi?id=27664 * Scripts/commit-log-editor: Add --regenerate-log option. The user is asked if he wants to suppress previous ChangeLog and regenerate it, if this option is enabled. --- 2 files changed, 43 insertions(+), 2 deletions(-) 135081 5 pdherbemont 2009-07-27 18:01:00 -0700 I have updated the patch so that: - Review comment are integrated - We implement usage() as a subroutine - Just one key is read - If there is no ChangeLog file modified we don't ask the user if he wants to get rid of its current log and regenerate the logs 135086 6 33585 ddkilzer 2009-07-27 18:11:07 -0700 Comment on attachment 33585 2009-07-24 Pierre d'Herbemont <pdherbemont@apple.com> > +sub usage > +{ > + print "Usage: [--help] [--regenerate-log] <log file>\n"; > + exit 0; > +} The usage() method should always exit with a non-zero status. I suggest "exit 1;". r=me with the above change. Thanks! 135122 7 pdherbemont 2009-07-27 21:27:10 -0700 https://trac.webkit.org/changeset/46453 33465 2009-07-24 12:53:32 -0700 2009-07-27 17:59:10 -0700 2009-07-24 Pierre d'Herbemont <pdherbemont@apple.com> bug-27664-20090724125331.patch text/plain 2589 pdherbemont ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCAwNmEzY2IwLi4yMDcyNTZlIDEwMDY0NAotLS0gYS9XZWJLaXRUb29scy9DaGFuZ2VM b2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMDktMDct MjQgIFBpZXJyZSBkJ0hlcmJlbW9udCAgPHBkaGVyYmVtb250QGFwcGxlLmNvbT4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBjb21taXQtbG9nLWVkaXRv ciBzaG91bGQgYWxsb3cgZ2l0IGNvbW1pdCAtLWFtZW5kIHRvIHJlZ2VuZXJhdGUgdGhlIGNvbW1p dCBsb2cgYmFzZWQgb24gdGhlIG1vZGlmZWQgQ2hhbmdlTG9nCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNzY2NAorCisgICAgICAgICogU2NyaXB0cy9j b21taXQtbG9nLWVkaXRvcjogQWRkIC0tYWxsb3ctY29tbWl0LWxvZy1zdXBwcmVzc2lvbiBvcHRp b24uCisgICAgICAgIFRoZSB1c2VyIGlzIGFza2VkIGlmIGhlIHdhbnRzIHRvIHN1cHByZXNzIHBy ZXZpb3VzIENoYW5nZUxvZyBhbmQgcmVnZW5lcmF0ZSBpdCwKKyAgICAgICAgaWYgdGhpcyBvcHRp b24gaXMgZW5hYmxlZC4KKwogMjAwOS0wNy0yMyAgTWFyayBSb3dlICA8bXJvd2VAYXBwbGUuY29t PgogCiAgICAgICAgIFJldmlld2VkIE9saXZlciBIdW50LgpkaWZmIC0tZ2l0IGEvV2ViS2l0VG9v bHMvU2NyaXB0cy9jb21taXQtbG9nLWVkaXRvciBiL1dlYktpdFRvb2xzL1NjcmlwdHMvY29tbWl0 LWxvZy1lZGl0b3IKaW5kZXggZmI4MDYyZC4uZjkwZmU3MCAxMDA3NTUKLS0tIGEvV2ViS2l0VG9v bHMvU2NyaXB0cy9jb21taXQtbG9nLWVkaXRvcgorKysgYi9XZWJLaXRUb29scy9TY3JpcHRzL2Nv bW1pdC1sb2ctZWRpdG9yCkBAIC0zOCw2ICszOCwxMyBAQCB1c2Ugd2Via2l0ZGlyczsKIAogc3Vi IG5vcm1hbGl6ZUxpbmVFbmRpbmdzKCQkKTsKIAorbXkgJGhlbHAgPSBjaGVja0ZvckFyZ3VtZW50 QW5kUmVtb3ZlRnJvbUFSR1YoIi0taGVscCIpOworaWYgKCRoZWxwKSB7CisgICAgcHJpbnQgIlVz YWdlOiBbLS1oZWxwXSBbLS1hbGxvdy1jb21taXQtbG9nLXN1cHByZXNzaW9uXSA8bG9nIGZpbGU+ XG4iOworICAgIGV4aXQgMDsKK30KKworbXkgJGFsbG93UHJldmlvdXNDb21taXRMb2dTdXBwcmVz c2lvbiA9IGNoZWNrRm9yQXJndW1lbnRBbmRSZW1vdmVGcm9tQVJHVigiLS1hbGxvdy1jb21taXQt bG9nLXN1cHByZXNzaW9uIik7CiBteSAkbG9nID0gJEFSR1ZbMF07CiAKIG15ICRiYXNlRGlyID0g YmFzZVByb2R1Y3REaXIoKTsKQEAgLTY1LDYgKzcyLDcgQEAgaWYgKCEkZWRpdG9yKSB7CiBteSAk aW5DaGFuZ2VzVG9CZUNvbW1pdHRlZCA9ICFpc0dpdCgpOwogbXkgQGNoYW5nZUxvZ3MgPSAoKTsK IG15ICRsb2dDb250ZW50cyA9ICIiOworbXkgJHByZXZpb3VzTG9nTWVzc2FnZSA9ICIiOwogbXkg JGV4aXN0aW5nTG9nID0gMDsKIG9wZW4gTE9HLCAkbG9nIG9yIGRpZTsKIHdoaWxlICg8TE9HPikg ewpAQCAtNzYsNyArODQsMTEgQEAgd2hpbGUgKDxMT0c+KSB7CiAgICAgICAgIH0KICAgICB9CiAK LSAgICAkbG9nQ29udGVudHMgLj0gJF87CisgICAgaWYoIWlzR2l0KCkgfHwgKC9eIy8pKSB7Cisg ICAgICAgICRsb2dDb250ZW50cyAuPSAkXzsKKyAgICB9IGVsc2UgeworICAgICAgICAkcHJldmlv dXNMb2dNZXNzYWdlIC49ICRfOworICAgIH0KICAgICAkZXhpc3RpbmdMb2cgPSBpc0dpdCgpICYm ICEoL14jLyB8fCAvXlxzKiQvKSB1bmxlc3MgJGV4aXN0aW5nTG9nOwogCiAgICAgcHVzaCBAY2hh bmdlTG9ncywgbWFrZUZpbGVQYXRoUmVsYXRpdmUoJDEpIGlmICRpbkNoYW5nZXNUb0JlQ29tbWl0 dGVkICYmICgvXk0uLi4uKC4qQ2hhbmdlTG9nKVxyP1xuPyQvIHx8IC9eI1x0bW9kaWZpZWQ6ICAg KC4qQ2hhbmdlTG9nKS8pICYmICEvLUNoYW5nZUxvZy87CkBAIC05MCw5ICsxMDIsMTYgQEAgaWYg KCRsb2dDb250ZW50cyA9fiAvKFxyP1xuKS9nKSB7CiAgICAgJGVuZGwgPSAkMTsKIH0KIAorbXkg JGtlZXBFeGlzdGluZ0xvZyA9IDE7CitpZiAoJGFsbG93UHJldmlvdXNDb21taXRMb2dTdXBwcmVz c2lvbiAmJiAkZXhpc3RpbmdMb2cpIHsKKyAgICBwcmludCAiRXhpc3RpbmcgbG9nIG1lc3NhZ2Ug ZGV0ZWN0ZWQsIGVudGVyICdkJyB0byBkZWxldGUsIGFuZCByZWdlbmVyYXRlIGxvZyBtZXNzYWdl IGZyb20gQ2hhbmdlTG9ncy4gUHJlc3MgUmV0dXJuIHRvIGNvbnRpbnVlLlxuIjsKKyAgICBteSAk a2V5ID0gZ2V0YyhTVERJTik7CisgICAgJGtlZXBFeGlzdGluZ0xvZyA9IDAgaWYgKCRrZXkgZXEg ImQiKTsKK30KKwogIyBEb24ndCBjaGFuZ2UgYW55dGhpbmcgaWYgdGhlcmUncyBhbHJlYWR5IGEg bG9nIG1lc3NhZ2UKICMgKGFzIGNhbiBoYXBwZW4gd2l0aCBnaXQtY29tbWl0IC0tYW1lbmQpCi1l eGVjICRlZGl0b3IsIEBBUkdWIGlmICRleGlzdGluZ0xvZzsKK2V4ZWMgJGVkaXRvciwgQEFSR1Yg aWYgJGV4aXN0aW5nTG9nICYmICRrZWVwRXhpc3RpbmdMb2c7CiAKIG15ICR0b3BMZXZlbCA9IHRv cExldmVsU291cmNlRGlyZWN0b3J5KCk7 33585 2009-07-27 17:59:12 -0700 2009-07-27 18:11:07 -0700 2009-07-24 Pierre d'Herbemont <pdherbemont@apple.com> bug-27664-20090727175912.patch text/plain 2594 pdherbemont ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCAwNmEzY2IwLi4zZTQ1NjYzIDEwMDY0NAotLS0gYS9XZWJLaXRUb29scy9DaGFuZ2VM b2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMDktMDct MjQgIFBpZXJyZSBkJ0hlcmJlbW9udCAgPHBkaGVyYmVtb250QGFwcGxlLmNvbT4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBjb21taXQtbG9nLWVkaXRv ciBzaG91bGQgYWxsb3cgZ2l0IGNvbW1pdCAtLWFtZW5kIHRvIHJlZ2VuZXJhdGUgdGhlIGNvbW1p dCBsb2cgYmFzZWQgb24gdGhlIG1vZGlmZWQgQ2hhbmdlTG9nCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNzY2NAorCisgICAgICAgICogU2NyaXB0cy9j b21taXQtbG9nLWVkaXRvcjogQWRkIC0tcmVnZW5lcmF0ZS1sb2cgb3B0aW9uLgorICAgICAgICBU aGUgdXNlciBpcyBhc2tlZCBpZiBoZSB3YW50cyB0byBzdXBwcmVzcyBwcmV2aW91cyBDaGFuZ2VM b2cgYW5kIHJlZ2VuZXJhdGUgaXQsCisgICAgICAgIGlmIHRoaXMgb3B0aW9uIGlzIGVuYWJsZWQu CisKIDIwMDktMDctMjMgIE1hcmsgUm93ZSAgPG1yb3dlQGFwcGxlLmNvbT4KIAogICAgICAgICBS ZXZpZXdlZCBPbGl2ZXIgSHVudC4KZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL1NjcmlwdHMvY29t bWl0LWxvZy1lZGl0b3IgYi9XZWJLaXRUb29scy9TY3JpcHRzL2NvbW1pdC1sb2ctZWRpdG9yCmlu ZGV4IGZiODA2MmQuLmQ4OTlkYjQgMTAwNzU1Ci0tLSBhL1dlYktpdFRvb2xzL1NjcmlwdHMvY29t bWl0LWxvZy1lZGl0b3IKKysrIGIvV2ViS2l0VG9vbHMvU2NyaXB0cy9jb21taXQtbG9nLWVkaXRv cgpAQCAtMzUsMTAgKzM1LDI2IEBAIHVzZSBGaW5kQmluOwogdXNlIGxpYiAkRmluZEJpbjo6Qmlu OwogdXNlIFZDU1V0aWxzOwogdXNlIHdlYmtpdGRpcnM7Cit1c2UgVGVybTo6UmVhZEtleTsKIAog c3ViIG5vcm1hbGl6ZUxpbmVFbmRpbmdzKCQkKTsKIAorc3ViIHVzYWdlCit7CisgICAgcHJpbnQg IlVzYWdlOiBbLS1oZWxwXSBbLS1yZWdlbmVyYXRlLWxvZ10gPGxvZyBmaWxlPlxuIjsKKyAgICBl eGl0IDA7Cit9CisKK215ICRoZWxwID0gY2hlY2tGb3JBcmd1bWVudEFuZFJlbW92ZUZyb21BUkdW KCItLWhlbHAiKTsKK2lmICgkaGVscCkgeworICAgIHVzYWdlKCk7Cit9CisKK215ICRyZWdlbmVy YXRlTG9nID0gY2hlY2tGb3JBcmd1bWVudEFuZFJlbW92ZUZyb21BUkdWKCItLXJlZ2VuZXJhdGUt bG9nIik7CiBteSAkbG9nID0gJEFSR1ZbMF07CitpZiAoISRsb2cpIHsKKyAgICB1c2FnZSgpOwor fQogCiBteSAkYmFzZURpciA9IGJhc2VQcm9kdWN0RGlyKCk7CiAKQEAgLTc2LDcgKzkyLDEyIEBA IHdoaWxlICg8TE9HPikgewogICAgICAgICB9CiAgICAgfQogCi0gICAgJGxvZ0NvbnRlbnRzIC49 ICRfOworICAgIGlmKCFpc0dpdCgpIHx8ICgvXiMvKSkgeworICAgICAgICAkbG9nQ29udGVudHMg Lj0gJF87CisgICAgfSBlbHNlIHsKKyAgICAgICAgIyAkXyBjb250YWlucyB0aGUgY3VycmVudCBn aXQgbG9nIG1lc3NhZ2UKKyAgICAgICAgIyAod2l0aG91dCB0aGUgbG9nIGNvbW1lbnQgaW5mbyku IFdlIGRvbid0IG5lZWQgaXQuCisgICAgfQogICAgICRleGlzdGluZ0xvZyA9IGlzR2l0KCkgJiYg ISgvXiMvIHx8IC9eXHMqJC8pIHVubGVzcyAkZXhpc3RpbmdMb2c7CiAKICAgICBwdXNoIEBjaGFu Z2VMb2dzLCBtYWtlRmlsZVBhdGhSZWxhdGl2ZSgkMSkgaWYgJGluQ2hhbmdlc1RvQmVDb21taXR0 ZWQgJiYgKC9eTS4uLi4oLipDaGFuZ2VMb2cpXHI/XG4/JC8gfHwgL14jXHRtb2RpZmllZDogICAo LipDaGFuZ2VMb2cpLykgJiYgIS8tQ2hhbmdlTG9nLzsKQEAgLTkwLDkgKzExMSwxOCBAQCBpZiAo JGxvZ0NvbnRlbnRzID1+IC8oXHI/XG4pL2cpIHsKICAgICAkZW5kbCA9ICQxOwogfQogCitteSAk a2VlcEV4aXN0aW5nTG9nID0gMTsKK2lmICgkcmVnZW5lcmF0ZUxvZyAmJiAkZXhpc3RpbmdMb2cg JiYgc2NhbGFyKEBjaGFuZ2VMb2dzKSA+IDApIHsKKyAgICBwcmludCAiRXhpc3RpbmcgbG9nIG1l c3NhZ2UgZGV0ZWN0ZWQsIFVzZSAncicgdG8gcmVnZW5lcmF0ZSBsb2cgbWVzc2FnZSBmcm9tIENo YW5nZUxvZ3MsIG9yIGFueSBvdGhlciBrZXkgdG8ga2VlcCB0aGUgZXhpc3RpbmcgbWVzc2FnZS5c biI7CisgICAgUmVhZE1vZGUoJ2NicmVhaycpOworICAgIG15ICRrZXkgPSBSZWFkS2V5KDApOwor ICAgIFJlYWRNb2RlKCdub3JtYWwnKTsKKyAgICAka2VlcEV4aXN0aW5nTG9nID0gMCBpZiAoJGtl eSBlcSAiciIpOworfQorCiAjIERvbid0IGNoYW5nZSBhbnl0aGluZyBpZiB0aGVyZSdzIGFscmVh ZHkgYSBsb2cgbWVzc2FnZQogIyAoYXMgY2FuIGhhcHBlbiB3aXRoIGdpdC1jb21taXQgLS1hbWVu ZCkKLWV4ZWMgJGVkaXRvciwgQEFSR1YgaWYgJGV4aXN0aW5nTG9nOworZXhlYyAkZWRpdG9yLCBA QVJHViBpZiAkZXhpc3RpbmdMb2cgJiYgJGtlZXBFeGlzdGluZ0xvZzsKIAogbXkgJHRvcExldmVs ID0gdG9wTGV2ZWxTb3VyY2VEaXJlY3RvcnkoKTs=