If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Diff tool fails to show lines removed

RESOLVED FIXED in 6.0.6

Status

addons.mozilla.org Graveyard
Admin/Editor Tools
P4
minor
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: cesar, Assigned: cesar)

Tracking

unspecified
6.0.6

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 384062 [details] [diff] [review]
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.
(Assignee)

Comment 1

8 years ago
Created attachment 384301 [details] [diff] [review]
patch + test

Patch with tests. Trying to get props from test-master J.
Attachment #384062 - Attachment is obsolete: true
Attachment #384301 - Flags: review?(jbalogh)
(Assignee)

Comment 2

8 years ago
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 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.
Attachment #384301 - Flags: review?(jbalogh) → review+
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`?
(Assignee)

Comment 5

8 years ago
(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.
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.

Updated

8 years ago
Severity: normal → minor
Depends on: 371207
Priority: -- → P4
Target Milestone: --- → 4.x (triaged)
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: 4.x (triaged) → 6.0.6
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.