Fix a crash with indentation computation on interdiffs.

Review Request #12362 — Created June 12, 2022 and submitted — Latest diff uploaded

Information

Review Board
release-4.0.x

Reviewers

When processing an interdiff, we filter out code we don't want to
specially highlight as "filtered-equal" chunks. These used to be ignored
by indentation detection, but Review Board 4.0.6 gained the ability to
show indentation changes for these lines, with the expectation being
that we'd show them for "equal" lines marked as "filtered-equal".
Unfortunately, this broke the indentation detection, which could see two
non-equal lines as having an indentation change.

This led to a crash in the chunk generator, where indentation
serialization code would expect to be able to iterate over a prepared
string of indentation characters, and would then reference the last
index processed during that iteration. In this case, the string was
empty, causing the loop to never run, and this caused a variable that
was needed to not yet be defined.

A few things were done to address this issue:

  1. The indentation calculations now compare strings after stripping
    them, ensuring that they still appear to be equal and safe for
    indentation calculation.

  2. The chunk generator now checks to ensure that the resulting
    indentation ranges aren't 0 (which is what we'd see in the failing
    case). While this should not be an issue anymore post-4.0.7, we could
    be dealing with something in cache. In this case, we'll end up still
    showing the chunks as their own empty uncollapsed range of lines in
    the diff viewer, but it's better than crashing.

  3. The indentation serialization code now asserts that the string is
    non-empty, so we don't hit the original crash case.

Reproduced this with a consistent failure case, and a verified the
fix (with and without the bad state in cache).

Unit tests pass.

Commits

Files

    Loading...