Martin Koegler wrote:
> 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
> separate units. Anyway, I'll combine them.
That was just a thought. If you think that separate patches would be
more readable, by all meens keep them splitted.
>>> 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.
Third line is not important, as it is you who control the calling
convention. Perhaps it should read:
+ $to_prefix = $from_prefix if (!defined $to_prefix);
And it would be fairly easy to change gitweb to allow empty file name
parameters; simply change
!validate_pathname($file_name)
to
!defined validate_pathname($file_name)
(and similarly for $file_parent).
But I'd rather not change _CGI parameter_ (URI) convention that we set
'fp' (file parent) parameter *only* if it is different from 'f' (file
name). Otherwise we would introduce backwards incompatibile change,
with respect to bookmarks and old URI-s. Cool URI-s don't change...
BTW. "git rev-parse <tree-ish>:" == "git rev-parse <tree-ish>^{tree}"
> 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.
Fine by me. It is just _internal_ call convention.
>> 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.
Please examine consequences of this, and changes in the output if you
decide to go this way (IMHO bit simpler).
--
Jakub Narebski
Poland
-
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