Closed Bug 430638 Opened 12 years ago Closed 12 years ago

Provide source diff between versions

Categories

(addons.mozilla.org Graveyard :: Public Pages, enhancement)

enhancement
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: bugzilla, Assigned: u278084)

Details

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: 

It may be useful (especially for editors) to be able to have a visual diff between versions when viewing the source code of an extension.

Reproducible: Always
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file Sample output
I've already started this, so I might as well finish it. I have attached what I am currently working on and integrating it with remora. This doesn't have to be final, but it is what I think works best.

The final UI should probably be similar to what browsing through file contents currently is. There should be a drag-able widget that lists the xpi structure. Clicking on a file would diff the sandbox version with the latest public version.

I don't see any sort of scenario where the editor would need to diff two old versions of the addon. And if there is, it's probably not common enough for the added complexity.
Assignee: nobody → cdolivei.bugzilla
Status: NEW → ASSIGNED
Attached patch v1.0 (obsolete) — Splinter Review
Attached is a patch that I think does what I/we want it to do. This requires the xdiff module of php to be installed. I could also use the /usr/bin/diff (or anything that outputs a unified diff) if getting that module isn't an option.

FYI, I added a string to en_US that needs to be merged with other locales.

I kept the output mostly the same from the sample output, but merged it with the editor's browse page. I had to add bits of extra logic, but I think it's ok.

Right now, it's accessible from an addon editor's review page only if there is a public version available (since that was the last addon to be reviewed by an editor). Supposedly, the diff could also be accessed if the addon author specified that the source is viewable online, but there is no user interface to get to it and I haven't tested it. But it should follow the same permissions as browsing the source code.
Attachment #329119 - Flags: review?(fligtar)
Attachment #329119 - Flags: review?(fligtar) → review?(fwenzel)
Comment on attachment 329119 [details] [diff] [review]
v1.0

I have a few things (so far ;)).

- I think you want to wrap your functions parse_diff etc. in a component (controllers/components/diff.php  or so) so they don't clutter the files controller.
- You may want to add some CSS to a CSS file instead of using inline CSS for the changed/removed lines
- yellow and pink is scary ;)
- and the most important thing: Can you make it a config option which kind of diff you want to use? I have trouble installing pecl extensions into my MAMP instance. If you allow choosing a path (or empty, meaning "use xdiff"), it'll be flexible enough for me and everybody else who wants to use it.
Attachment #329119 - Flags: review?(fwenzel) → review-
Attached patch version 1.1Splinter Review
strike 1. Here we go again :)

(In reply to comment #3)
> - I think you want to wrap your functions parse_diff etc. in a component
> (controllers/components/diff.php  or so) so they don't clutter the files
> controller.

Yes. I knew these belonged somewhere else, I wasn't sure if a component was it. I thought they were for frequently reused functions, and this certainly won't be. But reducing clutter is also a good reason :)

> - You may want to add some CSS to a CSS file instead of using inline CSS for
> the changed/removed lines
Done.

> - yellow and pink is scary ;)
Heh. I changed those to wikipedia's colours (brighter yellow + light green)

> - and the most important thing: Can you make it a config option which kind of
> diff you want to use? I have trouble installing pecl extensions into my MAMP
> instance. If you allow choosing a path (or empty, meaning "use xdiff"), it'll
> be flexible enough for me and everybody else who wants to use it.

Done. I included an option in config.php called DIFF_PATH. if you define it and set it's value to the path to a diff program, it will be called instead. Only works with GNU's diff program so far, but I think it'll be sufficient.
Attachment #329119 - Attachment is obsolete: true
Attachment #331146 - Flags: review?(fwenzel)
Comment on attachment 331146 [details] [diff] [review]
version 1.1

I hand-crafted two different versions of adblock plus on my local box, uploaded, then diffed them using your tool, and it works great.

Something to be considered would be indicating which files even changed at all? For big add-ons you have to click and scroll through all files otherwise. Anyway that's an additional feature request of mine, not a flaw in your implementation.

Well done, Cesar.
Attachment #331146 - Flags: review?(fwenzel) → review+
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → 3.4.7
I checked in the code for you: r17536. The localization is in r17538. I'll notify the localizers shortly.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: push-needed
Resolution: --- → FIXED
Btw: When pushing, IT needs instructions on defining the diff path in the config file.
Filed bug 449537 to bring this to IT's attention.
Verified FIXED on https://preview.addons.mozilla.org/en-US/firefox/files/diff/27074/; looks like this is up and running there.
Status: RESOLVED → VERIFIED
Keywords: push-needed
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.