threads / bug / 26257

[BUG] difference of info from diff and blame

Subject: [BUG] difference of info from diff and blame

## tl;dr

4 messages between Jan 11, 2011 and Jan 12, 2011.

replies: 3people: 2as markdown or json

Semyon Kirnosenko· Jan 11, 2011, 10:38 UTC · lore
Hi.

I'm working on automatic processing of info from git. And I have some problems. Let me show it on an axample.

I have jquery repo (https://github.com/jquery/jquery.git) Let's get blame for some file in some revision: git blame -l -s 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js According to blame, line 127 was added in revision 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6. Let's get diff for that revision: git diff-tree -p 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js We can see this:

@@ -105,19 +120,16 @@ jQuery.event = {

  		// Handle triggering a single element
  		else {
-			var handler = element["on" + type ], val,
-				fn = jQuery.isFunction( element[ type
+			var val, ret, fn = jQuery.isFunction( element
+			
+			// Pass along a fake event
+			data.unshift( this.fix({ type: type, target:

-			if ( handler ) {

As you can see line 127 is not marked with '+' char, which means it was 
not added in this revision. But blame sad otherwise.

I had the same problem on a wide range of repos.
I think it's a bug.
Thomas Rast· Jan 11, 2011, 13:40 UTC · re: Semyon Kirnosenko · lore

Re: [BUG] difference of info from diff and blame

Semyon Kirnosenko wrote: [Word wrap fixed.]

Show 6 quoted lines
> 
> I have jquery repo (https://github.com/jquery/jquery.git)
> Let's get blame for some file in some revision:
> git blame -l -s 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js
> According to blame, line 127 was added in revision 
> 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6.

The surrounding context (with authorship and some whitespace snipped, obviously it's always the same) is

2ad223ae (124) 2ad223ae (125) // Pass along a fake event 2ad223ae (126) data.unshift( this.fix({ type: type, target: element }) ); 2ad223ae (127) 2ad223ae (128) // Trigger the event 2ad223ae (129) if ( (val = this.handle.apply( element, data )) !== false ) 2ad223ae (130) this.triggered = true;

Show 14 quoted lines
> Let's get diff for that revision:
> git diff-tree -p 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js
> We can see this:
> @@ -105,19 +120,16 @@ jQuery.event = {
> 
>   		// Handle triggering a single element
>   		else {
> -			var handler = element["on" + type ], val,
> -				fn = jQuery.isFunction( element[ type
> +			var val, ret, fn = jQuery.isFunction( element
> +			
> +			// Pass along a fake event
> +			data.unshift( this.fix({ type: type, target:
> 
[this blank context line is line 127 in the postimage]
> -			if ( handler ) {
> 
> As you can see line 127 is not marked with '+' char, which means it was 
> not added in this revision. But blame sad otherwise.

git-blame internally runs a diff with no context lines to "pass blame". On all lines in this diff, the current commit can pass on blame to the parent, thus avoiding having to take it for itself.

And indeed, running
  git show -U0 2ad223ae -- src/event/event.js
in your repository gives a hunk
@@ -108,11 +123,8 @@ jQuery.event = {
-			var handler = element["on" + type ], val,
-				fn = jQuery.isFunction( element[ type ] );
-
-			if ( handler ) {
-				// Pass along a fake event
-				data.unshift( this.fix({ type: type, target: element }) );
-	
-				// Trigger the event
-				if ( (val = handler.apply( element, data )) !== false )
-					this.triggered = true;
-			}
+			var val, ret, fn = jQuery.isFunction( element[ type ] );
+			
+			// Pass along a fake event
+			data.unshift( this.fix({ type: type, target: element }) );
+
+			// Trigger the event
+			if ( (val = this.handle.apply( element, data )) !== false )
+				this.triggered = true;

So I would tend to agree with the blame implementation.  If anything
it's a bug in git-diff when not using any context.

Do you have a reproduction recipe that exhibits the flaw on a
non-whitespace line?  I'm not up to speed on the diff implementation
(maybe someone else can help?), but I wouldn't be too surprised if it
had heuristics that put lines consisting only of whitespace at a lower
importance than "actual" lines.
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Semyon Kirnosenko· Jan 12, 2011, 07:53 UTC · re: Thomas Rast · lore

Re: [BUG] difference of info from diff and blame

11.01.2011 16:40, Thomas Rast пишет:
Show 81 quoted lines
> Semyon Kirnosenko wrote:
> [Word wrap fixed.]
>>
>> I have jquery repo (https://github.com/jquery/jquery.git)
>> Let's get blame for some file in some revision:
>> git blame -l -s 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js
>> According to blame, line 127 was added in revision
>> 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6.
>
> The surrounding context (with authorship and some whitespace snipped,
> obviously it's always the same) is
>
> 2ad223ae (124)		
> 2ad223ae (125)		// Pass along a fake event
> 2ad223ae (126)		data.unshift( this.fix({ type: type, target: element }) );
> 2ad223ae (127)
> 2ad223ae (128)		// Trigger the event
> 2ad223ae (129)		if ( (val = this.handle.apply( element, data )) !== false )
> 2ad223ae (130)			this.triggered = true;
>
>
>> Let's get diff for that revision:
>> git diff-tree -p 2ad223aedd1f93c783d98d60adc9fda3bdfbb4b6 -- src/event/event.js
>> We can see this:
>> @@ -105,19 +120,16 @@ jQuery.event = {
>>
>>    		// Handle triggering a single element
>>    		else {
>> -			var handler = element["on" + type ], val,
>> -				fn = jQuery.isFunction( element[ type
>> +			var val, ret, fn = jQuery.isFunction( element
>> +			
>> +			// Pass along a fake event
>> +			data.unshift( this.fix({ type: type, target:
>>
> [this blank context line is line 127 in the postimage]
>> -			if ( handler ) {
>>
>> As you can see line 127 is not marked with '+' char, which means it was
>> not added in this revision. But blame sad otherwise.
>
> git-blame internally runs a diff with no context lines to "pass
> blame".  On all lines in this diff, the current commit can pass on
> blame to the parent, thus avoiding having to take it for itself.
>
> And indeed, running
>
>    git show -U0 2ad223ae -- src/event/event.js
>
> in your repository gives a hunk
>
> @@ -108,11 +123,8 @@ jQuery.event = {
> -			var handler = element["on" + type ], val,
> -				fn = jQuery.isFunction( element[ type ] );
> -
> -			if ( handler ) {
> -				// Pass along a fake event
> -				data.unshift( this.fix({ type: type, target: element }) );
> -	
> -				// Trigger the event
> -				if ( (val = handler.apply( element, data )) !== false )
> -					this.triggered = true;
> -			}
> +			var val, ret, fn = jQuery.isFunction( element[ type ] );
> +			
> +			// Pass along a fake event
> +			data.unshift( this.fix({ type: type, target: element }) );
> +
> +			// Trigger the event
> +			if ( (val = this.handle.apply( element, data )) !== false )
> +				this.triggered = true;
>
> So I would tend to agree with the blame implementation.  If anything
> it's a bug in git-diff when not using any context.
>
> Do you have a reproduction recipe that exhibits the flaw on a
> non-whitespace line?  I'm not up to speed on the diff implementation
> (maybe someone else can help?), but I wouldn't be too surprised if it
> had heuristics that put lines consisting only of whitespace at a lower
> importance than "actual" lines.
>
All cases I have seen were about whitespace lines.
-- 
Regards,
Semyon Kirnosenko
kirnosenko@mail.ru
Thomas Rast· Jan 12, 2011, 10:20 UTC · re: Semyon Kirnosenko · lore

diff -U0 occasionally misses a chance to make empty lines context [was: Re: [BUG] difference of info from diff and blame]

Ok, so to summarize for late joiners and the new Ccs:
git diff -U0 occasionally gives suboptimal results in hunks like
Semyon Kirnosenko wrote:
Show 21 quoted lines
> 11.01.2011 16:40, Thomas Rast пишет:
> > @@ -108,11 +123,8 @@ jQuery.event = {
> > -			var handler = element["on" + type ], val,
> > -				fn = jQuery.isFunction( element[ type ] );
> > -
> > -			if ( handler ) {
> > -				// Pass along a fake event
> > -				data.unshift( this.fix({ type: type, target: element }) );
> > -	
> > -				// Trigger the event
> > -				if ( (val = handler.apply( element, data )) !== false )
> > -					this.triggered = true;
> > -			}
> > +			var val, ret, fn = jQuery.isFunction( element[ type ] );
> > +			
> > +			// Pass along a fake event
> > +			data.unshift( this.fix({ type: type, target: element }) );
> > +
> > +			// Trigger the event
> > +			if ( (val = this.handle.apply( element, data )) !== false )
> > +				this.triggered = true;

where two of the whitespace lines are the same and thus could have been made context. Oddly enough, this specific line *is* made context for the default -U3. This occasionally causes git-blame (which uses the equivalent of 'git diff -U0' internally) to misattribute blank lines. This does *not* affect correctness of the diff, it is just suboptimal.

My own guess at the problem is
> > I wouldn't be too surprised if it had heuristics that put lines
> > consisting only of whitespace at a lower importance than "actual"
> > lines.
i.e. that it's whitespace-related, and indeed Semyon says
> All cases I have seen were about whitespace lines.

I don't really care, since I haven't seen a single use-case where the attribution of empty lines matters. Still, let's at least bring it to the attention of the people who have worked on libxdiff.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

← back to recent threads