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

From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
Date: Mar 26, 2007, 20:55 UTC
Message-ID: <20070326205508.GC1128@auto.tuwien.ac.at>
In-Reply-To: <200703261912.18549.jnareb@gmail.com>

On Mon, Mar 26, 2007 at 06:12:18PM +0100, Jakub Narebski wrote:

Show 9 quoted lines

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

I thought, each patch would be more readable, I split them in logical seperate units. Anyway, I'll combine them.

Show 10 quoted lines

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

I'll switch to $to_prefix and $from_prefix.

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

OK. But is not the 3 line useless, as $to_prefix is alway defined after the second line and you probable want $from_prefix ||= $to_prefix. This will cause problems, as I currently pass the root tree (=tree in commit object) as an missing file name parameter, as gitweb does not allow an empty file name.

With an propagation logic, comparing a root tree with an sub tree will only work in one direction.

So I prefer to do not implement any automatic propagation between the two prefixes.

Show 7 quoted lines

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

Modifing the paths before generating the links is a good idea. I'll look, where its useful.

mfg Martin Kögler