[PATCH/RFC 0/6] gitweb: Improve project search

[PATCH/RFC 0/6] gitweb: Improve project search

[Jakub Narebski]

This patch series started a bit strange, by noticing that I have had
gitweb-Project-search patch in my unapplied StGit stack for gitweb/web
branch.  I tried to re-do it on top of current codebase... only to
realize that gitweb already has project search :-)

So this series turned into enhancing project search to the state that
it was in said patch, splitting it into smaller parts (into separate
patches), and further improving.

The first patch is not really necessary, at least for now.  It was
intended for the situation where we could configure gitweb to show
_only_ project search form (like on http://repo.or.cz), to be able to
show all projects via "search type" show_all... which might be added
in the future.

The last two patches are new, and probably should be taken together.
The issue with them is that the second patch turns off shortening of
project description in project search view.  Fixing that would require
more complicated code... but perhaps current situation is all right.


CC-ing John 'Warthog9' Hawley of git.kernel.org, and Petr 'Pasky' Baudis
of repo.or.cz


Jakub Narebski (6):
  gitweb: Allow underscore in $searchtype ('st')
  gitweb: Improve projects search form
  gitweb: Option for filling only specified info in
    fill_project_list_info
  gitweb: Faster project search
  gitweb: Highlight matched part of project name when searching
    projects
  gitweb: Highlight matched part of project description when searching
    projects

 gitweb/gitweb.perl       |  110 +++++++++++++++++++++++++++++++++++++--------
 gitweb/static/gitweb.css |    7 +++-
 2 files changed, 96 insertions(+), 21 deletions(-)

-- 
1.7.5

[PATCH/RFC 1/6] gitweb: Allow underscore in $searchtype ('st')

[Jakub Narebski]

This makes searchtype containing underscore '_' to pass parameter
validation in evaluate_and_validate_params().

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is not necessary in this series, and could be left for later when
we need it.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f858d1b..7ec1621 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1057,7 +1057,7 @@ sub evaluate_and_validate_params {
 
 	our $searchtype = $input_params{'searchtype'};
 	if (defined $searchtype) {
-		if ($searchtype =~ m/[^a-z]/) {
+		if ($searchtype =~ m/[^a-z_]/) {
 			die_error(400, "Invalid searchtype parameter");
 		}
 	}
-- 
1.7.5

[PATCH/RFC 2/6] gitweb: Improve projects search form

[Jakub Narebski]

Refactor generating project search form into git_project_search_form(),
make text field wider and add on mouse over explanation (via "title"
attribute), add an option to use regular expressions, and replace
'Search:' label with [Search] button.

Also add "List all projects" link to make it easier to go back from
search result to list of all projects.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The most useful part is the user interface for turning on regular
expression search (it was always there, but you had to hand-edit URL),
I think.

By the way, I was reminded (on #perl IRC channel on FreeNode) that
allowing regular expression search is not safe against DoS because of
recursive backtracking regexps.  If one worries about that, one can
always change regexp engine to more safe one, for example putting the
following in gitweb config file:

  use re::engine::RE2 -strict => 1;

to use RE2 fast regexp engine from Google (http://code.google.com/p/re2).

There was some concern about extended pattern '(?{ code })' that
allows running arbitrary Perl code... but re(3pm) says that "no re
'eval';" is default, which means that such construct is disallowed
when regular expression contains variable interpolation.


Note that if there is no match, you would get 'list all project' link
twice, once from search form, one from search result... but I don't
think it is something to worry about.

 gitweb/gitweb.perl       |   27 ++++++++++++++++++++++-----
 gitweb/static/gitweb.css |    7 ++++++-
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7ec1621..3e69705 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4968,6 +4968,26 @@ sub git_patchset_body {
 
 # . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
 
+sub git_project_search_form {
+	my ($searchtext, $search_use_regexp);
+
+	print "<div class=\"projsearch\">\n";
+	print $cgi->startform(-method => 'get', -action => $my_uri) .
+	      $cgi->hidden(-name => 'a', -value => 'project_list') . "\n" .
+	      $cgi->textfield(-name => 's', -value => $searchtext,
+	                      -title => 'Search project by name and description',
+	                      -size => 60) . "\n" .
+	      "<span title=\"Extended regular expression\">" .
+	      $cgi->checkbox(-name => 'sr', -value => 1, -label => 're',
+	                     -checked => $search_use_regexp) .
+	      "</span>\n" .
+	      $cgi->submit(-name => 'btnS', -value => 'Search') .
+	      $cgi->end_form() . "\n" .
+	      $cgi->a({-href => href(project => undef, searchtext => undef)},
+	              'List all projects') . "<br />\n";
+	print "</div>\n";
+}
+
 # fills project list info (age, description, owner, category, forks)
 # for each project in the list, removing invalid projects from
 # returned list
@@ -5835,11 +5855,8 @@ sub git_project_list {
 		insert_file($home_text);
 		print "</div>\n";
 	}
-	print $cgi->startform(-method => "get") .
-	      "<p class=\"projsearch\">Search:\n" .
-	      $cgi->textfield(-name => "s", -value => $searchtext) . "\n" .
-	      "</p>" .
-	      $cgi->end_form() . "\n";
+
+	git_project_search_form($searchtext, $search_use_regexp);
 	git_project_list_body(\@list, $order);
 	git_footer_html();
 }
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 7d88509..3cdaaa3 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -490,8 +490,13 @@ div.search {
 	right: 12px
 }
 
-p.projsearch {
+div.projsearch {
 	text-align: center;
+	margin: 20px 0px;
+}
+
+div.projsearch form {
+	margin-bottom: 2px;
 }
 
 td.linenr {
-- 
1.7.5

[PATCH/RFC 3/6] gitweb: Option for filling only specified info in fill_project_list_info

[Jakub Narebski]

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The API was tweaked to better fit the following patch.

This patch probably needs longer commit message... or squashing with
the following commit.

 gitweb/gitweb.perl |   39 +++++++++++++++++++++++++++++----------
 1 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3e69705..4c66307 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4988,35 +4988,54 @@ sub git_project_search_form {
 	print "</div>\n";
 }
 
+# entry for given $key doesn't need filling if either $key already exists
+# in $project_info hash, or we are interested only in subset of keys
+# and given key is not among @fill_only.
+sub project_info_needs_filling {
+	my ($project_info, $key, @fill_only) = @_;
+
+	if (!@fill_only ||            # we are interested in everything
+	    grep { $key eq $_ } @fill_only) { # or key is in @fill_only
+		return !exists $project_info->{$key};
+	}
+	return 0;
+}
+
 # fills project list info (age, description, owner, category, forks)
 # for each project in the list, removing invalid projects from
-# returned list
+# returned list, or fill only specified info (removing invalid projects
+# only when filling 'age').
+#
 # NOTE: modifies $projlist, but does not remove entries from it
 sub fill_project_list_info {
-	my $projlist = shift;
+	my ($projlist, @fill_only) = @_;
 	my @projects;
 
 	my $show_ctags = gitweb_check_feature('ctags');
  PROJECT:
 	foreach my $pr (@$projlist) {
-		my (@activity) = git_get_last_activity($pr->{'path'});
-		unless (@activity) {
-			next PROJECT;
+		if (project_info_needs_filling($pr, 'age', @fill_only)) {
+			my (@activity) = git_get_last_activity($pr->{'path'});
+			unless (@activity) {
+				next PROJECT;
+			}
+			($pr->{'age'}, $pr->{'age_string'}) = @activity;
 		}
-		($pr->{'age'}, $pr->{'age_string'}) = @activity;
-		if (!defined $pr->{'descr'}) {
+		if (project_info_needs_filling($pr, 'descr', @fill_only)) {
 			my $descr = git_get_project_description($pr->{'path'}) || "";
 			$descr = to_utf8($descr);
 			$pr->{'descr_long'} = $descr;
 			$pr->{'descr'} = chop_str($descr, $projects_list_description_width, 5);
 		}
-		if (!defined $pr->{'owner'}) {
+		if (project_info_needs_filling($pr, 'owner', @fill_only)) {
 			$pr->{'owner'} = git_get_project_owner("$pr->{'path'}") || "";
 		}
-		if ($show_ctags) {
+		if ($show_ctags &&
+		    project_info_needs_filling($pr, 'ctags', @fill_only)) {
 			$pr->{'ctags'} = git_get_project_ctags($pr->{'path'});
 		}
-		if ($projects_list_group_categories && !defined $pr->{'category'}) {
+		if ($projects_list_group_categories &&
+		    project_info_needs_filling($pr, 'category', @fill_only)) {
 			my $cat = git_get_project_category($pr->{'path'}) ||
 			                                   $project_list_default_category;
 			$pr->{'category'} = to_utf8($cat);
-- 
1.7.5

[PATCH/RFC 4/6] gitweb: Faster project search

[Jakub Narebski]

Before searching by some field the information we search for must be
filled in.  For this fill_project_list_info() was enhanced in previous
commit to take additional parameters which part of projects info to
fill.  This way we can limit doing expensive calculations (like
running git-for-each-ref to get 'age' / "Last changed" info) only to
projects which we will show as search results.

With this commit the number of git commands used to generate search
results is 2*<matched projects> + 1, and depends on number of matched
projects rather than number of all projects (all repositories).

Note: this is 'git for-each-ref' to find last activity, and 'git config'
for each project, and 'git --version' once.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I have checked it by running gitweb with 'timed' feature enabled, but
I didn't benchmark it.  

This is a win especially for hosting sites with many projects, though
they either need to have caching, or not show full project list but
only search form by default, or paginate projects list.  

BTW. I am thinking about adding configuration which would make gitweb
show only search form if number of project is higher than some
configured threshold.  What do you think of this idea?

 gitweb/gitweb.perl |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4c66307..9c82d79 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2933,6 +2933,10 @@ sub search_projects_list {
 	return @$projlist
 		unless ($tagfilter || $searchtext);
 
+	# searching projects require filling to be run before it;
+	fill_project_list_info($projlist,
+	                       $tagfilter  ? 'ctags' : (),
+	                       $searchtext ? ('path', 'descr') : ());
 	my @projects;
  PROJECT:
 	foreach my $pr (@$projlist) {
@@ -5171,12 +5175,13 @@ sub git_project_list_body {
 	# filtering out forks before filling info allows to do less work
 	@projects = filter_forks_from_projects_list(\@projects)
 		if ($check_forks);
-	@projects = fill_project_list_info(\@projects);
-	# searching projects require filling to be run before it
+	# search_projects_list pre-fills required info
 	@projects = search_projects_list(\@projects,
 	                                 'searchtext' => $searchtext,
 	                                 'tagfilter'  => $tagfilter)
 		if ($tagfilter || $searchtext);
+	# fill the rest
+	@projects = fill_project_list_info(\@projects);
 
 	$order ||= $default_projects_order;
 	$from = 0 unless defined $from;
-- 
1.7.5

[PATCH/RFC 5/6] gitweb: Highlight matched part of project name when searching projects

[Jakub Narebski]

Use newly introduced esc_html_match_hl() to escape HTML and mark match
with span element with 'match' class.  Currently only 'path' part
(i.e. project name) is highlighted; match might be on the project
description.

The code makes use of the fact that defined $search_regexp means that
there was search going on.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The esc_html_match_hl() subroutine (I really need a better name for
it) should probably be used in more places, everywhere where we do
highlight match in non-shortened text.  It is safe wrt. codes that
need HTML escaping both in non-matched and in matched part, and with
regular expression containing characters that would be escaped, for
example "foo>?" (i.e. "foo|foo>").

 gitweb/gitweb.perl |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9c82d79..692a6bb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1690,6 +1690,30 @@ sub chop_and_escape_str {
 	}
 }
 
+# highlight match (if any), and escape HTML
+sub esc_html_match_hl {
+	my ($str, $regexp) = @_;
+	return esc_html($str) unless defined $regexp;
+
+	my @matches;
+	while ($str =~ /$regexp/g) {
+		push @matches, [$-[0], $+[0]];
+	}
+	return esc_html($str) unless @matches;
+
+	my $out = '';
+	my $pos = 0;
+	for my $m (@matches) {
+		$out .= esc_html(substr $str, $pos, $m->[0] - $pos);
+		$out .= $cgi->span({-class => 'match'},
+		                   esc_html(substr $str, $m->[0], $m->[1] - $m->[0]));
+		$pos = $m->[1];
+	}
+	$out .= esc_html(substr $str, $pos);
+
+	return $out;
+}
+
 ## ----------------------------------------------------------------------
 ## functions returning short strings
 
@@ -5143,7 +5167,9 @@ sub git_project_list_rows {
 			print "</td>\n";
 		}
 		print "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
-		                        -class => "list"}, esc_html($pr->{'path'})) . "</td>\n" .
+		                        -class => "list"},
+		                       esc_html_match_hl($pr->{'path'}, $search_regexp)) .
+		      "</td>\n" .
 		      "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list", -title => $pr->{'descr_long'}},
 		                        esc_html($pr->{'descr'})) . "</td>\n" .
-- 
1.7.5

[RFC/PATCH 6/6] gitweb: Highlight matched part of project description when searching projects

[Jakub Narebski]

Use esc_html_match_hl() from previous commit to mark match in the
_whole_ description when searching projects.

Currently, with this commit, when searching projects there is always
shown full description of a project, and not a shortened one (like for
ordinary projects list view), even if the match is on project name and
not project description.

Showing full description when there is match on it is useful to avoid
situation where match is in shortened, invisible part... well, perhaps
that could be solved (showing shortened description), but it would
require much more complicated code.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is slightly controversial part.

Note: some of changes is just reindent.

 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 692a6bb..6bc68c5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5172,7 +5172,10 @@ sub git_project_list_rows {
 		      "</td>\n" .
 		      "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list", -title => $pr->{'descr_long'}},
-		                        esc_html($pr->{'descr'})) . "</td>\n" .
+		                        $search_regexp
+		                        ? esc_html_match_hl($pr->{'descr_long'}, $search_regexp)
+		                        : esc_html($pr->{'descr'})) .
+		      "</td>\n" .
 		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
 		print "<td class=\"". age_class($pr->{'age'}) . "\">" .
 		      (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n" .
-- 
1.7.5

[PATCH/RFC 7/6] gitweb: Use esc_html_match_hl in 'grep' search

[Jakub Narebski]

Use newly introduced esc_html_match_hl() subroutine in
git_search_files() ('grep' search), instead of handcrafted code using
captures, which highlighted only first match in line.

This required to enhance esc_html_match_hl() to be able to pass-thru
options for esc_html like -nbsp=>1.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Note: because untabify() is run before match, some regexp might not
show match; this is not regression, because earlier versions had the
same behavior.

Code is shorter, and behavior is improved.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6bc68c5..d489640 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1692,24 +1692,24 @@ sub chop_and_escape_str {
 
 # highlight match (if any), and escape HTML
 sub esc_html_match_hl {
-	my ($str, $regexp) = @_;
-	return esc_html($str) unless defined $regexp;
+	my ($str, $regexp, @opts) = @_;
+	return esc_html($str, @opts) unless defined $regexp;
 
 	my @matches;
 	while ($str =~ /$regexp/g) {
 		push @matches, [$-[0], $+[0]];
 	}
-	return esc_html($str) unless @matches;
+	return esc_html($str, @opts) unless @matches;
 
 	my $out = '';
 	my $pos = 0;
 	for my $m (@matches) {
-		$out .= esc_html(substr $str, $pos, $m->[0] - $pos);
+		$out .= esc_html(substr($str, $pos, $m->[0] - $pos), @opts);
 		$out .= $cgi->span({-class => 'match'},
-		                   esc_html(substr $str, $m->[0], $m->[1] - 
$m->[0]));
+		                   esc_html(substr($str, $m->[0], $m->[1] - $m->[0]), 
@opts));
 		$pos = $m->[1];
 	}
-	$out .= esc_html(substr $str, $pos);
+	$out .= esc_html(substr($str, $pos), @opts);
 
 	return $out;
 }
@@ -5796,15 +5796,7 @@ sub git_search_files {
 			print "<div class=\"binary\">Binary file</div>\n";
 		} else {
 			$ltext = untabify($ltext);
-			if ($ltext =~ m/^(.*)($search_regexp)(.*)$/i) {
-				$ltext = esc_html($1, -nbsp=>1);
-				$ltext .= '<span class="match">';
-				$ltext .= esc_html($2, -nbsp=>1);
-				$ltext .= '</span>';
-				$ltext .= esc_html($3, -nbsp=>1);
-			} else {
-				$ltext = esc_html($ltext, -nbsp=>1);
-			}
+			esc_html_match_hl($ltext, $search_regexp, -nbsp => 1);
 			print "<div class=\"pre\">" .
 				$cgi->a({-href => href(action=>"blob", hash=>$co{'hash'},
 						       file_name=>"$file").'#l'.$lno,
-- 
1.7.5