Summary: | VCSUtils::gitdiff2svndiff() should accept strings ending in a newline | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||||||||||
Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, aroben, commit-queue, darin, ddkilzer, eric, hamaji | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Chris Jerdonek
2010-01-08 18:41:55 PST
Created attachment 46190 [details]
Patch
Created attachment 46191 [details]
Patch 2
Added "-w" and "use strict" to existing test file (and fixed resulting warnings).
I tried to review this, but it blew my mind. 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. :) 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 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.) 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 on attachment 46199 [details]
Patch 4
r=me!
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 (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. (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 on attachment 46199 [details]
Patch 4
r- to resolve issues with \V found by the commit-queue.
Created attachment 46212 [details]
Patch 5
That's too bad. Changing to [^\r\n] seems best for now.
Comment on attachment 46212 [details]
Patch 5
r=me
Comment on attachment 46212 [details] Patch 5 Clearing flags on attachment: 46212 Committed r53040: <http://trac.webkit.org/changeset/53040> All reviewed patches have been landed. Closing bug. |