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

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted 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 45747: Proposed patch 3
https://bugs.webkit.org/attachment.cgi?id=45747&action=review

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