Re: [PATCH] gitweb: support filename prefix in git_patchset_body

[Jakub Narebski <jnareb@gmail.com>]

On Sun, Mar 25, 2007, Martin Koegler wrote:

> git_treediff supports comparing subdirectories. As the output of
> git-difftree is missing the path to the compared directories,
> the links in the output would be wrong.
> 
> The patch adds two new parameters to add the missing path prefix.

Wouldn't it be better to concatenate the two "path prefix" patches
together? They are about the same thing.

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 4c371b2..4195b1a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2372,7 +2372,7 @@ sub git_difftree_body {
>  }
>  
>  sub git_patchset_body {
> -	my ($fd, $difftree, $hash, $hash_parent) = @_;
> +	my ($fd, $difftree, $hash, $hash_parent, $file_name, $file_parent) = @_;
>  
>  	my $patch_idx = 0;
>  	my $patch_line;

I'd rather use $from_prefix, $to_prefix here, or $basedif_name,
$basedir_parent, or $dir_name, $dir_parent (my preference is to
$from_prefix, $to_prefix variables).

> @@ -2380,6 +2380,9 @@ sub git_patchset_body {
>  	my $diffinfo;
>  	my (%from, %to);
>  
> +	$file_name = (!defined $file_name)?"":($file_name."/");
> +	$file_parent = (!defined $file_parent)?"":($file_parent."/");
> +
>  	print "<div class=\"patchset\">\n";
>  
>  	# skip to first patch

Minor nit: I'd rather write

+	$from_prefix = !defined $from_prefix ? '' : $from_prefix.'/';
+	$to_prefix   = !defined $to_prefix   ? '' : $to_prefix . '/';
+	$to_prefix ||= $from_prefix;  # to allow to pass common prefix once
+

or something like that, or just modify $from{'file'} and $to{'file'}

	$from{'file'} = (!defined $from_prefix ? '' : $from_prefix.'/') . $from{'file'};
	$to{'file'}   = (!defined $to_prefix   ? '' : $to_prefix . '/') . $to{'file'};

just after setting $from{'file'} and $to{'file'}, although the second
solution would additionally add prefix to the shown patch body itself.

> @@ -2439,14 +2442,14 @@ sub git_patchset_body {
>  			if ($diffinfo->{'status'} ne "A") { # not new (added) file
>  				$from{'href'} = href(action=>"blob", hash_base=>$hash_parent,
>  				                     hash=>$diffinfo->{'from_id'},
> -				                     file_name=>$from{'file'});
> +				                     file_name=>$file_parent.$from{'file'});
>  			} else {
>  				delete $from{'href'};
>  			}
>  			if ($diffinfo->{'status'} ne "D") { # not deleted file
>  				$to{'href'} = href(action=>"blob", hash_base=>$hash,
>  				                   hash=>$diffinfo->{'to_id'},
> -				                   file_name=>$to{'file'});
> +				                   file_name=>$file_name.$to{'file'});
>  			} else {
>  				delete $to{'href'};
>  			}

Another solution would be to not add additional parameters to
git_difftree_body and git_patchset_body subroutines (although it is nice
touch towards completeness), but modify %diffinfo in the caller, but this
would change also patch contents (in from-file / to-file diff header, etc.)
which might not be a good thing.

I'm not sure if we should not add information somewhere that paths are
prefixed/shortened, but this might be left for later patch.

-- 
Jakub Narebski
Poland