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

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 27, 2007, 01:07 UTC
Message-ID: <200703270307.26589.jnareb@gmail.com>
In-Reply-To: <20070326205508.GC1128@auto.tuwien.ac.at>

Martin Koegler wrote:

Show 14 quoted lines

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

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

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}"

Show 5 quoted lines

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

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

Please examine consequences of this, and changes in the output if you decide to go this way (IMHO bit simpler).

-- 
Jakub Narebski
Poland