Re: [PATCH] gitweb: Add treediff

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 27, 2007, 01:15 UTC
Message-ID: <200703270315.54901.jnareb@gmail.com>
In-Reply-To: <20070326210548.GD1128@auto.tuwien.ac.at>

Martin Koegler wrote:

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

Show 14 quoted lines

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

Show 6 quoted lines

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

Show 15 quoted lines

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