On Wed, Mar 28, 2007 at 12:58:54AM +0100, Jakub Narebski wrote: > On Thu, Mar 27, 2007, Martin Koegler wrote: > > On Tue, Mar 27, 2007 at 01:56:24AM +0100, Jakub Narebski wrote: > > If you think, its safe, I can simplify git_blobdiff. I propose > > doing the following way (pseudo-code): > > > $file_parent ||= $file_name; > [...] > > $hash=git_get_hash_by_path($hash_base,$file_name); > [...] > > $hash_parent=git_get_hash_by_path($hash_parent_base,$file_parent); > [...] > > open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts, > > $hash_parent, $hash > > or die_error(undef, "Open git-diff failed"); > [...] > > Else I will keep a reworked version of my patch. > > The trouble with this is that we may lose mode change (symlink to > ordinary file etc.) because we hand-generate %diffinfo. If we need the correct mode in %diffinfo, this is not difficult: $from_mode="blob"; if (defined $hash_base && defined $file_name) ($to_mode,$hash)=git_get_hash_by_path($hash_base,$file_name); $to_mode="blob"; if (defined $hash_parent_base && defined $file_parent) ($from_mode,$hash_parent)=git_get_hash_by_path_mode($hash_parent_base,$file_parent); Then we set to_mode and from_mode in %diffinfo to $to_mode and $from_mode. git_get_hash_by_path_mode is like git_get_hash_by_path, only that it returns an array with the mode and the hash. Blobdiff (html output) in its current version can not handle symlinks: > diff --git a/x b/x > deleted file mode 100644 (file) > index 190a180..873fb8d > --- a/x > +++ /dev/null > @@ -1 +0,0 @@ > -123 > diff --git a/ b/ > new file mode 120000 (symlink) > index 190a180..873fb8d > --- /dev/null > +++ b/ > @@ -0,0 +1 @@ > +file3 > \ No newline at end of file This was generated by "diff to current" in the history view of a file, which was changed between symlink and normal file. Additionally, to_mode and from_mode of %diffinfo seem to be ignored by git_patchset_body. > >> By the way, if you call git_get_hash_by_path (which is expensive, as it > >> calls git command), you can use resulting hash in place of > >> hash_base:filename as an argument to git-diff. > > > > I must check, if we need to resolve $hash ($hash_parent) by > > git_get_hash_by_path, if we construct it out of $hash_base and > > $file_name. Maybe we can avoid this call. > > We can use "$hash_base:$file_name" as second parameter to git-diff etc., > but I don't think we want to create links with "$hash_base:$file_name" > instead of sha-1 id of a blob as 'h' parameter. > > It can be first implementation, thought, and later we can try to use > "index <hash>..<hash> <mode>" lines from extended header to get $hash > and $hash_parent (with exception of pure rename, but then we need only > one invocation of git_get_hash_by_path subroutine). The <mode> must be discarded, as it is wrong for anything which as not a mode of 100644, as we specify a two blob as parameter to git-diff. > But I think it is better left for later patch. As git_patchset_body requires an information about the compared file as parameter, a new formating function will be needed. I'm not sure, if the overhead of git_get_hash_by_path is this worth. Additionally, if we have the hash passed by parameter in most cases, there is no need to call it in these cases. mfg Martin Kögler PS: I created a blob with a "strange" filename: &()=!"§$%[<>]*#+_;. In the result of the blob view, the " is not escaped in the filename in the header and a strage content type is returned: $ telnet localhost 80 Trying 127.0.0.1... Connected to localhost. Escape character is '^]'. GET /gitweb/gitweb.cgi?p=t/.git;a=blob;f=%26()%3D%21%22%A7%24%25%5B%3C%3E%5D%2A%23%2B_%3B.;hb=7bfed2588bee66b33db544830606fa6606478fd9 HTTP/1.0 HTTP/1.1 200 OK Date: Wed, 28 Mar 2007 19:55:36 GMT Server: Apache Content-disposition: inline; filename="&()=!"§$%[<>]*#+_;." Expires: Thu, 29 Mar 2007 19:55:39 GMT Connection: close Content-Type: application/vnd.mif xx - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html