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

counsel-ag + wgrep not working (No changes to be performed) #1841

Closed
m-renaud opened this issue Dec 5, 2018 · 22 comments
Closed

counsel-ag + wgrep not working (No changes to be performed) #1841

m-renaud opened this issue Dec 5, 2018 · 22 comments

Comments

@m-renaud
Copy link

@m-renaud m-renaud commented Dec 5, 2018

Overview

I have two machines, both running Debian and emacs v26.1, with the same counsel, swiper, wgep, and wgrep-ag packages installed. On one of them the workflow: counsel-ag, ivy-occur, ivy-wgrep-change-to-wgrep-mode works perfectly fine. I can edit the occur buffer then press C-x C-s to save and everything is updated properly. On the other, the occur buffer looks slightly different and when I try to save it says (No changes to be performed).

Strangely, for the working setup, ivy is not installed according the package manager, on the broken one it is (as a dependency of swiper), I'm not sure how this happened.

Occur buffer

Working:

-*- mode:grep; default-directory: "/home/mrenaud/" -*-


2 candidates:
./test.txt:2:b
./test.txt:4:b

Broken:

Ivy version: ivy-20181129.2105

-*- mode:grep; default-directory: "/home/mrenaud/" -*-


2 candidates:
    test.txt:2:b
    test.txt:4:b

The format in the broken one is different that the first which is (I assume) the problem. I'm confused because I've verified that the versions of the installed packages are the same (other than the missing ivy dependency on the working setup).

What I've tried

Uninstall and re-install

  • Uninstall all relevant packages (ivy, counsel, swiper, wgrep, wgrep-ag)
  • Restart emacs
  • Install ivy, counsel, swiper
  • Restart emacs
  • Install wgrep, wgrep-ag

remove .elc files
Removed all relevant .elc files in case there was some compile incompatibility

Edit: Summary of thread for future readers

The root cause of the issue is ivy--occur-insert-lines is adding leading spaces before the candidate matches when creating the ivy-occur buffer when the candidate string does not start with a . (which it does not for ag v2.1.0. wgrep requires that all candidates start at column 0 of the line.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 5, 2018

Could this be related to #1813? Are you sure you have the latest version of ivy/swiper/counsel installed? Is wgrep-ag needed in order to reproduce the issue? Have you been able to reproduce the issue from make plain and emacs -Q?

@m-renaud
Copy link
Author

@m-renaud m-renaud commented Dec 5, 2018

Are you sure you have the latest version of ivy/swiper/counsel installed?

Yup, the version numbers in my ~/.emacs.d/elpa/ directory are the exact same.

So, in the broken setup I've found in ivy.el the following:

(defun ivy--occur-insert-lines (cands)
  "Insert CANDS into `ivy-occur' buffer."
  (font-lock-mode -1)
  (dolist (str cands)
    (setq str (ivy--highlight-fuzzy (copy-sequence str)))
    (add-text-properties
     0 (length str)
     '(mouse-face
       highlight
       help-echo "mouse-1: call ivy-action")
     str)
    (insert (if (string-match-p "\\`.[/\\]" str) "" "    ")    ;; THIS LINE!
            str ?\n))
<snip>

Which checks to see if the file path begins with ., and if so doesn't insert leading space (a requirement for wgrep to work). For some reason the candidate files don't have a leading . for some reason (I've tried with just loading the offending packages). If I change this line to NEVER insert the 4 leading spaces this works as expected. This obviously isn't a proper fix but it unblocks my workflow.

When I have some time I can dig in more to try and identify the root cause. It's really bizarre how two setups that supposedly have the same versions have different behaviour :/

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 5, 2018

Are you sure you have the latest version of ivy/swiper/counsel installed?

Yup, the version numbers in my ~/.emacs.d/elpa/ directory are the exact same.

That they are the same does not mean they are the latest.

When I have some time I can dig in more to try and identify the root cause.

Thanks, please do.

It's really bizarre how two setups that supposedly have the same versions have different behaviour

What is their full M-xemacs-versionRET? Have you customised the value of counsel-ag-base-command? Do you have different versions of silversearcher-ag installed? Do you see different output on the two systems when invoking ag directly from the shell?

@m-renaud
Copy link
Author

@m-renaud m-renaud commented Dec 6, 2018

Did some more investigation and I think I found the root of the problem. First some preamble:

What is their full version?

Both the same, GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.24), modified by Debian

Do you have different versions of silversearcher-ag installed?

Nope, both:

ag version 2.1.0

Features:
  +jit +lzma +zlib

Do you see different output on the two systems when invoking ag directly from the shell?

Same output:

$ ag ^xyz$
test.txt
2:xyz
4:xyz

The Problem

So, I discovered that the working setup was using an older version of the code. Specifically, this is the implementation of ivy--occur-insert-lines that works:

(defun ivy--occur-insert-lines (cands)
  "Insert CANDS into `ivy-occur' buffer."
  (dolist (str cands)
    (add-text-properties
     0 (length str)
     `(mouse-face
       highlight
       help-echo "mouse-1: call ivy-action")
     str)
    (insert str "\n"))
  (goto-char (point-min))
  (forward-line 4))

Notice how it always insert the string starting from column 0.

Now, the broken setup uses the HEAD version:

(defun ivy--occur-insert-lines (cands)
  "Insert CANDS into `ivy-occur' buffer."
  (font-lock-mode -1)
  (dolist (str cands)
    (setq str (ivy--highlight-fuzzy (copy-sequence str)))
    (add-text-properties
     0 (length str)
     '(mouse-face
       highlight
       help-echo "mouse-1: call ivy-action")
     str)
    (insert (if (string-match-p "\\`.[/\\]" str) "" "    ")
            str ?\n))
  (goto-char (point-min))
  (forward-line 4)
  (while (re-search-forward "^[^:]+:[[:digit:]]+:" nil t)
    (ivy-add-face-text-property
     (match-beginning 0)
     (match-end 0)
     'compilation-info
     nil
     t)))

Which has the check:

(insert (if (string-match-p "\\`.[/\\]" str) "" "    ")

In other words, if the candidate (aka. matching file+content) starts with a . then don't insert any space at the beginning, otherwise insert 4 spaces and then write the candidate. The problem is ag's output (see above) doesn't produce a leading . in its output (at least not in the latest version version 2.1.0, I noticed 2.2.0 is out now) so it is inserting 4 leading spaces. This breaks wgrep which expects the candidates to start at column 0.

So, either ag changed recently to no longer output leading spaces, or ivy.el changed to insert 4 leading spaces incorrectly. Do you know why the code to add leading spaces was added? In any case, I've fixed this locally by changing ivy.el to never insert leading spaces before the candidate string.

Edit:

The specific command that counsel-ag runs is ag --nocolor --nogroup %s which produces the following output in the terminal, using ag v2.1.0:

$ ag --nocolor --nogroup ^xyz$
test.txt:2:xyz
test.txt:4:xyz

This above output is what appears to be passed to the function ivy--occur-insert-lines in ivy.el, which once again, does not contain the leading . that the function is checking against.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 6, 2018

Thanks for looking into this.

Do you know why the code to add leading spaces was added?

No, but @abo-abo and @mookid have been working on this stuff (e.g. #1795), so they should have some insights.

@mookid
Copy link
Contributor

@mookid mookid commented Dec 6, 2018

The cause is that in that case, ./ should be prepended.
This probably means that before 810d77e it used to work.

I'll take a look later today.

@m-renaud
Copy link
Author

@m-renaud m-renaud commented Dec 6, 2018

Yup, that looks like the culprit, thanks!

@abo-abo abo-abo closed this in be98b75 Dec 6, 2018
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 6, 2018

Thanks, please test.

1 similar comment
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 6, 2018

Thanks, please test.

@mookid
Copy link
Contributor

@mookid mookid commented Dec 6, 2018

Don't you want to modify the candidates list to prepend ./ if not present?

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 6, 2018

@mookid I think it's up to ag to decide if they want to prepend ./. Just because rg wants to do that, doesn't mean ag has to. Since in any case ./ is redundant because it points back to files in the current directory.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 6, 2018

@abo-abo I'm not familiar with the ivy-occur code, so forgive me if I'm wrong, but I don't think your kludge is doing the right thing. Neither counsel-ag nor counsel-rg prepends ./ to its results. The only reason (or (string-match-p ...) (eq (ivy-state-caller ivy-last) 'counsel-ag)) is non-nil for counsel-rg is because the ivy-state-caller of counsel-rg is counsel-ag, which itself is a kludge which we should not be relying on.

@m-renaud
Copy link
Author

@m-renaud m-renaud commented Dec 6, 2018

Verified that ivy.el from head works, thanks for the quick fix! Any chance of getting a new release on melpa with the fix? I really want to point a few friends who use emacs at this but don't want them to end up with a broken experience when they first try it out :)

@m-renaud
Copy link
Author

@m-renaud m-renaud commented Dec 6, 2018

Not directly related to this, but may be too heavyweight for a new issue, I noticed that when using swiper-all then ivy-occur it doesn't produce a wgrep compatible occur buffer, is there an easy way to configure that?

Edit: In other words, I always want ivy-occur to generate a wgrep compatible buffer.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 6, 2018

Any chance of getting a new release on melpa with the fix?

MELPA automatically gets updated from master every few hours. If the update isn't there already, it will be relatively soon.

@m-renaud
Copy link
Author

@m-renaud m-renaud commented Dec 6, 2018

@basil-conto Hmm, I just checked https://melpa.org/#/?q=swiper and it still says that swiper-20181120.1832 is the most recent version available and for Ivy ivy-20181129.2105 (https://melpa.org/#/ivy).

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 6, 2018

@m-renaud Like I said, if it's not updated yet, it will be relatively soon - MELPA only updates a couple of times per day or so. If you look at the notice on its homepage https://melpa.org, you will see the next build isn't for another 3 hours from the time of writing.

@m-renaud
Copy link
Author

@m-renaud m-renaud commented Dec 7, 2018

@basil-conto ah I see, I was confused because there are commits from yesterday which haven't been rebuilt, so maybe its not "every few hours" but "every few days" :)

Edit: It appears the last build "took 16 hours and ended 9 hours ago", and with 1 hour until the next build it seems like the schedule is every ~26 hours. Sorry for all the noise, I wasn't familiar with the Melpa release schedule, thanks for enlightening me :)

@JSpenced
Copy link

@JSpenced JSpenced commented Sep 24, 2019

Is anyone else having this issue again? My files won't update when using counsel-ag and wgrep but it used to work fine. It says also the wgrep buffer was modified at the end. I also tried using the helm-wgrep package so maybe it is an issue with wgrep, not something in the counsel or swiper.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Oct 8, 2019

@JSpenced I just checked with counsel-ag and it works. If the issue is still there for you, please open a new issue with reproduction steps.

@JSpenced
Copy link

@JSpenced JSpenced commented Oct 13, 2019

@abo-abo Just updated all packages and tried again with git-grep and counsel-ag. Wgrep seemed to be working fine with both. Not sure the issue before but thanks

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Oct 13, 2019

Thanks for checking.

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

Successfully merging a pull request may close this issue.

None yet
5 participants