Page MenuHomePhabricator

Phabricator task description diffs inaccurate due to 80-character line wrapping
Closed, ResolvedPublic

Description

Upstream: https://secure.phabricator.com/T3353
Very vaguely related upstream report (focuses on source diffs, not prose diffs): https://secure.phabricator.com/T6791
Related: https://secure.phabricator.com/T7643

Example. The actual change was to replace the wikilink (understood by Bugzilla but not by Phabricator) with an URL, but the darker red/green change markers are all over the place.

For reference, here is the old and new text:

From the conversation at [[commons:User_talk:Faidon_Liambotis_(WMF)#GWT_throttling]] it seems that the files downloaded by GWToolset are only deleted when the whole batch upload sequence is finished. Given that the files it uploads tend to be fairly large and one sequence can include thousands or even hundreds of thousands of them, and the files are published on Wikipedia as soon as they are downloaded (so they are not needed anymore once the upload job finishes), this might be suboptimal.
From the conversation at [[https://commons.wikimedia.org/wiki/User_talk:Faidon_Liambotis_%28WMF%29#GWT_throttling|commons:User_talk:Faidon_Liambotis_(WMF)#GWT_throttling]] it seems that the files downloaded by GWToolset are only deleted when the whole batch upload sequence is finished. Given that the files it uploads tend to be fairly large and one sequence can include thousands or even hundreds of thousands of them, and the files are published on Wikipedia as soon as they are downloaded (so they are not needed anymore once the upload job finishes), this might be suboptimal.

A standard diff algorithm such as wdiff does a more decent job:

$ wdiff old.txt new.txt
From the conversation at [-[[commons:User_talk:Faidon_Liambotis_(WMF)#GWT_throttling]]-] {+[[https://commons.wikimedia.org/wiki/User_talk:Faidon_Liambotis_%28WMF%29#GWT_throttling|commons:User_talk:Faidon_Liambotis_(WMF)#GWT_throttling]]+} it seems that the files downloaded by GWToolset are only deleted when the whole batch upload sequence is finished. Given that the files it uploads tend to be fairly large and one sequence can include thousands or even hundreds of thousands of them, and the files are published on Wikipedia as soon as they are downloaded (so they are not needed anymore once the upload job finishes), this might be suboptimal.

Event Timeline

Tgr created this task.Dec 17 2014, 8:56 PM
Tgr raised the priority of this task from to Needs Triage.
Tgr updated the task description. (Show Details)
Tgr added a project: Phabricator.
Tgr changed Security from none to None.
Tgr added a subscriber: Tgr.

From this I can't really tell what is "weird" about them?

Tgr added a comment.Dec 17 2014, 10:40 PM

See the link at the beginning. The extent of change is completely misidentified. This is how it looks:

This is how it should look:

This is how it looks with wdiff:

Qgil triaged this task as Lowest priority.Dec 18 2014, 1:38 PM
Qgil edited projects, added Phabricator (Upstream); removed Phabricator.
Qgil added a subscriber: Qgil.Dec 19 2014, 9:02 PM

Can https://secure.phabricator.com/T6791 (just reported today) serve as related upstream task?

We use diff, git diff, etc. We do not implement our own diff algorithm.

Qgil updated the task description. (Show Details)Dec 19 2014, 9:08 PM
Qgil moved this task from Ready To Go to Upstreamed on the Phabricator (Upstream) board.
Tgr added a comment.Dec 19 2014, 9:13 PM
In T78824#937047, @Qgil wrote:

Can https://secure.phabricator.com/T6791 (just reported today) serve as related upstream task?

Related but not the same. That ticket is unclear (could use an example diff and description of what the reporter would have expected) but seems to be about detecting which lines changed. This ticket is about inline markup - the diff display annotates correctly which lines changed (light green/red) but completely messes up which characters changed (dark green/red). Also...

We use diff, git diff, etc. We do not implement our own diff algorithm.

the standard command line tool for inline diffs is wdiff, which gives clean diffs. Either something else is used, or something is wrong with the way it is invoked.

Krinkle renamed this task from Phabricator inline diffs are weird to Phabricator inline inaccurate due to 80-character line wrapping.Mar 27 2015, 12:37 PM
Krinkle renamed this task from Phabricator inline inaccurate due to 80-character line wrapping to Phabricator task description diffs inaccurate due to 80-character line wrapping.

Josve05a on #wikimedia-tech:

andre__: this diff is borked.... https://phabricator.wikimedia.org/transactions/detail/PHID-XACT-TASK-qvlys6ukn6tma3i/ I only edited "Whn" > "When" and added "additional". THe diff breaks words... bug in phab?

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptMay 23 2016, 6:03 PM
epriestley updated the task description. (Show Details)May 25 2016, 4:45 PM

Upstream have changed the way diffs are viewed and it is defiantly an improvement, makes it more readable.

Upstream has split https://secure.phabricator.com/T3353 into three more specific tasks and initial work has taken place. Quoting upstream comment:

Seems fixed, yay!

mmodell closed this task as Resolved.Jun 16 2016, 4:09 PM
mmodell claimed this task.
Peachey88 removed mmodell as the assignee of this task.Jul 27 2016, 11:02 PM
Aklapper updated the task description. (Show Details)Nov 17 2016, 6:44 PM