[BUG] gitweb generates wrong links in grep search results (git_search_files)

[BUG] gitweb generates wrong links in grep search results (git_search_files)

[Thomas Perl]

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

Re: [BUG] gitweb generates wrong links in grep search results (git_search_files)

[Junio C Hamano]

Thomas Perl <th.perl@gmail.com> writes:

> 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:

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

Jakub, care to take a look?

>
> 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

Re: [BUG] gitweb generates wrong links in grep search results (git_search_files)

[Jakub Narębski]

On Wed, Jan 4, 2012 at 1:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Thomas Perl <th.perl@gmail.com> writes:
>
>> 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:
>
> 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

[PATCH 1/2] gitweb: Fix file links in "grep" search

[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.

Reported-by: Thomas Perl <th.perl@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Wed, 4 Jan 2012, Jakub Narębski wrote:
> On Wed, Jan 4, 2012 at 1:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Thomas Perl <th.perl@gmail.com> writes:
>>
>>> 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:
>>
>> 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...

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(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index fc41b07..fa58156 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -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

[PATCH 2/2] gitweb: Harden "grep" search against filenames with ':'

[Jakub Narebski]

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.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
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(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index fa58156..f884dfe 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -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

Re: [PATCH 1/2] gitweb: Fix file links in "grep" search

[Thomas Perl]

Hi,

2012/1/5 Jakub Narebski <jnareb@gmail.com>:
> 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'.
> [...]
> Thomas, could you check if this fixes your issue?

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

Re: [PATCH 1/2] gitweb: Fix file links in "grep" search

[Junio C Hamano]

Thomas Perl <th.perl@gmail.com> writes:

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

Thanks.