Bug 174197 – Make prepare-ChangeLog -g <commit> generate a more standard ChangeLog entry.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Emilio Cobos Álvarez]

Make prepare-ChangeLog -g <commit> generate a more standard ChangeLog entry.

Before this change, |prepare-ChangeLog -g HEAD| generated something like:

    <git commit title>

    <git commit long description>

    Need a short description (OOPS!)
    Need the bug URL (OOPS!)

    Reviewed by NOBODY (OOPS!).
    No new tests (OOPS!).

This change converts that in:

    <git commit title>
    Need the bug URL (OOPS!)

    Reviewed by NOBODY (OOPS!).

    <git commit long description>

    No new tests (OOPS!).

This bit me the first time I tried to submit a patch to WebKit, and still I have
to manually edit the ChangeLog every time I use the script.

This generates a more convenient and standard ChangeLog entry.

[Emilio Cobos Álvarez]

Created attachment 314708 [details]
Patch

[Emilio Cobos Álvarez]

Created attachment 314709 [details]
Patch

[Michael Catanzaro]

Comment on attachment 314709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314709&action=review

> Tools/Scripts/prepare-ChangeLog:684
> +        if (!$bugDescription && $description) {
> +            my @lines = split(/\n/, $description);
> +            $bugDescription = shift @lines;
> +            $bugDescription =~ s/^\s*//g;
> +            if (scalar @lines > 0 and $lines[0] =~ /^\s*$/) {
> +                shift @lines;
> +            }
> +            $description = join("\n", @lines)
> +        }

Hmmm. On the one hand, this seems like a great change.

On the other hand, this code looks like gobbledygook.

First, congratulate yourself on having learned enough Perl to do this. Next, try to find a reviewer who understands Perl too. I'm just not comfortable giving r+ when I have no clue how to read the code.

[Darin Adler]

Comment on attachment 314709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314709&action=review

Seems fine.

Since non-perl programmers can’t read the code to strip off the first line anyway, I would consider doing it with a single regular expression like this:

    ($bugDescription, $description)
        = ($description =~ /^(?:\s*(.*)\n)?(?:\s*\n)*(.*)/)
        if !$bugDescription && $description;

Could write that all on one line, too, if you prefer. Note that my version strips multiple blank lines, not just one. Could use a "?" instead of a "*" to strip only one.

> Tools/Scripts/prepare-ChangeLog:677
> +            my @lines = split(/\n/, $description);

I think it’s more efficient to split on "\n" rather than /\n/ but not sure. Of course, if you use my alternative version you won’t need that.

> Tools/Scripts/prepare-ChangeLog:679
> +            $bugDescription =~ s/^\s*//g;

No need for the "g" here; this only applies once, not multiple times. Also, I would have used + rather than * myself.

[Emilio Cobos Álvarez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 314709 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314709&action=review
> 
> Seems fine.
> 
> Since non-perl programmers can’t read the code to strip off the first line
> anyway, I would consider doing it with a single regular expression like this:
> 
>     ($bugDescription, $description)
>         = ($description =~ /^(?:\s*(.*)\n)?(?:\s*\n)*(.*)/)
>         if !$bugDescription && $description;
> 
> Could write that all on one line, too, if you prefer. Note that my version
> strips multiple blank lines, not just one. Could use a "?" instead of a "*"
> to strip only one.

Multiple sounds fine (though I don't think it's common).

I had it to tweak it slightly so it grabbed the multiline description entirely and not only the first line of it, but your suggestion is much more compact :)

Thanks for the review!

[Emilio Cobos Álvarez]

Created attachment 315219 [details]
Patch

[Build Bot]

Comment on attachment 315219 [details]
Patch

Attachment 315219 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4106398

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open.html

[Build Bot]

Created attachment 315230 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[WebKit Commit Bot]

Comment on attachment 315219 [details]
Patch

Clearing flags on attachment: 315219

Committed r219406: <http://trac.webkit.org/changeset/219406>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.