Re: [BUG] gitweb on repo.or.cz shows buggy commitdiff

[Jakub Narebski <jnareb@gmail.com>]

On Sat, Aug 25, 2007, Petr "Pasky" Baudis wrote:
> On Sun, Jul 29, 2007 at 11:46:45AM CEST, Yann Dirson wrote:

>> Looking at [1] one can see that the diff, instead of showing the
>> changes to the various .gitignore files touched by the commit,
>> displays again and again the data/.gitignore one, but even confuses a
>> Makefile modified by the commit for the .gitignore file to diff.
>> 
>> 
>> |diff --git a/data/pics/.gitignore b/data/pics/.gitignore
>> |index c5ec666af7db2d53e7ed86090c88f62ab8ec25a4..912bca74cbee222936f08e421230fd9dde903ecb 100644 (file)
>> |--- a/lua/Makefile
>> |+++ b/data/pics/.gitignore
>> 
>> Also, despite "+++ b/data/pics/.gitignore" being displayed in the diff
>> header, it is indeed showing the diffs to "b/lua/Makefile".
>> 
>> [1] http://repo.or.cz/w/tagua/ydirson.git?a=commitdiff;h=1f285b312fa526293164548d88e8403dfb354eb4
> 
> Looks funny. This is because the target object id of all the patches is
> the same as all the .gitignore files are identical, and this triggers
> "split patch detection" - I guess that triggers when diff thinks that a
> file was replaced, and injects full deletion and full creation diff
> inside. Due to this trigger, file diffs and raw diff records get out of
> sync and that results in Makefile diff misattribution.

Thanks a lot, Pasky, for identifying the cause of this problem. It does
indeed look like a bug in the split/continued patch detection.

See below.

> Jakub, any idea how to fix this? Maybe to check if filename matches
> either from_file or to_file?

There are two possible solutions. The generic one, and the simplyfying one.

1. We could parse from_file and to_file from the "git diff header" in the
patch, i.e. from the "^diff" line in patchset, and consider patch "split"
or "continued" if not only pre-image and post-image (from_id and to_id)
matches, but only when also from_name and to_name matches. Note that for
combined diff there is no to_file.

The problem lies in the fact that to_name and/or from_name can be quoted,
and gitweb would need some complicated regular expression to take this into
consideration (Text::Balanced module provides regexp for extracting quoted
part: 
  (?:\"(?:[^\\\"]*(?:\\.[^\\\"]*)*)\")
by the way of gen_delimited_pat(q{"})), i.e. to separate from_name from
to_name in "git diff header".

This is very generic solution, and should work against possible future
changes in git-diff patch output.


2. Alternate solution is to note that we have split patch only in the case
of 'T' (type change) status, and for typechange we always split patch in
two, so there are two patches corresponding to single raw diff-tree line.

So it would be enough to introduce $continued_patch variable, unset it
after writing that patch is continued if it is true, set it to true if
there was typechange, i.e. if status (one of status) is 'T'.

And we can get rid of buffering extended diff header and parsing it,
I think. This would simplify git_patchset_body, making it easier to
understand and maintain.

But this depends on my understanding on "git diff-tree --patch-with-raw"
output, and that it would not change in the future WRT split patches.

> But that would probably need a bit of code 
> refactoring in git_patchset_body(), which is something I'm not really
> into. :-)

I tried to make git_patchset_body easier to understand, but it looks like
I have failed... :-)

-- 
Jakub Narebski
Poland