threads / patch / 7523

patchgitweb: Fix bug in "blobdiff" view for split (e.g. file to symlink) patches

Subject: [PATCH (resend)] gitweb: Fix bug in "blobdiff" view for split (e.g. file to symlink) patches

## tl;dr

3 messages between Apr 5, 2007 and Apr 5, 2007. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Jakub Narebski· Apr 5, 2007, 11:45 UTC · lore

git_patchset_body needs patch generated with --full-index option to detect split patches, meaning two patches which corresponds to single difftree (raw diff) entry. An example of such situation is changing type (mode) of a file, e.g. from plain file to symbolic link.

Add, in git_blobdiff, --full-index option to patch generating git diff invocation, for the 'html' format output ("blobdiff" view).

"blobdiff_plain" still uses shortened sha1 in the extended git diff header "index <hash>..<hash>[ <mode>]" line.

Noticed-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Wed, Mar 28, 2007 at 23:03 +0200, Martin Koegler wrote:

Show 20 quoted lines

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

This patch fixes this bug in git_blobdiff (in "blobdiff" view).

Junio, you probably have missed this patch in the noise...

 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Show changes to gitweb/gitweb.perl +4 −2

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4f34c29..d1f4aeb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3936,7 +3936,8 @@ sub git_blobdiff {
 
 		# open patch output
 		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-			'-p', $hash_parent_base, $hash_base,
+			'-p', ($format eq 'html' ? "--full-index" : ()),
+			$hash_parent_base, $hash_base,
 			"--", (defined $file_parent ? $file_parent : ()), $file_name
 			or die_error(undef, "Open git-diff-tree failed");
 	}
@@ -3971,7 +3972,8 @@ sub git_blobdiff {
 		}
 
 		# open patch output
-		open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,
+		open $fd, "-|", git_cmd(), "diff", @diff_opts,
+			'-p', ($format eq 'html' ? "--full-index" : ()),
 			$hash_parent, $hash, "--"
 			or die_error(undef, "Open git-diff failed");
 	} else  {

-- 
1.5.0.5

Junio C Hamano· Apr 5, 2007, 20:47 UTC · re: Jakub Narebski · lore

Re: [PATCH (resend)] gitweb: Fix bug in "blobdiff" view for split (e.g. file to symlink) patches

Jakub Narebski <jnareb@gmail.com> writes:

Show 6 quoted lines

> git_patchset_body needs patch generated with --full-index option to
> detect split patches,...
> ...
> This patch fixes this bug in git_blobdiff (in "blobdiff" view).
>
> Junio, you probably have missed this patch in the noise...

Probably. Although I do remember seeing the patch, it was unclear why --full-index is needed (and it still is unclear to me). Do you have a parser that expects/depends on full 40-hexdigit hash on the index line?

Not a complaint but a question -- I do not mind (actually prefer, probably) if gitweb worked on full 40-hexdigit internally to avoid possible ambiguity issues, although the output may need to be truncated to save people from having to see unneeded precision.

Jakub Narebski· Apr 5, 2007, 22:58 UTC · re: Junio C Hamano · lore

Re: [PATCH (resend)] gitweb: Fix bug in "blobdiff" view for split (e.g. file to symlink) patches

DJunio C Hamano wrote:

Show 18 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> git_patchset_body needs patch generated with --full-index option to
>> detect split patches,...
>> ...
>> This patch fixes this bug in git_blobdiff (in "blobdiff" view).
>>
>> Junio, you probably have missed this patch in the noise...
> 
> Probably.  Although I do remember seeing the patch, it was
> unclear why --full-index is needed (and it still is unclear to
> me).  Do you have a parser that expects/depends on full
> 40-hexdigit hash on the index line?
> 
> Not a complaint but a question -- I do not mind (actually prefer,
> probably) if gitweb worked on full 40-hexdigit internally to
> avoid possible ambiguity issues, although the output may need to
> be truncated to save people from having to see unneeded precision.

Yes, the parser (in git_patchset_body subroutine) checks for presence of fully expanded 40-hexdigit hash, and compares it with the fully expanded 40-hexdigit hash it has in $difftree hashref (from the "raw" diff output, or "raw" part of diff output, or hand-crafted "raw" diff output). It is needed only for the 'html' format case (for "commitdiff" / "blobdiff" view), for git_patchset_body to know what values use for hyperlinking and replacements.

The output is truncated (and hyperlinked) in 'html' format case.

I could have relaxed the parser, and compare up to the precision of hash in the "^index ..." line of extended git diff header in "patch" diff output...

-- 
Jakub Narebski
Poland