threads / rfc / 27952

RFC patch, 6 partsgitweb: Improve project search

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

## tl;dr

8 messages between Jul 29, 2011 and Jul 30, 2011. Diffs are folded; open one to read it.

replies: 7people: 1as markdown or json

Jakub Narebski· Jul 29, 2011, 11:52 UTC · lore

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
Jakub Narebski· Jul 29, 2011, 11:52 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +1 −1
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
Jakub Narebski· Jul 29, 2011, 11:52 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to 2 files +28 −6

gitweb/gitweb.perl, gitweb/static/gitweb.css

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
Jakub Narebski· Jul 29, 2011, 11:52 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +29 −10
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
Jakub Narebski· Jul 29, 2011, 11:52 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +7 −2
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
Jakub Narebski· Jul 29, 2011, 11:52 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +27 −1
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
Jakub Narebski· Jul 29, 2011, 11:52 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +4 −1
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
Jakub Narebski· Jul 30, 2011, 14:57 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +7 −15
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

← back to recent threads