34058 2010-01-24 11:17:22 -0800 prepare-ChangeLog: Leading spacing of description can be improved when running with Git 2010-01-25 03:22:10 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All OS X 10.5 RESOLVED FIXED P2 Normal --- 1 cjerdonek cjerdonek abarth aroben cjerdonek commit-queue ddkilzer eric oldest_to_newest 184085 0 cjerdonek 2010-01-24 11:17:22 -0800 A couple things can be improved with the leading spacing of the ChangeLog description when running prepare-ChangeLog with Git: (1) Empty lines should include no white space. (2) Existing indentation (e.g. by subtracting a constant number of spaces determined by the first non-empty line) Below you can see an example of these two things (uncorrected) when regenerating a ChangeLog with Git. 2010-01-24 Chris Jerdonek <cjerdonek@webkit.org> Reviewed by NOBODY (OOPS!). Refactored svn-apply and svn-unapply to use the new parsePatch() subroutine. <-- Leading spaces. Reviewed by NOBODY (OOPS!). <-- Leading spaces. https://bugs.webkit.org/show_bug.cgi?id=34033 * Scripts/VCSUtils.pm: - Consolidated %diffHash documentation. - Added prepareParsedPatch(). <-- The secondary bullets should start here. 184086 1 cjerdonek 2010-01-24 11:19:40 -0800 (In reply to comment #0) > (2) Existing indentation (e.g. by subtracting a constant number of spaces > determined by the first non-empty line) Meant to say-- (2) Existing (relative) indentation should be preserved (e.g. by subtracting a constant number of spaces determined by the first non-empty line) 184101 2 eric 2010-01-24 14:04:05 -0800 What command are you using? I'm confused as to what you think it wrong here. 184114 3 cjerdonek 2010-01-24 14:37:10 -0800 (In reply to comment #2) > What command are you using? I'm confused as to what you think it wrong here. I'm using prepare-Changelog --git-commit to create a ChangeLog entry from a git commit that already has a message. prepare-ChangeLog inserts the git commit message as the description in the ChangeLog entry. I'm saying that the script should preserve the relative indentation of the commit message when inserting the message into the ChangeLog entry, and not introduce trailing white space into empty lines. 184149 4 47308 cjerdonek 2010-01-24 21:37:15 -0800 Created attachment 47308 Proposed patch 184157 5 abarth 2010-01-24 22:16:44 -0800 This looks sane to me (although I don't speak Perl very well). It would be nice if we had some tests for this script so we could see the effect clearly. 184165 6 cjerdonek 2010-01-24 22:41:04 -0800 (In reply to comment #5) > This looks sane to me (although I don't speak Perl very well). It would be > nice if we had some tests for this script so we could see the effect clearly. It would probably take a fair amount of intrusive refactoring to get the surrounding function into a unit-testable state (something I am currently doing for other Perl code). Alternatively, I'll paste below the effect of the change. For this example, the following is the git commit message prior to running prepare-ChangeLog: Improved prepare-ChangeLog so that it preserves the relative indentation of a git commit message. Reviewed by NOBODY (OOPS!). https://bugs.webkit.org/show_bug.cgi?id=34058 * Scripts/prepare-ChangeLog: - Also adjusted the script so that it does not add white space characters to empty lines. To make the effect more visible in the below, I've replaced spaces with periods, and truncated the right side to make sure that line-wrapping does not take place. WITHOUT.THE.CHANGE: 2010-01-24..Chris.Jerdonek..<cjerdonek ........Reviewed.by.NOBODY.(OOPS!). ........Improved.prepare-ChangeLog.so. ........indentation.of.a.git.commit.me ........ ........Reviewed.by.NOBODY.(OOPS!). ........ ........https://bugs.webkit.org/show_b ........ ........*.Scripts/prepare-ChangeLog: ........-.Also.adjusted.the.script.so. ........space.characters.to.empty.line ........Need.a.short.description.and.b ........*.Scripts/prepare-ChangeLog: WITH.THE.CHANGE: 2010-01-24..Chris.Jerdonek..<cjerdonek ........Reviewed.by.NOBODY.(OOPS!). ........Improved.prepare-ChangeLog.so. ........indentation.of.a.git.commit.me ........Reviewed.by.NOBODY.(OOPS!). ........https://bugs.webkit.org/show_b ........*.Scripts/prepare-ChangeLog: ..........-.Also.adjusted.the.script.s ............space.characters.to.empty. ........Need.a.short.description.and.b ........*.Scripts/prepare-ChangeLog: 184224 7 47308 abarth 2010-01-25 02:58:22 -0800 Comment on attachment 47308 Proposed patch That output is indeed much prettier. :) I didn't mean that we should hold up this patch for testing, just that I'm appreciative of your work on the rest of our tools. :) 184229 8 47308 commit-queue 2010-01-25 03:18:07 -0800 Comment on attachment 47308 Proposed patch Clearing flags on attachment: 47308 Committed r53796: <http://trac.webkit.org/changeset/53796> 184230 9 commit-queue 2010-01-25 03:18:14 -0800 All reviewed patches have been landed. Closing bug. 184231 10 cjerdonek 2010-01-25 03:22:10 -0800 (In reply to comment #7) > (From update of attachment 47308 [details]) > I didn't mean that we should hold up this patch for testing, just that I'm > appreciative of your work on the rest of our tools. :) I wasn't sure before. Thanks for the review+. And for the comments! :) 47308 2010-01-24 21:37:15 -0800 2010-01-25 03:18:07 -0800 Proposed patch _patch-34058-1.txt text/plain 2153 cjerdonek ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCBjNDlkMzE3Li45YTVjODY0IDEwMDY0NAotLS0gYS9XZWJLaXRUb29scy9DaGFuZ2VM b2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTAtMDEt MjQgIENocmlzIEplcmRvbmVrICA8Y2plcmRvbmVrQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW1wcm92ZWQgcHJlcGFyZS1DaGFu Z2VMb2cgc28gdGhhdCBpdCBwcmVzZXJ2ZXMgdGhlIHJlbGF0aXZlCisgICAgICAgIGluZGVudGF0 aW9uIG9mIGEgZ2l0IGNvbW1pdCBtZXNzYWdlLgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zNDA1OAorCisgICAgICAgICogU2NyaXB0cy9wcmVwYXJl LUNoYW5nZUxvZzoKKyAgICAgICAgICAtIEFsc28gYWRqdXN0ZWQgdGhlIHNjcmlwdCBzbyB0aGF0 IGl0IGRvZXMgbm90IGFkZCB3aGl0ZQorICAgICAgICAgICAgc3BhY2UgY2hhcmFjdGVycyB0byBl bXB0eSBsaW5lcy4KKwogMjAxMC0wMS0yNCAgRXJpYyBTZWlkZWwgIDxlcmljQHdlYmtpdC5vcmc+ CiAKICAgICAgICAgTm8gcmV2aWV3LCByb2xsaW5nIG91dCByNTM3NjMuCmRpZmYgLS1naXQgYS9X ZWJLaXRUb29scy9TY3JpcHRzL3ByZXBhcmUtQ2hhbmdlTG9nIGIvV2ViS2l0VG9vbHMvU2NyaXB0 cy9wcmVwYXJlLUNoYW5nZUxvZwppbmRleCA0YzU5YWY5Li41Nzc0M2U3IDEwMDc1NQotLS0gYS9X ZWJLaXRUb29scy9TY3JpcHRzL3ByZXBhcmUtQ2hhbmdlTG9nCisrKyBiL1dlYktpdFRvb2xzL1Nj cmlwdHMvcHJlcGFyZS1DaGFuZ2VMb2cKQEAgLTE2MDYsNiArMTYwNiw3IEBAIHN1YiByZXZpZXdl ckFuZERlc2NyaXB0aW9uRm9yR2l0Q29tbWl0KCQpCiAgICAgICAgICRkZXNjcmlwdGlvbiAuPSAi XG4iIGlmICRjb21taXRMb2dDb3VudDsKICAgICAgICAgJGNvbW1pdExvZ0NvdW50Kys7CiAgICAg ICAgIG15ICRpbkhlYWRlciA9IDE7CisgICAgICAgIG15ICRjb21taXRMb2dJbmRlbnQ7IAogICAg ICAgICBteSBAbGluZXMgPSBzcGxpdCgvXG4vLCAkY29tbWl0TG9nKTsKICAgICAgICAgc2hpZnQg QGxpbmVzOyAjIFJlbW92ZSBpbml0aWFsIGJsYW5rIGxpbmUKICAgICAgICAgZm9yZWFjaCBteSAk bGluZSAoQGxpbmVzKSB7CkBAIC0xNjIwLDExICsxNjIxLDE4IEBAIHN1YiByZXZpZXdlckFuZERl c2NyaXB0aW9uRm9yR2l0Q29tbWl0KCQpCiAgICAgICAgICAgICAgICAgfSBlbHNlIHsKICAgICAg ICAgICAgICAgICAgICAgJHJldmlld2VyIC49ICIsICIgLiAkMTsKICAgICAgICAgICAgICAgICB9 Ci0gICAgICAgICAgICB9IGVsc2lmIChsZW5ndGggJGxpbmUgPT0gMCkgeworICAgICAgICAgICAg fSBlbHNpZiAoJGxpbmUgPX4gL15ccyokLykgewogICAgICAgICAgICAgICAgICRkZXNjcmlwdGlv biA9ICRkZXNjcmlwdGlvbiAuICJcbiI7CiAgICAgICAgICAgICB9IGVsc2UgewotICAgICAgICAg ICAgICAgICRsaW5lID1+IHMvXlxzKi8vOwotICAgICAgICAgICAgICAgICRkZXNjcmlwdGlvbiA9 ICRkZXNjcmlwdGlvbiAuICIgICAgICAgICIgLiAkbGluZSAuICJcbiI7CisgICAgICAgICAgICAg ICAgaWYgKCFkZWZpbmVkKCRjb21taXRMb2dJbmRlbnQpKSB7CisgICAgICAgICAgICAgICAgICAg ICMgTGV0IHRoZSBmaXJzdCBsaW5lIHdpdGggbm9uLXdoaXRlIHNwYWNlIGRldGVybWluZQorICAg ICAgICAgICAgICAgICAgICAjIHRoZSBnbG9iYWwgaW5kZW50LgorICAgICAgICAgICAgICAgICAg ICAkbGluZSA9fiAvXihccyopXFMvOworICAgICAgICAgICAgICAgICAgICAkY29tbWl0TG9nSW5k ZW50ID0gbGVuZ3RoKCQxKTsKKyAgICAgICAgICAgICAgICB9CisgICAgICAgICAgICAgICAgIyBT dHJpcCBhdCBtb3N0IHRoZSBpbmRlbnQgdG8gcHJlc2VydmUgcmVsYXRpdmUgaW5kZW50cy4KKyAg ICAgICAgICAgICAgICAkbGluZSA9fiBzL15cc3swLCRjb21taXRMb2dJbmRlbnR9Ly87CisgICAg ICAgICAgICAgICAgJGRlc2NyaXB0aW9uID0gJGRlc2NyaXB0aW9uIC4gKCIgIiB4IDgpIC4gJGxp bmUgLiAiXG4iOwogICAgICAgICAgICAgfQogICAgICAgICB9CiAgICAgfQo=