Page MenuHomePhabricator

Handle ComplexityException when creating the diffs for the conflict view
Closed, InvalidPublic

Description

For creating the diffs that we use for the conflict view we use the Diff class. That class throws an exception when the diff is to big to be calculated. Currently we're not doing anything when that happens so the user would run into an error with the edits lost.

One way to handle these exception could be falling back to the core conflict interface ( that potentially uses an different diff algorithm (e.g. wikidiff2 ). The downside of this would obviously be that we're depending on that old workflow ( that we would want to replace ).

The other alternative would be showing an error but dealing with the potential data loss by e.g.

  • showing the edited raw text ( so users could copy it )
  • implementing a general edit loss prevention mechanism ( see T246783 )

Event Timeline

There are additional exceptions we might run into, these should also be considered when designing fallback behaviors. When we start work on this task, let's also investigate the related cases.

thiemowmde subscribed.

The Essential-Work part of this ticket: Make a list of all relevant cases/exceptions. Explain what information we have in such an error situation, and which information we don't have. Make a list of possible ways forward. See if there is a quick workaround that at least prevents data loss, and implement it to have some working baseline.

The OKR-Work part that needs PM/UX input: Decide what's best for the user, and what we can do with our budget.

Change 785277 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/TwoColConflict@master] Handle complexity exception in ResolutionSuggester

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

I tried a simple proof of concept today. My idea was that SplitTwoColConflictHelper can start falling back to their parent TextConflictHelper when it runs into the described complexity exception. To test this I hacked Diff to always throw the exception. To my surprise the parent can't handle this exception as well, and ends crashing the same way. In other words: There is not much benefit in handling this exception in our code, esp. not when the idea is to fall back to the old conflict resolution interface. That would fail for the same reason.

Furthermore I found that this exception is handled within the set of Diff classes, and can never been seen from the outside. That was only possible because of my hack. Only WordLevelDiff sets a "bailout complexity". It expects the failure and falls back to showing the whole paragraph as a change, instead of a word-level diff. Paragraph-level Diff doesn't have a bailout.

A comment mentions other exceptions, but I'm not sure what they are. I can't find a try-catch in the core code that deals with diffs. My argument would be: As long as core's conflict resolution interface doesn't handle an exception, why would our alternative conflict resolution interface need to be better than core's? Or to phrase the argument differently: If there is an exception that needs to be catched and properly handled, that would need to be done in both the old and the new interface. That would be more a core task, not an exclusive Two-Column-Edit-Conflict-Merge task. Let's open one when the need arises.

Change 785277 merged by jenkins-bot:

[mediawiki/extensions/TwoColConflict@master] Handle complexity exception in ResolutionSuggester

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