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

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

[Yann Dirson]

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,
-- 
Yann

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

[Petr Baudis]

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.

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

-- 
				Petr "Pasky" Baudis
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
		-- Samuel Beckett

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

[Jakub Narebski]

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

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

[Petr Baudis]

On Mon, Aug 27, 2007 at 02:30:04AM CEST, Jakub Narebski wrote:
> 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'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 "Pasky" Baudis
Early to rise and early to bed makes a male healthy and wealthy and dead.
                -- James Thurber

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

[Jakub Narebski]

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

>> 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.
[...]
> 
> 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?).

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

-- 
Jakub Narebski
Poland