Bug 33415 - VCSUtils::gitdiff2svndiff() should accept strings ending in a newline
Summary: VCSUtils::gitdiff2svndiff() should accept strings ending in a newline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-08 18:41 PST by Chris Jerdonek
Modified: 2010-01-09 14:51 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.91 KB, patch)
2010-01-08 19:11 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Patch 2 (4.41 KB, patch)
2010-01-08 19:21 PST, Chris Jerdonek
ddkilzer: review+
Details | Formatted Diff | Diff
Patch 3 (4.50 KB, patch)
2010-01-08 21:10 PST, Chris Jerdonek
ddkilzer: review-
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
Patch 4 (4.71 KB, patch)
2010-01-08 23:22 PST, Chris Jerdonek
ddkilzer: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch 5 (4.72 KB, patch)
2010-01-09 11:14 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-08 18:41:55 PST
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;
}
Comment 1 Chris Jerdonek 2010-01-08 19:11:50 PST
Created attachment 46190 [details]
Patch
Comment 2 Chris Jerdonek 2010-01-08 19:21:48 PST
Created attachment 46191 [details]
Patch 2

Added "-w" and "use strict" to existing test file (and fixed resulting warnings).
Comment 3 Adam Barth 2010-01-08 20:16:51 PST
I tried to review this, but it blew my mind.
Comment 4 David Kilzer (:ddkilzer) 2010-01-08 20:21:45 PST
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.  :)
Comment 5 Chris Jerdonek 2010-01-08 21:10:36 PST
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).
Comment 6 David Kilzer (:ddkilzer) 2010-01-08 22:39:15 PST
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.)
Comment 7 Chris Jerdonek 2010-01-08 23:22:35 PST
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".
Comment 8 David Kilzer (:ddkilzer) 2010-01-09 08:28:47 PST
Comment on attachment 46199 [details]
Patch 4

r=me!
Comment 9 WebKit Commit Bot 2010-01-09 08:43:56 PST
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
Comment 10 David Kilzer (:ddkilzer) 2010-01-09 09:12:46 PST
(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.
Comment 11 David Kilzer (:ddkilzer) 2010-01-09 09:16:50 PST
(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.
Comment 12 David Kilzer (:ddkilzer) 2010-01-09 09:17:26 PST
Comment on attachment 46199 [details]
Patch 4

r- to resolve issues with \V found by the commit-queue.
Comment 13 Chris Jerdonek 2010-01-09 11:14:17 PST
Created attachment 46212 [details]
Patch 5

That's too bad.  Changing to [^\r\n] seems best for now.
Comment 14 David Kilzer (:ddkilzer) 2010-01-09 14:26:53 PST
Comment on attachment 46212 [details]
Patch 5

r=me
Comment 15 WebKit Commit Bot 2010-01-09 14:51:02 PST
Comment on attachment 46212 [details]
Patch 5

Clearing flags on attachment: 46212

Committed r53040: <http://trac.webkit.org/changeset/53040>
Comment 16 WebKit Commit Bot 2010-01-09 14:51:17 PST
All reviewed patches have been landed.  Closing bug.