Page MenuHomePhabricator

Don't use wfDiff() in Echo
Closed, ResolvedPublic

Description

Shelling out to /usr/bin/diff and then parsing its output by hand in DiffParser sounds too complicated in addition to being slowww, just use DiffEngine to get the list of changes directly. Also, I'm deprecating wfDiff().

Details

Related Gerrit Patches:
mediawiki/extensions/Echo : masterRemove test for internal diff engine acceptance
mediawiki/extensions/Echo : masterUse internal diff engine

Event Timeline

MaxSem created this task.Mar 23 2015, 6:32 PM
MaxSem raised the priority of this task from to Needs Triage.
MaxSem updated the task description. (Show Details)
MaxSem added a project: Notifications.
MaxSem added a subscriber: MaxSem.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 23 2015, 6:32 PM
EBernhardson triaged this task as Low priority.Mar 24 2015, 5:44 PM
EBernhardson added a subscriber: EBernhardson.
ori claimed this task.May 20 2015, 8:08 PM
ori set Security to None.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptMay 20 2015, 8:08 PM

I'd rather say don't call external processes at all :)

ori closed this task as Resolved.May 25 2015, 11:42 PM

Done in Ia5239c1e7.

Krinkle moved this task from Inbox to Doing on the Performance-Team board.Jun 23 2015, 5:26 AM
Legoktm reopened this task as Open.Sep 16 2015, 7:20 AM
Legoktm added a subscriber: Legoktm.
In T93625#1311400, @ori wrote:

Done in Ia5239c1e7.

I think that's supposed to be rOMWC4f333a850fe9: Unset $wgDiff, so we stop shelling out to diff, but that still doesn't fix this bug, which is to not use wfDiff().

A volunteer wants to wfDeprecate() it, thus Echo's unit tests migh break if diffing is tested at all.

Change 335791 had a related patch set uploaded (by Legoktm):
Use internal diff engine

https://gerrit.wikimedia.org/r/335791

Change 335791 merged by jenkins-bot:
Use internal diff engine

https://gerrit.wikimedia.org/r/335791

After Echo, there is one usage left in Wikimedia Git (not counting unit tests and maintenance scripts).

wikimedia/mediawiki-extensions-AbuseFilter – AFComputedVariable.php
157					$text2 = $vars->getVar( $text2Var )->toString() . "\n";
158					$result = wfDiff( $text1, $text2 );
Aklapper reassigned this task from ori to WMDE-Fisch.Feb 23 2017, 12:53 PM
Aklapper added a subscriber: ori.

After Echo, there is one usage left in Wikimedia Git (not counting unit tests and maintenance scripts). wikimedia/mediawiki-extensions-AbuseFilter

I created T158850: Don't use wfDiff() in AbuseFilter for AbuseFilter.

So this task T93625 could get closed as resolved?

Krinkle closed this task as Resolved.Feb 24 2017, 4:51 AM
Legoktm renamed this task from Don't use wfDiff() to Don't use wfDiff() in Echo.Feb 24 2017, 11:15 AM

Change 339622 had a related patch set uploaded (by WMDE-Fisch):
Remove test for internal diff engine acceptance

https://gerrit.wikimedia.org/r/339622

Change 339622 merged by jenkins-bot:
Remove test for internal diff engine acceptance

https://gerrit.wikimedia.org/r/339622