361297 - invalid steps not handled correctly

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Robert Helmer [:rhelmer]]

Specifying an invalid step name to "release -o" (run only this step) or "release -s" (start at this step) currently has the same effect as not specifying a step. The correct behavior should be to print an error and exit.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: handle missing step properly

($stepNumber > scalar(@allSteps)) will never be true here.

Comment 2

[J. Paul Reed [:preed]]

Comment on attachment 246040 [details] [diff] [review]
handle missing step properly

My preference in areas such as these is to assert() validity early, and then when your preconditions are met, then do the necessary logic.

There's also the interesting bit that the loop will continue to run, even if the step has been found. Granted, this probably isn't too big a deal, but if there were a million steps, maybe it would be.

I'm also confused as to why the initialization of currentStep got moved.

I'd probably do something like (pseudocode):

if (!not grep(/step/, @validSteps) {
  print "Barf early.";
}

my $stepNumber = -1;

for($i = 0; $i < scalar(@steps); $i++) { # let the loop take care of the incrementing logic
  if (stepMatches) {
     $stepNumber = $i;
     last;
  }
}

if ($stepNumber == -1) {
   print "Ugh."
}

Comment 3

[Robert Helmer [:rhelmer]]

Comment on attachment 246040 [details] [diff] [review]
handle missing step properly

this is better in the latest version, but I like the assertion idea. I'll submit another one for this later.

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: fail early if the step name is not valid

Comment 5

[J. Paul Reed [:preed]]

Comment on attachment 259051 [details] [diff] [review]
fail early if the step name is not valid


>+        if (not grep(/$desiredStep/, @allSteps)) {

I think you want
  if not grep(/^$desiredStep$/, @allSteps)) {

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: address concerns from previous patch, also remove duplicate $currentStep declaration

Comment 7

[J. Paul Reed [:preed]]

Comment on attachment 259259 [details] [diff] [review]
address concerns from previous patch, also remove duplicate $currentStep declaration

>+            if ($allSteps[$i] eq "$desiredStep") {

Nit: $desiredStep does not need to be quoted.

Otherwise, looks ok.

Comment 8

[Robert Helmer [:rhelmer]]

Landed, with nit fixed, thanks!

Checking in release;
/cvsroot/mozilla/tools/release/release,v  <--  release
new revision: 1.8; previous revision: 1.7
done