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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 20, 2007, 10:34 UTC
Message-ID: <200704201234.50134.jnareb@gmail.com>
In-Reply-To: <11766699701308-git-send-email-mkoegler@auto.tuwien.ac.at>

Martin Koegler wrote:

Show 8 quoted lines

> Currently, blobdiff can only compare blobs with different file
> names, if no hb/hpb parameters are present.
> 
> This patch adds support for comparing two blobs specified by any
> combination of hb/f/h and hpb/fp/hp.
> 
> Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> ---

Shouldn't the comment below be also a part of commit message (perhaps
changed to use passive form)?

Show 11 quoted lines

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

I have mixed feelings about this. On the one hand side the blobdiff generation is simplified as we have only one codepath instead of two, and removed codepath in rare cases (not generated by gitweb currently) might return incorrect results in the case of requesting diff between two arbitrary and unrelated files.

On the other hand side we do loose some information, contained in extended diff header, like mode changes, renames and such.

It's a pity that extended sha-1 syntax to specify blobs, namely the <tree-ish>:<path> syntax is opaque, and git-diff machinery sees only the result of it, as if it was called directly with the sha-1 of blob. It would be nice if "git diff <tree1>:<path1> <tree2>:<path2>" took the information about file name and mode (permissions) from <tree1> and <tree2> and included such information in extended git diff header.

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.

I'd rather have core git support for this first before changing how git_blobdiff is implemented in gitweb...

-- 
Jakub Narebski
Poland