Right now the "quicksearch" subroutine is really long with a LOT of indentation. It should be broken up into a smaller series of subroutines.

This looks very involved, but really it's very simple. I just moved the code into subroutines. I tested it as I went and all the features still work. I changed only one behavior--the magic thing that happened where short_desc and whiteboard were added inside the "keywords" if block in _default_quicksearch_words only if they were...longer than two characters. That didn't make sense, so I just got rid of it, and always add short_desc and whiteboard.

The patch I attached is dependent upon the patch in bug 314364.

Comment on attachment 374976 [details] [diff] [review] v1 Since the dependent patch in bug 314364 got a r- it will most likely end up bitrotting this patch. Please, consider updating/doing this bug first. I'll promise a rapid review for this bug.

Fix bitrot.

Comment on attachment 387399 [details] [diff] [review] v2 Awesome, main sub is so short that it's even readable now. :) >--- Bugzilla/Search/Quicksearch.pm 8 Jul 2009 07:21:50 -0000 >@@ -125,41 +125,10 @@ Since there is only one use left for the $urlbase variable in quicksearch sub then it should be removed and correct_urlbase called directly while handling the load param. >+sub _bug_numbers_only { ... >+ my $cgi = Bugzilla->cgi; Since you don't create a $cgi variable in any other sub, why here? >+sub _handle_alias { ... >+ -uri => correct_urlbase() . "show_bug.cgi?id=$1"); You have $alias variable so please use it here. >+sub _handle_status_and_resolution { .. >+ # It's no alias either, so it's a more complex query. This comment doesn't make sense in the sub. Please, just remove it since you removed other such comments from the mainline code. >+sub _special_field_syntax { >+ # Priority >+ if (grep { lc($_) eq lc($word) } @$legal_priorities) { ... >+ } >+ # P1-5 Syntax >+ elsif ($word =~ m/^P(\d+)(?:-(\d+))?$/i) { ... >+ } Returns missing from both of these if blocks so you just broke priority special field syntax. And why latter is an elsif and not simply if like the others?

(In reply to comment #6) > >+sub _bug_numbers_only { > ... > >+ my $cgi = Bugzilla->cgi; > > Since you don't create a $cgi variable in any other sub, why here? Because I need it here? I fixed everything else--I'll attach a new patch now.

Okay, fixed all the issues.

(In reply to comment #7) > > Since you don't create a $cgi variable in any other sub, why here? > Because I need it here? You used "Bugzilla->cgi" syntax everywhere else (including previous if block of the same sub!) so this is different for no apparent reason.

(In reply to comment #9) > You used "Bugzilla->cgi" syntax everywhere else (including previous if block of > the same sub!) so this is different for no apparent reason. Oh, I see what you mean. I can do a "my $cgi" at the top of that sub when I check the patch in, if everything else is OK.

Comment on attachment 395139 [details] [diff] [review] v3 >Index: Bugzilla/Search/Quicksearch.pm >=================================================================== >+sub _handle_field_names { ... >+ if ($or_operand =~ /^votes:([0-9]+)$/) { >+ # votes:xx ("at least xx votes") ... >+ if ($or_operand =~ /^(?:flag:)?([^\?]+\?)([^\?]*)$/) { >+ # Flag and requestee shortcut ... >+ if ($or_operand =~ /^([^:]+):([^:]+)$/) { >+ # generic field1,field2,field3:value1,value2 notation Nit: I would have moved these comments before the "if" but maybe that's just me. :) >+sub _special_field_syntax { ... >+ my $legal_priorities = get_legal_field_values('priority'); >+ # Priority Nit: Put comment before variable assignment to syntax used later. (In reply to comment #10) > Oh, I see what you mean. I can do a "my $cgi" at the top of that sub when I > check the patch in, if everything else is OK. Yes, please do that. And don't forget to change redirect call in that sub accordingly.

Thanks, wicked! I fixed all your nits on checkin--they were all good points. :-) Checking in Bugzilla/Search/Quicksearch.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Quicksearch.pm,v <-- Quicksearch.pm new revision: 1.26; previous revision: 1.25 done