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: Fix escaping problem #420

Merged
merged 0 commits into from Mar 7, 2016

Conversation

Projects
None yet
2 participants
@justbur
Copy link
Contributor

justbur commented Mar 7, 2016

Before this it is not possible to pass the string "string1|string2" to
ag, because inputing string1|string2 gets translated into
"string1\|string2" via %S.

Instead I use %s and shell-quote-argument to take care of escaping for
the command line, which fixes the issue for me.

Please test.

Also see syl20bnr/spacemacs#5378

@abo-abo

This comment has been minimized.

Copy link
Owner

abo-abo commented Mar 7, 2016

Thanks, but while the current way isn't correct, your suggested change isn't the right thing either: foo\\|bar gets escaped into foo\\\\|bar.

I'd like to have the same behavior with counsel-ag as with counsel-git-grep.

For example, trying counsel-git-grep with input quote\|async the on this repository results in:

3 candidates:
./counsel.el:845:  "Quickly and asynchronously count the amount of git grep REGEX matches.
./doc/Changelog.org:121:Make it fully async: the process =git grep= will be killed and
./doc/Changelog.org:511:**** Use single quotes for the regex

Same input with counsel-ag either with the current code or with your change results in no candidates. Could you please try to get this test case to work with counsel-ag? The regex syntax on ag is a bit different than for git grep (I think), hence the issue.

@justbur

This comment has been minimized.

Copy link
Contributor Author

justbur commented Mar 7, 2016

Hm, I guess the question for you is: what do you want the interface to be like?

It seems that you want quote\|async the to be quote or async, but to me that looks like you're escaping the pipe to get a literal quote|async. Emacs differs from perl here.

I think the simplest thing would be to type in the same thing that you'd type in on the command line, but what is correct on the command line is a function of the flags passed in the default command.

@abo-abo

This comment has been minimized.

Copy link
Owner

abo-abo commented Mar 7, 2016

It seems that you want quote|async the to be quote or async, but to me that looks like you're escaping the pipe to get a literal quote|async. Emacs differs from perl here.

I think the simplest thing would be to type in the same thing that you'd type in on the command line, but what is correct on the command line is a function of the flags passed in the default command.

I prefer to have the defaults be the Emacs regex. git grep already supports this.

ag doesn't have a switch to use Emacs regex, so they need to be translated when converting to a shell command. The basic translations to be added are: \| -> | and \(\) -> (). Do you wish to work on this (otherwise I could add it at some later point)?

@justbur

This comment has been minimized.

Copy link
Contributor Author

justbur commented Mar 7, 2016

I'm not that good with emacs regex, so it would be better if you did it I think.

One other feature I want at some point is the ability to pass command line args through the search pattern. For example, I'd like to be able to add -Gel$ to narrow the search to elisp files.

@abo-abo

This comment has been minimized.

Copy link
Owner

abo-abo commented Mar 7, 2016

I'm not that good with emacs regex, so it would be better if you did it I think.

OK, I'll see if I can tweak counsel-unquote-regex-parens for this.

For example, I'd like to be able to add -Gel$ to narrow the search to elisp files.

counsel-git-grep has this: you select the command if a prefix arg is given. So I do C-u C-c j, M-i -- *.el C-m. If you want, you could implement the same interface for counsel-ag.

@abo-abo

This comment has been minimized.

Copy link
Owner

abo-abo commented Mar 7, 2016

Have a look. It should fix the issue that you referenced (of course, the input should be quoted).

@justbur

This comment has been minimized.

Copy link
Contributor Author

justbur commented Mar 7, 2016

That's one of the things I was thinking of doing, but unfortunately it doesn't work for windows which I often have to use. Double quotes would work. I think with a note in the docstring explaining that you have to escape double quotes in your search string this could work well. What do you think?

@justbur

This comment has been minimized.

Copy link
Contributor Author

justbur commented Mar 7, 2016

you select the command if a prefix arg is given. So I do C-u C-c j, M-i -- *.el C-m. If you want, you could implement the same interface for counsel-ag.

My keys are different, so I'm not sure I understand the sequence. Are you saying that you specify the command before the search starts?

@abo-abo abo-abo merged commit 79f9cda into abo-abo:master Mar 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abo-abo

This comment has been minimized.

Copy link
Owner

abo-abo commented Mar 7, 2016

That's one of the things I was thinking of doing, but unfortunately it doesn't work for windows which I often have to use. Double quotes would work.

OK, I see now why shell-quote-argument was necessary. And | basically works, I just have to remember not to quote it, like I would for counsel-git-grep.

@abo-abo

This comment has been minimized.

Copy link
Owner

abo-abo commented Mar 7, 2016

My keys are different, so I'm not sure I understand the sequence. Are you saying that you specify the command before the search starts?

C-u is universal-argument.
C-c j is counsel-git-grep.

After that, it's straightforward, although I recommend M-i (ivy-insert-current) to ease the input of the full command.

@justbur

This comment has been minimized.

Copy link
Contributor Author

justbur commented Mar 7, 2016

C-u is universal-argument.
C-c j is counsel-git-grep.

After that, it's straightforward, although I recommend M-i (ivy-insert-current) to ease the input of the full command.

That's what I figured. Maybe it's laziness, but I like the ability to change the command line args while I'm searching. helm-ag has this ability, and it's something I miss sometimes. Not a huge loss though.

OK, I see now why shell-quote-argument was necessary. And | basically works, I just have to remember not to quote it, like I would for counsel-git-grep.

I'll keep an eye out for any problems. I wasn't able to test this extensively on different OSes.

@justbur justbur deleted the justbur:quoting branch Mar 7, 2016

@abo-abo

This comment has been minimized.

Copy link
Owner

abo-abo commented Mar 7, 2016

That's what I figured. Maybe it's laziness, but I like the ability to change the command line args while I'm searching. helm-ag has this ability, and it's something I miss sometimes. Not a huge loss though.

You'll have to remember that the input is to be converted into a regex by a regex-builder. Adding the ability to modify the command from the input would lead to horrible complexity.
However, nothing prevents us from having:

(define-key counsel-git-grep-map
    (kbd "C-c C-m")
  (lambda ()
    (interactive)
    (setq counsel-git-grep-cmd
          (ivy-read "cmd: " counsel-git-grep-cmd-history
                    :history 'counsel-git-grep-cmd-history))
    (unless (ivy-state-dynamic-collection ivy-last)
      (setq ivy--all-candidates
            (all-completions "" 'counsel-git-grep-function)))))
@justbur

This comment has been minimized.

Copy link
Contributor Author

justbur commented Mar 7, 2016

You'll have to remember that the input is to be converted into a regex by a regex-builder. Adding the ability to modify the command from the input would lead to horrible complexity.

Actually, I like your solution better, to temporarily step out of the search and modify the whole command.

abo-abo added a commit that referenced this pull request Mar 7, 2016

counsel.el (counsel-git-grep-map): Bind "C-c C-m" to counsel-git-grep…
…-switch-cmd

* counsel.el (counsel-git-grep-switch-cmd): New command.

The initial command always runs on all files.

To run only on *.el files, "C-c C-m" followed by "M-i" -- *.el.
To run on *.c and *.h files, "C-c C-m" followed by "M-i" -- *.c *.h.
To switch to all files again, "C-c C-m" and select the appropriate
entry.

Re #420
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.