Re: [PATCH] gitweb: Add treediff

[Jakub Narebski <jnareb@gmail.com>]

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 

> +
> +	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");

> +		}
> +		$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);


Here I think (contrary to blobdiff case) it is better to use extended
SHA-1 syntax (taken from git-rev-parse(1)):

 * A suffix `:' followed by a path; this names the blob or tree at the given
   path in the tree-ish object named by the part before the colon.

> +	}
> +
> +	# we need to prepare $formats_nav before any parameter munging
> +	my $formats_nav;
> +	if ($format eq 'html') {
> +		$formats_nav =
> +			$cgi->a({-href => href(action=>"treediff_plain",
> +			                       hash=>$hash, hash_parent=>$hash_parent,
> +			                       hash_base=>$hash_base, hash_parent_base=>$hash_parent_base,
> +			                       file_name=>$file_name, file_parent=>$file_parent)},
> +			        "raw");


I certainly agree to that.

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

> +
> +	# 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?

> +	} elsif ($format eq 'plain') {
> +		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
> +			'-p', $hash_parent, $hash, "--"
> +			or die_error(undef, "Open git-diff-tree failed");

For "commitdiff_plain" view the email-like format, with commit message
and the patch is I think enough. The commit message serves as summary
for this view.

I think that it would be better for "treediff_plain" to have
whatchanged-like (with shothened sha1 for example) plain difftree
before the patchset.

> +	} else {
> +		die_error(undef, "Unknown treediff format");

And here you use standard convention to call die_error.

> +	}
> +
> +	# non-textual hash id's can be cached
> +	my $expires;
> +	if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
> +		$expires = "+1d";
> +	}
> +
> +	# write header
> +	if ($format eq 'html') {
> +
> +		git_header_html(undef, $expires);
> +		if (defined $hash_base && (my %co = parse_commit($hash_base))) {
> +			git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
> +			git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
> +			print "<div class=\"title\">$hash_base:$from vs $hash_parent_base:$to</div>\n";
> +		} else {
> +			print "<div class=\"page_nav\"><br/>$formats_nav<br/></div>\n";
> +			print "<div class=\"title\">$hash vs $hash_parent</div>\n";
> +		}
> +		print "<div class=\"page_body\">\n";

Usually we use title of the $hash_base ('hb') commit as a page header,
and I think we should do the same for "treediff" view. Only if it is not
possible we use "$hash vs $hash_parent" or equivalent. Do not cause
inconsistency, please. You can propose patch changing this, but make
it separate issue.

Additionally, "blob" view has page_path, and so has "blobdiff" view (it is
for $hash / $filename / $hash_base). And "tree" view has page_path, so
I think should "treediff" have; of course if $hash_base is defined.

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

[...]

Some of further code could also be shared between git_treediff and
git_commitdiff, but this could wait I guess for another separate patch.

-- 
Jakub Narebski
Poland