Bug 9350 – Use pathcmp() when sorting paths in svn-create-patch

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Per Bug 9322 Comment #2, Darin would like the sort() methods in svn-create-path to use pathcmp().

[David Kilzer (:ddkilzer)]

Created attachment 8764 [details]
Patch v1

- Moved pathcmp() and helper methods to WebKit::Util module.
- Created WebKitTools/Scripts/lib directory for modules.  (I hate *.pm files inside the Scripts directory.)
- Changed svn-create-patch to use pathcmp() subroutine.
- Various Perl clean-up in svn-create-patch (subroutine prototypes; added -h command-line switch).

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Created an attachment (id=8764) [edit]
> Patch v1

- Also rewrote splitpath() to use File::Basename subs basename() and dirname(), although I couldn't find a variable representing the per-platform path separator, e.g., something like $File::Spec::Separator.

[Darin Adler]

Comment on attachment 8764 [details]
Patch v1

I'm not entirely happy with the name "Util", with the idea of creating a "lib" subdirectory of the Scripts directory, or with moving these functions to a library. On the other hand, I see the appeal of sharing the code.

1) In general, I'm not at a fan of using abbreviations unless the full word is too long to be practical to use.

2) I don't like the name "Utilities". That's a catch-all term and it's hard to know what things are or are not utilities. For example, I'd prefer a name like Sorting or SortingFunctions for a library of functions for sorting.

3) Up until this point the only library we've had is webkitdirs. As I created scripts I went out of my way to keep each the number of scripts small, keep each script as small as possible and relatively independent; I hope this makes them relatively hackable.

Given all of that, I have mixed feelings about the cost/benefit of the patch, even though it was my suggestion to use the pathcmp sort function.

I guess I'll mark this review+ despite my reservations, but please consider my comments.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> I'm not entirely happy with the name "Util", with the idea of creating a "lib"
> subdirectory of the Scripts directory, or with moving these functions to a
> library. On the other hand, I see the appeal of sharing the code.

I usually try to avoid code duplication (except when it interferes with readability), hence the reason for creating a Perl module.  If you don't mind me copying the code into the svn-create-patch script, then I'll just do that instead.  (Had I know that sooner, it would have saved me a lot of work.  I almost wrote full "pod" documentation for all the functions. :)

> 3) Up until this point the only library we've had is webkitdirs. As I created
> scripts I went out of my way to keep each the number of scripts small, keep
> each script as small as possible and relatively independent; I hope this makes
> them relatively hackable.

Actually, there are two modules now:

$ ls *.pm
SpacingHeuristics.pm    webkitdirs.pm

I understand the desire to keep the scripts hackable, although I didn't know it at the time.  I've done some Perl development in the past, and in my world, modules should go into a library ('lib') directory.  That's a contradiction of the 'hackability' directive, though.

I'll just rework the patch tonight to copy the functions to svn-create-patch.

[David Kilzer (:ddkilzer)]

Created attachment 8782 [details]
Patch v2

New in Patch v2:

- Nothing from Patch v1 was kept except for the code clean-up in svn-create-patch (subroutine prototypes and addition of -h|--help switch and printUsage()).
- Modified splitpath() in run-webkit-tests to use File::Basename methods instead of regex.
- Copied numericcmp(), pathcmp() and splitpath() from run-webkit-tests to svn-create-path.
- Use pathcmp() on sort() functions in svn-create-path.

[David Kilzer (:ddkilzer)]

Created attachment 8785 [details]
Patch v3

Same as Patch v3 plus:

- Inlined printUsage() method which was only used once and was poorly named.
- Renamed $help variable to $showHelp.

[Geoffrey Garen]

Committed revision 14781.