gitweb generates wrong links in grep search results (git_search_files)

Hi,

I think I found a bug in gitweb when grep'ing for text in a branch different from "master". Here's how to reproduce it:

1. Have a project with a master branch and a branch different from master
2. Start gitweb for that project (e.g. using "git instaweb") and open it in a web browser
3. Switch to the non-master branch (e.g.
http://127.0.0.1:1234/?p=.git;a=shortlog;h=refs/heads/mynonmasterbranch)
4. In the top right search box, select "grep" in the combo box and enter a text that only appears in the non-master branch
5. Submit the search by pressing enter, you should be at:
http://127.0.0.1:1234/?p=.git&a=search&h=refs%2Fheads%2Fmynonmasterbranch&st=grep&s=somesearchtext

ACTUAL RESULT
In that list of results, you should now see some files matching the search - note that the links for the file names and the line numbers go to e.g. http://127.0.0.1:1234/?p=.git;a=blob;f=somefile.txt for a file "somefile.txt". The links therefore go to the master branch, while the search results refer to the non-master branch.

EXPECTED RESULT
The link should (presumably) go to
http://127.0.0.1:1234/?p=.git;a=blob;hb=refs%2Fheads%2Fmynonmasterbranch;f=somefile.txt
so that when the link is clicked, the right file (somefile.txt in mynonmasterbranch) is shown.

I also investigated a bit in where the problem happens, and nailed it down to: gitweb/gitweb.perl, sub git_search_files, line 5871 in commit 17b4e93d5b849293e6a3659bbc4075ed8a6e97e2 (current master tip of
https://github.com/gitster/git). I haven't looked at the intrinsics of the "href" sub, but I believe that it should somehow get the "h" parameter from the original page and incorporate it into the final link (as "hb" parameter?) to the file. The same fix that is applied there then also needs to be applied at line 5891 (same commit, same file).

No patch, because after several tries, I didn't get it to work, my Perl foo might not be up to the task, and I believe that someone more familiar with gitweb's code base might have an easier time to fix this.

Thanks,
Thomas

Thanks for a detailed report (and thanks for gpodder ;-).

Jakub, care to take a look?

I see the bug: it should be 'hash_base' not 'hash' in href() creating link to "blob" view in git_search_files().

I'll try to send a fix soon...

Jakub Narebski

There were two bugs in generating file links (links to "blob" view), one hidden by the other. The correct way of generating file link is

	href(action=>"blob", hash_base=>$co{'id'},
	     file_name=>$file);

It was $co{'hash'} (this key does not exist, and therefore this is undef), and 'hash' instead of 'hash_base'.

To have this fix applied in single place, this commit also reduces code duplication by saving file link (which is used for line links) in $file_href.

---

Actually there were two errors, one hiding the other...

Thomas, could you check if this fixes your issue?

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

index fc41b07..fa58156 100755
@@ -5852,7 +5852,7 @@ sub git_search_files {
 	my $lastfile = '';
 	while (my $line = <$fd>) {
 		chomp $line;
-		my ($file, $lno, $ltext, $binary);
+		my ($file, $file_href, $lno, $ltext, $binary);
 		last if ($matches++ > 1000);
 		if ($line =~ /^Binary file (.+) matches$/) {
 			$file = $1;

@@ -5867,10 +5867,10 @@ sub git_search_files {
 			} else {
 				print "<tr class=\"light\">\n";
 			}
+			$file_href = href(action=>"blob", hash_base=>$co{'id'},
+			                  file_name=>$file);
 			print "<td class=\"list\">".
-				$cgi->a({-href => href(action=>"blob", hash=>$co{'hash'},
-						       file_name=>"$file"),
-					-class => "list"}, esc_path($file));
+				$cgi->a({-href => $file_href, -class => "list"}, esc_path($file));
 			print "</td><td>\n";
 			$lastfile = $file;
 		}

@@ -5888,10 +5888,9 @@ sub git_search_files {
 				$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";
+				$cgi->a({-href => $file_href.'#l'.$lno,
+				        -class => "linenr"}, sprintf('%4i', $lno)) .
+				' ' .  $ltext . "</div>\n";
 		}
 	}
 	if ($lastfile) {

1.7.6

Run "git grep" in "grep" search with '-z' option, to be able to parse response also for files with filename containing ':' character. The ':' character is otherwise (without '-z') used to separate filename from line number and from matched line.

Note that this does not protect files with filename containing embedded newline. This would be hard but doable for text files, and harder or even currently impossible with binary files: git does not quote filename in

"Binary file <foo> matches"

message, but new --break and/or --header options to git-grep could help here.

---
This is what I did after fixing previous issue, after looking at current code. Hopefully nobody sane uses filenames with embedded newlines...

http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html

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

index fa58156..f884dfe 100755
@@ -5836,7 +5836,7 @@ sub git_search_files {
 	my %co = @_;
 
 	local $/ = "\n";
-	open my $fd, "-|", git_cmd(), 'grep', '-n',
+	open my $fd, "-|", git_cmd(), 'grep', '-n', '-z',
 		$search_use_regexp ? ('-E', '-i') : '-F',
 		$searchtext, $co{'tree'}
 			or die_error(500, "Open git-grep failed");

@@ -5858,7 +5858,8 @@ sub git_search_files {
 			$file = $1;
 			$binary = 1;
 		} else {
-			(undef, $file, $lno, $ltext) = split(/:/, $line, 4);
+			($file, $lno, $ltext) = split(/\0/, $line, 3);
+			$file =~ s/^$co{'tree'}://;
 		}
 		if ($file ne $lastfile) {
 			$lastfile and print "</td></tr>\n";

1.7.6

Hi,

2012/1/5 Jakub Narebski <[email protected]>:

Sorry for taking a bit longer to respond on this one, but I just got around to test all problematic cases that I described with the patch applied - it fixes the problem for me (i.e. I can successfully grep in non-master branches and then clicking the link brings me to the right location).

As far as I'm concerned, the patch can be applied and fixes the bug.

Thanks for the quick fix! :)
Thomas

Thanks.