Bug 31137 – Adds a cursory check of the ChangeLog in svn-create-patch

• Status: NEW
• Component: Tools / Tests

[Eric Ayers]

I've submitted more than one patch where I fat fingered the ChangeLog file in one way or another.  I've updated my editor settings now, but its tedious and time consuming for committers to throw out patches for these nits that the commit hooks catch.

svn-create-patch may not be the ideal place to put these, as it won't be used by developers using git or svn directly, but the routine that does the checks is in the VCSUtils library and could be re-used.

[Eric Ayers]

Created attachment 42508 [details]
Adds some style checks of the ChangeLog file in svn-create-patch

This came out of some discussion during the commit of bug 31121

[Timothy Hatcher]

Comment on attachment 42508 [details]
Adds some style checks of the ChangeLog file in svn-create-patch


> +    if ($patch =~ /^\+\t/m) {
> +        push(@styleMessages, " - ChangeLog contains leading tab character.");

No tabs are allowed, leading or otherwise.


> +    if (!($patch =~ /^\+.*https:\/\/bugs.webkit.org\//m)) {
> +        push(@styleMessages, " - No bug URL found in ChangeLog entry.");

Another valid format is http://webkit.org/b/XXXXX. If we care.


> +            warn("* Problems found in ".$file.":\n" . join("\n", @styleMessages) . "\n");

Put spaces around the "."s or put $file directly in the string.

r- for the tab check being restrictive.

[Eric Ayers]

Created attachment 42519 [details]
Updates the tab test in changelog to match tabs anywhere, incorporates other review feedback.

[Eric Ayers]

(In reply to comment #2)
> (From update of attachment 42508 [details])
> 
> > +    if ($patch =~ /^\+\t/m) {
> > +        push(@styleMessages, " - ChangeLog contains leading tab character.");
> 
> No tabs are allowed, leading or otherwise.

Changed to spit out a warning on any tab char (after ignoring diff headers)

> 
> > +    if (!($patch =~ /^\+.*https:\/\/bugs.webkit.org\//m)) {
> > +        push(@styleMessages, " - No bug URL found in ChangeLog entry.");
> 
> Another valid format is http://webkit.org/b/XXXXX. If we care.

Done.
 
> > +            warn("* Problems found in ".$file.":\n" . join("\n", @styleMessages) . "\n");
> 
> Put spaces around the "."s or put $file directly in the string.

Done.

 
> r- for the tab check being restrictive.

[Mark Rowe (bdash)]

Why put this in svn-create-patch rather than check-webkit-style?  The latter is explicitly about checking for these sorts of style issues.  Doing it during patch creation is a little strange.

[Eric Seidel (no email)]

Mark is right.  This makes more sense as part of check-webkit-style or similar.  That said, It might make sense for svn-create-patch to automatically check the style (or at least accept a --check-style flag) by calling check-webkit-style.

[Eric Ayers]

My rationale for putting it in the patch creation is that at least one of these checks causes a review to be automatically thrown out by the commit hook on the server, and for my env, running svn-create-patch is mandatory and guaranteed to be done after the ChangeLog is updated.

I will work this up to run under webkit-check-style.  It would be nice if the svn-create-patch could feed the data to create-webkit-check without having to make the diff twice (takes a while on my box.)

[Eric Ayers]

(In reply to comment #7)
>...  It would be nice if the
> svn-create-patch could feed the data to create-webkit-check without having to
> make the diff twice (takes a while on my box.)

I meant, feed the diff data from svn-create-patch to check-webkit-style.