[PATCH (BUGFIX)] gitweb: Handle invalid regexp in regexp search

[PATCH (BUGFIX)] gitweb: Handle invalid regexp in regexp search

[Jakub Narebski]

When using regexp search ('sr' parameter / $search_use_regexp variable
is true), check first that regexp is valid.

Without this patch we would get an error from Perl during search (if
searching is performed by gitweb), or highlighting matches substring
(if applicable), if user provided invalid regexp... which means broken
HTML, with error page (including HTTP headers) generated after gitweb
already produced some output.

Add test that illustrates such error: for example for regexp "*\.git"
we would get the following error:

  Quantifier follows nothing in regex; marked by <-- HERE in m/* <-- HERE \.git/
  at /var/www/cgi-bin/gitweb.cgi line 3084.

Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
See "Re: gitweb: (potential) problems with new installation"
http://thread.gmane.org/gmane.comp.version-control.git/191746

 gitweb/gitweb.perl                       |   11 ++++++++++-
 t/t9501-gitweb-standalone-http-status.sh |   10 ++++++++++
 2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1fc5361..22ad279 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1081,7 +1081,16 @@ sub evaluate_and_validate_params {
 		if (length($searchtext) < 2) {
 			die_error(403, "At least two characters are required for search parameter");
 		}
-		$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
+		if ($search_use_regexp) {
+			$search_regexp = $searchtext;
+			if (!eval { qr/$search_regexp/; 1; }) {
+				(my $error = $@) =~ s/ at \S+ line \d+.*\n?//;
+				die_error(400, "Invalid search regexp '$search_regexp'",
+				          esc_html($error));
+			}
+		} else {
+			$search_regexp = quotemeta $searchtext;
+		}
 	}
 }
 
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 26102ee..31076ed 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -134,4 +134,14 @@ our $maxload = undef;
 EOF
 
 
+# ----------------------------------------------------------------------
+# invalid arguments
+
+test_expect_success 'invalid arguments: invalid regexp (in project search)' '
+	gitweb_run "a=project_list;s=*\.git;sr=1" &&
+	grep "Status: 400" gitweb.headers &&
+	grep "400 - Invalid.*regexp" gitweb.body
+'
+test_debug 'cat gitweb.headers'
+
 test_done

Re: [PATCH (BUGFIX)] gitweb: Handle invalid regexp in regexp search

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> When using regexp search ('sr' parameter / $search_use_regexp variable
> is true), check first that regexp is valid.

Thanks.

How old is this bug?  Should it go to older maitenance tracks like 1.7.6?

Re: [PATCH (BUGFIX)] gitweb: Handle invalid regexp in regexp search

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > When using regexp search ('sr' parameter / $search_use_regexp variable
> > is true), check first that regexp is valid.
> 
> Thanks.
> 
> How old is this bug?  Should it go to older maitenance tracks like 1.7.6?

>From what I examined this bug is from the very beginning when gitweb
started to distinguish regexp search and fixed string search in

  0e55991 (gitweb: Clearly distinguish regexp / exact match searches, 2008-02-26)

It was present in 1.5.5 (including beginnings of match highlighting, which
trigger this bug).


This bug was present so long without detection because circumstances must
be quite specific: you have to select regexp search, and to provide invalid
regexp.  If you know what regexp is, you probably write correct ones...
but there always room for mistake.

-- 
Jakub Narebski
Poland

Re: [PATCH (BUGFIX)] gitweb: Handle invalid regexp in regexp search

[Ramsay Jones]

Jakub Narebski wrote:
> When using regexp search ('sr' parameter / $search_use_regexp variable
> is true), check first that regexp is valid.
> 
> Without this patch we would get an error from Perl during search (if
> searching is performed by gitweb), or highlighting matches substring
> (if applicable), if user provided invalid regexp... which means broken
> HTML, with error page (including HTTP headers) generated after gitweb
> already produced some output.
> 
> Add test that illustrates such error: for example for regexp "*\.git"
> we would get the following error:
> 
>   Quantifier follows nothing in regex; marked by <-- HERE in m/* <-- HERE \.git/
>   at /var/www/cgi-bin/gitweb.cgi line 3084.
> 
> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> See "Re: gitweb: (potential) problems with new installation"
> http://thread.gmane.org/gmane.comp.version-control.git/191746

This patch solves the problem for me when using a regex search
(re checkbox checked), but *not* for a non-regex search.

If you have a leading '*' or '+', in the non-regex case, then you
still get the above complaint (and xml error page etc.), although
the line number has changed slightly from that given above.

ATB,
Ramsay Jones

[PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

[Jakub Narebski]

Use $search_regexp, where regex metacharacters are quoted, for
searching projects list, rather than $searchtext, which contains
original search term.

Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I think this bug was here from the very beginning of adding project
search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
project search form, 2008-10-03)  which was present since 1.6.1

On Fri, 2 Mar 2012, Ramsay Jones wrote:
> Jakub Narebski wrote:

> > When using regexp search ('sr' parameter / $search_use_regexp variable
> > is true), check first that regexp is valid.
> > 
> > Without this patch we would get an error from Perl during search (if
> > searching is performed by gitweb), or highlighting matches substring
> > (if applicable), if user provided invalid regexp... which means broken
> > HTML, with error page (including HTTP headers) generated after gitweb
> > already produced some output.
> > 
> > Add test that illustrates such error: for example for regexp "*\.git"
> > we would get the following error:
> > 
> >   Quantifier follows nothing in regex; marked by <-- HERE in m/* <-- HERE \.git/
> >   at /var/www/cgi-bin/gitweb.cgi line 3084.
> > 
> > Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > See "Re: gitweb: (potential) problems with new installation"
> > http://thread.gmane.org/gmane.comp.version-control.git/191746
> 
> This patch solves the problem for me when using a regex search
> (re checkbox checked), but *not* for a non-regex search.
> 
> If you have a leading '*' or '+', in the non-regex case, then you
> still get the above complaint (and xml error page etc.), although
> the line number has changed slightly from that given above.

Ramsay, please provide those line number in the future, together with
line and if possible some context.

The line is different because it is different bug: this is about not
using quotemeta'ed string for search for fixed-string search.

 gitweb/gitweb.perl |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 22ad279..7398be1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3072,16 +3072,16 @@ sub filter_forks_from_projects_list {
 # for 'descr_long' and 'ctags' to be filled
 sub search_projects_list {
 	my ($projlist, %opts) = @_;
-	my $tagfilter  = $opts{'tagfilter'};
-	my $searchtext = $opts{'searchtext'};
+	my $tagfilter = $opts{'tagfilter'};
+	my $search_re = $opts{'search_regexp'};
 
 	return @$projlist
-		unless ($tagfilter || $searchtext);
+		unless ($tagfilter || $search_re);
 
 	# searching projects require filling to be run before it;
 	fill_project_list_info($projlist,
-	                       $tagfilter  ? 'ctags' : (),
-	                       $searchtext ? ('path', 'descr') : ());
+	                       $tagfilter ? 'ctags' : (),
+	                       $search_re ? ('path', 'descr') : ());
 	my @projects;
  PROJECT:
 	foreach my $pr (@$projlist) {
@@ -3092,10 +3092,10 @@ sub search_projects_list {
 				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
 		}
 
-		if ($searchtext) {
+		if ($search_re) {
 			next unless
-				$pr->{'path'} =~ /$searchtext/ ||
-				$pr->{'descr_long'} =~ /$searchtext/;
+				$pr->{'path'} =~ /$search_re/ ||
+				$pr->{'descr_long'} =~ /$search_re/;
 		}
 
 		push @projects, $pr;
@@ -5498,9 +5498,9 @@ sub git_project_list_body {
 		if ($check_forks);
 	# search_projects_list pre-fills required info
 	@projects = search_projects_list(\@projects,
-	                                 'searchtext' => $searchtext,
-	                                 'tagfilter'  => $tagfilter)
-		if ($tagfilter || $searchtext);
+	                                 'search_regexp' => $search_regexp,
+	                                 'tagfilter' => $tagfilter)
+		if ($tagfilter || $search_regexp);
 	# fill the rest
 	@projects = fill_project_list_info(\@projects);
 
-- 
1.7.9

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Use $search_regexp, where regex metacharacters are quoted, for
> searching projects list, rather than $searchtext, which contains
> original search term.
>
> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> I think this bug was here from the very beginning of adding project
> search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
> project search form, 2008-10-03)  which was present since 1.6.1
>
> On Fri, 2 Mar 2012, Ramsay Jones wrote:
> 
>> This patch solves the problem for me when using a regex search
>> (re checkbox checked), but *not* for a non-regex search.
>> 

This patch depends on the more recent changes than the regexp fix, no?  I
was hoping that we could merge the earlier fix for the regexp case to
older maintenance tracks later, but if we were going to do so, we would
want to do the same for a fix for fixed-string case.

I am fine with not to worrying too much about older maintenance tracks,
and applying this directly to 'master', but just wanted to see what your
preference is.

Thanks.

>> If you have a leading '*' or '+', in the non-regex case, then you
>> still get the above complaint (and xml error page etc.), although
>> the line number has changed slightly from that given above.
>
> Ramsay, please provide those line number in the future, together with
> line and if possible some context.
>
> The line is different because it is different bug: this is about not
> using quotemeta'ed string for search for fixed-string search.
>
>  gitweb/gitweb.perl |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 22ad279..7398be1 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3072,16 +3072,16 @@ sub filter_forks_from_projects_list {
>  # for 'descr_long' and 'ctags' to be filled
>  sub search_projects_list {
>  	my ($projlist, %opts) = @_;
> -	my $tagfilter  = $opts{'tagfilter'};
> -	my $searchtext = $opts{'searchtext'};
> +	my $tagfilter = $opts{'tagfilter'};
> +	my $search_re = $opts{'search_regexp'};
>  
>  	return @$projlist
> -		unless ($tagfilter || $searchtext);
> +		unless ($tagfilter || $search_re);
>  
>  	# searching projects require filling to be run before it;
>  	fill_project_list_info($projlist,
> -	                       $tagfilter  ? 'ctags' : (),
> -	                       $searchtext ? ('path', 'descr') : ());
> +	                       $tagfilter ? 'ctags' : (),
> +	                       $search_re ? ('path', 'descr') : ());
>  	my @projects;
>   PROJECT:
>  	foreach my $pr (@$projlist) {
> @@ -3092,10 +3092,10 @@ sub search_projects_list {
>  				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
>  		}
>  
> -		if ($searchtext) {
> +		if ($search_re) {
>  			next unless
> -				$pr->{'path'} =~ /$searchtext/ ||
> -				$pr->{'descr_long'} =~ /$searchtext/;
> +				$pr->{'path'} =~ /$search_re/ ||
> +				$pr->{'descr_long'} =~ /$search_re/;
>  		}
>  
>  		push @projects, $pr;
> @@ -5498,9 +5498,9 @@ sub git_project_list_body {
>  		if ($check_forks);
>  	# search_projects_list pre-fills required info
>  	@projects = search_projects_list(\@projects,
> -	                                 'searchtext' => $searchtext,
> -	                                 'tagfilter'  => $tagfilter)
> -		if ($tagfilter || $searchtext);
> +	                                 'search_regexp' => $search_regexp,
> +	                                 'tagfilter' => $tagfilter)
> +		if ($tagfilter || $search_regexp);
>  	# fill the rest
>  	@projects = fill_project_list_info(\@projects);

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

[Jakub Narebski]

On Sat, 3 Mar 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Use $search_regexp, where regex metacharacters are quoted, for
>> searching projects list, rather than $searchtext, which contains
>> original search term.
>>
>> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>> ---
>> I think this bug was here from the very beginning of adding project
>> search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
>> project search form, 2008-10-03)  which was present since 1.6.1
>>
>> On Fri, 2 Mar 2012, Ramsay Jones wrote:
>> 
>>> This patch solves the problem for me when using a regex search
>>> (re checkbox checked), but *not* for a non-regex search.
>>> 
> 
> This patch depends on the more recent changes than the regexp fix, no?  I
> was hoping that we could merge the earlier fix for the regexp case to
> older maintenance tracks later, but if we were going to do so, we would
> want to do the same for a fix for fixed-string case.

The regexp and non-regexp bugs and fixes are different.

The regexp "bug" was just us forgetting that regexp is provided by user
input, and should be validated.  The bug as reported by Ramsay was here
from the very beginning, i.e. commit 0e55991 (gitweb: Clearly distinguish
regexp / exact match searches, 2008-02-26), which was present in v1.5.1
if I have checked correctly.  The fix is about adding new code and should
apply cleanly to 'maint' and even to older versions; the only trouble
with older version might be whitespace issue related to refactoring
code into subroutines.

The non-regexp project search bug was using $searchtext instead of
$search_regexp as search regexp in gitweb.  The bug was present from
the very addition of project search, namely commit 0d1d154 (gitweb:
Support for simple project search form, 2008-10-03), which was present
in v1.5.1 if I have checked correctly.  Unfortunately the fix affects
code that was changed recently in a1e1b2d (gitweb: improve usability
of projects search form, 2012-01-31); I'll try to come up with equivalent
patch to 'maint' soon (if the current one does not apply, and I guess it
doesn't).

-- 
Jakub Narebski
Poland

[PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

[Jakub Narebski]

On Sat, 3 Mar 2012, Jakub Narebski wrote:
> On Sat, 3 Mar 2012, Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>>> Use $search_regexp, where regex metacharacters are quoted, for
>>> searching projects list, rather than $searchtext, which contains
>>> original search term.
>>>
>>> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>>> ---
>>> I think this bug was here from the very beginning of adding project
>>> search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
>>> project search form, 2008-10-03)  which was present since 1.6.1
>>>
>>> On Fri, 2 Mar 2012, Ramsay Jones wrote:
>>> 
>>>> This patch solves the problem for me when using a regex search
>>>> (re checkbox checked), but *not* for a non-regex search.
>>>> 
>> 
>> This patch depends on the more recent changes than the regexp fix, no?  I
>> was hoping that we could merge the earlier fix for the regexp case to
>> older maintenance tracks later, but if we were going to do so, we would
>> want to do the same for a fix for fixed-string case.
> 
> The regexp and non-regexp bugs and fixes are different.
[...]
> The non-regexp project search bug was using $searchtext instead of
> $search_regexp as search regexp in gitweb.  The bug was present from
> the very addition of project search, namely commit 0d1d154 (gitweb:
> Support for simple project search form, 2008-10-03), which was present
> in v1.5.1 if I have checked correctly.  Unfortunately the fix affects
> code that was changed recently in a1e1b2d (gitweb: improve usability
> of projects search form, 2012-01-31); I'll try to come up with equivalent
> patch to 'maint' soon (if the current one does not apply, and I guess it
> doesn't).

And here is the patch for maint
-->8-- -------------------------------------------------------- -->8--
Subject: gitweb: Fix fixed string (non-regexp) project search

Use $search_regexp, where regex metacharacters are quoted, for
searching projects list, rather than $searchtext, which contains
original search term.

Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d5dbd64..e248792 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2968,11 +2968,11 @@ sub filter_forks_from_projects_list {
 # for 'descr_long' and 'ctags' to be filled
 sub search_projects_list {
 	my ($projlist, %opts) = @_;
-	my $tagfilter  = $opts{'tagfilter'};
-	my $searchtext = $opts{'searchtext'};
+	my $tagfilter = $opts{'tagfilter'};
+	my $search_re = $opts{'search_regexp'};
 
 	return @$projlist
-		unless ($tagfilter || $searchtext);
+		unless ($tagfilter || $search_re);
 
 	my @projects;
  PROJECT:
@@ -2984,10 +2984,10 @@ sub search_projects_list {
 				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
 		}
 
-		if ($searchtext) {
+		if ($search_re) {
 			next unless
-				$pr->{'path'} =~ /$searchtext/ ||
-				$pr->{'descr_long'} =~ /$searchtext/;
+				$pr->{'path'} =~ /$search_re/ ||
+				$pr->{'descr_long'} =~ /$search_re/;
 		}
 
 		push @projects, $pr;
@@ -5290,9 +5290,11 @@ sub git_project_list_body {
 	@projects = fill_project_list_info(\@projects);
 	# searching projects require filling to be run before it
 	@projects = search_projects_list(\@projects,
-	                                 'searchtext' => $searchtext,
-	                                 'tagfilter'  => $tagfilter)
-		if ($tagfilter || $searchtext);
+	                                 'search_regexp' => $search_regexp,
+	                                 'tagfilter' => $tagfilter)
+		if ($tagfilter || $search_regexp);
+	# fill the rest
+	@projects = fill_project_list_info(\@projects);
 
 	$order ||= $default_projects_order;
 	$from = 0 unless defined $from;
-- 
1.7.9

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> And here is the patch for maint
> -->8-- -------------------------------------------------------- -->8--
> Subject: gitweb: Fix fixed string (non-regexp) project search
>
> Use $search_regexp, where regex metacharacters are quoted, for
> searching projects list, rather than $searchtext, which contains
> original search term.
>
> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
>  gitweb/gitweb.perl |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d5dbd64..e248792 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5290,9 +5290,11 @@ sub git_project_list_body {
>  	@projects = fill_project_list_info(\@projects);
>  	# searching projects require filling to be run before it
>  	@projects = search_projects_list(\@projects,
> -	                                 'searchtext' => $searchtext,
> -	                                 'tagfilter'  => $tagfilter)
> -		if ($tagfilter || $searchtext);
> +	                                 'search_regexp' => $search_regexp,
> +	                                 'tagfilter' => $tagfilter)
> +		if ($tagfilter || $search_regexp);
> +	# fill the rest
> +	@projects = fill_project_list_info(\@projects);

Hmph, didn't you already call fill_project_list_info(\@projects) before
search_projects_list() already?

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > And here is the patch for maint
> > -->8-- -------------------------------------------------------- -->8--
> > Subject: gitweb: Fix fixed string (non-regexp) project search
> >
> > Use $search_regexp, where regex metacharacters are quoted, for
> > searching projects list, rather than $searchtext, which contains
> > original search term.
> >
> > Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> >  gitweb/gitweb.perl |   20 +++++++++++---------
> >  1 files changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index d5dbd64..e248792 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -5290,9 +5290,11 @@ sub git_project_list_body {
> >  	@projects = fill_project_list_info(\@projects);
> >  	# searching projects require filling to be run before it
> >  	@projects = search_projects_list(\@projects,
> > -	                                 'searchtext' => $searchtext,
> > -	                                 'tagfilter'  => $tagfilter)
> > -		if ($tagfilter || $searchtext);
> > +	                                 'search_regexp' => $search_regexp,
> > +	                                 'tagfilter' => $tagfilter)
> > +		if ($tagfilter || $search_regexp);
> > +	# fill the rest
> > +	@projects = fill_project_list_info(\@projects);
> 
> Hmph, didn't you already call fill_project_list_info(\@projects) before
> search_projects_list() already?

True.  Sorry about that. 

Can you fix that, or should I resend?

-- 
Jakub Narebski
Poland

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>> > And here is the patch for maint
>> ...
>> > +		if ($tagfilter || $search_regexp);
>> > +	# fill the rest
>> > +	@projects = fill_project_list_info(\@projects);
>> 
>> Hmph, didn't you already call fill_project_list_info(\@projects) before
>> search_projects_list() already?
>
> True.  Sorry about that. 
>
> Can you fix that, or should I resend?

Could you check the following two diffs?

$ git show debd1c2

This is jn/maint-do-not-match-with-unsanitized-searchtext that
should be merged to maintenance track that lack the lazy filling.

And then

$ git show --first-parent d4b52c2

This is how the above was merged to 'pu' and the conflict resolution
should be the same when we merge it to 'master'. As our @projects may
still be only sparsely filled when search_projects_list() returns,
we do call fill_project_list_info(\@projects) ourselves with the
lazy filling codebase.

There are a few places I noticed that check $searchtext to see if we
are running a search, and techinically $search_regexp might be a
more correct thing to use, but I do not think it matters that much.

Thanks.

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jakub Narebski <jnareb@gmail.com> writes:
>
>>> Hmph, didn't you already call fill_project_list_info(\@projects) before
>>> search_projects_list() already?
>>
>> True.  Sorry about that. 
>>
>> Can you fix that, or should I resend?
>
> Could you check the following two diffs?
>
> $ git show debd1c2
>
> This is jn/maint-do-not-match-with-unsanitized-searchtext that
> should be merged to maintenance track that lack the lazy filling.
>
> And then
>
> $ git show --first-parent d4b52c2
>
> This is how the above was merged to 'pu' and the conflict resolution
> should be the same when we merge it to 'master'. As our @projects may
> still be only sparsely filled when search_projects_list() returns,
> we do call fill_project_list_info(\@projects) ourselves with the
> lazy filling codebase.

The latter is now

$ git show --first-parent 657c6d0

on today's 'pu'.

Thanks.

Re: [PATCH (for maint)] gitweb: Fix fixed string (non-regexp) project search

[Jakub Narebski]

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Jakub Narebski <jnareb@gmail.com> writes:
>>
>>>> Hmph, didn't you already call fill_project_list_info(\@projects) before
>>>> search_projects_list() already?
>>>
>>> True.  Sorry about that. 
>>>
>>> Can you fix that, or should I resend?
>>
>> Could you check the following two diffs?
>>
>> $ git show debd1c2
>>
>> This is jn/maint-do-not-match-with-unsanitized-searchtext that
>> should be merged to maintenance track that lack the lazy filling.
>>
>> And then
>>
>> $ git show --first-parent d4b52c2
>>
>> This is how the above was merged to 'pu' and the conflict resolution
>> should be the same when we merge it to 'master'. As our @projects may
>> still be only sparsely filled when search_projects_list() returns,
>> we do call fill_project_list_info(\@projects) ourselves with the
>> lazy filling codebase.
> 
> The latter is now
> 
> $ git show --first-parent 657c6d0
> 
> on today's 'pu'.
> 
> Thanks.

Both look all right (the only difference in diff is use of $searchtext
vs $search_regexp global variable to check if search is on, but for that
purpose those variables are equivalent).

Thanks.

-- 
Jakub Narebski
Poland

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

[Jakub Narebski]

Jakub Narebski wrote:
> On Sat, 3 Mar 2012, Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>>> Use $search_regexp, where regex metacharacters are quoted, for
>>> searching projects list, rather than $searchtext, which contains
>>> original search term.
>>>
>>> Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>>> ---
>>> I think this bug was here from the very beginning of adding project
>>> search, i.e. from  v1.6.0.2-446-g0d1d154 (gitweb: Support for simple
>>> project search form, 2008-10-03)  which was present since 1.6.1
>>>
>>> On Fri, 2 Mar 2012, Ramsay Jones wrote:
>>> 
>>>> This patch solves the problem for me when using a regex search
>>>> (re checkbox checked), but *not* for a non-regex search.
>>>> 
>> 
>> This patch depends on the more recent changes than the regexp fix, no?  I
>> was hoping that we could merge the earlier fix for the regexp case to
>> older maintenance tracks later, but if we were going to do so, we would
>> want to do the same for a fix for fixed-string case.
> 
> The regexp and non-regexp bugs and fixes are different.
> 
> The regexp "bug" was just us forgetting that regexp is provided by user
> input, and should be validated.  The bug as reported by Ramsay was here
> from the very beginning, i.e. commit 0e55991 (gitweb: Clearly distinguish
> regexp / exact match searches, 2008-02-26), which was present in v1.5.1
> if I have checked correctly.  The fix is about adding new code and should
> apply cleanly to 'maint' and even to older versions; the only trouble
> with older version might be whitespace issue related to refactoring
> code into subroutines.
> 
> The non-regexp project search bug was using $searchtext instead of
> $search_regexp as search regexp in gitweb.  The bug was present from
> the very addition of project search, namely commit 0d1d154 (gitweb:
> Support for simple project search form, 2008-10-03), which was present
> in v1.5.1 if I have checked correctly.  Unfortunately the fix affects
> code that was changed recently in a1e1b2d (gitweb: improve usability
> of projects search form, 2012-01-31); I'll try to come up with equivalent
> patch to 'maint' soon (if the current one does not apply, and I guess it
> doesn't).

In other words: while "*foo" is invalid regular expression, it is
perfectly valid fixed string search term (which translates to "\*foo"
regexp).

-- 
Jakub Narebski
Poland

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> ....  The fix is about adding new code and should
> apply cleanly to 'maint' and even to older versions; the only trouble
> with older version might be whitespace issue related to refactoring
> code into subroutines.

OK, so the global $searchtext is what came from form submit from the end
user, while the global $search_regexp is what the code should be using
for matching throughout the program, prepared by eval-and-validate-params.

Here is a hand-ported version of your patch that should apply to 1.7.6.6;
does it look sane?

 gitweb/gitweb.perl |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 50a835a..d1698b7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2905,10 +2905,10 @@ sub filter_forks_from_projects_list {
 sub search_projects_list {
 	my ($projlist, %opts) = @_;
 	my $tagfilter  = $opts{'tagfilter'};
-	my $searchtext = $opts{'searchtext'};
+	my $search_re = $opts{'search_regexp'};
 
 	return @$projlist
-		unless ($tagfilter || $searchtext);
+		unless ($tagfilter || $search_re);
 
 	my @projects;
  PROJECT:
@@ -2920,10 +2920,10 @@ sub search_projects_list {
 				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
 		}
 
-		if ($searchtext) {
+		if ($search_re) {
 			next unless
-				$pr->{'path'} =~ /$searchtext/ ||
-				$pr->{'descr_long'} =~ /$searchtext/;
+				$pr->{'path'} =~ /$search_re/ ||
+				$pr->{'descr_long'} =~ /$search_re/;
 		}
 
 		push @projects, $pr;
@@ -5097,7 +5097,7 @@ sub git_project_list_body {
 	@projects = fill_project_list_info(\@projects);
 	# searching projects require filling to be run before it
 	@projects = search_projects_list(\@projects,
-	                                 'searchtext' => $searchtext,
+	                                 'search_regexp' => $search_regexp,
 	                                 'tagfilter'  => $tagfilter)
 		if ($tagfilter || $searchtext);
 

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > ....  The fix is about adding new code and should
> > apply cleanly to 'maint' and even to older versions; the only trouble
> > with older version might be whitespace issue related to refactoring
> > code into subroutines.
> 
> OK, so the global $searchtext is what came from form submit from the end
> user, while the global $search_regexp is what the code should be using
> for matching throughout the program, prepared by eval-and-validate-params.
> 
> Here is a hand-ported version of your patch that should apply to 1.7.6.6;
> does it look sane?
> 
>  gitweb/gitweb.perl |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 50a835a..d1698b7 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2905,10 +2905,10 @@ sub filter_forks_from_projects_list {
>  sub search_projects_list {
>  	my ($projlist, %opts) = @_;
>  	my $tagfilter  = $opts{'tagfilter'};
> -	my $searchtext = $opts{'searchtext'};
> +	my $search_re = $opts{'search_regexp'};
>  
>  	return @$projlist
> -		unless ($tagfilter || $searchtext);
> +		unless ($tagfilter || $search_re);
>  
>  	my @projects;
>   PROJECT:
> @@ -2920,10 +2920,10 @@ sub search_projects_list {
>  				grep { lc($_) eq lc($tagfilter) } keys %{$pr->{'ctags'}};
>  		}
>  
> -		if ($searchtext) {
> +		if ($search_re) {
>  			next unless
> -				$pr->{'path'} =~ /$searchtext/ ||
> -				$pr->{'descr_long'} =~ /$searchtext/;
> +				$pr->{'path'} =~ /$search_re/ ||
> +				$pr->{'descr_long'} =~ /$search_re/;
>  		}
>  
>  		push @projects, $pr;
> @@ -5097,7 +5097,7 @@ sub git_project_list_body {
>  	@projects = fill_project_list_info(\@projects);
>  	# searching projects require filling to be run before it
>  	@projects = search_projects_list(\@projects,
> -	                                 'searchtext' => $searchtext,
> +	                                 'search_regexp' => $search_regexp,
>  	                                 'tagfilter'  => $tagfilter)
>  		if ($tagfilter || $searchtext);
>  

It looks sane, though 

  	@projects = search_projects_list(\@projects,
 -	                                 'searchtext' => $searchtext,
 +	                                 'search_regexp' => $search_regexp,
  	                                 'tagfilter'  => $tagfilter)
  		if ($tagfilter || $searchtext);

should be better written as

  	@projects = search_projects_list(\@projects,
 -	                                 'searchtext' => $searchtext,
 +	                                 'search_regexp' => $search_regexp,
  	                                 'tagfilter'  => $tagfilter)
 - 		if ($tagfilter || $searchtext);
 + 		if ($tagfilter || $search_regexp);

It is functionally the same, because $search_regexp is derived from
$searchtext, but IMHO it is more clear.

-- 
Jakub Narebski
Poland

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

[Ramsay Jones]

Jakub Narebski wrote:
>> This patch solves the problem for me when using a regex search
>> (re checkbox checked), but *not* for a non-regex search.
>>
>> If you have a leading '*' or '+', in the non-regex case, then you
>> still get the above complaint (and xml error page etc.), although
>> the line number has changed slightly from that given above.
> 
> Ramsay, please provide those line number in the future, together with
> line and if possible some context.

Yeah, sorry about that; when I wrote that I didn't have the information
readily available (I would have had to shutdown Windows, boot Linux, ...)
and I was about to go out. So, the choice was to wait about 24hrs to report
with full info, or provide the feedback earlier; I chose the latter. ;-)

>  gitweb/gitweb.perl |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)

[patch snipped]

This patch works great for me. Thanks!

ATB,
Ramsay Jones

Re: [PATCH (BUGFIX)] gitweb: Fix fixed string (non-regexp) project search

[Jakub Narebski]

Ramsay Jones wrote:
> Jakub Narebski wrote:

>>> This patch solves the problem for me when using a regex search
>>> (re checkbox checked), but *not* for a non-regex search.
>>>
>>> If you have a leading '*' or '+', in the non-regex case, then you
>>> still get the above complaint (and xml error page etc.), although
>>> the line number has changed slightly from that given above.
>> 
>> Ramsay, please provide those line number in the future, together with
>> line and if possible some context.
> 
> Yeah, sorry about that; when I wrote that I didn't have the information
> readily available (I would have had to shutdown Windows, boot Linux, ...)
> and I was about to go out.

You could have told us _that_...

> So, the choice was to wait about 24hrs to report 
> with full info, or provide the feedback earlier; I chose the latter. ;-)

That's understandable.


Thanks again for reporting these issues.

-- 
Jakub Narebski
Poland