Closed
Bug 499259
Opened 15 years ago
Closed 13 years ago
Diff tool fails to show lines removed
Categories
(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P4)
addons.mozilla.org Graveyard
Admin/Editor Tools
Tracking
(Not tracked)
RESOLVED
FIXED
6.0.6
People
(Reporter: u278084, Assigned: u278084)
References
()
Details
Attachments
(1 file, 1 obsolete file)
5.16 KB,
patch
|
jbalogh
:
review+
|
Details | Diff | Splinter Review |
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.
Patch with tests. Trying to get props from test-master J.
Attachment #384062 -
Attachment is obsolete: true
Attachment #384301 -
Flags: review?(jbalogh)
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•15 years ago
|
||
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+
Comment 4•15 years ago
|
||
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`?
(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•15 years ago
|
||
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•15 years ago
|
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: 4.x (triaged) → 6.0.6
Updated•8 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•