Bug 174551 - [Scripts] Make svn-create-patch work better when called in sub directories
Summary: [Scripts] Make svn-create-patch work better when called in sub directories
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-15 17:43 PDT by Sam Weinig
Modified: 2017-07-17 10:22 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.26 KB, patch)
2017-07-15 17:47 PDT, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-07-15 17:43:27 PDT
[Scripts] Make svn-create-patch work better when called in sub directories
Comment 1 Sam Weinig 2017-07-15 17:47:05 PDT
Created attachment 315573 [details]
Patch
Comment 2 Sam Weinig 2017-07-15 17:52:24 PDT
Ugh, my changelog is terrible. The real bug this is fixing is that when you move/rename a file, and then use svn-create-patch to create a patch for it, you currently have to be at the root of the svn tree. This fixes things so you can once again do it from the subdirectory.
Comment 3 Darin Adler 2017-07-15 20:13:07 PDT
Comment on attachment 315573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315573&action=review

> Tools/Scripts/svn-create-patch:287
> +        # svn may output explanitory lines describing more detail about a file change

Spelling error here: explanatory

> Tools/Scripts/svn-create-patch:289
> +        next if $line =~ / +>/;

I think you want a ^ at the start of the regular expression here, before the space.

> Tools/Scripts/svn-create-patch:378
> +        print STDERR "Performing `svn diff -r 0:${sourceRevision} ${escapedSourceFile} | tail -n +5`\n" if $verbose;

I would not use backquotes in STDERR. That’s perl syntax to mean "execute this and turn the result into a string", but doesn’t seem right for the logging. Maybe just normal \" quotes?

> Tools/Scripts/svn-create-patch:380
>          print `svn diff -r 0:${sourceRevision} ${escapedSourceFile} | tail -n +5`;

It’s surprising that we use print combined with backquotes here. Kind of a peculiar way to run a command and let the output go to STDOUT as normal. I think there are better perl idioms for this.
Comment 4 Sam Weinig 2017-07-15 20:45:17 PDT
Committed revision 219540.
Comment 5 Sam Weinig 2017-07-15 20:46:27 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 315573 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315573&action=review
> 
> > Tools/Scripts/svn-create-patch:287
> > +        # svn may output explanitory lines describing more detail about a file change
> 
> Spelling error here: explanatory

Fixed.

> 
> > Tools/Scripts/svn-create-patch:289
> > +        next if $line =~ / +>/;
> 
> I think you want a ^ at the start of the regular expression here, before the
> space.

Yup. Fixed.

> 
> > Tools/Scripts/svn-create-patch:378
> > +        print STDERR "Performing `svn diff -r 0:${sourceRevision} ${escapedSourceFile} | tail -n +5`\n" if $verbose;
> 
> I would not use backquotes in STDERR. That’s perl syntax to mean "execute
> this and turn the result into a string", but doesn’t seem right for the
> logging. Maybe just normal \" quotes?

Switched to quotes.

> 
> > Tools/Scripts/svn-create-patch:380
> >          print `svn diff -r 0:${sourceRevision} ${escapedSourceFile} | tail -n +5`;
> 
> It’s surprising that we use print combined with backquotes here. Kind of a
> peculiar way to run a command and let the output go to STDOUT as normal. I
> think there are better perl idioms for this.

Switch to the more common idiom on the file of using open/close, and implemented tail with a loop and counter.

Thanks for the review.