threads / patch / 33443

patch, 4 partsgitweb: Fix broken blob action parameters on blob/commitdiff pages

Subject: [PATCH 4/4] gitweb: Fix broken blob action parameters on blob/commitdiff pages

## tl;dr

3 messages between Apr 8, 2013 and Apr 9, 2013. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Jürgen Kreileder· Apr 8, 2013, 20:10 UTC · lore

Fix broken blob action parameters on blobdiff and commitdiff pages by explicitly passing variables instead of relying on global ones.

(The broken parameters on blob links lead to blob pages which show the blob but with a hash instead of a commit message and have broken blob_plain (404 - Cannot find file) and tree links (404 - Reading tree failed))

Signed-off-by: Jürgen Kreileder <jk@blackdown.de>
---
 gitweb/gitweb.perl |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
Show changes to gitweb/gitweb.perl +8 −7
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1309196..995e8fb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2258,7 +2258,7 @@ sub format_extended_diff_header_line {
 
 # format from-file/to-file diff header
 sub format_diff_from_to_header {
-	my ($from_line, $to_line, $diffinfo, $from, $to, @parents) = @_;
+	my ($from_line, $to_line, $diffinfo, $from, $to, $hash, @parents) = @_;
 	my $line;
 	my $result = '';
 
@@ -2324,7 +2324,7 @@ sub format_diff_from_to_header {
 
 # create note for patch simplified by combined diff
 sub format_diff_cc_simplified {
-	my ($diffinfo, @parents) = @_;
+	my ($diffinfo, $hash, @parents) = @_;
 	my $result = '';
 
 	$result .= "<div class=\"diff header\">" .
@@ -3592,7 +3592,8 @@ sub parse_ls_tree_line {
 
 # generates _two_ hashes, references to which are passed as 2 and 3 argument
 sub parse_from_to_diffinfo {
-	my ($diffinfo, $from, $to, @parents) = @_;
+	my ($diffinfo, $from, $to, $hash, @parents) = @_;
+	my ($hash_parent) = $parents[0];
 
 	if ($diffinfo->{'nparents'}) {
 		# combined diff
@@ -5312,7 +5313,7 @@ sub git_patchset_body {
 			if ($is_combined) {
 				while ($to_name ne $diffinfo->{'to_file'}) {
 					print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n" .
-					      format_diff_cc_simplified($diffinfo, @hash_parents) .
+					      format_diff_cc_simplified($diffinfo, $hash, @hash_parents) .
 					      "</div>\n";  # class="patch"
 
 					$patch_idx++;
@@ -5324,7 +5325,7 @@ sub git_patchset_body {
 			}
 
 			# modifies %from, %to hashes
-			parse_from_to_diffinfo($diffinfo, \%from, \%to, @hash_parents);
+			parse_from_to_diffinfo($diffinfo, \%from, \%to, $hash, @hash_parents);
 
 			# this is first patch for raw difftree line with $patch_idx index
 			# we index @$difftree array from 0, but number patches from 1
@@ -5367,7 +5368,7 @@ sub git_patchset_body {
 
 		print format_diff_from_to_header($last_patch_line, $patch_line,
 		                                 $diffinfo, \%from, \%to,
-		                                 @hash_parents);
+		                                 $hash, @hash_parents);
 
 		# the patch itself
 	LINE:
@@ -5404,7 +5405,7 @@ sub git_patchset_body {
 
 		# generate anchor for "patch" links in difftree / whatchanged part
 		print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n" .
-		      format_diff_cc_simplified($diffinfo, @hash_parents) .
+		      format_diff_cc_simplified($diffinfo, $hash, @hash_parents) .
 		      "</div>\n";  # class="patch"
 
 		$patch_number++;
-- 
1.7.10.4
Jakub Narębski· Apr 9, 2013, 15:25 UTC · re: Jürgen Kreileder · lore

Re: [PATCH 4/4] gitweb: Fix broken blob action parameters on blob/commitdiff pages

W dniu 08.04.2013 22:10, Jürgen Kreileder pisze:
> Fix broken blob action parameters on blobdiff and commitdiff pages by
> explicitly passing variables instead of relying on global ones.

Do I understand it correctly that those variables (e.g. $hash variable in git_patchset_body in second chunk below, after this change passed as parameter to format_diff_cc_simplified()) can be filled in then, or at least adjusted correctly?

Show 6 quoted lines
> (The broken parameters on blob links lead to blob pages which show the
> blob but with a hash instead of a commit message and have broken
> blob_plain (404 - Cannot find file) and tree links (404 - Reading tree
> failed))
> 
> Signed-off-by: Jürgen Kreileder <jk@blackdown.de>

I wonder how we missed this. Does this happen always, or in some specific conditions?

Anyway, this change is a good change. Internal subroutines should not use global variables.

I hope that in the future we would get rid of most global variables...
Acked-by: Jakub Narebski <jnareb@gmail.com>
[not tested]
Show 7 quoted lines
>  # create note for patch simplified by combined diff
>  sub format_diff_cc_simplified {
> -	my ($diffinfo, @parents) = @_;
> +	my ($diffinfo, $hash, @parents) = @_;
>  	my $result = '';
>  
>  	$result .= "<div class=\"diff header\">" .
[...]
Show 10 quoted lines
> @@ -5404,7 +5405,7 @@ sub git_patchset_body {
>  
>  		# generate anchor for "patch" links in difftree / whatchanged part
>  		print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n" .
> -		      format_diff_cc_simplified($diffinfo, @hash_parents) .
> +		      format_diff_cc_simplified($diffinfo, $hash, @hash_parents) .
>  		      "</div>\n";  # class="patch"
>  
>  		$patch_number++;
> 
-- 
Jakub Narębski
Jürgen Kreileder· Apr 9, 2013, 20:17 UTC · re: Jakub Narębski · lore

Re: [PATCH 4/4] gitweb: Fix broken blob action parameters on blob/commitdiff pages

Jakub Narębski <jnareb@gmail.com> writes:
Show 9 quoted lines
> W dniu 08.04.2013 22:10, Jürgen Kreileder pisze:
>
>> Fix broken blob action parameters on blobdiff and commitdiff pages by
>> explicitly passing variables instead of relying on global ones.
>
> Do I understand it correctly that those variables (e.g. $hash variable
> in git_patchset_body in second chunk below, after this change passed
> as parameter to format_diff_cc_simplified()) can be filled in then,
> or at least adjusted correctly?
Yep.
Show 7 quoted lines
>> (The broken parameters on blob links lead to blob pages which show the
>> blob but with a hash instead of a commit message and have broken
>> blob_plain (404 - Cannot find file) and tree links (404 - Reading tree
>> failed))
>
> I wonder how we missed this.  Does this happen always, or in some
> specific conditions?

Just having a hash in the diff header and missing navigation links for lhs links in diffs happen pretty much all the time: E.g. follow any a/... link on http://repo.or.cz/w/alt-git.git/commitdiff/5bda18c186e455f8e65f976d3bf333ab1f4b5b53

(Btw, on http://repo.or.cz/w/alt-git.git/blobdiff/6466fbbeef67ec398f6f5d5b5da5d6f107a647b2..5bda18c186e455f8e65f976d3bf333ab1f4b5b53:/GIT-VERSION-GEN both the a/ und b/ link are broken. No idea if my patch fixes that, I don't use pathinfo because I've had similar problems with it when I tested it a year ago).

Broken a/ and b/ + broken tree + broken blob_plain links are more rare. I see them on https://git.blackdown.de/old.cgi?p=contactalbum.git;a=blobdiff;f=Classes/ContactAlbum.h;h=8f2b6a772ba0a4530e0c441db53ae5bbc0f2e277;hp=03c43ea01a3a88bbce3c73df9bfde64ee7a13717;hb=cc4eaa64c2b399dd9bdbf1f67f6d621aa24df5f8;hpb=43ff97d2c247517f43d83314aaa215e9f61d5d0c;js=1 for example: When following either a/ or b/, all links in "[contactalbum.git] / Classes / ContactAlbum.h" are broken.

With my patch I don't see these problems: https://git.blackdown.de/?p=contactalbum.git;a=blobdiff;f=Classes/ContactAlbum.h;h=8f2b6a772ba0a4530e0c441db53ae5bbc0f2e277;hp=03c43ea01a3a88bbce3c73df9bfde64ee7a13717;hb=cc4eaa64c2b399dd9bdbf1f67f6d621aa24df5f8;hpb=43ff97d2c247517f43d83314aaa215e9f61d5d0c;js=1

        Juergen

← back to recent threads