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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 20, 2007, 20:49 UTC
Message-ID: <200704202249.08464.jnareb@gmail.com>
In-Reply-To: <7vabx3z3tf.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:

Show 17 quoted lines

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

Show 6 quoted lines

> 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