On Mon, Mar 26, 2007 at 06:12:18PM +0100, Jakub Narebski wrote:
> 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.
> > 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.
> 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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html