Bug 13565 – Change svn-create-patch to put LayoutTests in the end

• Component: Tools / Tests
• Status: RESOLVED FIXED

It's arguably easier to review patches when the code changes come before the tests.

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

[David Kilzer (:ddkilzer)]

Created attachment 14633 [details]
Patch v1

[Adam Roben (:aroben)]

Comment on attachment 14633 [details]
Patch v1

+        In addition reordering non-binary files under the LayoutTests directory, this

Typo: "In addition" -> "In addition to"

+        $dir = File::Spec->catdir($dirs[0], ".svn") if -d $dirs[0];
+        $dir = File::Spec->catdir(dirname($dirs[0]), ".svn") if -f $dirs[0];

It would be nice not to repeat the catdir call -- can you put the directory name in a local and then call catdir?

+# Generate the diff for text files, layout files then binary files for easy reviewing

"layout files" is a confusing term (and "layoutFiles" is a similarly misleading variable name). Perhaps "test files"/"testFiles" would be more appropriate?

r=me

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> (From update of attachment 14633 [details] [edit])
> +        In addition reordering non-binary files under the LayoutTests
> directory, this
> 
> Typo: "In addition" -> "In addition to"

Thanks!

> +        $dir = File::Spec->catdir($dirs[0], ".svn") if -d $dirs[0];
> +        $dir = File::Spec->catdir(dirname($dirs[0]), ".svn") if -f $dirs[0];
> 
> It would be nice not to repeat the catdir call -- can you put the directory
> name in a local and then call catdir?

It won't because the 'if' part of the statement will be executed first in both lines, and a path can't be both a file and a directory.

Regardless, the code is confusing to read, so I'll change this anyway.  :)

> +# Generate the diff for text files, layout files then binary files for easy
> reviewing
> 
> "layout files" is a confusing term (and "layoutFiles" is a similarly misleading
> variable name). Perhaps "test files"/"testFiles" would be more appropriate?

"layoutTestsFiles"?  (Kidding!)  I'll change it to "test files"/"testFiles" and land.

[David Kilzer (:ddkilzer)]

$ svn commit WebKitTools/ChangeLog WebKitTools/Scripts/prepare-ChangeLog WebKitTools/Scripts/svn-create-patch       
Sending        WebKitTools/ChangeLog
Sending        WebKitTools/Scripts/prepare-ChangeLog
Sending        WebKitTools/Scripts/svn-create-patch
Transmitting file data ...
Committed revision 21608.