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

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano <junkio@cox.net> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Currently we have:
>>
>>   $ git diff -p HEAD^ HEAD -- gitweb/test/hardlink
>>   diff --git a/gitweb/test/hardlink b/gitweb/test/hardlink
>>   old mode 100644
>>   new mode 100755
>>
>> but
>>
>>   $ git diff -p HEAD^:gitweb/test/hardlink HEAD:gitweb/test/hardlink
>>
>> returns empty diff.
> 
> By "blobdiff", I understand you are talking about the "diff"
> link that appears on each path at the bottom of the commit view.

By "blobdiff" I mean "blobdiff" action (a=blobdiff), generated by
git_blobdiff subroutine.  The "diff" link that appears for each
path at the bottom of the "commit" view (in the whatchanged / difftree
part), and the "diff to current" link in the "history" view for files
both lead to "blobdiff" view.
 
> I think the bug is the way gitweb drives the core; it uses the
> latter form, when it has perfectly good information to run the
> former.  It's not like you have an UI that says "pick any path
> from any comit first; after you have picked one, pick the other
> arbitrary one; now we run compare them".  The UI says "Give me
> diff for this path in this commit".

_Currently_ gitweb uses git-diff-tree with path limiting when there
is enough information given, which means for all "blobdiff" links
currently generated by gitweb.  It uses git-diff for blobs only if
there is not enough information, e.g. for legacy URL; before moving
to generating diffs with git-diff-tree and git-diff, and all patches
were generated by external /usr/bin/diff, there were no 'hp' and 'hpb'
parameters which are needed for git-diff-tree.

But using git-diff-tree with path limiter to extract diff between
given blobs for "blobdiff" view is not without its caveats.  For example
for a rename you need both pre and post filenames in path limiter, 
otherwise you would get only "half" of rename diff, namely appropriate
creation diff.  And you can select only between blobs which have changed
from one commit (tree) to other; you cannot for example generate diff
between README at 'master' and INSTALL at 'next'.  Although why one 
would want such diff are beyond me... well, except for [overrated] 
completeness...

Martin wants to have ability to generate diff (blobdiff) between two
arbitrary blobs (two arbitrary files at arbitrary revisions). Earlier
patch if I remember correctly introduced yet another codepath; this one
tries to generate everything using one codepath, the git-diff one.
"Pick any path from any comit first; after you have picked one, pick the 
other arbitrary one; now we run compare them".

It adds new feature to gitweb I think almost nobody would use, namely
comparing arbitrary blobs (arbitrary files, with different names), and 
unifies diff generation in git_blobdiff (everything does use git-diff).
But it does so [currently] at the loss of information, namely ignoring 
all mode changes.  The mode changes can be done without lost of 
unification at the cost of performance of always having to do 
git-ls-tree, twice, or at the loss of having git_blobdiff quite 
complicated.

So from me it is slight Nak to this patch, at least unless "git diff 
<tree1>:<path1> <tree2>:<path2>" will take the information about file 
name and mode (permissions) from <tree1> and <tree2>, and included such 
information in extended git diff header.

-- 
Jakub Narebski
Poland