Re: [PATCH] gitweb: Add treediff

[Jakub Narebski <jnareb@gmail.com>]

Martin Koegler wrote:
> On Mon, Mar 26, 2007 at 06:12:27PM +0100, Jakub Narebski wrote:
>> On Sun, Mar 25, 2007, Martin Koegler wrote:

>>> +sub git_treediff {
>>> +	my $format = shift || 'html';
>>> +	my $from = $file_parent || "";
>>> +	my $to = $file_name || "";
>> 
>> I'd use  $file_name || '';  here, and of course
>> 
>> +	my $from = $file_parent || $file_name || '';
>> 
>> The unwritten rule is that we use 'fp' parameter (thet $file_parent
>> variable is set) only 
> 
> How do I specifiy the root tree (=tree in commit) with hpb/fp, as fp
> can not be empty, but only undefined?

As I said in previous message, we can simply relax check for 'f' and 'fp'
parameters, by changing

	!validate_pathname($file_name)

to

	!defined validate_pathname($file_name)
 
Still, there are some places where we assume that 'f' and 'fp' cannot be
empty, like in above proposal. It would be:

+	my $from = (defined $file_parent ? $file_parent : $file_name) || '';


Again, I don't want to loose the assumption that we generate 'fp' _only_
if it is different from 'f'. Otherwise old links would cease working,
which breaks backwards-compatibility, and is not cool. "Cool UR_is don't
change."

>>> +
>>> +	if (!defined $hash) {
>>> +		if (!defined $hash_base) {
>>> +			die_error('','tree parameter missing');
>> 
>> This conflicts with the coding style used elsewhere in the gitweb
[...]
>> Examples:
>> 	die_error(undef, "Couldn't find base commit");
[...]
> I didn't know this. I'll change this.

What's strange in other place you used die_error accoring to coding
guidelines.

>>> +
>>> +	} elsif ($format eq 'plain') {
>>> +		my $filename = basename($project) . "-diff.patch";
>>> +
>> 
>> In "commitdiff_plain" view we use
>> 
>> 		my $filename = basename($project) . "-$hash.patch";
>> 
>> Perhaps we should use the same: "-diff.patch" does not make much sense.
>> Is it a typo?
> 
> No. What unique name do you propose? It needs to include $hash and $hase_parent.
> In this case, $hash could be $hash_base:$file_name, I'm not sure, how to escape 
> $file_name.

For "commitdiff" case sole $hash is also not unique. But if not -hash,
then perhaps -treediff instead of simply -diff?

-- 
Jakub Narebski
Poland