On Wed, Jun 5, 2013 at 9:17 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Charles McGarvey <chazmcgar...@brokenzipper.com> writes:
>> The bug is manifest when running gitweb in a persistent process (e.g.
>> FastCGI, PSGI), and it's easy to reproduce.  If a gitweb request
>> includes the searchtext parameter (i.e. s), subsequent requests using
>> the project_list action--which is the default action--and without
>> a searchtext parameter will be filtered by the searchtext value of the
>> first request.  This is because the value of the $search_regexp global
>> (the value of which is based on the searchtext parameter) is currently
>> being persisted between requests.
>> Instead, clear $search_regexp before dispatching each request.
>> Signed-off-by: Charles McGarvey <chazmcgar...@brokenzipper.com>

Acked-by: Jakub Narebski <jna...@gmail.com>

>> ---
>> I don't think there are currently any persistent-process gitweb tests to
>> copy from, so writing a test for this seems to be non-trivial.

Well, there is Plack::Test, and gitweb supports running as PSGI app.
Though all such test would have to be under new PLACK or PSGI

> The problem description makes sense to me, and clearing with "undef"
> is in line with the case where the CGI is run for the first time, so
> I think this patch is correct.
> Will wait for a while to give Jakub some time to comment on (like:
> Ack ;-) and then apply.  Thanks.
> By the way, I looked at how $search_regexp is used in the code:

How $search_regexp is used does not matter. What was intended
(but was not implemented) is for $search_regexp to matter and to
be used only if $searchtext is defined.  $searchtext is reset on each
request, so $search_regexp should be also reset... like in Charles's

Anyway in the analysis below we need to check if code checks
$searchtext first.

>  * esc_html_match_hl and esc_html_match_hl__chopped (called from
>    git_project_list_rows, for example) want to have "undef"; an
>    empty string will not do.
>  * search_projects_list (called from git_project_list_body)
>  x git_search_files and git_search_grep_body assume that
>    $search_regexp can be interpolated in m//, which is not very
>    nice.  They want an empty string.

But both git_search_files() and git_search_grep_body() are run from
git_search(), which "dies" (returns HTTP 400 "Text field is empty" error)
if $searchtext is not defined; if $searchtext is defined then $search_regexp
is string and is never undef.

> So as an independent fix, the two subs may want to be fixed if we
> want to be undef clean.  Or am I missing something?  Jakub, isn't
> this kind of "undef" reference t9500 was designed to catch?
>>  gitweb/gitweb.perl | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 80950c0..8d69ada 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -1086,7 +1086,7 @@ sub evaluate_and_validate_params {
>>       our $search_use_regexp = $input_params{'search_use_regexp'};
>>       our $searchtext = $input_params{'searchtext'};
>> -     our $search_regexp;
>> +     our $search_regexp = undef;
>>       if (defined $searchtext) {
>>               if (length($searchtext) < 2) {
>>                       die_error(403, "At least two characters are required 
>> for search parameter");

Jakub Narebski
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to