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

helm-do-grep dissapeared? #1377

Closed
vspinu opened this issue Feb 6, 2016 · 20 comments
Closed

helm-do-grep dissapeared? #1377

vspinu opened this issue Feb 6, 2016 · 20 comments

Comments

@vspinu
Copy link
Contributor

vspinu commented Feb 6, 2016

There is no more helm-do-grep? I am getting Wrong type argument: commandp, helm-do-grep. I am on MELPA 20160204.926. I don't see it defined in the source anymore but plenty of references to it, including the menu definition.

@thierryvolpiatto
Copy link
Member

Vitalie Spinu notifications@github.com writes:

There is no more helm-do-grep?

Yes it have been removed. I have removed the remaining references, thanks.

Thierry

@vspinu
Copy link
Contributor Author

vspinu commented Feb 7, 2016

Ok. So what's the alternative then? Would be convenient if you could obsolete a function with a warning with new alternative, so that people won't get surprised.

@vspinu
Copy link
Contributor Author

vspinu commented Feb 7, 2016

I see that there is no alternative. You can call grep from helm files commands only. Why did you make this decision? I am not using helm-files ecosistem. I find it ineficient, but I use everything else.

I also don't like helm-do-grep-ag because of the innability to have multiple matches and decent highlighting.

@thierryvolpiatto
Copy link
Member

Vitalie Spinu notifications@github.com writes:

Ok. So what's the alternative then?

Start with helm-find-files.

Thierry

@thierryvolpiatto
Copy link
Member

Vitalie Spinu notifications@github.com writes:

I see that there is no alternative. You can call grep from helm files
commands only. Why did you make this decision? I am not using
helm-files ecosistem. I find it ineficient, but I use everything else.

You were using helm files stuff anyway from helm-do-grep (helm-read-file-name).

I also don't like helm-do-grep-ag because of the innability to have
multiple matches and decent highlighting.

So don't use it.

Thierry

@vspinu
Copy link
Contributor Author

vspinu commented Feb 7, 2016

Start with helm-find-files.

Sorry, but that just doesn't make sense. Why would I start with helm-find-files? I just want to get grep without much ado. In my case it would amount to M-s f then C-s. That's one keycombo too many. I had it on M-s M-g which is very fast on QWERTY.

I am baffled. Why would you remove such an essential functionality from top level?

@michael-heerdegen
Copy link
Contributor

I have no strong opinion about that. But it's very easy to define it by yourself:

(defun helm-do-grep ()
  (interactive)
  (helm-do-grep-1 (helm-read-file-name 
                   "Search in file(s): "
                   :marked-candidates t)
                  (or current-prefix-arg helm-current-prefix-arg)))

@vspinu
Copy link
Contributor Author

vspinu commented Feb 8, 2016

That solves my problem. Even more so, I am skipping the read-file-name part altogether:

(defun helm-do-grep ()
  (interactive)
  (helm-do-grep-1 (list default-directory)
                  (or current-prefix-arg helm-current-prefix-arg)))

I think this version is consistent with the helm-do-grep-ag command and begs to be part of helm proper.

@michael-heerdegen
Copy link
Contributor

Great. I think referring to helm-current-prefix-arg does also not make sense when you call this from the top level, so

(defun helm-do-grep (&optional arg)
  (interactive "P")
  (helm-do-grep-1 (list default-directory) arg))

should do the same.

(Does that so something useful with no prefix arg given?)

@vspinu
Copy link
Contributor Author

vspinu commented Feb 8, 2016

should do the same.

Indeed!

(Does that so something useful with no prefix arg given?)

It's clearly not as useful as ag but it has much better highlighting and allows multi matching. Till grep-ag can do that I am using do-grep for current dir and -ag for recursive stuff.

A much more useful version would run a project based grep (like helm-grep-do-git-grep) and then fall back on simple grep if not in a project directory. Something along the lines of how helm-browse-project behaves right now.

@alphapapa
Copy link
Member

This was confusing for me as well. I used to use "C-M-g" for helm-do-grep, but then one day when I did that (after having updated Helm at some time in the past), it gave an error saying the function couldn't be autoloaded. I looked through all the helm files to find the function, but it was gone. Finally I came to GitHub and looked through the source for it, but it was not here either. Then I searched the issues and found this.

ag is nice, but sometimes grep is needed, like when searching non-source files. And I used to access it with C-M-g directly, so having to go through helm-find-files is just another step to have to take. It would be nice to have helm-do-grep back. It would also make it much more discoverable, because until I came here, I didn't even realize that helm-find-files had an action for grep; I was looking in M-x and describe-function, like I do for everything else. :)

@vspinu What do you mean by "multi matching"? I thought you meant that helm-do-grep-1 supported multiple patterns, like using helm-swoop for a buffer, but when I type a second pattern after a space, it only finds matches for the entire string, not for individual strings. I know that grep itself supports multiple patterns with -e PATTERN, but does Helm's grep?

Thanks.

@vspinu
Copy link
Contributor Author

vspinu commented Mar 18, 2016

What do you mean by "multi matching"? I thought you meant that helm-do-grep-1 supported multiple patterns,

I was somewhat confused back then. helm-grep indeed allows for multiple matches but those are treated as OR contrary to standard AND behavior everywhere else. So, in the current implementation,helm-do-grep-1 is not such an useful feature after all.

@thierryvolpiatto
Copy link
Member

Vitalie Spinu notifications@github.com writes:

helm-grep indeed allows for multiple matches

It doesn't.

but those are treated as OR

They are not unless you type "foo|bar".
"foo bar" will match "foo" followed by one space followed by "bar".

contrary to standard AND behavior everywhere else.

You have to understand that helm-grep is a frontend of diverse grep
backend, and BTW behave like the backend in use, so stop comparing with
the behavior of e.g helm-occur.

So, in the current implementation,helm-do-grep-1 is not such an useful
feature after all.

So I suggest you use something else.

Thierry

@alphapapa
Copy link
Member

Well, I still find it useful. :) It can be a little confusing sometimes, if I've forgotten that it doesn't accept multiple patterns. I wonder, would it be possible for some kind of hybrid mode that used grep for the first pattern and did "normal" helm filtering on additional ones, using the results from grep? It would be sort of like a pseudo-multi-AND-pattern grep, almost like helm-swoop across multiple files. It would be nearly as good as "the real thing." Maybe it could be called helm-grep-80/20. :)

@vspinu
Copy link
Contributor Author

vspinu commented Mar 19, 2016

helm-grep indeed allows for multiple matches
It doesn't.

This is what I see on my machine. I do use grep with helm-grep-default-command "grep --color=always -d skip -n%cH -e %p %f". Not going to investigate this further.

so stop comparing with the behavior of e.g helm-occur.

I use helm, not grep in terminal. Even if it's just a front end. I expect it to have uniform UI regardless of the domain.

@alphapapa
Copy link
Member

@vspinu If I understand grep and Helm correctly, making helm-grep accept multiple patterns in an AND fashion would require stringing together multiple grep invocations, one for each pattern, and combining the results. I suppose that could be done, but it would be slow for large filesets, and maybe Thierry has other reasons he doesn't want to implement that.

But if you wanted to do it yourself, it probably wouldn't be too difficult. You could capture the results of the first grep in a temp buffer, then pipe that buffer through another grep for each pattern, and then present the final results. You'd probably want a longer typing delay before executing the action.

@vspinu
Copy link
Contributor Author

vspinu commented Mar 19, 2016

would require stringing together multiple grep invocations

Piping grep results for each pattern is probably the way to go. This one works for me reasonably well:

(defun my-helm-pipe-grep-match (fun &rest args)
  (let* ((patterns (split-string helm-pattern))
         (helm-grep-default-command
          (cl-reduce (lambda (grep pat)
                       (concat grep " | grep --color=always " pat))
                     (cdr patterns)
                     :initial-value (replace-regexp-in-string "%p" (car patterns) helm-grep-default-command))))
    (apply fun args)))

(advice-add 'helm-grep--prepare-cmd-line :around 'my-helm-pipe-grep-match)

@alphapapa
Copy link
Member

@thierryvolpiatto I still don't understand why you removed helm-do-grep though. Going through helm-find-files doesn't replace it, because it only works on marked files, so the user has to do something like C-x c C-x C-f M-a C-s, whereas before, when helm-do-grep existed, I could access it with a single command bound to one key. That's 6 key-combinations vs. 1.

I looked through the commits in question, including 013ee11, but I can't find any explanation.

So, why did you remove it? And since ag does not replace it, can we please have it back? :) Specifically, this code works great, and is extremely useful:

(defun my-helm-pipe-grep-match (fun &rest args)
  (let* ((patterns (split-string helm-pattern))
         (helm-grep-default-command
          (cl-reduce (lambda (grep pat)
                       (concat grep " | grep --color=always " pat))
                     (cdr patterns)
                     :initial-value (replace-regexp-in-string "%p" (car patterns) helm-grep-default-command))))
    (apply fun args)))

(advice-add 'helm-grep--prepare-cmd-line :around 'my-helm-pipe-grep-match)

(defun helm-do-grep (&optional arg)
  (interactive "P")
  (helm-do-grep-1 (list default-directory) arg))

@thierryvolpiatto
Copy link
Member

alphapapa notifications@github.com writes:

@thierryvolpiatto I still don't understand why you removed
helm-do-grep though.

I am now tired to explain, I have added an entry in FAQ.

Going through helm-find-files doesn't replace it, because it only
works on marked files, so the user has to do something like C-x c C-x
C-f M-a C-s, whereas before, when helm-do-grep existed, I could access
it with a single command bound to one key. That's 6 key-combinations
vs. 1.

No, in your case it 3 vs 2.

BTW I have added a new user variable called helm-ff-no-preselect which
will reduce the keys pressed in your case to:

2 vs 2

So please stop complaining.

So, why did you remove it?

See FAQ

And since ag does not replace it,

It does in your case.

can we please have it back?

No.

:) Specifically, this code works great, and is extremely useful:

Perhaps.

Thierry

@thierryvolpiatto
Copy link
Member

Vitalie Spinu notifications@github.com writes:

Piping grep results for each pattern is probably the way to go.

This proposition looks reasonable, I will write something in this sens.

Thanks.

Thierry

thierryvolpiatto pushed a commit that referenced this issue Mar 21, 2016
* helm-grep.el (helm-grep--prepare-cmd-line):
Pipe multiples patterns to original cmd line.
thierryvolpiatto pushed a commit that referenced this issue Mar 21, 2016
* helm-grep.el (helm-grep-ag-prepare-cmd-line): New.
(helm-grep-ag-init): Use it to pipe ag result to ack.
thierryvolpiatto pushed a commit that referenced this issue Mar 21, 2016
* helm-grep.el (helm-grep-default-command): Docstring only.
(helm-grep--prepare-cmd-line): extract ack cmd.
(helm-grep-ag-prepare-cmd-line): Check if ack-grep is present
otherwise fallback to grep -P.
thierryvolpiatto pushed a commit that referenced this issue Mar 21, 2016
* helm-grep.el (helm-grep-ag-prepare-cmd-line): Quote pattern piped.
thierryvolpiatto pushed a commit that referenced this issue Mar 21, 2016
* helm-grep.el (helm-grep--prepare-cmd-line): all on one line.
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