Bug 33415 – VCSUtils::gitdiff2svndiff() should accept strings ending in a newline

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

[Chris Jerdonek]

This will let us avoid having to use an $eol variable in the below code in svn-apply and svn-unapply.

while (<>) {
    s/([\n\r]+)$//mg;
    my $eol = $1;
    if (!defined($indexPath) && m#^diff --git \w/#) {
        $filter = \&gitdiff2svndiff;
    }
    ...
    $patch .= $_;
    $patch .= $eol;
}

[Chris Jerdonek]

Created attachment 46190 [details]
Patch

[Chris Jerdonek]

Created attachment 46191 [details]
Patch 2

Added "-w" and "use strict" to existing test file (and fixed resulting warnings).

[Adam Barth]

I tried to review this, but it blew my mind.

[David Kilzer (:ddkilzer)]

Comment on attachment 46191 [details]
Patch 2

> Index: WebKitTools/Scripts/VCSUtils.pm
> +# Convert a line of a git-formatted patch to SVN format, while
> +# preserving any end-of-line characters.
>  sub gitdiff2svndiff($)
>  {
>      $_ = shift @_;
> -    if (m#^diff --git \w/(.+) \w/(.+)#) {
> -        return "Index: $1";
> +    
> +    # \V is any character that is not vertical white space
> +    if (m#^diff --git \w/(.+) \w/(\V+)#) {
> +        return "Index: $1$'";

I find that $' is pretty obscure.  Might be nice to "use English;" then use $POSTMATCH instead of $'.

>      } elsif (m#^index [0-9a-f]{7}\.\.[0-9a-f]{7} [0-9]{6}#) {
> -        return "===================================================================";
> -    } elsif (m#^--- \w/(.+)#) {
> -        return "--- $1";
> -    } elsif (m#^\+\+\+ \w/(.+)#) {
> -        return "+++ $1";
> +        return "===================================================================$'";
> +    } elsif (m#^--- \w/(\V+)#) {
> +        return "--- $1$'";
> +    } elsif (m#^\+\+\+ \w/(\V+)#) {
> +        return "+++ $1$'";
>      }
>      return $_;
>  }

Technically we could just use if() statements instead of a large elsif() block since there are return statements, but it's okay as-is.

r=me

You can decide if you want to set cq+ or if you want to address either of the above issues.  :)

[Chris Jerdonek]

Created attachment 46196 [details]
Patch 3

Thanks for the comments, guys! :)

David, I incorporated your suggestions -- thanks a lot.

After reading http://perldoc.perl.org/perlre.html , it's not clear to me if I need to use the "preserve" modifier (/p) to use ${^POSTMATCH}.  The unit tests seem to work okay without it, so I decided to leave it off (though it's conceivable that it's working only because of previous regexes in the tests).

[David Kilzer (:ddkilzer)]

Comment on attachment 46196 [details]
Patch 3

> Index: WebKitTools/Scripts/VCSUtils.pm
> +    # \V is any character that is not vertical white space
> +    if (m#^diff --git \w/(.+) \w/(\V+)#) {
> +        return "Index: $1${^POSTMATCH}";
> +    }

Yes, you should use 'p' modifier with ${^POSTMATCH} and friends (per the perlre manpage).

To use $POSTMATCH instead of $', you need the "use English;" statement.

r- to either add the 'p' switches or go back to using $POSTMATCH with 'use English'.  (I'd go back to using $' or $POSTMATCH since I'm not sure everyone is running Perl 5.10 or newer yet.)

[Chris Jerdonek]

Created attachment 46199 [details]
Patch 4

Thanks for the clarification.  I thought you were just being creative -- it didn't occur to me that there was an actual module called "English".

[David Kilzer (:ddkilzer)]

Comment on attachment 46199 [details]
Patch 4

r=me!

[WebKit Commit Bot]

Comment on attachment 46199 [details]
Patch 4

Rejecting patch 46199 from commit-queue.

Failed to run "['WebKitTools/Scripts/test-webkitperl']" exit_code: 25
Last 500 characters of output:
/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 376.
Unrecognized escape \V passed through at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 379.
ok
Failed Test                       Stat Wstat Total Fail  Failed  List of Failed
-------------------------------------------------------------------------------
/Users/eseidel/Projects/CommitQue   14  3584    20   14  70.00%  1-3 7-12 16-20
Failed 1/4 test scripts, 75.00% okay. 14/41 subtests failed, 65.85% okay.

Full output: http://webkit-commit-queue.appspot.com/results/175393

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> (From update of attachment 46199 [details])
> Rejecting patch 46199 from commit-queue.
> 
> Unrecognized escape \V passed through at
> /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 379.

Hmm...is "\V" something new in Perl 5.10?  That would also be an issue for older Perls.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> Hmm...is "\V" something new in Perl 5.10?  That would also be an issue for
> older Perls.

Looks like that may be the case.  You may need to use [^\r\n] in place of \V in the patch.  I'm not sure if \V includes stuff like the form feed (FF) character or not, although we actually don't want that in this case.

[David Kilzer (:ddkilzer)]

Comment on attachment 46199 [details]
Patch 4

r- to resolve issues with \V found by the commit-queue.

[Chris Jerdonek]

Created attachment 46212 [details]
Patch 5

That's too bad.  Changing to [^\r\n] seems best for now.

[David Kilzer (:ddkilzer)]

Comment on attachment 46212 [details]
Patch 5

r=me

[WebKit Commit Bot]

Comment on attachment 46212 [details]
Patch 5

Clearing flags on attachment: 46212

Committed r53040: <http://trac.webkit.org/changeset/53040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.