[webkit-reviews] review granted: [Bug 16142] Add --range option to run-sunspider : [Attachment 17528] partial fix (which actually runs)

[bugzilla-daemon at webkit.org]

Adam Roben <aroben at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 16142: Add --range option to run-sunspider
http://bugs.webkit.org/show_bug.cgi?id=16142

Attachment 17528: partial fix (which actually runs)
http://bugs.webkit.org/attachment.cgi?id=17528&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
 130 $resultsFile = $outputFilePath  if $outputFilePath;

Looks like you've got an extra space in there.

 128	 return gitBranch()  if isGit();

Ditto.

 151	     return split('\n', $revisionsString);
 152	 } elsif (isSVN()) {

 154		 return [$1];
 155	     } elsif ($range =~ /^r?([0-9]+):?r?([0-9]+|HEAD)$/i) {

No need to say "else" after a return.

 156		 return [$1..$2];

What does the range operator do if $1 or $2 is "HEAD"?

 61   --range		Get samples across the provided git range of builds

You shouldn't say "git" here.

 122 # Takes 0 or 1 args
 123 sub runUsingCurrentCheckout

You can still use a prototype:

sub runUsingCurrentCheckout(;$)

 179	     last  unless ($result == 0);

Extra space here as well.

It seems silly to do two chdirs for every test run (plus an extra one at the
start of the revision range). Can't we do the chdirs ahead of time and just do
them once?

"--range" makes me think of a range of times or ranges of error -- maybe
there's some more specific name we can give to this option? Maybe
--revision-range?

r=me