Bug 33098

Created attachment 45737 [details] Proposed patch Let me know if there's a better way to suppress STDERR in the added unit tests than what I've done. Thanks!

style-queue ran check-webkit-style on attachment 45737 [details] without any errors.

Comment on attachment 45737 [details] Proposed patch Overall this looks great! I like that you're getting rid of the $pathScriptWasRunFrom global variable from svn-apply and svn-unapply. > Index: WebKitTools/Scripts/VCSUtils.pm > +sub runPatchCommand($$$;$) > +{ > + my ($patch, $repositoryRootPath, $pathRelativeToRoot, $options) = @_; > + > + $options = [] if (! $options); > + > + my $patchCommand = "patch " . join(" ", "-p0", @{$options}); > + my $exitCode; > + > + # Temporarily change the working directory since the path found > + # in the patch's "Index:" line is relative to the repository root > + # (i.e. the same as $pathRelativeToRoot). > + my $cwd = Cwd::getcwd(); > + chdir $repositoryRootPath; > + open PATCH, "| $patchCommand" or die "Failed to patch file \"$pathRelativeToRoot\": $!"; > + print PATCH $patch; > + close PATCH; > + chdir $cwd; > + > + $exitCode = $? >> 8; Setting $exitCode should really go *before* the chdir line. Also, $exitCode should really use exitStatus() here since it's more cross-platform compatible. Note that we don't want to introduce a dependency on webkitdirs.pm (where exitStatus() is defined) from VCSUtils.pm. (As a temporary measure, we could define exitStatus() in webkitdirs.pm outside the 'package' statement so the code may be shared and we can remove the ugly main::exitStatus() hack currently in VCSUtils.pm.) > + return ($patchCommand, $exitCode); > +} > Index: WebKitTools/Scripts/VCSUtils_unittest.pl > =================================================================== > --- WebKitTools/Scripts/VCSUtils_unittest.pl (revision 52685) > +++ WebKitTools/Scripts/VCSUtils_unittest.pl (working copy) > @@ -30,10 +30,24 @@ > > # Unit tests of VCSUtils.pm. > > -use Test::Simple tests => 7; > +use Test::Simple tests => 9; > > use VCSUtils; > > +# Call a function while suppressing STDERR. > +sub callSilently($@) { > + my ($func, @args) = @_; > + > + open(OLDERR, ">&STDOUT"); > + open(STDERR, ">/dev/null"); > + my @return_value = &$func(@args); > + close(STDERR); > + open(STDERR, ">&OLDERR"); > + close(OLDERR); > + > + return @return_value; > +} Was this taken from the Perl Recipes book? I'm not very familiar with this pattern for reassigning STDERR, and I'm not sure it would work on Windows or not. This should work on Windows so we can run unit tests there. > @@ -292,3 +306,22 @@ END > > ok(fixChangeLogPatch($in) eq $out, $title); > > +# Test: runPatchCommand > + > +# Since $patch has no "Index:" path, passing this to runPatchCommand > +# should not affect any files. > +my $patch = <<'END'; > +Garbage patch contents > +END > + > +# We call silently to avoid the following output to STDERR: > +# patch: **** Only garbage was found in the patch input. > +my ($patchCommand, $exitCode) = callSilently(\&runPatchCommand, $patch, ".", "file_to_patch.txt", ["--force", "-c"]); > + > +$title = "runPatchCommand: Pass arguments."; > + > +ok($patchCommand eq "patch -p0 --force -c", $title); > + > +$title = "runPatchCommand: Bad patch."; > + > +ok($exitCode == 2, $title); It would be nice to test successful patching as well (both applying and unapplying), but I guess it's not necessary. > Index: WebKitTools/Scripts/svn-unapply > @@ -258,16 +257,26 @@ sub revertDirectories() > } > } > > +# Unapply the given patch string. > +# > +# Args: > +# $patch: a patch string. > +# $pathRelativeToRoot: the path of the file to be patched, relative to the > +# repository root. This should normally be the path > +# found in the patch's "Index:" line. > +# $options: a reference to an array of options to pass to the patch command. > sub unapplyPatch($$;$) > { > - my ($patch, $fullPath, $options) = @_; > - chdir $pathForRepositoryRoot; > - $options = [] if (! $options); > - my $command = "patch " . join(" ", "-p0", "-R", @{$options}); > - open PATCH, "| $command" or die "Failed to patch $fullPath: $!"; > - print PATCH $patch; > - close PATCH; > - chdir $pathScriptWasRunFrom; > + my ($patch, $pathRelativeToRoot, $options) = @_; > + > + if (! $options) { > + $options = []; > + } else { > + $options = [@{$options}]; # Copy to avoid side effects. > + } > + > + push @{$options}, "--reverse"; > + runPatchCommand($patch, $repositoryRootPath, $pathRelativeToRoot, $options); > } The unapplyPatch() subroutine should really do the same check of $exitStatus and $force that applyPatch() in svn-apply does. It's not strictly necessary to fix this, but since you're moving common functionality into a new method, it would be nice to pick this up as well if the logic can be shared. r- for getting $exitCode from exitStatus() in VCSUtils::applyPatch(), making sure the STDERR redirection works on Windows, and looking into sharing the --force code in unapplyPath() for svn-unapply from applyPatch() in svn-apply.

(In reply to comment #3) Thanks a lot for the rapid and thorough review, David. Once again, great feedback! > (From update of attachment 45737 [details]) > Also, $exitCode should really use exitStatus() here since it's more > cross-platform compatible. Note that we don't want to introduce a dependency > on webkitdirs.pm (where exitStatus() is defined) from VCSUtils.pm. (As a > temporary measure, we could define exitStatus() in webkitdirs.pm outside the > 'package' statement so the code may be shared and we can remove the ugly > main::exitStatus() hack currently in VCSUtils.pm.) What's the better solution -- create another file? I don't have any thoughts yet on what such a file should be called. I wouldn't mind creating a "perl" folder so we don't have to keep putting supporting files in the top level (e.g. VCSUtils_unittest.pl) -- similar to the way we have such a location for our Python code. > > +sub callSilently($@) { > > + my ($func, @args) = @_; > > + > > + open(OLDERR, ">&STDOUT"); > > + open(STDERR, ">/dev/null"); > > + my @return_value = &$func(@args); > > + close(STDERR); > > + open(STDERR, ">&OLDERR"); > > + close(OLDERR); > > + > > + return @return_value; > > +} > > Was this taken from the Perl Recipes book? I'm not very familiar with this > pattern for reassigning STDERR, and I'm not sure it would work on Windows or > not. This should work on Windows so we can run unit tests there. I got the idea from here: http://www.sdsc.edu/~moreland/courses/IntroPerl/docs/manual/pod/perlfunc/open.html though here is a better link: http://perldoc.perl.org/functions/open.html (search for "open OLDERR") It seems the final "close" is superfluous, but I'm not sure yet why. Do you know who might be good to ask to run on Windows? > > +$title = "runPatchCommand: Bad patch."; > > + > > +ok($exitCode == 2, $title); > > It would be nice to test successful patching as well (both applying and > unapplying), but I guess it's not necessary. Yes, it wouldn't increase the code coverage in this instance, but it would be nice. It would take a bit more setup though. I'm not sure yet if there's a way to do it without creating temporary files. Are there any guidelines for creating temporary files during unit tests, like where to put them and what to name them? > The unapplyPatch() subroutine should really do the same check of $exitStatus > and $force that applyPatch() in svn-apply does. It's not strictly necessary to > fix this, but since you're moving common functionality into a new method, it > would be nice to pick this up as well if the logic can be shared. I'm glad you mentioned this. I was wondering that myself, and was going to ask. I thought perhaps there was a rationale behind it -- that unapplying should be allowed to continue since it was being used in clean-up situations that svn-apply wasn't. Browsing through svn-apply and svn-unapply, there seem to be a lot of such "near symmetries". I started with the easiest one. I will look for more such opportunities as I work on bug 27204.

Created attachment 45738 [details] Proposed patch 2 Except for the unit tests on Windows, I believe this addresses what you wanted me to address. Of course, it's also possible that the changes introduced new things that need to be addressed. Regarding exitStatus(), it turns out that webkitdirs.pm already references VCSUtils.pm (I hadn't known that when I responded to you before). So I was simply able to move the implementation of exitStatus() from webkitdirs.pm to VCSUtils.pm, and leave behind a subroutine declaration inside webkitdirs.

style-queue ran check-webkit-style on attachment 45738 [details] without any errors.

Comment on attachment 45738 [details] Proposed patch 2 Working on more improvements.

(In reply to comment #5) > Except for the unit tests on Windows, I believe this addresses what you wanted > me to address. Of course, it's also possible that the changes introduced new > things that need to be addressed. Since you got the file handle code from a Perl manpage, I suspect it's already cross-platform. Let's worry about this if it's shown to be an issue later.

Created attachment 45747 [details] Proposed patch 3 Lots more changes here for the better (many relating to your suggestions), including-- - Broke the code to assemble the patch command syntax out into a separate subroutine for cleaner testing, and added a bunch of unit tests. - Broke out the optional arguments to runPatchCommand() into a hash reference (named parameters) since the argument list was getting longer. - Moved more code from applyPatch/unapplyPatch into runPatchCommand(). - Removed from the unit tests the reference to /dev/null, so the bad patch test is more likely to work on Windows now. - Added unit tests to test successful patches (both silent and "real"). - svn-unapply now returns a non-zero exit status if one of the patches failed (it didn't before, since it was effectively always in --force mode).

Attachment 45747 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/Scripts/VCSUtils_unittest.pl:390: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils_unittest.pl:391: Line contains tab character. [whitespace/tab] [5] Total errors found: 2

(In reply to comment #10) > Attachment 45747 [details] did not pass style-queue: > > Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 > WebKitTools/Scripts/VCSUtils_unittest.pl:390: Line contains tab character. > [whitespace/tab] [5] > WebKitTools/Scripts/VCSUtils_unittest.pl:391: Line contains tab character. > [whitespace/tab] [5] > Total errors found: 2 These tabs appear in a patch string generated by "patch" and used as input to some unit tests: +$patch = <<END; +Index: $fileToPatch +=================================================================== +--- $fileToPatch (revision 0) ++++ $fileToPatch (revision 0) +@@ -0,0 +1,5 @@ The patch files generated by patch include tabs. Perhaps the style checker should skip any files ending in _unittest.* since unit tests can test a wide variety of inputs. Or alternatively, the unit test file could include some sort of marker, dilineators, etc. that the style checker could recognize -- as has been suggested before.

Comment on attachment 45747 [details] Proposed patch 3 That might be a good idea. We won't be able to use commit-queue to land the patch because the file needs the magical allow-tabs SVN property.

(In reply to comment #12) > (From update of attachment 45747 [details]) > We won't be able to use commit-queue to land the patch because the file needs > the magical allow-tabs SVN property. Could we also set the property in advance, and then use cq? Otherwise, it sounds like I will need to modify this patch with the property so they can both go in together.

Oh, it's fine. We'll just land the patch by hand.

Comment on attachment 45747 [details] Proposed patch 3 Looks great! Just a couple of nits/comments below: > Index: WebKitTools/Scripts/VCSUtils.pm > + # Merges hash references. It's okay here if passed hash reference is undefined. > + @{$argsHashRef}{keys %{$passedArgsHashRef}} = values %{$passedArgsHashRef}; Wow, I've never seen this syntax before! Is it known to work for more than a trivial number of hash entries? > + my $patchCommand = "patch " . join(" ", "-p0", @{$options}); Nit: You could include "patch" in the join which may read a little nicer: my $patchCommand = join(" ", "patch", "-p0", @{$options}); > Index: WebKitTools/Scripts/VCSUtils_unittest.pl > @@ -30,10 +30,23 @@ > > # Unit tests of VCSUtils.pm. > > -use Test::Simple tests => 7; > +use Test::Simple tests => 21; The tests are great, but I think putting every test in a single source file is going to get out of hand soon. We should consider alternatives like putting each individual test in its own file in a t/ subdirectory. > Index: WebKitTools/Scripts/svn-unapply > @@ -77,13 +78,21 @@ sub unapplyPatch($$;$); > sub unsetChangeLogDate($$); > > my $showHelp = 0; > -if (!GetOptions("help!" => \$showHelp) || $showHelp) { > - print STDERR basename($0) . " [-h|--help] patch1 [patch2 ...]\n"; > +my $force = 0; Nit: $force should be declared before $showHelp. > +my $optionParseSuccess = GetOptions( > + "help!" => \$showHelp, > + "force!" => \$force > +); Nit: 'force' should be before 'help!'. > Index: WebKitTools/Scripts/webkitdirs.pm > @@ -34,6 +34,7 @@ use FindBin; > use File::Basename; > use File::Spec; > use POSIX; > + > use VCSUtils; Nit: We usually don't put a blank line here. r=me Note that since the test file requires the 'allow-tabs' svn property, so this can't be landed via git or the commit-queue.

(In reply to comment #15) > > Index: WebKitTools/Scripts/VCSUtils.pm > > + # Merges hash references. It's okay here if passed hash reference is undefined. > > + @{$argsHashRef}{keys %{$passedArgsHashRef}} = values %{$passedArgsHashRef}; > > Wow, I've never seen this syntax before! Is it known to work for more than a > trivial number of hash entries? I came across a couple pages that say it's very efficient, but I don't really know. This page says it "requires enough memory for lists of the keys and values of [the right-hand hash]", so perhaps memory is the limitation. > > > + my $patchCommand = "patch " . join(" ", "-p0", @{$options}); > > Nit: You could include "patch" in the join which may read a little nicer: > > my $patchCommand = join(" ", "patch", "-p0", @{$options}); Or even-- :) > my $patchCommand = join(" ", "patch -p0", @{$options}); Thanks again.

(In reply to comment #16) > (In reply to comment #15) > > > Index: WebKitTools/Scripts/VCSUtils.pm > > > + # Merges hash references. It's okay here if passed hash reference is undefined. > > > + @{$argsHashRef}{keys %{$passedArgsHashRef}} = values %{$passedArgsHashRef}; > > > > Wow, I've never seen this syntax before! Is it known to work for more than a > > trivial number of hash entries? > > I came across a couple pages that say it's very efficient, but I don't really > know. This page says it "requires enough memory for lists of the keys and > values of [the right-hand hash]", so perhaps memory is the limitation. http://docstore.mik.ua/orelly/perl/cookbook/ch05_11.htm

Created attachment 45756 [details] Proposed patch 4 Incorporated David's final suggested tweaks. Could one of you commit this for me (including the required SVN tab property)? Thanks a lot.

Attachment 45756 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/Scripts/VCSUtils_unittest.pl:390: Line contains tab character. [whitespace/tab] [5] WebKitTools/Scripts/VCSUtils_unittest.pl:391: Line contains tab character. [whitespace/tab] [5] Total errors found: 2

Comment on attachment 45756 [details] Proposed patch 4 r=me I'll submit this via svn.

(In reply to comment #20) > I'll submit this via svn. $ svn commit WebKitTools Sending WebKitTools/ChangeLog Sending WebKitTools/Scripts/VCSUtils.pm Sending WebKitTools/Scripts/VCSUtils_unittest.pl Sending WebKitTools/Scripts/svn-apply Sending WebKitTools/Scripts/svn-unapply Sending WebKitTools/Scripts/webkitdirs.pm Transmitting file data ...... Committed revision 52692. <http://trac.webkit.org/changeset/52692>

(In reply to comment #15) > > Index: WebKitTools/Scripts/svn-unapply > > @@ -77,13 +78,21 @@ sub unapplyPatch($$;$); > > sub unsetChangeLogDate($$); > > > > my $showHelp = 0; > > -if (!GetOptions("help!" => \$showHelp) || $showHelp) { > > - print STDERR basename($0) . " [-h|--help] patch1 [patch2 ...]\n"; > > +my $force = 0; > > Nit: $force should be declared before $showHelp. > > > +my $optionParseSuccess = GetOptions( > > + "help!" => \$showHelp, > > + "force!" => \$force > > +); > > Nit: 'force' should be before 'help!'. I meant to ask -- what was the underlying rule or reason for these suggestions? For example, is it to alphabetize all consecutive variable declarations, and alphabetize all hard-coded hashes by key name? If so, one consequence for code like the above is that the variable declarations could be in a different order from their corresponding occurrence in the parameter listing, which would make it more difficult to visually check a one-to-one correspondence (e.g. if $showHelp were called $displayHelp).

(In reply to comment #22) > > Nit: 'force' should be before 'help!'. > > I meant to ask -- what was the underlying rule or reason for these suggestions? > For example, is it to alphabetize all consecutive variable declarations, and > alphabetize all hard-coded hashes by key name? Yes, to keep them alphabetized. > If so, one consequence for code like the above is that the variable > declarations could be in a different order from their corresponding occurrence > in the parameter listing, which would make it more difficult to visually check > a one-to-one correspondence (e.g. if $showHelp were called $displayHelp). Or simply $help. I guess I haven't been too concerned about keeping the variables in the same order as the options--just keeping the variables alphabetized and the options alphabetized for easier searching.