Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

custom color settings in .ripgreprc and xterm-color-filter break rg-results #65

Open
nverno opened this issue Nov 4, 2019 · 15 comments

Comments

@nverno
Copy link
Contributor

nverno commented Nov 4, 2019

It took me a bit to figure out what was wrong here, and I don't think it is documented
anywhere. Here are a couple issues I ran into:

  1. If custom colors are defined in ~/.ripgreprc, eg. --colors=path:fg:cyan etc. then
    font-locking and other functions in the rg-results buffer don't work.

  2. Using xterm-color-filter also breaks rg's compilation (from xterm-color library)
    I usually use xterm-color-filter in my compilation-start-hook to interpret shell escapes,
    however, since rg (like ag.el) relies on shell escapes for parsing, font-lock/other functions
    also break in the rg-results buffer.

Quick fix is to remove xterm-color support around rg-run

(define-advice rg-run (:around (orig-fn &rest args) "no-xterm-color")
  (let ((compilation-start-hook
         (remove 'my-compilation-start-hook compilation-start-hook))
        compilation-environment)
    (apply orig-fn args)))

It looks like it would probably be out-of-the question to add support to rg-results that
didn't rely on escape codes, but it might be helpful to mention incompatibilities somewhere.

@nverno
Copy link
Contributor Author

nverno commented Nov 4, 2019

After some experimentation, the following modifications let xterm-color handle interpretation of the escape codes in the rg-mode buffer, which allows interpretation of --color flags and settings in ~/.ripgreprc.

;; Override rg's `compilation-error-regexp-alist' matching
;; to allow use with `xterm-color-filter'
(defconst rg-file-column-pattern-group
  "^\\([0-9]+\\):\\([0-9]+\\):")

(defconst rg-grouped-file-regex
  "^\\(?:File:\\s-*\\)?\\([^ 	].*\\)$")

(defun rg-match-grouped-filename-xc ()
  "Match grouped filename in compilation output."
  (save-match-data
    (save-excursion
      (beginning-of-line)
      (while (and (not (bobp))
                  (looking-at-p rg-file-column-pattern-group))
        (forward-line -1))
      (and (looking-at rg-grouped-file-regex)
           (list (match-string 1))))))

(defalias 'rg-filter #'ignore) ;; no need for `rg-filter`
(defalias 'rg-match-grouped-filename 'rg-match-grouped-filename-xc)

(defun rg-use-xterm-color ()
  ;; (setq-local compilation-transform-file-match-alist nil)
  (push 'rg-xterm-group compilation-error-regexp-alist)
  (push (cons 'rg-xterm-group
              (list rg-file-column-pattern-group
                    'rg-match-grouped-filename 1 2))
        compilation-error-regexp-alist-alist))

(add-hook 'rg-mode-hook #'rg-use-xterm-color)

@dajva
Copy link
Owner

dajva commented Nov 5, 2019

Thanks for the report.

  1. If custom colors are defined in ~/.ripgreprc, eg. --colors=path:fg:cyan etc. then
    font-locking and other functions in the rg-results buffer don't work.

This should work on master. Which version did you test on?

  1. Using xterm-color-filter also breaks rg's compilation (from xterm-color library)

I don't know how you inject xterm-color into compilation-mode but it should work with something like (add-hook 'compilation-filter-hook 'xterm-color-filter-function 'append 'local) to allow this package to override it.

Regarding your second modification, note that some features in the results buffer will not work, like some navigation, alignment of line and color numbers, hiding of color numbers etc.
I guess this could be supported in the package by making the filtering configurable, applying the necessary modifications while preserving the escape codes for xterm-color to interpret afterwards. Don't know how common this is but something in the documentation would be appropriate as you suggest.
BTW, isnt't the font-locking interfering with xterm-color as well?

@nverno
Copy link
Contributor Author

nverno commented Nov 5, 2019

I'm using version 1.8, from melpa yesterday -- I think it is the same as master?

The specific --colors options that result in errors in the process-filter that I tried have to do with underline/font-weight, eg.

--colors=path:style:bold
--colors=path:style:underline

which looks like this with xterm-color-filter

The actual error is caused by compilation-error-properties trying to match against compilation-transform-file-match-alist when no file is found, but a match occurs in the compilation-error-regexp-alist-alist. I think this is a bug in compilation-error-properties. (I'm on emacs 27, and there is a note that the default compilation-transform-file-match-alist changed in this version, so there may not be an error in earlier versions if its default is nil)

But, regardless of the error (if I set that variable to nil, there is no error), the rg-results isn't parsed correctly -- no font-lock, etc. -- with the additional --colors flags.

  1. I apply xterm-color-filter before other compilation-filters on the raw string, since it's purpose is to interpret the raw escapes, but you're right, it wouldn't be a problem to apply afterward, but it would also serve no purpose at that point since the escapes are already handled.

Yea, the things you mentioned don't work without rg-filter.

I haven't run into any problems b/w font-locking and xterm-color AFAIR.

@dajva
Copy link
Owner

dajva commented Nov 6, 2019

Oh, yeah, those are probably added to the color is specified on command the line. I think I would have to use the --no-config flag to fix that. I will do that and add an option to disable it for brave souls like you that want to do advanced customisations.

The suggestion for the order of compilation-filter was to get xterm-color out of the way when using this package. It would not give any powers to xterm-color as you want.

Having a more tight integration with xterm-color isn't high on my priority list right now but let's keep this issue open for now for visibility in case someone else think it's useful.

@nverno
Copy link
Contributor Author

nverno commented Nov 7, 2019

Sounds good, thanks!

@atomontage
Copy link

I'm the author of xterm-color.el. I don't use rg.el or ag.el but someone filed an ag.el ANSI escape matching issue recently and I was curious to see if other packages were affected.

My answer also applies to rg.el. You should absolutely not depend on ANSI escapes when you do matching on results. Hardcoding specific ANSI SGR sequences in the code should be considered a broken design since it makes terminal assumptions that could very well not hold and is also a boundary violation. I urge you to find something else to match on.

@dajva
Copy link
Owner

dajva commented Oct 10, 2020

Thanks for the input. In theory you are probably correct. This works well enough in practice though.
I think the main practical problem here is that xterm-color.el instructs its users to advice compilation-filter instead of using compilation-filter-hook for its modifications. This breaks any reasonable expectations downstream packages have on the input to its own filtering.

@atomontage
Copy link

atomontage commented Nov 15, 2020

I don't agree that raw terminal ANSI sequences being part of the compilation-filter output is a reasonable expectation.

Main issue: rg.el (and ag.el) match on raw hardcoded terminal-specific ANSI sequences. As I explained, this is broken design.
By advising compilation-filter xterm-color simply strips those sequences. It doesn't break any compilation-mode contracts but it does expose model brokenness and assumptions that do not generalize very well.

compilation-mode uses these variables that contain patterns to match on:

  • compilation-error-regexp-alist-alist
  • grep-regexp-alist

Notice that there are absolutely no terminal escapes in there. It's just matching on text output.

@diamond-lizard
Copy link

I want to chime in here as a mere user of both rg.el and xterm-color (and ag.el, which has the same issue as rg.el) and say that I'd really like to be able use all three of these packages, but right now I'm faced with the unenviable choice of either having rg.el and ag.el work, and not use xterm-color, or use xterm-color and have the other two packages break.

Can't we get past this impasse?

@atomontage
Copy link

atomontage commented Nov 15, 2020

How hard can it be to have rg.el match on text output? I don't use it so I can't really say, but it doesn't sound hard.
Could it be a 20 minute fix (that would also increase overall robustness) ?

@dajva
Copy link
Owner

dajva commented Nov 15, 2020

@diamond-lizard Not much I can do as a downstream package.
The problem here is that the the minimal expectation a downstream package can make on its upstream counter part is that it works as it's implemented. If that is no longer true (by advising the upstream package), this fundamental requirement is broken. If xterm-color still wants to recommend this way of integration it may do so of course but I think it should be clear that users are on their own when doing that. The setup even breaks emacs built in functionality in grep.el for instance.
So what you can do is to hook in xterm-color into compilation-filter-hook instead (I hope that is supported). That would allow a downstream package to have control over its input.

@atomontage As for how hard it would be, essentially you'd had to emulate ripgrep's regexp engine to be able to highlight search matches. That is not all but should at least take more than 20 minutes for most people...

@nverno
Copy link
Contributor Author

nverno commented Nov 15, 2020

  1. Using xterm-color-filter also breaks rg's compilation (from xterm-color library)

I believe this should at least be fixed on emacs 28 master, where the aforementioned error in compilation-error-properties was fixed (at least I was notified it was, although haven't tried it myself yet) bug#38081

@atomontage
Copy link

atomontage commented Nov 15, 2020

The setup even breaks emacs built in functionality in grep.el for instance.

Let me expand on this since you didn't provide details. grep.el has an optional "highlight matches" mode,
that relies on terminal escapes and performs its own interpretation and removal of said escapes.
Highlighting in this fashion, taking into account the hardcoded escape sequences, is as broken as highlighting
in rg.el and ag.el (broken in terms of assuming specific escapes and doing regular expression parsing rather than state
machine processing). The good news is that in grep.el this is optional/configurable. The even better news is that
grep.el does not conflate highlighting with semantic information extraction and does not rely on terminal
escapes for the latter.

There is a separation of concerns between semantic matching and highlighting. Since xterm-color
does a much better job at highlighting, it makes sense to disable that part of grep.el and have xterm-color do it.
This isn't possible with rg.el since it uses terminal escapes for both matching and highlighting.

For anyone that wants rg.el to work with xterm-color, the following steps ought to work:

  • Don't match on terminal color escapes for semantic (non-highlight related) data. This is a must.

  • Make highlighting optional. Folks that use xterm-color.el can then tell rg.el to not highlight faces and have xterm-color
    use its much more elaborate emulation to get better results. This is optional since xterm-color will perform its own
    emulation/highlighting before rg.el even sees terminal escapes. If you implement it, you make it crystal clear that
    you support operation in non-highlighting mode and avoid possible confusion.

So you don't need to emulate ripgrep's regexp engine to highlight search matches. Assume that xterm-color will do that,
and all you need to do is find a way to extract the semantic information you need without relying on terminal escapes. This
by the way is exactly what grep.el (and compilation-mode) does.

EDIT: I will update xterm-color documentation and make it obvious that configuring compilation-mode in the way I recommend, breaks rg and ag.el.

@dajva
Copy link
Owner

dajva commented Dec 12, 2020

Well, the thing is of course that escape codes are used for semantic data. This is the case for this package and also for built-in grep.el despite the statements above. I have no idea what ag.el does here though.
And there is no sane way around that unless (as I said) you want to implement ripgrep's regexp behavior in elisp (or similar) or restricting functionality.
In addition to that, regexps are not enough for perfectly matching of different parts of the output from ripgrep. Ripgrep itself has all the info it needs to do this perfectly and using that information gives the most robust solution.

So, essentially this all boils down to trade-offs (which is the usual case in all software projects). So I am not closing the door to actually supporting this but any comments claiming it's a "20 min fix" or similar is clearly missing the target. This package has been moving in the opposite direction for a while now and might even come to the point where using xterm-color doesn't make sense. I have been experimenting with using the ripgrep json putput which will solve all matching issues without using color escapes. Unfortunately the performance has not been up to par with current implementation so unsure if I will switch.

With all that said, my main concern is to avoid that this package is broken for users wanting to use xterm-color (which is a really cool package btw). Hooking xterm-color into compilation-filter-hook should solve that so for any rg.el users that have this problem, I recommend to investigate that path.

@atomontage
Copy link

I did some experiments with ripgrep's json output and parsing it is super fast in an Emacs built with jansson.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants