499259 - Diff tool fails to show lines removed

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P4)

Description

[u278084]

Attachment: wip

I found this while doing a local diff that was broken on AMO.

It's kinda hard to explain. If code is removed in a new version of an add-on, and the new code starts with a blank line (or a line with just spaces), then the removed code will fail to show.

I have attached a solution, which is just some code commented out (which I think i put in to reduce output). A test case will be needed to reduce the risk of a regression.

Comment 1

[u278084]

Attachment: patch + test

Patch with tests. Trying to get props from test-master J.

Comment 2

[u278084]

I should probably write the STR, since it is going to be asked. I was able to reproduce this locally by :

1) uploading Juice 0.1.10 and make it public
2) upload Juice 1.0.1
3) Go to editor tools and go to the juice add-on. Do a comparison
4) View components > dao.js file

AR :
Only code added is displayed

ER :
Code that has been removed should be also listed

Comment 3

[Jeff Balogh (:jbalogh)]

Comment on attachment 384301 [details] [diff] [review]
patch + test

An oh-so-slightly hesitant r+ because I can't even get the diff tools working.  Your steps to reproduce helped a bunch, but now I'm getting iconv errors and then xdiff errors, which your patch shouldn't be affecting.  I'm assuming it works because you would obviously prefer to submit working code, and because it has a test so it can't fail.

Comment 4

[Wil Clouser [:clouserw]]

I'm a little nervous/curious about creating a new object for every line that is changed.  I don't know how PHP objects fare memory wise these days but that seems a little excessive.

Hopefully we'll be getting a new MXR for add-ons soon with developer.amo that can take the stress off PHP when diffing our add-ons.  In the mean time, why can't we just store the output of `diff`?

Comment 5

[u278084]

(In reply to comment #3) 
> Your steps to reproduce helped a bunch, but now I'm getting iconv errors and
> then xdiff errors, which your patch shouldn't be affecting.  I'm assuming it
> works because you would obviously prefer to submit working code, and because it
> has a test so it can't fail.

Thanks Jeff. iconv errors/warnings are normal. The xdiff errors are not. Do you have DIFF_PATH commented out in your config.php? If you have that defined, it should work.

(In reply to comment #4)
> I'm a little nervous/curious about creating a new object for every line that is
> changed.  I don't know how PHP objects fare memory wise these days but that
> seems a little excessive.
> 
> Hopefully we'll be getting a new MXR for add-ons soon with developer.amo that
> can take the stress off PHP when diffing our add-ons.  In the mean time, why
> can't we just store the output of `diff`?

Not sure either. I just needed a structure to hold additional data :) Most source files are hundreds of lines long, with some going over 10K. We seem to handle that sort of data using hundreds of classes without hitting memory issues.

I had another editor wanting the `diff` output as well. IMO, it's both difficult to read and doesn't give sufficient amount of context to do a review. If it's useful, then we should definitely have it in as well.

Comment 6

[Wil Clouser [:clouserw]]

We can adjust the amount of context `diff` gives.  What are you storing that diff doesn't offer you?  The code above just stores text, symbol, and changed, all of which a diff gives you.