DiffViewer: no-op IsCriticalException helper makes catch-when filter pointless

[geevensingh]

Problem

RepositoryService.cs:515-520 has an exception filter that's a no-op:

catch (Exception) when (!IsCriticalException())
{
    return (false, false);
}

static bool IsCriticalException() => false;

The local helper unconditionally returns false, so !IsCriticalException() is always true, making the filter equivalent to a bare catch (Exception). The shape of the code (a when filter + a named helper) suggests the author intended to discriminate between recoverable and critical exceptions but never finished the implementation — possibly left as a placeholder.

The catch is in the binary/LFS detection path:

try
{
    ...
    bool isLfs = LfsPointerDetector.IsLfsPointer(span);
    bool isBinary = blob.IsBinary || BinaryDetector.IsBinary(span);
    return (isBinary, isLfs);
}
catch (Exception) when (!IsCriticalException())
{
    return (false, false);
}

static bool IsCriticalException() => false;

Returning (false, false) on any failure means a corrupted/inaccessible blob silently becomes "treat as text" — and the user gets garbage rendered as a diff instead of a "couldn't probe blob" placeholder.

Suggested fix

Either:

A. Remove the no-op filter. If the intent really is "swallow everything for binary/LFS detection," at least make it explicit:

catch (Exception)
{
    // Heuristic detection failed; assume text/non-LFS. Worst case the
    // diff renders binary garbage which is a recoverable UX issue.
    return (false, false);
}

B. Implement the helper properly. What "critical" usually means in .NET:

static bool IsCriticalException(Exception ex) =>
    ex is OutOfMemoryException
       or StackOverflowException
       or AccessViolationException
       or ThreadAbortException;

Then use it as a filter: catch (Exception ex) when (!IsCriticalException(ex)). This lets the genuinely-fatal conditions propagate so the process can fail fast or be diagnosed properly. This is what the helper looks like it was supposed to do.

I'd recommend B, since the same pattern would be useful in other catches across the codebase (e.g., SafeReadSide — see issue #3).

Citations

• DiffViewer/Services/RepositoryService.cs:515-520 — the offending block
• Related: see issue Paste button #3 (SafeReadSide) — same problem, different file.

Severity

Low-Medium. Likely never hits in practice (the ReadFully upstream is robust), but it's clearly an unfinished placeholder and worth either deleting or completing.