Bug 26999 – bugzilla-tool/svn-apply can't handle patches made from a non-root directory

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

bugzilla-tool/svn-apply can't handle patches made from a non-root directory

We need to make some part of this scripting smarter so that it can figure out where the patch was made from instead of failing when people attach patches made in just WebCore/ or JavaScriptCore/

[Joseph Pecoraro]

Should this be handled on the patch generation side? By changing WebKit/WebKitTools/Scripts/svn-create-patch?

Calculate the path difference between the current working directory and the WebKit root (substring of the pwd).  Run generateFileList() as normal from the pwd.  Now you have the list of all files changed from the pwd (current behavior) but you can change directory to the WebKit root and run the diffs from the root (for `svn diff`).

Or should this be handled on the applying side?

[Joseph Pecoraro]

Created attachment 32491 [details]
This would make it so svn-create-patch provides full paths from the WebKit root directory.

This should keep the current svn-create-patch behavior, but just make it so that the paths for the files are from the WebKit root directory.  This would be on the create patch side, not on the apply patch side.  I don't know how common it is to get patches in a form other then one produced by svn-create-patch.  If its better to handle the apply side then I'm perfectly fine with this getting r-.  I tested this a number of different ways on OS X, I haven't tried Windows.

[Eric Seidel (no email)]

These seems like an OK idea to me.

# Change to WebKit dir and get the difference from the current dir
 458 sub chdirWebKitAndGetPrefix
 459 {
 460     my $before = File::Spec->rel2abs( File::Spec->curdir() );
 461     chdirWebKit();
 462     my $after = File::Spec->rel2abs( File::Spec->curdir() );
 463     $before =~ s/^$after\/?//;
 464     return $before if $before eq '';    # Already at WebKit dir
 465     $before .= '/' if $before !~ /\/$/; # Prefix needs a trailing '/'
 466     return $before;
 467 }

seems a little complicated and hard to read...  Shouldn't we just have a command in wkdirs.pm like:
git rev-parse --show-prefix
?

Which is then separate from the chdirWebKit command?

[Eric Seidel (no email)]

I would like an svn user, like Mark or Darin or someone to comment though.

[David Kilzer (:ddkilzer)]

Comment on attachment 32491 [details]
This would make it so svn-create-patch provides full paths from the WebKit root directory.

> +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.

[Darin Adler]

(In reply to comment #5)
> 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.)

Exactly. This was originally intended to be a Subversion script, not a WebKit-project-specific script.

> 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.

I agree.

[Joseph Pecoraro]

David, thanks for the through review.  My inexperience with Perl is apparent. =)

However, is it worth cleaning this up?  I still haven't gotten an answer wether or not it makes more sense to just fix the "svn-apply" side instead.  I am not a committer so I don't know if even making this modification to svn-create-patch will stop enough of these problems, or end up just fixing a few.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> David, thanks for the through review.  My inexperience with Perl is apparent.
> =)

Your Perl is fine.  We're just picky.  :)

> However, is it worth cleaning this up?  I still haven't gotten an answer wether
> or not it makes more sense to just fix the "svn-apply" side instead.  I am not
> a committer so I don't know if even making this modification to
> svn-create-patch will stop enough of these problems, or end up just fixing a
> few.

It's a step forward, which is what's important.  If there are still issues with svn-apply, we can address those after this fix lands.

[Joseph Pecoraro]

Created attachment 32742 [details]
svn-create-patch provides full paths Subversion Repository's Root Directory

Much cleaner!  This addresses the points in David's review.

[Joseph Pecoraro]

Created attachment 32743 [details]
svn-create-patch provides full paths Subversion Repository's Root Directory

Much cleaner!  This addresses the points in David's review.

[Joseph Pecoraro]

(In reply to comment #5)
> (From update of attachment 32491 [details])
> > +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.)

This removes the dependency on "webkitdirs" which removes a few other includes that this had introduced.


> > +sub chdirWebKitAndGetPrefix;
> 
> This needs empty parenthesis after the declaration (an empty prototype) to
> match the style of the rest of the source.

Done. This and all other style changes.


> > +    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().

Done. Much cleaner.


> > +{
> > +    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);

Done. Much cleaner.


> 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.

The way I went about it was using "svn info [PATH]".  And building up that path as a trail of "../".  I also use system() to check the error code of svn info.  Let me know if there is a problem with this approach (cross platform?) or if you think of a better way and I'll see what I can do.

[David Kilzer (:ddkilzer)]

Comment on attachment 32743 [details]
svn-create-patch provides full paths Subversion Repository's Root Directory

(In reply to comment #11)
> (In reply to comment #5)
> > 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.
> 
> The way I went about it was using "svn info [PATH]".  And building up that path
> as a trail of "../".  I also use system() to check the error code of svn info. 
> Let me know if there is a problem with this approach (cross platform?) or if
> you think of a better way and I'll see what I can do.

I thought of doing that after my last comment as well!  The only situation where this won't work is if a user has a "mixed" svn working directory where they have one svn working directory at "dir" and a second svn working directory (from a different repository) under "dir", and the user ran svn-create-patch in the second working directory.  I don't know how prevalent this use case is, and I doubt it's common with WebKit, so let's not worry about that for now.

>+sub chdirAndGetDifference($);

Nit: Instead of "GetDifference", I think something like "ReturnRelativePath" might be more descriptive.  I can change this when I land the patch.

r=me!

[Joseph Pecoraro]

> The only situation where this won't work is [nested svn working dirs]
> I don't know how prevalent this use case is, and I doubt it's common
> with WebKit, so let's not worry about that for now.

I thought about this too.  But I don't even think that is realistic or would work properly.  My only guess would be the actual Subversion team testing Subversion =P.  But maybe it is possible and you're approach from before (checking the URL from svn info and using that) would be more appropriate.


> >+sub chdirAndGetDifference($);
> 
> Nit: Instead of "GetDifference", I think something like "ReturnRelativePath"
> might be more descriptive.  I can change this when I land the patch.

Sounds good.

Thanks for the review! Cheers.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45939.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> I thought of doing that after my last comment as well!  The only situation
> where this won't work is if a user has a "mixed" svn working directory where
> they have one svn working directory at "dir" and a second svn working directory
> (from a different repository) under "dir", and the user ran svn-create-patch in
> the second working directory.  I don't know how prevalent this use case is, and
> I doubt it's common with WebKit, so let's not worry about that for now.

Oops.  See Bug 27323 Comment #6.

[Eric Seidel (no email)]

*** Bug 28421 has been marked as a duplicate of this bug. ***