Bug 34058 - prepare-ChangeLog: Leading spacing of description can be improved when running with Git
Summary: prepare-ChangeLog: Leading spacing of description can be improved when runnin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-24 11:17 PST by Chris Jerdonek
Modified: 2010-01-25 03:22 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (2.10 KB, patch)
2010-01-24 21:37 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-01-24 11:17:22 PST
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.
Comment 1 Chris Jerdonek 2010-01-24 11:19:40 PST
(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)
Comment 2 Eric Seidel (no email) 2010-01-24 14:04:05 PST
What command are you using?  I'm confused as to what you think it wrong here.
Comment 3 Chris Jerdonek 2010-01-24 14:37:10 PST
(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.
Comment 4 Chris Jerdonek 2010-01-24 21:37:15 PST
Created attachment 47308 [details]
Proposed patch
Comment 5 Adam Barth 2010-01-24 22:16:44 PST
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.
Comment 6 Chris Jerdonek 2010-01-24 22:41:04 PST
(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:
Comment 7 Adam Barth 2010-01-25 02:58:22 PST
Comment on attachment 47308 [details]
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.  :)
Comment 8 WebKit Commit Bot 2010-01-25 03:18:07 PST
Comment on attachment 47308 [details]
Proposed patch

Clearing flags on attachment: 47308

Committed r53796: <http://trac.webkit.org/changeset/53796>
Comment 9 WebKit Commit Bot 2010-01-25 03:18:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Chris Jerdonek 2010-01-25 03:22:10 PST
(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! :)