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

From: Jakub Narębski <jnareb@gmail.com>
Date: Apr 9, 2013, 15:25 UTC
Message-ID: <51643307.9090603@gmail.com>
In-Reply-To: <m2vc7wbx76.fsf@blackdown.de>

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