[webkit-reviews] review denied: [Bug 33098] Refactor svn-apply and svn-unapply to use a common "patch" method : [Attachment 45737] Proposed patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Chris Jerdonek
<chris.jerdonek at gmail.com>'s request for review:
Bug 33098: Refactor svn-apply and svn-unapply to use a common "patch" method
https://bugs.webkit.org/show_bug.cgi?id=33098

Attachment 45737: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=45737&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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.