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

[Jakub Narebski <jnareb@gmail.com>]

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 <mkoegler@auto.tuwien.ac.at>
> ---
> 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