Re: [PATCH] gitweb: Add treediff

[mkoegler@auto.tuwien.ac.at (Martin Koegler)]

On Mon, Mar 26, 2007 at 06:12:27PM +0100, Jakub Narebski wrote:
> On Sun, Mar 25, 2007, Martin Koegler wrote:
> > treediff supports comparing different trees. A tree can be specified
> > either as hash or as base hash and filename.
> 
> I'd use "treediff" view, or git_treediff subroutine. Just a minor nit.
>  
> > +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?

> > +
> > +	if (!defined $hash) {
> > +		if (!defined $hash_base) {
> > +			die_error('','tree parameter missing');
> 
> This conflicts with the coding style used elsewhere in the gitweb
> (the informal coding convention / guideline for gitweb).
> 
> First, we either use  undef  and not  ''  to say: use default value
> of the first parameter (HTTP status) of die_error, or provide our
> own value in single quotes.
> 
> Second, the second parameter, error message, is a sentence (without
> final fullstop) describing error; it means starting it with capital
> letter. And we use double quotes for this parameter.
> 
> Examples:
> 	die_error(undef, "Couldn't find base commit");
> 	die_error(undef, "Unknown commit object");
> 	die_error(undef, "No file name defined");
> 	die_error(undef, "Open git-diff-tree failed");
> 
> 	die_error('403 Permission denied', "Permission denied");
> 	die_error('404 Not Found', "File name not defined");
> 	die_error('404 Not Found', "Not enough information to find object");
> 	die_error('400 Bad Request', "Object is not a blob");

I didn't know this. I'll change this.

> > +		}
> > +		$hash = $hash_base;
> > +		$hash .= ":".$file_name if (defined $file_name);
> > +	}
> > +
> > +	if (!defined $hash_parent) {
> > +		if (!defined $hash_parent_base) {
> > +			die_error('','tree parameter missing');
> > +		}
> > +		$hash_parent = $hash_parent_base;
> > +		$hash_parent .= ":".$file_parent if (defined $file_parent);
> 
> The same problem as above: we do not set 'fp' parameter (and $file_parent
> variable) if name does not change. So you should use instead:
> 
> +		$hash_parent .= ":".($file_parent || $file_name)
> +			if (defined $file_parent || defined $file_name);
> 

Same as above: How to specifiy the root tree with hpb/fp?

> > +		if (defined $hash_parent_base && (my %co = parse_commit($hash_parent_base))) {
> > +	 		$formats_nav .= " | from: ".
> > +				$cgi->a({-href => href(action=>"commit",
> > +			        	               hash=>$hash_parent_base)},
> > +				        "commit")
> > +				." | ".
> > +				$cgi->a({-href => href(action=>"commitdiff",
> > +				                       hash=>$hash_parent_base)},
> > +				        "commitdiff")
> > +				." | ".
> > +				$cgi->a({-href => href(action=>"tree",
> > +						       hash=>$co{'tree'},
> > +				                       hash_base=>$hash_parent_base)},
> > +				        "tree");
> > +		}
> > +	}
> 
> This depends if the similar change (feature) for "blobdiff" view
> (in git_blobdiff subroutine) is accepted. Perhaps this could be
> separated into separate patch?

I'll do that.

> > +
> > +	# read treediff
> > +	my $fd;
> > +	my @difftree;
> > +	if ($format eq 'html') {
> > +		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
> > +			"--no-commit-id", "--patch-with-raw", "--full-index",
> > +			$hash_parent, $hash, "--"
> > +			or die_error(undef, "Open git-diff-tree failed");
> > +
> > +		while (my $line = <$fd>) {
> > +			chomp $line;
> > +			# empty line ends raw part of diff-tree output
> > +			last unless $line;
> > +			push @difftree, $line;
> > +		}
> 
> This is also common with git_commitdiff. Should be it put into separate
> subroutine? Or do this refactoring in another patch?

I would like to save the refactoring for later. Maybe we will need a
change, which will not work for git_commitdiff.

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

mfg Martin Kögler