Re: [PATCH 2/7] gitweb: Support comparing blobs with different names

On Monday, 16 April 2007, Martin Koegler wrote:

Currently, blobdiff can only compare blobs with different file
names, if no hb/hpb parameters are present.

Not true. Since commit 5ae917acdf40444945271e5e014cda37e202504e

  "gitweb: Support comparing blobs (files) with different names"

git_blobdiff ('blobdiff' view) suports comparing arbitrary blobs
(also with different file names) when no hp/hpb parameters are
present (although there would be no mode change information),
but supports comparing blobs with different file names in presence
of hp/hpb parameters, provided that it is proper rename diff.

Since this commit you can select rename diff which is part of
'commitdiff' view as a 'blobdiff' view, correctly.

This patch adds support for comparing two blobs specified by any
combination of hb/f/h and hpb/fp/hp.

But it used to lose mode change information. This info should be
I think in commit message.

Because your work on including mode changes for git-diff with
<tree-ish>:<path-to-blob> extended SHA-1 syntax (thanks a lot!)
I guess that you would want to rewrite this patch.
 

Signed-off-by: Martin Koegler <redacted>
---
New version, as I found a bug in the expiration handling code.

I unified all blobdiff variants and added support for comparing blobs
with different names.

If h/hp parameter are missing, I need to generate them with
git_get_hash_by_path, as the are needed for the html header, which is
generated before parsing the git-diff output.

git_get_hash_by_path uses git-ls-tree but it does not catch all the info;
perhaps git_get_info_by_path would be called for here.

I currently ignore all mode changes, as they are part of the tree. I
don't think that displaying a mode change message justifes two call to
git-ls-tree for each blob diff (Currently it only calls git-ls-tree
for each missing h/hp parameter).

That is not a problem now, thanks to your patches... although it would
be nice to have subroutine which would get the difftree information
from extended git diff header (if it is possible).

+	my $expires = '+1d';
+	# non-textual hash id's can be cached
+	if (defined $hash && $hash !~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = undef;
+	} elsif (defined $hash_parent && $hash_parent !~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = undef;
+	} elsif (defined $hash_base && $hash_base !~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = undef;
+	} elsif (defined $hash_parent_base && $hash_parent_base !~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = undef;
+	}

Usually gitweb _changes_ $expires to '+1d' when appropriate, instead
of defaulting to '+1d' and undefining it. But this might be the matter
of style consistency, and personal preferences: one complicated boolean
expression or set of if ... elsif ... conditionals.

The idea is that both TO and FROM are to be immutable; this means
either $hash_parent (or $hash_parent_base) being full SHA-1 and having
filename defined, or if appropriate "parent" hash is not present (because
"parent" hash has precedence over blob hash) it means that $hash
(or $hash_parent) being full SHA-1.

[...]  

+	if (defined $hash_parent_base && defined $file_parent && !defined $hash_parent) {
+	    $hash_parent = git_get_hash_by_path($hash_parent_base, $file_parent);
+	}

[...]

+	# open patch output
+	open $fd, "-|", git_cmd(), "diff", @diff_opts,
+	$hash_parent, $hash, "--"
+		or die_error(undef, "Open git-diff failed");

You would most probably use now "$hash_base:$file_name" instead of $hash
if $hash_base is defined, i.e.

  defined $hash_base ? "$hash_base:$file_name" : $hash

and similarly for $hash_parent parameter now that <tree>:<path> form
respects mode changes information.

-- 
Jakub Narebski
ShadeHawk on #git
Poland