Issue #902: Side-by-side diff breaks on carriage returns (marcinkuzminski/rhodecode)

reima's Avatar

reima

29 Nov, 2013 07:27 PM

New issue 902: Side-by-side diff breaks on carriage returns
https://bitbucket.org/marcinkuzminski/rhodecode/issue/902/side-by-side-diff-breaks-on-carriage

reima:

# Problem #

When a diff contains a carriage return character (`\r`), the side-by-side diff page is missing the actual diff section and a JavaScript error is logged ("Uncaught SyntaxError: Unexpected token ILLEGAL" on Chrome, "SyntaxError: unterminated string literal" on Firefox) on the line starting with `var orig1 =`.

The reason is that carriage returns are pasted verbatim into the JavaScript code, where they are interpreted as line breaks inside a string literal.

# Quick solution #
A quick fix would be to add the entry

```
#!python
'\r': '\\r',
```
to the `html_escape_table` in `FilesController.diff_2way`.

# Preferred solution #
However, a more robust solution would be to escape everything outside the ASCII printable characters range (32 to 126) using unicode escape sequences.

For reference, the [Apache Commons Lang](http://commons.apache.org/proper/commons-lang/) library provides a method `escapeEcmaScript`, which uses an equivalent to the following translation table (see [1](http://commons.apache.org/proper/commons-lang/javadocs/api-3.1/src-html/org/apache/commons/lang3/StringEscapeUtils.html#line.63) and [2](http://commons.apache.org/proper/commons-lang/apidocs/src-html/org/apache/commons/lang3/text/translate/EntityArrays.html#line.395)):
```
#!python
html_escape_table = {
    "'": "\\'",
    "\"": "\\\"",
    "\\": "\\\\",
    "/": "\\/",
    "\b": "\\b",
    "\n": "\\n",
    "\t": "\\t",
    "\f": "\\f",
    "\r": "\\r",
}
```
It *additionally* replaces characters outside the ASCII printable characters range.

# Alternate solution #
As an alternative suggestion, one could also circumvent the problem by not embedding the diff contents at all. Instead, the file contents at the two revisions could be loaded via additional AJAX requests. A "proof of concept" patch for this solution is attached (it doesn't handle binary files for example).

Responsible: marcinkuzminski

  1. Support Staff 1 Posted by Marcin Kuzminsk... on 15 Dec, 2013 07:54 PM

    Marcin Kuzminski's Avatar

    This is now fixed and merged into stable branch, we load files via ajax for two way compare.

  2. Marcin Kuzminski closed this discussion on 15 Dec, 2013 07:54 PM.

Comments are currently closed for this discussion. You can start a new one.

Keyboard shortcuts

Generic

? Show this help
ESC Blurs the current field

Comment Form

r Focus the comment reply box
^ + ↩ Submit the comment

You can use Command ⌘ instead of Control ^ on Mac

Recent Discussions

21 Sep, 2018 04:40 PM
20 Sep, 2018 07:42 PM
18 Sep, 2018 03:30 PM
11 Sep, 2018 09:12 AM
11 Sep, 2018 08:12 AM