Bug 33098 - Refactor svn-apply and svn-unapply to use a common "patch" method
Summary: Refactor svn-apply and svn-unapply to use a common "patch" method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-01 16:55 PST by Chris Jerdonek
Modified: 2010-01-04 21:00 PST (History)
7 users (show)

See Also:


Attachments
Proposed patch (8.29 KB, patch)
2010-01-01 17:12 PST, Chris Jerdonek
ddkilzer: review-
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 2 (12.25 KB, patch)
2010-01-02 01:15 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 3 (18.05 KB, patch)
2010-01-02 13:01 PST, Chris Jerdonek
ddkilzer: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 4 (18.02 KB, patch)
2010-01-03 11:01 PST, Chris Jerdonek
ddkilzer: review+
cjerdonek: commit-queue-
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-01 16:55:30 PST
This is partially in preparation for bug 27204.
Comment 1 Chris Jerdonek 2010-01-01 17:12:04 PST
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!
Comment 2 WebKit Review Bot 2010-01-01 17:15:02 PST
style-queue ran check-webkit-style on attachment 45737 [details] without any errors.
Comment 3 David Kilzer (:ddkilzer) 2010-01-01 18:25:48 PST
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.
Comment 4 Chris Jerdonek 2010-01-01 19:47:25 PST
(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.
Comment 5 Chris Jerdonek 2010-01-02 01:15:06 PST
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.
Comment 6 WebKit Review Bot 2010-01-02 01:36:26 PST
style-queue ran check-webkit-style on attachment 45738 [details] without any errors.
Comment 7 Chris Jerdonek 2010-01-02 09:05:16 PST
Comment on attachment 45738 [details]
Proposed patch 2

Working on more improvements.
Comment 8 David Kilzer (:ddkilzer) 2010-01-02 09:24:03 PST
(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.
Comment 9 Chris Jerdonek 2010-01-02 13:01:30 PST
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).
Comment 10 WebKit Review Bot 2010-01-02 13:06:25 PST
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
Comment 11 Chris Jerdonek 2010-01-02 13:22:46 PST
(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 12 Adam Barth 2010-01-02 13:28:02 PST
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.
Comment 13 Chris Jerdonek 2010-01-02 13:53:35 PST
(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.
Comment 14 Adam Barth 2010-01-02 14:08:11 PST
Oh, it's fine.  We'll just land the patch by hand.
Comment 15 David Kilzer (:ddkilzer) 2010-01-03 08:52:47 PST
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.
Comment 16 Chris Jerdonek 2010-01-03 10:25:50 PST
(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.
Comment 17 Chris Jerdonek 2010-01-03 10:27:28 PST
(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
Comment 18 Chris Jerdonek 2010-01-03 11:01:50 PST
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.
Comment 19 WebKit Review Bot 2010-01-03 11:05:36 PST
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 20 David Kilzer (:ddkilzer) 2010-01-03 11:50:42 PST
Comment on attachment 45756 [details]
Proposed patch 4

r=me

I'll submit this via svn.
Comment 21 David Kilzer (:ddkilzer) 2010-01-03 13:26:28 PST
(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>
Comment 22 Chris Jerdonek 2010-01-04 20:15:19 PST
(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).
Comment 23 David Kilzer (:ddkilzer) 2010-01-04 21:00:10 PST
(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.