[webkit-reviews] review denied: [Bug 26999] bugzilla-tool/svn-apply can't handle patches made from a non-root directory : [Attachment 32491] This would make it so svn-create-patch provides full paths from the WebKit root directory.

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Joseph Pecoraro
<joepeck02 at gmail.com>'s request for review:
Bug 26999: bugzilla-tool/svn-apply can't handle patches made from a non-root
directory
https://bugs.webkit.org/show_bug.cgi?id=26999

Attachment 32491: This would make it so svn-create-patch provides full paths
from the WebKit root directory.
https://bugs.webkit.org/attachment.cgi?id=32491&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> +use webkitdirs;

It's unfortunate that we need to use the webkitdirs module since this script
could be used on any SVN working directory previously.	(Darin Adler may have
more input on this.)

> +sub chdirWebKitAndGetPrefix;

This needs empty parenthesis after the declaration (an empty prototype) to
match the style of the rest of the source.

> +    my $file = $prefix . $fileData->{path};

This should use File::Spec->catdir($prefix, $fileData->{path}) instead.  Then
you don't have to worry about the trailing '/' in chdirWebKitAndGetPrefix().

> +# Change to WebKit dir and get the difference from the current dir

This comment isn't really needed; the method name explains what it does
already.

> +sub chdirWebKitAndGetPrefix

Needs parenthesis after the method to match the style of the rest of the
source.

> +{
> +    my $before = File::Spec->rel2abs( File::Spec->curdir() );
> +    chdirWebKit();
> +    my $after = File::Spec->rel2abs( File::Spec->curdir() );
> +    $before =~ s/^$after\/?//;

I think it would be clearer to use File::Spec::abs2rel() instead of rolling
your own regex here:

my $relativePath = File::Spec->abs2rel($after, $before);

> +    return $before if $before eq '';    # Already at WebKit dir
> +    $before .= '/' if $before !~ /\/$/; # Prefix needs a trailing '/'
> +    return $before;
> +}

With the change to use File::Spec::catdir() above, and because
File::Spec::abs2rel() will either return a relative path or an empty string,
this logic goes away:

    return $relativePath;

In fact, you should just remove the $relativePath variable and return the
result directly:

    return File::Spec->abs2rel($after, $before);

r- to fix everything except the 'use webkitdirs' issue.

IMO, it would be great to have a replacement subroutine for chdirwebkit() that
got the svn URL from the "svn info" command in the current directory, then did
a "chdir .." until the current svn URL no longer matched the previous svn URL
(minus one directory), then you could chdir back into the last directory.  This
would then put you at the root of the svn working directory regardless of
whether you're working on WebKit or not.