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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html