[PATCH 0/5] gitweb: Improve search code

[PATCH 0/5] gitweb: Improve search code

[Jakub Narebski]

Originally I have intended to go with it further, and improve how
search results look like, for example by using 'log' view code to
display 'commit', 'author' and 'committer' search results, now managed
by git_search_message.

But then I realized that I would need committags machinery for that.
So what we are left with are some code refactoring and cleanups.

Not for 1.7.6 (not a regression fix).

Jakub Narebski (5):
  gitweb: 'pickaxe' and 'grep' features requires 'search' to be enabled
  gitweb: Check permissions first in git_search
  gitweb: Split body of git_search into subroutines
  gitweb: Clean up code in git_search_* subroutines
  gitweb: Make git_search_* subroutines render whole pages

 gitweb/gitweb.perl |  442 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 243 insertions(+), 199 deletions(-)


P.S. Another addition that I sometimes wanted git to have would be
'filename' or 'find' search: searching for file by name.

What do you think of it?
-- 
1.7.5

[PATCHv3 1/5] gitweb: 'pickaxe' and 'grep' features requires 'search' to be enabled

[Jakub Narebski]

Both 'pickaxe' (searching changes) and 'grep' (searching files)
require basic 'search' feature to be enabled to work.  Enabling
e.g. only 'pickaxe' won't work.

Add a comment about this.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is slightly more expanded version than what was sent to git
mailing list as

  [PATCH (amend)] gitweb: 'pickaxe' and 'grep' features requires 'search' to be enabled
  http://thread.gmane.org/gmane.comp.version-control.git/176095/focus=176127

What's new is comment for 'search' feature.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0221eb9..fd93d45 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -321,6 +321,10 @@ our %feature = (
 	# Enable text search, which will list the commits which match author,
 	# committer or commit text to a given string.  Enabled by default.
 	# Project specific override is not supported.
+	#
+	# Note that this controls all search features, which means that if
+	# it is disabled, then 'grep' and 'pickaxe' search would also be
+	# disabled.
 	'search' => {
 		'override' => 0,
 		'default' => [1]},
@@ -328,6 +332,7 @@ our %feature = (
 	# Enable grep search, which will list the files in currently selected
 	# tree containing the given string. Enabled by default. This can be
 	# potentially CPU-intensive, of course.
+	# Note that you need to have 'search' feature enabled too.
 
 	# To enable system wide have in $GITWEB_CONFIG
 	# $feature{'grep'}{'default'} = [1];
@@ -342,6 +347,7 @@ our %feature = (
 	# 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.
+	# Note that you need to have 'search' feature enabled too.
 
 	# To enable system wide have in $GITWEB_CONFIG
 	# $feature{'pickaxe'}{'default'} = [1];
-- 
1.7.5

[PATCH 2/5] gitweb: Check permissions first in git_search

[Jakub Narebski]

Check first if relevant features: 'search', 'pickaxe', 'grep', as
appropriate, are enabled before doing anything else in git_search.
This should make git_search code more clear.

While at it, expand a bit error message (e.g. 'Pickaxe' ->
'Pickaxe search').

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   30 +++++++++++++++++-------------
 1 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index fd93d45..32e50b4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7094,7 +7094,23 @@ sub git_history {
 }
 
 sub git_search {
-	gitweb_check_feature('search') or die_error(403, "Search is disabled");
+	$searchtype ||= 'commit';
+
+	# check if appropriate features are enabled
+	gitweb_check_feature('search')
+		or die_error(403, "Search is disabled");
+	if ($searchtype eq 'pickaxe') {
+		# pickaxe may take all resources of your box and run for several minutes
+		# with every query - so decide by yourself how public you make this feature
+		gitweb_check_feature('pickaxe')
+			or die_error(403, "Pickaxe search is disabled");
+	}
+	if ($searchtype eq 'grep') {
+		# grep search might be potentially CPU-intensive, too
+		gitweb_check_feature('grep')
+			or die_error(403, "Grep search is disabled");
+	}
+
 	if (!defined $searchtext) {
 		die_error(400, "Text field is empty");
 	}
@@ -7109,18 +7125,6 @@ sub git_search {
 		$page = 0;
 	}
 
-	$searchtype ||= 'commit';
-	if ($searchtype eq 'pickaxe') {
-		# pickaxe may take all resources of your box and run for several minutes
-		# with every query - so decide by yourself how public you make this feature
-		gitweb_check_feature('pickaxe')
-		    or die_error(403, "Pickaxe is disabled");
-	}
-	if ($searchtype eq 'grep') {
-		gitweb_check_feature('grep')
-		    or die_error(403, "Grep is disabled");
-	}
-
 	git_header_html();
 
 	if ($searchtype eq 'commit' or $searchtype eq 'author' or $searchtype eq 'committer') {
-- 
1.7.5

[PATCH 3/5] gitweb: Split body of git_search into subroutines

[Jakub Narebski]

Create separate subroutines for handling each of aspects of searching
the repository:
* git_search_mesaage ('commit', 'author', 'committer')
* git_search_changes ('pickaxe')
* git_search_content_of_files ('grep')

Almost pure code movement (and unindent), which you can check e.g. via

  $ git blame -w --date=short -C -C HEAD^.. -- gitweb/gitweb.perl |
    grep -C 3 -e '^[^^]' | less -S

(assuming that you have this version checked out).  No functional
changes.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |  381 +++++++++++++++++++++++++++-------------------------
 1 files changed, 199 insertions(+), 182 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 32e50b4..c350c05 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5540,6 +5540,197 @@ sub git_remotes_body {
 	}
 }
 
+sub git_search_message {
+	my %co = @_;
+
+	my $greptype;
+	if ($searchtype eq 'commit') {
+		$greptype = "--grep=";
+	} elsif ($searchtype eq 'author') {
+		$greptype = "--author=";
+	} elsif ($searchtype eq 'committer') {
+		$greptype = "--committer=";
+	}
+	$greptype .= $searchtext;
+	my @commitlist = parse_commits($hash, 101, (100 * $page), undef,
+	                               $greptype, '--regexp-ignore-case',
+	                               $search_use_regexp ? '--extended-regexp' : '--fixed-strings');
+
+	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(-replay=>1, page=>$page-1),
+			         -accesskey => "p", -title => "Alt-p"}, "prev");
+	} else {
+		$paging_nav .= "first";
+		$paging_nav .= " &sdot; prev";
+	}
+	my $next_link = '';
+	if ($#commitlist >= 100) {
+		$next_link =
+			$cgi->a({-href => href(-replay=>1, page=>$page+1),
+			         -accesskey => "n", -title => "Alt-n"}, "next");
+		$paging_nav .= " &sdot; $next_link";
+	} else {
+		$paging_nav .= " &sdot; next";
+	}
+
+	git_print_page_nav('','', $hash,$co{'tree'},$hash, $paging_nav);
+	git_print_header_div('commit', esc_html($co{'title'}), $hash);
+	if ($page == 0 && !@commitlist) {
+		print "<p>No match.</p>\n";
+	} else {
+		git_search_grep_body(\@commitlist, 0, 99, $next_link);
+	}
+}
+
+sub git_search_changes {
+	my %co = @_;
+
+	git_print_page_nav('','', $hash,$co{'tree'},$hash);
+	git_print_header_div('commit', esc_html($co{'title'}), $hash);
+
+	print "<table class=\"pickaxe search\">\n";
+	my $alternate = 1;
+	local $/ = "\n";
+	open my $fd, '-|', git_cmd(), '--no-pager', 'log', @diff_opts,
+		'--pretty=format:%H', '--no-abbrev', '--raw', "-S$searchtext",
+		($search_use_regexp ? '--pickaxe-regex' : ());
+	undef %co;
+	my @files;
+	while (my $line = <$fd>) {
+		chomp $line;
+		next unless $line;
+
+		my %set = parse_difftree_raw_line($line);
+		if (defined $set{'commit'}) {
+			# finish previous commit
+			if (%co) {
+				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 ($alternate) {
+				print "<tr class=\"dark\">\n";
+			} else {
+				print "<tr class=\"light\">\n";
+			}
+			$alternate ^= 1;
+			%co = parse_commit($set{'commit'});
+			my $author = chop_and_escape_str($co{'author_name'}, 15, 5);
+			print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
+			      "<td><i>$author</i></td>\n" .
+			      "<td>" .
+			      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}),
+			              -class => "list subject"},
+			              chop_and_escape_str($co{'title'}, 50) . "<br/>");
+		} elsif (defined $set{'to_id'}) {
+			next if ($set{'to_id'} =~ m/^0{40}$/);
+
+			print $cgi->a({-href => href(action=>"blob", hash_base=>$co{'id'},
+			                             hash=>$set{'to_id'}, file_name=>$set{'to_file'}),
+			              -class => "list"},
+			              "<span class=\"match\">" . esc_path($set{'file'}) . "</span>") .
+			      "<br/>\n";
+		}
+	}
+	close $fd;
+
+	# finish last commit (warning: repetition!)
+	if (%co) {
+		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";
+	}
+
+	print "</table>\n";
+}
+
+sub git_search_files {
+	my %co = @_;
+
+	git_print_page_nav('','', $hash,$co{'tree'},$hash);
+	git_print_header_div('commit', esc_html($co{'title'}), $hash);
+
+	print "<table class=\"grep_search\">\n";
+	my $alternate = 1;
+	my $matches = 0;
+	local $/ = "\n";
+	open my $fd, "-|", git_cmd(), 'grep', '-n',
+		$search_use_regexp ? ('-E', '-i') : '-F',
+		$searchtext, $co{'tree'};
+	my $lastfile = '';
+	while (my $line = <$fd>) {
+		chomp $line;
+		my ($file, $lno, $ltext, $binary);
+		last if ($matches++ > 1000);
+		if ($line =~ /^Binary file (.+) matches$/) {
+			$file = $1;
+			$binary = 1;
+		} else {
+			(undef, $file, $lno, $ltext) = split(/:/, $line, 4);
+		}
+		if ($file ne $lastfile) {
+			$lastfile and print "</td></tr>\n";
+			if ($alternate++) {
+				print "<tr class=\"dark\">\n";
+			} else {
+				print "<tr class=\"light\">\n";
+			}
+			print "<td class=\"list\">".
+				$cgi->a({-href => href(action=>"blob", hash=>$co{'hash'},
+						       file_name=>"$file"),
+					-class => "list"}, esc_path($file));
+			print "</td><td>\n";
+			$lastfile = $file;
+		}
+		if ($binary) {
+			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);
+			}
+			print "<div class=\"pre\">" .
+				$cgi->a({-href => href(action=>"blob", hash=>$co{'hash'},
+						       file_name=>"$file").'#l'.$lno,
+					-class => "linenr"}, sprintf('%4i', $lno))
+				. ' ' .  $ltext . "</div>\n";
+		}
+	}
+	if ($lastfile) {
+		print "</td></tr>\n";
+		if ($matches > 1000) {
+			print "<div class=\"diff nodifferences\">Too many matches, listing trimmed</div>\n";
+		}
+	} else {
+		print "<div class=\"diff nodifferences\">No matches found</div>\n";
+	}
+	close $fd;
+
+	print "</table>\n";
+}
+
 sub git_search_grep_body {
 	my ($commitlist, $from, $to, $extra) = @_;
 	$from = 0 unless defined $from;
@@ -7127,190 +7318,16 @@ sub git_search {
 
 	git_header_html();
 
-	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=";
-		}
-		$greptype .= $searchtext;
-		my @commitlist = parse_commits($hash, 101, (100 * $page), undef,
-		                               $greptype, '--regexp-ignore-case',
-		                               $search_use_regexp ? '--extended-regexp' : '--fixed-strings');
-
-		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(-replay=>1, page=>$page-1),
-				         -accesskey => "p", -title => "Alt-p"}, "prev");
-		} else {
-			$paging_nav .= "first";
-			$paging_nav .= " &sdot; prev";
-		}
-		my $next_link = '';
-		if ($#commitlist >= 100) {
-			$next_link =
-				$cgi->a({-href => href(-replay=>1, page=>$page+1),
-				         -accesskey => "n", -title => "Alt-n"}, "next");
-			$paging_nav .= " &sdot; $next_link";
-		} else {
-			$paging_nav .= " &sdot; next";
-		}
-
-		git_print_page_nav('','', $hash,$co{'tree'},$hash, $paging_nav);
-		git_print_header_div('commit', esc_html($co{'title'}), $hash);
-		if ($page == 0 && !@commitlist) {
-			print "<p>No match.</p>\n";
-		} else {
-			git_search_grep_body(\@commitlist, 0, 99, $next_link);
-		}
+	if ($searchtype eq 'commit' ||
+	    $searchtype eq 'author' ||
+	    $searchtype eq 'committer') {
+		git_search_message(%co);
+	} elsif ($searchtype eq 'pickaxe') {
+		git_search_changes(%co);
+	} elsif ($searchtype eq 'grep') {
+		git_search_files(%co);
 	}
 
-	if ($searchtype eq 'pickaxe') {
-		git_print_page_nav('','', $hash,$co{'tree'},$hash);
-		git_print_header_div('commit', esc_html($co{'title'}), $hash);
-
-		print "<table class=\"pickaxe search\">\n";
-		my $alternate = 1;
-		local $/ = "\n";
-		open my $fd, '-|', git_cmd(), '--no-pager', 'log', @diff_opts,
-			'--pretty=format:%H', '--no-abbrev', '--raw', "-S$searchtext",
-			($search_use_regexp ? '--pickaxe-regex' : ());
-		undef %co;
-		my @files;
-		while (my $line = <$fd>) {
-			chomp $line;
-			next unless $line;
-
-			my %set = parse_difftree_raw_line($line);
-			if (defined $set{'commit'}) {
-				# finish previous commit
-				if (%co) {
-					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 ($alternate) {
-					print "<tr class=\"dark\">\n";
-				} else {
-					print "<tr class=\"light\">\n";
-				}
-				$alternate ^= 1;
-				%co = parse_commit($set{'commit'});
-				my $author = chop_and_escape_str($co{'author_name'}, 15, 5);
-				print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
-				      "<td><i>$author</i></td>\n" .
-				      "<td>" .
-				      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}),
-				              -class => "list subject"},
-				              chop_and_escape_str($co{'title'}, 50) . "<br/>");
-			} elsif (defined $set{'to_id'}) {
-				next if ($set{'to_id'} =~ m/^0{40}$/);
-
-				print $cgi->a({-href => href(action=>"blob", hash_base=>$co{'id'},
-				                             hash=>$set{'to_id'}, file_name=>$set{'to_file'}),
-				              -class => "list"},
-				              "<span class=\"match\">" . esc_path($set{'file'}) . "</span>") .
-				      "<br/>\n";
-			}
-		}
-		close $fd;
-
-		# finish last commit (warning: repetition!)
-		if (%co) {
-			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";
-		}
-
-		print "</table>\n";
-	}
-
-	if ($searchtype eq 'grep') {
-		git_print_page_nav('','', $hash,$co{'tree'},$hash);
-		git_print_header_div('commit', esc_html($co{'title'}), $hash);
-
-		print "<table class=\"grep_search\">\n";
-		my $alternate = 1;
-		my $matches = 0;
-		local $/ = "\n";
-		open my $fd, "-|", git_cmd(), 'grep', '-n',
-			$search_use_regexp ? ('-E', '-i') : '-F',
-			$searchtext, $co{'tree'};
-		my $lastfile = '';
-		while (my $line = <$fd>) {
-			chomp $line;
-			my ($file, $lno, $ltext, $binary);
-			last if ($matches++ > 1000);
-			if ($line =~ /^Binary file (.+) matches$/) {
-				$file = $1;
-				$binary = 1;
-			} else {
-				(undef, $file, $lno, $ltext) = split(/:/, $line, 4);
-			}
-			if ($file ne $lastfile) {
-				$lastfile and print "</td></tr>\n";
-				if ($alternate++) {
-					print "<tr class=\"dark\">\n";
-				} else {
-					print "<tr class=\"light\">\n";
-				}
-				print "<td class=\"list\">".
-					$cgi->a({-href => href(action=>"blob", hash=>$co{'hash'},
-							       file_name=>"$file"),
-						-class => "list"}, esc_path($file));
-				print "</td><td>\n";
-				$lastfile = $file;
-			}
-			if ($binary) {
-				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);
-				}
-				print "<div class=\"pre\">" .
-					$cgi->a({-href => href(action=>"blob", hash=>$co{'hash'},
-							       file_name=>"$file").'#l'.$lno,
-						-class => "linenr"}, sprintf('%4i', $lno))
-					. ' ' .  $ltext . "</div>\n";
-			}
-		}
-		if ($lastfile) {
-			print "</td></tr>\n";
-			if ($matches > 1000) {
-				print "<div class=\"diff nodifferences\">Too many matches, listing trimmed</div>\n";
-			}
-		} else {
-			print "<div class=\"diff nodifferences\">No matches found</div>\n";
-		}
-		close $fd;
-
-		print "</table>\n";
-	}
 	git_footer_html();
 }
 
-- 
1.7.5

[PATCH 4/5] gitweb: Clean up code in git_search_* subroutines

[Jakub Narebski]

1. Replace sequence of

     $foo .= "bar";
     $foo .= "baz";

   with

     $foo .= "bar" .
             "baz";

2. Use href(-replay=>1, -page=>undef) for first page of a multip-page
   view.

3. Rewrap (rearrange) some lines to reduce their length. Some lines
   still have more than 80 characters, but lines are shorter now.

No functional changes.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c350c05..1c06476 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5559,16 +5559,13 @@ sub git_search_message {
 	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(-replay=>1, page=>undef)},
+			        "first") .
+			" &sdot; " .
 			$cgi->a({-href => href(-replay=>1, page=>$page-1),
 			         -accesskey => "p", -title => "Alt-p"}, "prev");
 	} else {
-		$paging_nav .= "first";
-		$paging_nav .= " &sdot; prev";
+		$paging_nav .= "first &sdot; prev";
 	}
 	my $next_link = '';
 	if ($#commitlist >= 100) {
@@ -5613,10 +5610,13 @@ sub git_search_changes {
 			if (%co) {
 				print "</td>\n" .
 				      "<td class=\"link\">" .
-				      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'})}, "commit") .
+				      $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" .
+				      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'},
+				                             hash_base=>$co{'id'})},
+				              "tree") .
+				      "</td>\n" .
 				      "</tr>\n";
 			}
 
@@ -5650,10 +5650,13 @@ sub git_search_changes {
 	if (%co) {
 		print "</td>\n" .
 		      "<td class=\"link\">" .
-		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'})}, "commit") .
+		      $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" .
+		      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'},
+		                             hash_base=>$co{'id'})},
+		              "tree") .
+		      "</td>\n" .
 		      "</tr>\n";
 	}
 
-- 
1.7.5

[PATCH 5/5] gitweb: Make git_search_* subroutines render whole pages

[Jakub Narebski]

This means moving git_header_html() and git_footer_html() invocation
from git_search() to individual git_search_* subroutines.

While at it reorganize search-related code a bit, moving invoking of
git commands before any output is generated.


This has the following advantages:

* gitweb now shows an error page if there was unknown search type
  (evaluate_and_validate_params checks only that it looks sanely);
  remember that we shouldn't call die_error after any output.

* git_search_message is now safe agains die_error in parse_commits
  (though this is very unlikely).

* gitweb now can check errors while invoking git commands and show
  error page (again, quite unlikely).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   38 ++++++++++++++++++++++++++------------
 1 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1c06476..f8df027 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5577,6 +5577,8 @@ sub git_search_message {
 		$paging_nav .= " &sdot; next";
 	}
 
+	git_header_html();
+
 	git_print_page_nav('','', $hash,$co{'tree'},$hash, $paging_nav);
 	git_print_header_div('commit', esc_html($co{'title'}), $hash);
 	if ($page == 0 && !@commitlist) {
@@ -5584,20 +5586,26 @@ sub git_search_message {
 	} else {
 		git_search_grep_body(\@commitlist, 0, 99, $next_link);
 	}
+
+	git_footer_html();
 }
 
 sub git_search_changes {
 	my %co = @_;
 
+	local $/ = "\n";
+	open my $fd, '-|', git_cmd(), '--no-pager', 'log', @diff_opts,
+		'--pretty=format:%H', '--no-abbrev', '--raw', "-S$searchtext",
+		($search_use_regexp ? '--pickaxe-regex' : ())
+			or die_error(500, "Open git-log failed");
+
+	git_header_html();
+
 	git_print_page_nav('','', $hash,$co{'tree'},$hash);
 	git_print_header_div('commit', esc_html($co{'title'}), $hash);
 
 	print "<table class=\"pickaxe search\">\n";
 	my $alternate = 1;
-	local $/ = "\n";
-	open my $fd, '-|', git_cmd(), '--no-pager', 'log', @diff_opts,
-		'--pretty=format:%H', '--no-abbrev', '--raw', "-S$searchtext",
-		($search_use_regexp ? '--pickaxe-regex' : ());
 	undef %co;
 	my @files;
 	while (my $line = <$fd>) {
@@ -5661,21 +5669,27 @@ sub git_search_changes {
 	}
 
 	print "</table>\n";
+
+	git_footer_html();
 }
 
 sub git_search_files {
 	my %co = @_;
 
+	local $/ = "\n";
+	open my $fd, "-|", git_cmd(), 'grep', '-n',
+		$search_use_regexp ? ('-E', '-i') : '-F',
+		$searchtext, $co{'tree'}
+			or die_error(500, "Open git-grep failed");
+
+	git_header_html();
+
 	git_print_page_nav('','', $hash,$co{'tree'},$hash);
 	git_print_header_div('commit', esc_html($co{'title'}), $hash);
 
 	print "<table class=\"grep_search\">\n";
 	my $alternate = 1;
 	my $matches = 0;
-	local $/ = "\n";
-	open my $fd, "-|", git_cmd(), 'grep', '-n',
-		$search_use_regexp ? ('-E', '-i') : '-F',
-		$searchtext, $co{'tree'};
 	my $lastfile = '';
 	while (my $line = <$fd>) {
 		chomp $line;
@@ -5732,6 +5746,8 @@ sub git_search_files {
 	close $fd;
 
 	print "</table>\n";
+
+	git_footer_html();
 }
 
 sub git_search_grep_body {
@@ -7319,8 +7335,6 @@ sub git_search {
 		$page = 0;
 	}
 
-	git_header_html();
-
 	if ($searchtype eq 'commit' ||
 	    $searchtype eq 'author' ||
 	    $searchtype eq 'committer') {
@@ -7329,9 +7343,9 @@ sub git_search {
 		git_search_changes(%co);
 	} elsif ($searchtype eq 'grep') {
 		git_search_files(%co);
+	} else {
+		die_error(400, "Unknown search type");
 	}
-
-	git_footer_html();
 }
 
 sub git_search_help {
-- 
1.7.5

Re: [PATCH 0/5] gitweb: Improve search code

[Phil Hord]

Admin note: Should I have changed the subject or kept it?

On 06/22/2011 11:28 AM, Jakub Narebski wrote:
> P.S. Another addition that I sometimes wanted git to have would be
> 'filename' or 'find' search: searching for file by name.
>
> What do you think of it?

I like it.  +1 from me.

I normally revert to 'git log --name-status | less <CR> /filename.foo',
which is awful, of course.  I've always assumed there's a better way I
haven't discovered yet.  But if there is, well... I haven't discovered
it yet.

This syntax works on some files, but is limited and/or broken:
   # Finds all commits touching the file named './foo.bar', iff
./foo.bar exists in the current commit.
   git log -- foo.bar

I say 'broken', but maybe it's not; it feels like it is when I do this:

   # Returns zero logs
   git log -- some-deleted-file.txt

   # Returns at least two logs
   git log --all -- some-deleted-file.txt

I think I understand why that happens (search optimization), but it is
unexpected from the user's perspective.  I also suspect it will miss the
'pre-resurrection' commits for files which were deleted and resurrected
in the past.


What do you think of these as new 'Commit limiters' for git log:

       --name=<pattern>, --name-glob=<glob>
           Limit the commits output to ones touching files that match the
           specified pattern (regular expression) or glob (shell glob
pattern).

This could be further modified by --diff-filter, --all-match, etc.

   # Find all commits which deleted a file named **/foo.**
   git log --name="/foo[.]" --diff-filter=D

   # Find all commits which deleted a file named **/foo.**
   #   or one named **/bar.**
   git log --name="/foo[.]" --name="/bar[.]" --diff-filter=D

   # Find all commits which touched both a file named **/foo.**
   #   and one named **/bar.**
   git log --name="/foo[.]" --name="/bar[.]" --all-match


Or this alternate form:

       --name-pattern, --name-glob
           Treat all <path> selectors as pattern (regular expression) or
glob (shell glob pattern).

I think I prefer the former, since the latter does not allow for a
combination of raw names, pattern names and glob names.  Also, it does
not (intuitively) support --all-match.

   # Find all commits which deleted a file named **/foo.**
   git log --name-pattern --diff-filter=D -- "/foo[.]"
   git log --name-glob --diff-filter=D -- "/foo.*"

   # Find all commits which deleted a file named **/foo.**
   #   or one named **/bar.**
   git log --name-pattern --diff-filter=D -- "/foo[.]" "/bar[.]"

   # Find all commits which touched both a file named **/foo.**
   #   and one named **/bar.**
   #  -- Can't be done?

Finally, do the extensions belong on rev-list as well?  The 'git
rev-list' command seems underpowered compared to 'git log'.

Phil

Re: [PATCH 0/5] gitweb: Improve search code

[Jakub Narebski]

On Wed, 22 Jun 2011, Phil Hord wrote:
> On 06/22/2011 11:28 AM, Jakub Narebski wrote:

> > P.S. Another addition that I sometimes wanted git to have would be
                                                  ^^^

This is a typo: I actually wanted to say "gitweb" here.

Therefore this discussion is totally OFF-TOPIC now.

> > 'filename' or 'find' search: searching for file by name.
> >
> > What do you think of it?
> 
> I like it.  +1 from me.
> 
> I normally revert to 'git log --name-status | less <CR> /filename.foo',
> which is awful, of course.  I've always assumed there's a better way I
> haven't discovered yet.  But if there is, well... I haven't discovered
> it yet.
> 
> This syntax works on some files, but is limited and/or broken:
>    # Finds all commits touching the file named './foo.bar', iff
> ./foo.bar exists in the current commit.
>    git log -- foo.bar

Errr... if you use "git log foo.bar" it is true, but "git log -- foo.bar"
will find commits even if foo.bar existed only in the past... though
history simplification can make git return empty set.

> 
> I say 'broken', but maybe it's not; it feels like it is when I do this:
> 
>    # Returns zero logs
>    git log -- some-deleted-file.txt
> 
>    # Returns at least two logs
>    git log --all -- some-deleted-file.txt

     git log --full-history -- some-deleted-file.txt

For example in git.git repository:

   $ # git log --full-history --oneline -- gitweb.pl | cat
   2ad9331 v053
   185f09e v049
   ff7669a v048
   fbb592a v043
   [...]
   e0389bd v001
   ecb378f v000
   4c02e3c v000
   161332a first working version

(Don't you just love Kay Sievers commit messages ;-) ?).

> I think I understand why that happens (search optimization), but it is
> unexpected from the user's perspective.  I also suspect it will miss the
> 'pre-resurrection' commits for files which were deleted and resurrected
> in the past.
> 
> 
> What do you think of these as new 'Commit limiters' for git log:
> 
>        --name=<pattern>, --name-glob=<glob>
>            Limit the commits output to ones touching files that match the
>            specified pattern (regular expression) or glob (shell glob
>            pattern).

Why not use "git log --full-history -- '<glob>'" (i.e. remember about
shell escaping glob)?  I don't know if it works as intended in current
git or not...

-- 
Jakub Narebski
Poland

Re: [PATCH 0/5] gitweb: Improve search code

[Phil Hord]

On 06/22/2011 02:00 PM, Jakub Narebski wrote:
> On Wed, 22 Jun 2011, Phil Hord wrote:
>> On 06/22/2011 11:28 AM, Jakub Narebski wrote:
>>> P.S. Another addition that I sometimes wanted git to have would be
>                                                   ^^^
>
> This is a typo: I actually wanted to say "gitweb" here.
>
> Therefore this discussion is totally OFF-TOPIC now.
Dang.

>> I normally revert to 'git log --name-status | less <CR> /filename.foo',
>> which is awful, of course.  I've always assumed there's a better way I
>> haven't discovered yet.  But if there is, well... I haven't discovered
>> it yet.
>>
>> This syntax works on some files, but is limited and/or broken:
>>    # Finds all commits touching the file named './foo.bar', iff
>> ./foo.bar exists in the current commit.
>>    git log -- foo.bar
> Errr... if you use "git log foo.bar" it is true, but "git log -- foo.bar"
> will find commits even if foo.bar existed only in the past... though
> history simplification can make git return empty set.

That doesn't work for me.  Does it work for you?  I only see the
gitweb.pl history if I include --full-history.

    git version
    #-- git version 1.7.5.rc1

    git log -- gitweb.pl | wc
    #--   0       0       0

    git log --full-history -- gitweb.pl | wc
    #--    175     412    4087


>> I say 'broken', but maybe it's not; it feels like it is when I do this:
>>
>>    # Returns zero logs
>>    git log -- some-deleted-file.txt
>>
>>    # Returns at least two logs
>>    git log --all -- some-deleted-file.txt
>      git log --full-history -- some-deleted-file.txt
>

Thanks very much.  That is exactly what I was missing.

I obviously didn't know about --full-history.  I didn't notice it in
spite of looking for this before and consulting 'git help log'.  So I'm
guessing the documentation could use some help there.  It's already
pretty crowded, though.  Maybe just a note next to '--all' which, for
me, served as a distractingly red herring.

> For example in git.git repository:
>
>    $ # git log --full-history --oneline -- gitweb.pl | cat
>    2ad9331 v053
>    185f09e v049
>    ff7669a v048
>    fbb592a v043
>    [...]
>    e0389bd v001
>    ecb378f v000
>    4c02e3c v000
>    161332a first working version
>
> (Don't you just love Kay Sievers commit messages ;-) ?).
>
>> I think I understand why that happens (search optimization), but it is
>> unexpected from the user's perspective.  I also suspect it will miss the
>> 'pre-resurrection' commits for files which were deleted and resurrected
>> in the past.
>>
>>
>> What do you think of these as new 'Commit limiters' for git log:
>>
>>        --name=<pattern>, --name-glob=<glob>
>>            Limit the commits output to ones touching files that match the
>>            specified pattern (regular expression) or glob (shell glob
>>            pattern).
> Why not use "git log --full-history -- '<glob>'" (i.e. remember about
> shell escaping glob)?  I don't know if it works as intended in current
> git or not...
>

I think that'll work fine.  But glob paths failed for me when I tried it
while composing the original email.  I think I just mis-interpreted the
reason for the missing commits when I didn't have --full-history.

Also, globbing is not mentioned in the '[--] <path>' section of 'git log
--help'.  In fact, the only references to globs in the 'git log' help
[1] are related to refs, not filenames.  Ditto for 'git rev-list'.

I initially expected "git log -- 'foo.*'" to work.  But it didn't (foo.*
had been deleted) or it only partially worked (no --full-history).  So I
interpreted this as it being only partially supported.  I did consult
the man page, but it did not do anything to correct my wrong
interpretation.  A now-enwisened google search turned up a thread [2]
from 2009 apparently pre-dating this feature.

Thanks again for the education, Jakub.  I really appreciate how helpful
this list is.

[1] excepting '-O'
[2] http://article.gmane.org/gmane.comp.version-control.git/109506/


Phil

Re: [PATCH 0/5] gitweb: Improve search code

[Junio C Hamano]

Looks good. Will queue.