{"thread":{"id":"26257","subject":"[BUG] difference of info from diff and blame","startedAt":"2011-01-11T10:38:50Z","lastAt":"2011-01-12T10:20:26Z","messageCount":4,"participants":["Semyon Kirnosenko","Thomas Rast"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"159317","messageId":"4D2C333A.3010401@gmail.com","threadId":"26257","inReplyTo":null,"subject":"[BUG] difference of info from diff and blame","fromName":"Semyon Kirnosenko","fromEmail":"semyon.kirnosenko@gmail.com","sentAt":"2011-01-11T10:38:50Z","receivedAt":"2011-01-11T10:38:50Z","isPatch":false,"sender":{"key":"semyon.kirnosenko@gmail.com","avatar":null},"body":"Hi.\n\nI'm working on automatic processing of info from git. And I have some \nproblems. Let me show it on an axample.\n\nI have jquery repo (https://github.com/jquery/jquery.git)\nLet's get blame for some file in some revision:\ngit blame -l -s 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- \nsrc/event/event.js\nAccording to blame, line 127 was added in revision \n2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6.\nLet's get diff for that revision:\ngit diff-tree -p 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- \nsrc/event/event.js\nWe can see this:\n@@ -105,19 +120,16 @@ jQuery.event = {\n\n  \t\t// Handle triggering a single element\n  \t\telse {\n-\t\t\tvar handler = element[\"on\" + type ], val,\n-\t\t\t\tfn = jQuery.isFunction( element[ type\n+\t\t\tvar val, ret, fn = jQuery.isFunction( element\n+\t\t\t\n+\t\t\t// Pass along a fake event\n+\t\t\tdata.unshift( this.fix({ type: type, target:\n\n-\t\t\tif ( handler ) {\n\nAs you can see line 127 is not marked with '+' char, which means it was \nnot added in this revision. But blame sad otherwise.\n\nI had the same problem on a wide range of repos.\nI think it's a bug.\n"},{"id":"159332","messageId":"201101111440.08619.trast@student.ethz.ch","threadId":"26257","inReplyTo":"4D2C333A.3010401@gmail.com","subject":"Re: [BUG] difference of info from diff and blame","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-01-11T13:40:08Z","receivedAt":"2011-01-11T13:40:08Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Semyon Kirnosenko wrote:\n[Word wrap fixed.]\n> \n> I have jquery repo (https://github.com/jquery/jquery.git)\n> Let's get blame for some file in some revision:\n> git blame -l -s 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js\n> According to blame, line 127 was added in revision \n> 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6.\n\nThe surrounding context (with authorship and some whitespace snipped,\nobviously it's always the same) is\n\n2ad223ae (124)\t\t\n2ad223ae (125)\t\t// Pass along a fake event\n2ad223ae (126)\t\tdata.unshift( this.fix({ type: type, target: element }) );\n2ad223ae (127)\n2ad223ae (128)\t\t// Trigger the event\n2ad223ae (129)\t\tif ( (val = this.handle.apply( element, data )) !== false )\n2ad223ae (130)\t\t\tthis.triggered = true;\n\n\n> Let's get diff for that revision:\n> git diff-tree -p 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js\n> We can see this:\n> @@ -105,19 +120,16 @@ jQuery.event = {\n> \n>   \t\t// Handle triggering a single element\n>   \t\telse {\n> -\t\t\tvar handler = element[\"on\" + type ], val,\n> -\t\t\t\tfn = jQuery.isFunction( element[ type\n> +\t\t\tvar val, ret, fn = jQuery.isFunction( element\n> +\t\t\t\n> +\t\t\t// Pass along a fake event\n> +\t\t\tdata.unshift( this.fix({ type: type, target:\n> \n[this blank context line is line 127 in the postimage]\n> -\t\t\tif ( handler ) {\n> \n> As you can see line 127 is not marked with '+' char, which means it was \n> not added in this revision. But blame sad otherwise.\n\ngit-blame internally runs a diff with no context lines to \"pass\nblame\".  On all lines in this diff, the current commit can pass on\nblame to the parent, thus avoiding having to take it for itself.\n\nAnd indeed, running\n\n  git show -U0 2ad223ae -- src/event/event.js\n\nin your repository gives a hunk\n\n@@ -108,11 +123,8 @@ jQuery.event = {\n-\t\t\tvar handler = element[\"on\" + type ], val,\n-\t\t\t\tfn = jQuery.isFunction( element[ type ] );\n-\n-\t\t\tif ( handler ) {\n-\t\t\t\t// Pass along a fake event\n-\t\t\t\tdata.unshift( this.fix({ type: type, target: element }) );\n-\t\n-\t\t\t\t// Trigger the event\n-\t\t\t\tif ( (val = handler.apply( element, data )) !== false )\n-\t\t\t\t\tthis.triggered = true;\n-\t\t\t}\n+\t\t\tvar val, ret, fn = jQuery.isFunction( element[ type ] );\n+\t\t\t\n+\t\t\t// Pass along a fake event\n+\t\t\tdata.unshift( this.fix({ type: type, target: element }) );\n+\n+\t\t\t// Trigger the event\n+\t\t\tif ( (val = this.handle.apply( element, data )) !== false )\n+\t\t\t\tthis.triggered = true;\n\nSo I would tend to agree with the blame implementation.  If anything\nit's a bug in git-diff when not using any context.\n\nDo you have a reproduction recipe that exhibits the flaw on a\nnon-whitespace line?  I'm not up to speed on the diff implementation\n(maybe someone else can help?), but I wouldn't be too surprised if it\nhad heuristics that put lines consisting only of whitespace at a lower\nimportance than \"actual\" lines.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"159394","messageId":"4D2D5E08.9040804@gmail.com","threadId":"26257","inReplyTo":"201101111440.08619.trast@student.ethz.ch","subject":"Re: [BUG] difference of info from diff and blame","fromName":"Semyon Kirnosenko","fromEmail":"semyon.kirnosenko@gmail.com","sentAt":"2011-01-12T07:53:44Z","receivedAt":"2011-01-12T07:53:44Z","isPatch":false,"sender":{"key":"semyon.kirnosenko@gmail.com","avatar":null},"body":"11.01.2011 16:40, Thomas Rast пишет:\n> Semyon Kirnosenko wrote:\n> [Word wrap fixed.]\n>>\n>> I have jquery repo (https://github.com/jquery/jquery.git)\n>> Let's get blame for some file in some revision:\n>> git blame -l -s 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js\n>> According to blame, line 127 was added in revision\n>> 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6.\n>\n> The surrounding context (with authorship and some whitespace snipped,\n> obviously it's always the same) is\n>\n> 2ad223ae (124)\t\t\n> 2ad223ae (125)\t\t// Pass along a fake event\n> 2ad223ae (126)\t\tdata.unshift( this.fix({ type: type, target: element }) );\n> 2ad223ae (127)\n> 2ad223ae (128)\t\t// Trigger the event\n> 2ad223ae (129)\t\tif ( (val = this.handle.apply( element, data )) !== false )\n> 2ad223ae (130)\t\t\tthis.triggered = true;\n>\n>\n>> Let's get diff for that revision:\n>> git diff-tree -p 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js\n>> We can see this:\n>> @@ -105,19 +120,16 @@ jQuery.event = {\n>>\n>>    \t\t// Handle triggering a single element\n>>    \t\telse {\n>> -\t\t\tvar handler = element[\"on\" + type ], val,\n>> -\t\t\t\tfn = jQuery.isFunction( element[ type\n>> +\t\t\tvar val, ret, fn = jQuery.isFunction( element\n>> +\t\t\t\n>> +\t\t\t// Pass along a fake event\n>> +\t\t\tdata.unshift( this.fix({ type: type, target:\n>>\n> [this blank context line is line 127 in the postimage]\n>> -\t\t\tif ( handler ) {\n>>\n>> As you can see line 127 is not marked with '+' char, which means it was\n>> not added in this revision. But blame sad otherwise.\n>\n> git-blame internally runs a diff with no context lines to \"pass\n> blame\".  On all lines in this diff, the current commit can pass on\n> blame to the parent, thus avoiding having to take it for itself.\n>\n> And indeed, running\n>\n>    git show -U0 2ad223ae -- src/event/event.js\n>\n> in your repository gives a hunk\n>\n> @@ -108,11 +123,8 @@ jQuery.event = {\n> -\t\t\tvar handler = element[\"on\" + type ], val,\n> -\t\t\t\tfn = jQuery.isFunction( element[ type ] );\n> -\n> -\t\t\tif ( handler ) {\n> -\t\t\t\t// Pass along a fake event\n> -\t\t\t\tdata.unshift( this.fix({ type: type, target: element }) );\n> -\t\n> -\t\t\t\t// Trigger the event\n> -\t\t\t\tif ( (val = handler.apply( element, data )) !== false )\n> -\t\t\t\t\tthis.triggered = true;\n> -\t\t\t}\n> +\t\t\tvar val, ret, fn = jQuery.isFunction( element[ type ] );\n> +\t\t\t\n> +\t\t\t// Pass along a fake event\n> +\t\t\tdata.unshift( this.fix({ type: type, target: element }) );\n> +\n> +\t\t\t// Trigger the event\n> +\t\t\tif ( (val = this.handle.apply( element, data )) !== false )\n> +\t\t\t\tthis.triggered = true;\n>\n> So I would tend to agree with the blame implementation.  If anything\n> it's a bug in git-diff when not using any context.\n>\n> Do you have a reproduction recipe that exhibits the flaw on a\n> non-whitespace line?  I'm not up to speed on the diff implementation\n> (maybe someone else can help?), but I wouldn't be too surprised if it\n> had heuristics that put lines consisting only of whitespace at a lower\n> importance than \"actual\" lines.\n>\n\nAll cases I have seen were about whitespace lines.\n\n-- \nRegards,\nSemyon Kirnosenko\nkirnosenko@mail.ru\n"},{"id":"159401","messageId":"201101121120.26724.trast@student.ethz.ch","threadId":"26257","inReplyTo":"4D2D5E08.9040804@gmail.com","subject":"diff -U0 occasionally misses a chance to make empty lines context [was: Re: [BUG] difference of info from diff and blame]","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-01-12T10:20:26Z","receivedAt":"2011-01-12T10:20:26Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Ok, so to summarize for late joiners and the new Ccs:\n\ngit diff -U0 occasionally gives suboptimal results in hunks like\n\nSemyon Kirnosenko wrote:\n> 11.01.2011 16:40, Thomas Rast пишет:\n> > @@ -108,11 +123,8 @@ jQuery.event = {\n> > -\t\t\tvar handler = element[\"on\" + type ], val,\n> > -\t\t\t\tfn = jQuery.isFunction( element[ type ] );\n> > -\n> > -\t\t\tif ( handler ) {\n> > -\t\t\t\t// Pass along a fake event\n> > -\t\t\t\tdata.unshift( this.fix({ type: type, target: element }) );\n> > -\t\n> > -\t\t\t\t// Trigger the event\n> > -\t\t\t\tif ( (val = handler.apply( element, data )) !== false )\n> > -\t\t\t\t\tthis.triggered = true;\n> > -\t\t\t}\n> > +\t\t\tvar val, ret, fn = jQuery.isFunction( element[ type ] );\n> > +\t\t\t\n> > +\t\t\t// Pass along a fake event\n> > +\t\t\tdata.unshift( this.fix({ type: type, target: element }) );\n> > +\n> > +\t\t\t// Trigger the event\n> > +\t\t\tif ( (val = this.handle.apply( element, data )) !== false )\n> > +\t\t\t\tthis.triggered = true;\n\nwhere two of the whitespace lines are the same and thus could have\nbeen made context.  Oddly enough, this specific line *is* made context\nfor the default -U3.  This occasionally causes git-blame (which uses\nthe equivalent of 'git diff -U0' internally) to misattribute blank\nlines.  This does *not* affect correctness of the diff, it is just\nsuboptimal.\n\nMy own guess at the problem is\n\n> > I wouldn't be too surprised if it had heuristics that put lines\n> > consisting only of whitespace at a lower importance than \"actual\"\n> > lines.\n\ni.e. that it's whitespace-related, and indeed Semyon says\n\n> All cases I have seen were about whitespace lines.\n\nI don't really care, since I haven't seen a single use-case where the\nattribution of empty lines matters.  Still, let's at least bring it to\nthe attention of the people who have worked on libxdiff.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}