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

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

Best regards,

On Sun, Jul 29, 2007 at 11:46:45AM CEST, Yann Dirson wrote:

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.

Jakub, any idea how to fix this? Maybe to check if filename matches either from_file or to_file? But that would probably need a bit of code refactoring in git_patchset_body(), which is something I'm not really into. :-)

On Sat, Aug 25, 2007, Petr "Pasky" Baudis wrote:

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.

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.

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

On Mon, Aug 27, 2007 at 02:30:04AM CEST, Jakub Narebski wrote:

I'd prefer this solution, though I think split patch could also happen if you use -B (which I think is very good idea to use and it's a pity that it isn't default - or is it?).

Petr Baudis wrote:

[...]

No, 'B' status does not means split patch: it only affects how the patchset looks like, i.e first delete all then add all, instead of doing change chunk by chunk.

Or I think it does. "git log --raw -p --diff-filter=B -B origin" on git.git repository returns rather strange results for the difftree part...