Bug 174551 – [Scripts] Make svn-create-patch work better when called in sub directories

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Sam Weinig]

[Scripts] Make svn-create-patch work better when called in sub directories

[Sam Weinig]

Created attachment 315573 [details]
Patch

[Sam Weinig]

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.

[Darin Adler]

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.

[Sam Weinig]

Committed revision 219540.

[Sam Weinig]

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