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

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 26, 2007, 17:12 UTC
Message-ID: <200703261912.18549.jnareb@gmail.com>
In-Reply-To: <11748548622060-git-send-email-mkoegler@auto.tuwien.ac.at>

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

Show 5 quoted lines

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

Show 13 quoted lines

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

Show 10 quoted lines

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

Show 17 quoted lines

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