Workaround for `ansi-color-apply' Emacs bug#53808

[ikappaki]

Hi,

could you please consider proper fix for #3153 which addresses Emacs bug#53808, whereby stray ESC characters in the nREPL output can block the ansi-color-apply Emacs fn used by the CIDER nREPL client to colorise the incoming REPL output.

The proposed patch supersedes @dpsutton 's #1813 excelled work, which worked around the chunked nREPL output split on partial ANSI control sequences by immediately flushing out any partial control sequence caught up between the chunks (and this is why we see valid ANSI color control chars printout in the #3153 example).

The patch improves on #1813 by only flushing out the partial ANSI control seq if it determines it is definitely not a partial or full C1 SGR seq (i.e. ESC followed by [ followed by zero or more [:digit:]+; followed by one or more digits, followed by m), otherwise it waits for the next chunk until the SGR seq is complete or becomes invalid.

Added additional test assertions to catch the case where ANSI color output is split at various points in the SGR seq.

Enabled only in Emacs versions < 29.

Also fixed a couple of old linter docstring warnings, while at it.

Thanks

---

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

• The commits are consistent with our contribution guidelines
• You've added tests (if possible) to cover your change(s)
• All tests are passing (eldev test)
• All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the user manual (if adding/changing user-visible functionality)

[vemv]

Seems reasonable to me, thanks for your work!

Some suggestions.

[vemv]

This logic seems worth commenting; while the variable names are useful one might not necessarily follow the intent

[ikappaki]

Logic has been simplified to only check for a partial SGR seq. I realised that the looking for a full SGR expression in the fragment is redundant, since the ansi-color-context fragment can only hold partial ansi control seq by definition.

Please let me know if the comments are still not self explanatory.

[vemv]

Ideally this defun (or the core of it) would be unit tested. I realise you have expanded the existing tests, but those look integration-ish (they are concerned with buffers etc; a more direct test would be simply show an input->output mapping for a pure function)

[bbatsov]

I'd name this cider-repl--ansi-color-apply. I don't like having in the package functions that don't have a cider- prefix, as this makes it harder for users to figure out where they come from.

[ikappaki]

@vemv Not sure if the mention of "pure function" refers to that function, it is not pure, since it calls on ansi-color-apply which keeps state in ansi-color-context. My opinion is that we are fine with the current testing since the tests are meant to be exercising this function, and is the only way it can get through to it. I think i have to duplicate exactly the same input seq, which I wouldn't recommend. Furthermore, this function will eventually become inactive in Emacs 29, in which case the tests will not run, though the "integration" tests will still be valid.

@bbatsov, name changed.

[vemv]

I realise cider-repl--ansi-color-apply is not pure, however often these defuns can be decomposed into pure defuns, which can be tested in a particularly illustrative way. Which would be useful here, since this particular domain seems rather dense.

Please don't consider my suggestion a blocker, you are right in that the CIDER test suite is what it is (and it's ok, just not awesome, arguably).

Feel free to give it a shot if it doesn't turn out to be a disproportionate effort.

[ikappaki]

I think unless there is a strong recommendation otherwise, I will pass on this one. It is as you said that this particular domain is dense, but I don't think it worth spending more time trying to structure a throwaway hack around a bug into something that spans multiple functions that are each well tested. I find with the update that the fn is now well documented, and whoever wants to look into the full details, they need have a read through the ansi-color lib, the ansi control sequences, and in particular the SGR subset. I also think that the fun is well tested as part of exercising the functionality above it that uses it with all possible combination of ansi color sequences. Nevertheless I can still have a look at breaking it up if you or somebody else insists. Thanks :)

[ikappaki]

Hi, I've updated the comments are per feedback, I've also realised that there is no need to check for the presence of a fully realised SGR seq in the ansi-color-context fragment, since by definition the fragment only holds partial ANSI control seq of which the SGR seq is a subset of).

Thanks

[vemv]

Thanks much for your contribution!