Bug 26999

Summary: bugzilla-tool/svn-apply can't handle patches made from a non-root directory
Product: WebKit Reporter: Eric Seidel <eric@webkit.org>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham@webkit.org, darin@apple.com, ddkilzer@webkit.org, joepeck@webkit.org, mrowe@apple.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
Attachments:
Description Flags
This would make it so svn-create-patch provides full paths from the WebKit root directory.
ddkilzer: review-
svn-create-patch provides full paths Subversion Repository's Root Directory
none
svn-create-patch provides full paths Subversion Repository's Root Directory ddkilzer: review+

Description From 2009-07-06 13:45:15 PST
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/
------- Comment #1 From 2009-07-08 09:00:33 PST -------
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?
------- Comment #2 From 2009-07-08 17:29:58 PST -------
Created an attachment (id=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.
------- Comment #3 From 2009-07-11 11:28:37 PST -------
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?
------- Comment #4 From 2009-07-11 11:29:06 PST -------
I would like an svn user, like Mark or Darin or someone to comment though.
------- Comment #5 From 2009-07-14 09:07:59 PST -------
(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.)

> +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.
------- Comment #6 From 2009-07-14 09:35:34 PST -------
(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.
------- Comment #7 From 2009-07-14 09:38:39 PST -------
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.
------- Comment #8 From 2009-07-14 10:06:24 PST -------
(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.
------- Comment #9 From 2009-07-14 15:55:27 PST -------
Created an attachment (id=32742) [details]
svn-create-patch provides full paths Subversion Repository's Root Directory

Much cleaner!  This addresses the points in David's review.
------- Comment #10 From 2009-07-14 15:55:27 PST -------
Created an attachment (id=32743) [details]
svn-create-patch provides full paths Subversion Repository's Root Directory

Much cleaner!  This addresses the points in David's review.
------- Comment #11 From 2009-07-14 16:03:19 PST -------
(In reply to comment #5)
> (From update of attachment 32491 [details] [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.
------- Comment #12 From 2009-07-14 18:30:06 PST -------
(From update of attachment 32743 [details])
(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!
------- Comment #13 From 2009-07-14 18:39:20 PST -------
> 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.
------- Comment #14 From 2009-07-15 13:11:25 PST -------
Landed in http://trac.webkit.org/changeset/45939.
------- Comment #15 From 2009-07-16 16:44:41 PST -------
(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.
------- Comment #16 From 2009-10-23 14:31:33 PST -------
*** Bug 28421 has been marked as a duplicate of this bug. ***