Bug 34058 – prepare-ChangeLog: Leading spacing of description can be improved when running with Git

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

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  <[e-mail]>

        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.

[Chris Jerdonek]

(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)

[Eric Seidel (no email)]

What command are you using?  I'm confused as to what you think it wrong here.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

Created attachment 47308 [details]
Proposed patch

[Adam Barth]

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.

[Chris Jerdonek]

(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:

[Adam Barth]

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.  :)

[WebKit Commit Bot]

Comment on attachment 47308 [details]
Proposed patch

Clearing flags on attachment: 47308

Committed r53796: <http://trac.webkit.org/changeset/53796>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Jerdonek]

(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! :)