Skip to contentSkip to conversation

gitweb: Use rev-list pattern search options.

PATCH In review Robert Fitzsimons 2 participants 10 messages Jun 15, 2016

lore
Share & tools
lore
#0 lore raw

PATCH 1/3 gitweb: Use rev-list pattern search options.

Use rev-list pattern search options instead of hand coded perl.

---
 gitweb/gitweb.perl |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)
gitweb/gitweb.perl +10 -10
index ebbc397..cc6bd0c 100755
@@ -4172,20 +4172,20 @@ sub git_search {
 	print "<table cellspacing=\"0\">\n";
 	my $alternate = 1;
 	if ($searchtype eq 'commit' or $searchtype eq 'author' or $searchtype eq 'committer') {
+		my $greptype;
+		if ($searchtype eq 'commit') {
+			$greptype = "--grep=";
+		} elsif ($searchtype eq 'author') {
+			$greptype = "--author=";
+		} elsif ($searchtype eq 'committer') {
+			$greptype = "--committer=";
+		}
 		$/ = "\0";
 		open my $fd, "-|", git_cmd(), "rev-list",
-			"--header", "--parents", $hash, "--"
+			"--header", "--parents", ($greptype . $searchtext),
+			 $hash, "--"
 			or next;
 		while (my $commit_text = <$fd>) {
-			if (!grep m/$searchtext/i, $commit_text) {
-				next;
-			}
-			if ($searchtype eq 'author' && !grep m/\nauthor .*$searchtext/i, $commit_text) {
-				next;
-			}
-			if ($searchtype eq 'committer' && !grep m/\ncommitter .*$searchtext/i, $commit_text) {
-				next;
-			}
 			my @commit_lines = split "\n", $commit_text;
 			my %co = parse_commit(undef, \@commit_lines);
 			if (!%co) {
Signature · 1 line
1.4.4.3.gae7ae3
#1 lore raw

PATCH 2/3 gitweb: Require a minimum of two character for the search text.

---
 gitweb/gitweb.perl |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
gitweb/gitweb.perl +3 -0
index cc6bd0c..6778b24 100755
@@ -351,6 +351,9 @@ if (defined $searchtext) {
 	if ($searchtext =~ m/[^a-zA-Z0-9_\.\/\-\+\:\@ ]/) {
 		die_error(undef, "Invalid search parameter");
 	}
+ 	if (length($searchtext) < 2) {
+		die_error(undef, "At least two characters are required for search parameter");
+	}
 	$searchtext = quotemeta $searchtext;
 }
 
Signature · 1 line
1.4.4.3.gae7ae3
#2 lore raw

PATCH 3/3 gitweb: Allow search to be disabled from the config file.

---
 gitweb/gitweb.perl |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
gitweb/gitweb.perl +13 -0
index 6778b24..e8f63aa 100755
@@ -128,6 +128,12 @@ our %feature = (
 		#         => [content-encoding, suffix, program]
 		'default' => ['x-gzip', 'gz', 'gzip']},
 
+	# Enable text search, which will list the commits which match author, 
+	# committer or commit text to a given string.  Enabled by default.
+	'search' => {
+		'override' => 0,
+		'default' => [1]},
+
 	# Enable the pickaxe search, which will list the commits that modified
 	# a given string in a file. This can be practical and quite faster
 	# alternative to 'blame', but still potentially CPU-intensive.
@@ -1729,6 +1735,9 @@ EOF
 			print " / $action";
 		}
 		print "\n";
+	}
+	my ($have_search) = gitweb_check_feature('search');
+	if ((defined $project) && ($have_search)) {
 		if (!defined $searchtext) {
 			$searchtext = "";
 		}
@@ -4147,6 +4156,10 @@ sub git_history {
 }
 
 sub git_search {
+	my ($have_search) = gitweb_check_feature('search');
+	if (!$have_search) {
+		die_error('403 Permission denied', "Permission denied");
+	}
 	if (!defined $searchtext) {
 		die_error(undef, "Text field empty");
 	}
Signature · 1 line
1.4.4.3.gae7ae3
#3 lore raw

Re: [PATCH 3/3] gitweb: Allow search to be disabled from the config file.

Robert Fitzsimons wrote:

[...]
I'm not sure if it is worth disabling such not demanding in resources (contrary to pickaxe, blame and to some extent snapshot). Perhaps it would be better to simply paginate search result, like "history" view got paginated?

Signature · 3 lines
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
#4 lore raw

Re: [PATCH 3/3] gitweb: Allow search to be disabled from the config file.

Quoted 4 lines

I'm not sure if it is worth disabling such not demanding in resources (contrary to pickaxe, blame and to some extent snapshot). Perhaps it would be better to simply paginate search result, like "history" view got paginated?

Yes that makes sense. I'll withdraw this patch and try and come up with a new one which can paginate search results.

Robert

#5 lore raw

Re: [PATCH 3/3] gitweb: Allow search to be disabled from the config file.

Quoted 9 lines · Robert Fitzsimons
Quoted 4 lines · Jakub Narebski

I'm not sure if it is worth disabling such not demanding in resources (contrary to pickaxe, blame and to some extent snapshot). Perhaps it would be better to simply paginate search result, like "history" view got paginated?

Yes that makes sense. I'll withdraw this patch and try and come up with a new one which can paginate search results.

Besides having removed search, it would follow removing search _form_. Hmmm... perhaps we should add 'pickaxe' to search form only if it is enabled?

Something like (warning: this diff is certainly whitespace damaged!):

gitweb/gitweb.perl +2 -1
index 5feebaf..585d9fd 100755
@@ -1894,7 +1903,8 @@ EOF
                      $cgi->hidden(-name => "a") . "\n" .
                      $cgi->hidden(-name => "h") . "\n" .
                      $cgi->popup_menu(-name => 'st', -default => 'commit',
-                                      -values => ['commit', 'author', 'committer', 'pickaxe
+                                      -values => ['commit', 'author', 'committer',
+                                      gitweb_check_feature('pickaxe') ? 'pickaxe' : ()]) .
                      $cgi->sup($cgi->a({-href => href(action=>"search_help")}, "?")) .
                      " search:\n",
                      $cgi->textfield(-name => "s", -value => $searchtext) . "\n" .

Take a look how it was done for "history" view in commit 8be683520e
"gitweb: Paginate history output"

Although with search you have additional complication with marking match, and "log" view like rather than "shortlog" like view... so I'm not sure if it would truly help. On the other hand you can use --skip option you have introduced...

Signature · 2 lines
Jakub Narebski
Poland
#6 lore raw

PATCH 1/1 gitweb: Paginate commit/author/committer search output

Paginate commit/author/committer search output to only show 100 commits at a time, added appropriate nav links.

---

Quoted 4 lines

Although with search you have additional complication with marking match, and "log" view like rather than "shortlog" like view... so I'm not sure if it would truly help. On the other hand you can use --skip option you have introduced...

I used the slower non--skip workflow for the moment, so at least there is no need to upgrade the core git commands.

Robert

 gitweb/gitweb.perl |  148 ++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 103 insertions(+), 45 deletions(-)
gitweb/gitweb.perl +103 -45
index cc6bd0c..e4378b9 100755
@@ -2837,6 +2837,58 @@ sub git_heads_body {
 	print "</table>\n";
 }
 
+sub git_search_grep_body {
+	my ($greplist, $from, $to, $extra) = @_;
+	$from = 0 unless defined $from;
+	$to = $#{$greplist} if (!defined $to || $#{$greplist} < $to);
+
+	print "<table class=\"grep\" cellspacing=\"0\">\n";
+	my $alternate = 1;
+	for (my $i = $from; $i <= $to; $i++) {
+		my $commit = $greplist->[$i];
+		my %co = parse_commit($commit);
+		if (!%co) {
+			next;
+		}
+		if ($alternate) {
+			print "<tr class=\"dark\">\n";
+		} else {
+			print "<tr class=\"light\">\n";
+		}
+		$alternate ^= 1;
+		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
+		      "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 5)) . "</i></td>\n" .
+		      "<td>" .
+		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}), -class => "list subject"},
+			       esc_html(chop_str($co{'title'}, 50)) . "<br/>");
+		my $comment = $co{'comment'};
+		foreach my $line (@$comment) {
+			if ($line =~ m/^(.*)($searchtext)(.*)$/i) {
+				my $lead = esc_html($1) || "";
+				$lead = chop_str($lead, 30, 10);
+				my $match = esc_html($2) || "";
+				my $trail = esc_html($3) || "";
+				$trail = chop_str($trail, 30, 10);
+				my $text = "$lead<span class=\"match\">$match</span>$trail";
+				print chop_str($text, 80, 5) . "<br/>\n";
+			}
+		}
+		print "</td>\n" .
+		      "<td class=\"link\">" .
+		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'})}, "commit") .
+		      " | " .
+		      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$co{'id'})}, "tree");
+		print "</td>\n" .
+		      "</tr>\n";
+	}
+	if (defined $extra) {
+		print "<tr>\n" .
+		      "<td colspan=\"3\">$extra</td>\n" .
+		      "</tr>\n";
+	}
+	print "</table>\n";
+}
+
 ## ======================================================================
 ## ======================================================================
 ## actions
@@ -4154,6 +4206,9 @@ sub git_search {
 	if (!%co) {
 		die_error(undef, "Unknown commit object");
 	}
+	if (!defined $page) {
+		$page = 0;
+	}
 
 	$searchtype ||= 'commit';
 	if ($searchtype eq 'pickaxe') {
@@ -4166,11 +4221,7 @@ sub git_search {
 	}
 
 	git_header_html();
-	git_print_page_nav('','', $hash,$co{'tree'},$hash);
-	git_print_header_div('commit', esc_html($co{'title'}), $hash);
 
-	print "<table cellspacing=\"0\">\n";
-	my $alternate = 1;
 	if ($searchtype eq 'commit' or $searchtype eq 'author' or $searchtype eq 'committer') {
 		my $greptype;
 		if ($searchtype eq 'commit') {
@@ -4180,52 +4231,58 @@ sub git_search {
 		} elsif ($searchtype eq 'committer') {
 			$greptype = "--committer=";
 		}
-		$/ = "\0";
 		open my $fd, "-|", git_cmd(), "rev-list",
-			"--header", "--parents", ($greptype . $searchtext),
-			 $hash, "--"
+			("--max-count=" . (100 * ($page+1))),
+			($greptype . $searchtext),
+			$hash, "--"
 			or next;
-		while (my $commit_text = <$fd>) {
-			my @commit_lines = split "\n", $commit_text;
-			my %co = parse_commit(undef, \@commit_lines);
-			if (!%co) {
-				next;
-			}
-			if ($alternate) {
-				print "<tr class=\"dark\">\n";
-			} else {
-				print "<tr class=\"light\">\n";
-			}
-			$alternate ^= 1;
-			print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
-			      "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 5)) . "</i></td>\n" .
-			      "<td>" .
-			      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}), -class => "list subject"},
-			               esc_html(chop_str($co{'title'}, 50)) . "<br/>");
-			my $comment = $co{'comment'};
-			foreach my $line (@$comment) {
-				if ($line =~ m/^(.*)($searchtext)(.*)$/i) {
-					my $lead = esc_html($1) || "";
-					$lead = chop_str($lead, 30, 10);
-					my $match = esc_html($2) || "";
-					my $trail = esc_html($3) || "";
-					$trail = chop_str($trail, 30, 10);
-					my $text = "$lead<span class=\"match\">$match</span>$trail";
-					print chop_str($text, 80, 5) . "<br/>\n";
-				}
-			}
-			print "</td>\n" .
-			      "<td class=\"link\">" .
-			      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'})}, "commit") .
-			      " | " .
-			      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$co{'id'})}, "tree");
-			print "</td>\n" .
-			      "</tr>\n";
-		}
+		my @revlist = map { chomp; $_ } <$fd>;
 		close $fd;
+
+		my $paging_nav = '';
+		if ($page > 0) {
+			$paging_nav .=
+				$cgi->a({-href => href(action=>"search", hash=>$hash,
+						       searchtext=>$searchtext, searchtype=>$searchtype)},
+					"first");
+			$paging_nav .= " &sdot; " .
+				$cgi->a({-href => href(action=>"search", hash=>$hash,
+						       searchtext=>$searchtext, searchtype=>$searchtype,
+						       page=>$page-1),
+					 -accesskey => "p", -title => "Alt-p"}, "prev");
+		} else {
+			$paging_nav .= "first";
+			$paging_nav .= " &sdot; prev";
+		}
+		if ($#revlist >= (100 * ($page+1)-1)) {
+			$paging_nav .= " &sdot; " .
+				$cgi->a({-href => href(action=>"search", hash=>$hash,
+						       searchtext=>$searchtext, searchtype=>$searchtype,
+						       page=>$page+1),
+					 -accesskey => "n", -title => "Alt-n"}, "next");
+		} else {
+			$paging_nav .= " &sdot; next";
+		}
+		my $next_link = '';
+		if ($#revlist >= (100 * ($page+1)-1)) {
+			$next_link =
+				$cgi->a({-href => href(action=>"search", hash=>$hash,
+						       searchtext=>$searchtext, searchtype=>$searchtype,
+						       page=>$page+1),
+					 -accesskey => "n", -title => "Alt-n"}, "next");
+		}
+
+		git_print_page_nav('','', $hash,$co{'tree'},$hash, $paging_nav);
+		git_print_header_div('commit', esc_html($co{'title'}), $hash);
+		git_search_grep_body(\@revlist, ($page * 100), $#revlist, $next_link);
 	}
 
 	if ($searchtype eq 'pickaxe') {
+		git_print_page_nav('','', $hash,$co{'tree'},$hash);
+		git_print_header_div('commit', esc_html($co{'title'}), $hash);
+
+		print "<table cellspacing=\"0\">\n";
+		my $alternate = 1;
 		$/ = "\n";
 		my $git_command = git_cmd_str();
 		open my $fd, "-|", "$git_command rev-list $hash | " .
@@ -4280,8 +4337,9 @@ sub git_search {
 			}
 		}
 		close $fd;
+
+		print "</table>\n";
 	}
-	print "</table>\n";
 	git_footer_html();
 }
 
Signature · 1 line
1.4.4.3.gae7ae3
#7 lore raw

Re: [PATCH] gitweb: Paginate commit/author/committer search output

Quoted 6 lines · Robert Fitzsimons

Paginate commit/author/committer search output to only show 100 commits at a time, added appropriate nav links.

---

Quoted 7 lines
Quoted 4 lines

Although with search you have additional complication with marking match, and "log" view like rather than "shortlog" like view... so I'm not sure if it would truly help. On the other hand you can use --skip option you have introduced...

I used the slower non--skip workflow for the moment, so at least there is no need to upgrade the core git commands.

First, git has tradition of introducing options (first) meant for gitweb, and immediately making use of them. Examples: --git-dir=<path> option to git wrapper because in mod_perl doesn't pass environmental variables to subprocesses so setting $ENV{'GIT_DIR'} in gitweb wouldn't work; --full-history option to git-rev-list for "history" view, because using path limit instead of piping to git-diff-tree and using path limit of git-diff-tree changed returned revisions, git-for-each-ref introduced for better gitweb performance in "summary" view... So you wouldn't do something unusual. And it is fairly easy to compile and install additional, newest version of git.

Second, without --skip you have ugly tradeoff if you want to paginate (search result, but not only that): either get pages*page-size revisions and call parse_commit which in turn usually calls git-rev-list page-size times; or get full info pages*page-size and skip (pages - 1)*page-size bits of output.

And finally, --skip with your abandoned for now parsing revisions not one by one, but by a bunch using one git command call would help performance not only of non-pickaxe search, but also history view, and log and shortlog views.

[...]

Quoted 1 line

+sub git_search_grep_body {

I'm not sure if it wouldn't be better to try to reuse git_log machinery, just adding marking match, and removing everything but the immediate context of match, to format_log_line_html... Just a thought...

Signature · 2 lines
Jakub Narebski
Poland
#8 lore raw
In reply to Robert Fitzsimons

I forgot the --compose flag on these.

The patch 1 is just a rewrite to take advantage of the native search support in rev-list, patch 2 and 3 are a slight change in functionality.

Robert

#9 lore raw
In reply to Robert Fitzsimons
Quoted 1 line · Robert Fitzsimons

Use rev-list pattern search options instead of hand coded perl.

Very nice. Ack (FWIW).

Signature · 3 lines
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git