Re: [PATCH] gitweb: fix problem causing erroneous project list
On Wed, Jun 5, 2013 at 9:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Charles McGarvey <chazmcgarvey@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 <chazmcgarvey@brokenzipper.com>
Acked-by: Jakub Narebski <jnareb@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 prerequisite.
Show 8 quoted lines
> 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 patch.
Anyway in the analysis below we need to check if code checks $searchtext first.
Show 9 quoted lines
> * 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.
Show 20 quoted lines
> 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
Charles McGarvey
