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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 29, 2007, 21:35 UTC
Message-ID: <200704292335.50102.jnareb@gmail.com>
In-Reply-To: <20070416201813.GA2592@auto.tuwien.ac.at>

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.

Show 10 quoted lines

> 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).

Show 11 quoted lines

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