{"thread":{"id":"9287","subject":"[BUG] gitweb on repo.or.cz shows buggy commitdiff","startedAt":"2007-07-29T09:46:45Z","lastAt":"2007-08-27T01:12:18Z","messageCount":5,"participants":["Yann Dirson","Petr Baudis","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"48973","messageId":"20070729094645.GU23229@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"9287","inReplyTo":null,"subject":"[BUG] gitweb on repo.or.cz shows buggy commitdiff","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2007-07-29T09:46:45Z","receivedAt":"2007-07-29T09:46:45Z","isPatch":false,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"Looking at [1] one can see that the diff, instead of showing the\nchanges to the various .gitignore files touched by the commit,\ndisplays again and again the data/.gitignore one, but even confuses a\nMakefile modified by the commit for the .gitignore file to diff.\n\n\n|diff --git a/data/pics/.gitignore b/data/pics/.gitignore\n|index c5ec666af7db2d53e7ed86090c88f62ab8ec25a4..912bca74cbee222936f08e421230fd9dde903ecb 100644 (file)\n|--- a/lua/Makefile\n|+++ b/data/pics/.gitignore\n\nAlso, despite \"+++ b/data/pics/.gitignore\" being displayed in the diff\nheader, it is indeed showing the diffs to \"b/lua/Makefile\".\n\n\n[1] http://repo.or.cz/w/tagua/ydirson.git?a=commitdiff;h=1f285b312fa526293164548d88e8403dfb354eb4\n\nBest regards,\n-- \nYann\n"},{"id":"51520","messageId":"20070825202530.GB1219@pasky.or.cz","threadId":"9287","inReplyTo":"20070729094645.GU23229@nan92-1-81-57-214-146.fbx.proxad.net","subject":"Re: [BUG] gitweb on repo.or.cz shows buggy commitdiff","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2007-08-25T20:25:30Z","receivedAt":"2007-08-25T20:25:30Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Sun, Jul 29, 2007 at 11:46:45AM CEST, Yann Dirson wrote:\n> Looking at [1] one can see that the diff, instead of showing the\n> changes to the various .gitignore files touched by the commit,\n> displays again and again the data/.gitignore one, but even confuses a\n> Makefile modified by the commit for the .gitignore file to diff.\n> \n> \n> |diff --git a/data/pics/.gitignore b/data/pics/.gitignore\n> |index c5ec666af7db2d53e7ed86090c88f62ab8ec25a4..912bca74cbee222936f08e421230fd9dde903ecb 100644 (file)\n> |--- a/lua/Makefile\n> |+++ b/data/pics/.gitignore\n> \n> Also, despite \"+++ b/data/pics/.gitignore\" being displayed in the diff\n> header, it is indeed showing the diffs to \"b/lua/Makefile\".\n> \n> \n> [1] http://repo.or.cz/w/tagua/ydirson.git?a=commitdiff;h=1f285b312fa526293164548d88e8403dfb354eb4\n\nLooks funny. This is because the target object id of all the patches is\nthe same as all the .gitignore files are identical, and this triggers\n\"split patch detection\" - I guess that triggers when diff thinks that a\nfile was replaced, and injects full deletion and full creation diff\ninside. Due to this trigger, file diffs and raw diff records get out of\nsync and that results in Makefile diff misattribution.\n\nJakub, any idea how to fix this? Maybe to check if filename matches\neither from_file or to_file? But that would probably need a bit of code\nrefactoring in git_patchset_body(), which is something I'm not really\ninto. :-)\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nEver try. Ever fail. No matter. // Try again. Fail again. Fail better.\n\t\t-- Samuel Beckett\n"},{"id":"51620","messageId":"200708270230.04537.jnareb@gmail.com","threadId":"9287","inReplyTo":"20070825202530.GB1219@pasky.or.cz","subject":"Re: [BUG] gitweb on repo.or.cz shows buggy commitdiff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-08-27T00:30:04Z","receivedAt":"2007-08-27T00:30:04Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, Aug 25, 2007, Petr \"Pasky\" Baudis wrote:\n> On Sun, Jul 29, 2007 at 11:46:45AM CEST, Yann Dirson wrote:\n\n>> Looking at [1] one can see that the diff, instead of showing the\n>> changes to the various .gitignore files touched by the commit,\n>> displays again and again the data/.gitignore one, but even confuses a\n>> Makefile modified by the commit for the .gitignore file to diff.\n>> \n>> \n>> |diff --git a/data/pics/.gitignore b/data/pics/.gitignore\n>> |index c5ec666af7db2d53e7ed86090c88f62ab8ec25a4..912bca74cbee222936f08e421230fd9dde903ecb 100644 (file)\n>> |--- a/lua/Makefile\n>> |+++ b/data/pics/.gitignore\n>> \n>> Also, despite \"+++ b/data/pics/.gitignore\" being displayed in the diff\n>> header, it is indeed showing the diffs to \"b/lua/Makefile\".\n>> \n>> [1] http://repo.or.cz/w/tagua/ydirson.git?a=commitdiff;h=1f285b312fa526293164548d88e8403dfb354eb4\n> \n> Looks funny. This is because the target object id of all the patches is\n> the same as all the .gitignore files are identical, and this triggers\n> \"split patch detection\" - I guess that triggers when diff thinks that a\n> file was replaced, and injects full deletion and full creation diff\n> inside. Due to this trigger, file diffs and raw diff records get out of\n> sync and that results in Makefile diff misattribution.\n\nThanks a lot, Pasky, for identifying the cause of this problem. It does\nindeed look like a bug in the split/continued patch detection.\n\nSee below.\n\n> Jakub, any idea how to fix this? Maybe to check if filename matches\n> either from_file or to_file?\n\nThere are two possible solutions. The generic one, and the simplyfying one.\n\n1. We could parse from_file and to_file from the \"git diff header\" in the\npatch, i.e. from the \"^diff\" line in patchset, and consider patch \"split\"\nor \"continued\" if not only pre-image and post-image (from_id and to_id)\nmatches, but only when also from_name and to_name matches. Note that for\ncombined diff there is no to_file.\n\nThe problem lies in the fact that to_name and/or from_name can be quoted,\nand gitweb would need some complicated regular expression to take this into\nconsideration (Text::Balanced module provides regexp for extracting quoted\npart: \n  (?:\\\"(?:[^\\\\\\\"]*(?:\\\\.[^\\\\\\\"]*)*)\\\")\nby the way of gen_delimited_pat(q{\"})), i.e. to separate from_name from\nto_name in \"git diff header\".\n\nThis is very generic solution, and should work against possible future\nchanges in git-diff patch output.\n\n\n2. Alternate solution is to note that we have split patch only in the case\nof 'T' (type change) status, and for typechange we always split patch in\ntwo, so there are two patches corresponding to single raw diff-tree line.\n\nSo it would be enough to introduce $continued_patch variable, unset it\nafter writing that patch is continued if it is true, set it to true if\nthere was typechange, i.e. if status (one of status) is 'T'.\n\nAnd we can get rid of buffering extended diff header and parsing it,\nI think. This would simplify git_patchset_body, making it easier to\nunderstand and maintain.\n\nBut this depends on my understanding on \"git diff-tree --patch-with-raw\"\noutput, and that it would not change in the future WRT split patches.\n\n> But that would probably need a bit of code \n> refactoring in git_patchset_body(), which is something I'm not really\n> into. :-)\n\nI tried to make git_patchset_body easier to understand, but it looks like\nI have failed... :-)\n\n-- \nJakub Narebski\nPoland\n"},{"id":"51622","messageId":"20070827004838.GO1219@pasky.or.cz","threadId":"9287","inReplyTo":"200708270230.04537.jnareb@gmail.com","subject":"Re: [BUG] gitweb on repo.or.cz shows buggy commitdiff","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2007-08-27T00:48:38Z","receivedAt":"2007-08-27T00:48:38Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Mon, Aug 27, 2007 at 02:30:04AM CEST, Jakub Narebski wrote:\n> 2. Alternate solution is to note that we have split patch only in the case\n> of 'T' (type change) status, and for typechange we always split patch in\n> two, so there are two patches corresponding to single raw diff-tree line.\n> \n> So it would be enough to introduce $continued_patch variable, unset it\n> after writing that patch is continued if it is true, set it to true if\n> there was typechange, i.e. if status (one of status) is 'T'.\n> \n> And we can get rid of buffering extended diff header and parsing it,\n> I think. This would simplify git_patchset_body, making it easier to\n> understand and maintain.\n> \n> But this depends on my understanding on \"git diff-tree --patch-with-raw\"\n> output, and that it would not change in the future WRT split patches.\n\nI'd prefer this solution, though I think split patch could also happen\nif you use -B (which I think is very good idea to use and it's a pity\nthat it isn't default - or is it?).\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nEarly to rise and early to bed makes a male healthy and wealthy and dead.\n                -- James Thurber\n"},{"id":"51625","messageId":"200708270312.18905.jnareb@gmail.com","threadId":"9287","inReplyTo":"20070827004838.GO1219@pasky.or.cz","subject":"Re: [BUG] gitweb on repo.or.cz shows buggy commitdiff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-08-27T01:12:18Z","receivedAt":"2007-08-27T01:12:18Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Petr Baudis wrote:\n> On Mon, Aug 27, 2007 at 02:30:04AM CEST, Jakub Narebski wrote:\n\n>> 2. Alternate solution is to note that we have split patch only in the case\n>> of 'T' (type change) status, and for typechange we always split patch in\n>> two, so there are two patches corresponding to single raw diff-tree line.\n[...]\n> \n> I'd prefer this solution, though I think split patch could also happen\n> if you use -B (which I think is very good idea to use and it's a pity\n> that it isn't default - or is it?).\n\nNo, 'B' status does not means split patch: it only affects how the\npatchset looks like, i.e first delete all then add all, instead of\ndoing change chunk by chunk.\n\nOr I think it does. \"git log --raw -p --diff-filter=B -B origin\"\non git.git repository returns rather strange results for the difftree\npart...\n\n-- \nJakub Narebski\nPoland\n"}]}