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

Jakub Narebski [off-list ref] 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 <redacted>
Signed-off-by: Jakub Narebski <redacted>
---
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:

quoted

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.

On Sat, 3 Mar 2012, Junio C Hamano wrote:

Jakub Narebski [off-list ref] writes:

quoted

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

Reported-by: Ramsay Jones <redacted>
Signed-off-by: Jakub Narebski <redacted>
---
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:

quoted

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

On Sat, 3 Mar 2012, Jakub Narebski wrote:

On Sat, 3 Mar 2012, Junio C Hamano wrote:

quoted

Jakub Narebski [off-list ref] writes:

quoted

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

Reported-by: Ramsay Jones <redacted>
Signed-off-by: Jakub Narebski <redacted>
---
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:

quoted

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 <redacted>
Signed-off-by: Jakub Narebski <redacted>
---
 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

Jakub Narebski wrote:

On Sat, 3 Mar 2012, Junio C Hamano wrote:

quoted

Jakub Narebski [off-list ref] writes:

quoted

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

Reported-by: Ramsay Jones <redacted>
Signed-off-by: Jakub Narebski <redacted>
---
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:

quoted

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

Jakub Narebski [off-list ref] 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);
 

Junio C Hamano wrote:

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