bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

[Antero Mejr via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 165 bytes --]


eshell has builtins for agrep/egrep/fgrep that use the Emacs grep
feature, but rgrep is not included so it behaves differently. This patch
adds the rgrep builtin.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-eshell-Add-rgrep-builtin.patch --]
[-- Type: text/x-patch, Size: 2429 bytes --]

From 7a56cea33b5f6d0025003405b83da028e93576a8 Mon Sep 17 00:00:00 2001
From: Antero Mejr <antero@mailbox.org>
Date: Fri, 24 Mar 2023 20:41:41 +0000
Subject: [PATCH] eshell: Add 'rgrep' builtin.

* lisp/eshell/em-unix.el (eshell/rgrep): New procedure.
(eshell-unix-initialize): Add "rgrep" to eshell-complex-commands.
* etc/NEWS: Add NEWS entry for rgrep.
* doc/misc/eshell.texi (Built-ins): Add documentation for rgrep.
---
 doc/misc/eshell.texi   | 2 ++
 etc/NEWS               | 5 +++++
 lisp/eshell/em-unix.el | 8 ++++++--
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 1c33c04f647..4e2bddf42af 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -602,6 +602,8 @@ Built-ins
 @cmindex egrep
 @itemx fgrep
 @cmindex fgrep
+@itemx rgrep
+@cmindex rgrep
 @itemx glimpse
 @cmindex glimpse
 The @command{grep} commands are compatible with GNU @command{grep},
diff --git a/etc/NEWS b/etc/NEWS
index 2a87bf08406..240ecb37f62 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -174,6 +174,11 @@ correctly unloads Eshell and all of its modules.
 After manually editing 'eshell-aliases-file', you can use this command
 to load the edited aliases.
 
++++
+*** 'rgrep' is now an builtin command.
+Running "rgrep" is eshell now uses the Emacs grep facility instead of
+calling external rgrep.
+
 ** Prog Mode
 
 +++
diff --git a/lisp/eshell/em-unix.el b/lisp/eshell/em-unix.el
index d550910f4f0..0681ba11691 100644
--- a/lisp/eshell/em-unix.el
+++ b/lisp/eshell/em-unix.el
@@ -145,8 +145,8 @@ eshell-unix-initialize
     (add-hook 'pcomplete-try-first-hook
 	      'eshell-complete-host-reference nil t))
   (setq-local eshell-complex-commands
-	(append '("grep" "egrep" "fgrep" "agrep" "glimpse" "locate"
-		  "cat" "time" "cp" "mv" "make" "du" "diff")
+	(append '("grep" "egrep" "fgrep" "agrep" "rgrep" "glimpse"
+		  "locate" "cat" "time" "cp" "mv" "make" "du" "diff")
 		eshell-complex-commands)))
 
 (defalias 'eshell/date     'current-time-string)
@@ -773,6 +773,10 @@ eshell/agrep
   "Use Emacs grep facility instead of calling external agrep."
   (eshell-grep "agrep" args))
 
+(defun eshell/rgrep (&rest args)
+  "Use Emacs grep facility instead of calling external rgrep."
+  (eshell-grep "grep" (append '("-rH") args) t))
+
 (defun eshell/glimpse (&rest args)
   "Use Emacs grep facility instead of calling external glimpse."
   (let (null-device)
-- 
2.38.1

bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

[Jim Porter]

On 3/24/2023 2:02 PM, Antero Mejr via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> 
> eshell has builtins for agrep/egrep/fgrep that use the Emacs grep
> feature, but rgrep is not included so it behaves differently. This patch
> adds the rgrep builtin.

Thanks, I think it would make sense to add an rgrep built-in to Eshell. 
However, I'm not sure if this is the best way for it to work. 
Personally, I'd have expected this to use the 'rgrep' function in Emacs 
Lisp (which uses find + grep to do its job). That's a bit different from 
/usr/bin/rgrep, but I find it a lot more useful since it ignores 
"uninteresting" files by default. For example, 'M-x rgrep' in a Git repo 
ignores the .git/ subdir, but /usr/bin/rgrep includes that subdir. 
Hence, I'd almost always prefer Emacs' version of rgrep.

That said, it might be a bit strange for Eshell's rgrep to work this 
way, when Eshell's agrep/egrep/fgrep work like the external versions. 
This seems like a tricky case...

bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

[Sean Whitton]

Hello,

On Fri 24 Mar 2023 at 05:04PM -07, Jim Porter wrote:

> On 3/24/2023 2:02 PM, Antero Mejr via Bug reports for GNU Emacs, the Swiss
> army knife of text editors wrote:
>> eshell has builtins for agrep/egrep/fgrep that use the Emacs grep
>> feature, but rgrep is not included so it behaves differently. This patch
>> adds the rgrep builtin.
>
> Thanks, I think it would make sense to add an rgrep built-in to
> Eshell. However, I'm not sure if this is the best way for it to
> work. Personally, I'd have expected this to use the 'rgrep' function in Emacs
> Lisp (which uses find + grep to do its job). That's a bit different from
> /usr/bin/rgrep, but I find it a lot more useful since it ignores
> "uninteresting" files by default. For example, 'M-x rgrep' in a Git repo
> ignores the .git/ subdir, but /usr/bin/rgrep includes that subdir. Hence, I'd
> almost always prefer Emacs' version of rgrep.
>
> That said, it might be a bit strange for Eshell's rgrep to work this way, when
> Eshell's agrep/egrep/fgrep work like the external versions. This seems like a
> tricky case...

Tricky indeed.  Here is an attempt:

Normally with M-x rgrep one benefits from how Emacs prompts you
separately for the parameters to the function, right?  In particular,
you get completion for the second and third parameters.  Given that,
you're unlikely to want to type 'rgrep foo bar baz' into Eshell, and not
benefit from that completion, when you could just type M-x rgrep in the
same buffer, and benefit from it.

Given this, I suggest rgrep in Eshell should be /usr/bin/rgrep.

-- 
Sean Whitton

bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

[Jim Porter]

On 3/25/2023 10:22 AM, Sean Whitton wrote:
> Normally with M-x rgrep one benefits from how Emacs prompts you
> separately for the parameters to the function, right?  In particular,
> you get completion for the second and third parameters.  Given that,
> you're unlikely to want to type 'rgrep foo bar baz' into Eshell, and not
> benefit from that completion, when you could just type M-x rgrep in the
> same buffer, and benefit from it.

It wouldn't be too difficult to add Pcomplete support for an "rgrep" 
Eshell command that calls 'M-x rgrep' under the hood. There's no reason 
you'd *have* to explicitly type all three arguments to 'M-x rgrep' on 
the command line. (Though doing this the Right Way would take a bit of 
effort, since we'd probably want 'eshell-eval-using-options' to 
automatically generate the appropriate Pcomplete function.)

Even without Pcomplete support, there's still a benefit to a command 
like this though: you could use Eshell to pipe the results of 'M-x 
rgrep' to some other command. Looking at the code for 'eshell-grep', I 
don't think it'd be terribly difficult to support this case.

> Given this, I suggest rgrep in Eshell should be /usr/bin/rgrep.

Maybe there should be a defcustom for this ("use M-x rgrep" vs "use 
/usr/bin/rgrep")? Or maybe it should be easier to configure various 
Eshell commands so they open in a compilation buffer when appropriate? 
You can do this now with an alias, but the syntax is a bit tricky:

   alias rgrep 'eshell-grep grep ${append (list "-rH") $*}'

Something like this would be nicer:

   alias rgrep 'to-compilation-buffer rgrep -rH $*'

That would make it easier for users to define their own commands that 
work like this, which would (probably) be generally useful and provide a 
partial solution for this bug while we consider the available options. 
(My main goal with doing this now is so that we don't merge something 
and then change our minds later, disrupting users' habits.)

bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

[Sean Whitton]

Hello,

On Sat 25 Mar 2023 at 12:07PM -07, Jim Porter wrote:

> Even without Pcomplete support, there's still a benefit to a command
> like this though: you could use Eshell to pipe the results of 'M-x
> rgrep' to some other command. Looking at the code for 'eshell-grep', I
> don't think it'd be terribly difficult to support this case.

Can you give a concrete use case?  If you're piping won't you typically
want to reuse your knowledge of traditional grep(1)?  If I'm piping then
I'm probably thinking in non-Emacs terms.

> Maybe there should be a defcustom for this ("use M-x rgrep" vs "use
> /usr/bin/rgrep")? Or maybe it should be easier to configure various
> Eshell commands so they open in a compilation buffer when appropriate?
> You can do this now with an alias, but the syntax is a bit tricky:
>
>   alias rgrep 'eshell-grep grep ${append (list "-rH") $*}'
>
> Something like this would be nicer:
>
>   alias rgrep 'to-compilation-buffer rgrep -rH $*'
>
> That would make it easier for users to define their own commands that
> work like this, which would (probably) be generally useful and provide
> a partial solution for this bug while we consider the available
> options. (My main goal with doing this now is so that we don't merge
> something and then change our minds later, disrupting users' habits.)

A command-specific defcustom doesn't seem ideal because we could end up
with very many such things.  Something like your compilation buffer idea
sounds good.

-- 
Sean Whitton

bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

[Jim Porter]

On 3/30/2023 2:19 PM, Sean Whitton wrote:
>> Even without Pcomplete support, there's still a benefit to a command
>> like this though: you could use Eshell to pipe the results of 'M-x
>> rgrep' to some other command. Looking at the code for 'eshell-grep', I
>> don't think it'd be terribly difficult to support this case.
> 
> Can you give a concrete use case?  If you're piping won't you typically
> want to reuse your knowledge of traditional grep(1)?  If I'm piping then
> I'm probably thinking in non-Emacs terms.

I'm somewhat biased (or maybe I've just thought about this too much?) 
because I wrote urgrep[1], which is essentially an extension of the 
basic recursive-grep concept: it provides a layer of abstraction over 
*any* recursive-grep-like command (rgrep, ag, ack, etc) so that you have 
a single API and can use whatever the flavor of the week is (possibly 
even using different programs on remote hosts).

In that package, I wrote an Eshell command, also called "urgrep", that 
replaces the user-entered command with whatever the *real* command is; 
then you can use that in Eshell like it's a regular command and pipe it 
around and do all the usual shell things with it. The Eshell urgrep 
command is designed to feel like grep so that you can (mostly) use your 
existing knowledge to search for things.

For Eshell's "rgrep" support, I think you could do the same thing 
without too much work, even if it delegated to "M-x rgrep" instead of 
"/usr/bin/rgrep". Since I almost never want to search the files in my 
.git dir, "M-x rgrep" is more convenient for me.

That said, ...

>> Maybe there should be a defcustom for this ("use M-x rgrep" vs "use
>> /usr/bin/rgrep")? Or maybe it should be easier to configure various
>> Eshell commands so they open in a compilation buffer when appropriate?
>> You can do this now with an alias, but the syntax is a bit tricky:
[snip]
> A command-specific defcustom doesn't seem ideal because we could end up
> with very many such things.  Something like your compilation buffer idea
> sounds good.

My thinking here was that some users might simply prefer "M-x rgrep", 
and others might prefer /usr/bin/rgrep. Having an easy way to make 
Eshell conform to users' preferences would be nice. But maybe we could 
do this by defaulting to use "M-x rgrep", and making it very easy to 
write an alias to prefer /usr/bin/rgrep (e.g. with the 
compilation-buffer wrapper I suggested). We could even document this 
explicitly in the manual.

I'll try to get at least a prototype of this into a patch in a week or 
so. Then people can try it out and have something a little less nebulous 
to comment on.

[1] https://github.com/jimporter/urgrep

bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

[Jim Porter]

On 3/25/2023 10:22 AM, Sean Whitton wrote:
> On Fri 24 Mar 2023 at 05:04PM -07, Jim Porter wrote:
> 
>> That said, it might be a bit strange for Eshell's rgrep to work this way, when
>> Eshell's agrep/egrep/fgrep work like the external versions. This seems like a
>> tricky case...
> 
> Tricky indeed.  Here is an attempt:
> 
> Normally with M-x rgrep one benefits from how Emacs prompts you
> separately for the parameters to the function, right?  In particular,
> you get completion for the second and third parameters.  Given that,
> you're unlikely to want to type 'rgrep foo bar baz' into Eshell, and not
> benefit from that completion, when you could just type M-x rgrep in the
> same buffer, and benefit from it.
> 
> Given this, I suggest rgrep in Eshell should be /usr/bin/rgrep.

I've dug through the Eshell grep code over the last few days, and after 
some thinking, I agree that the initial patch is the right way to go. 
While I think it would be nice to get the benefits of M-x rgrep into 
Eshell, it's just too much of a deviation from the existing Eshell grep 
builtins. For example, M-x grep defaults to using "--color=auto", but 
eshell/grep doesn't: eshell/grep is (close to) what you'd get if you 
called /usr/bin/grep directly.

It's all a bit strange though since eshell/grep forces you to have the 
flags "-nH" (file and line number) so that the compilation buffer works 
right. But Eshell sets those flags even when you're *not* using a 
compilation buffer (e.g. when piping the output of grep to some other 
process). I'm not sure it's the behavior I'd want, but it is the way it 
is, and I think it's too late to change it now.

I just see one issue with the patch: the NEWS entry and docstring are a 
bit misleading. I know these are just copying from the existing Eshell 
functions, but this isn't really accurate: "Use Emacs grep facility 
instead of calling external rgrep." It *always* calls the external 
rgrep; it's just that sometimes the output goes into a compilation buffer.

Instead, how about something like this? "Call the external rgrep 
program, opening its output in a compilation buffer when possible." 
(Plus similar changes to the other existing functions.) I think that 
makes it clearer what's actually happening.

Anyway, sorry for the back and forth on this. This is a part of Eshell 
that I haven't looked at in much detail before, and I wanted to get a 
better sense of what our options were here.

bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

[Antero Mejr via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter <jporterbugs@gmail.com> writes:

> I just see one issue with the patch: the NEWS entry and docstring are a bit
> misleading. I know these are just copying from the existing Eshell functions,
> but this isn't really accurate: "Use Emacs grep facility instead of calling
> external rgrep." It *always* calls the external rgrep; it's just that sometimes
> the output goes into a compilation buffer.

I don't think that's always the case. In the "eshell-grep" function it
checks if external grep is available, and if not then it uses a slow
elisp-only implementation. Then it checks to see if the output is being
redirected, and so on.

IMO that entire process constitutes the "emacs grep facility" as
described in the docstring.

> Instead, how about something like this? "Call the external rgrep program,
> opening its output in a compilation buffer when possible." (Plus similar changes
> to the other existing functions.) I think that makes it clearer what's actually
> happening.

I think the "Emacs grep facility" description is technically accurate
and concise enough for the eshell/*grep stub functions.

> Anyway, sorry for the back and forth on this. This is a part of Eshell that I
> haven't looked at in much detail before, and I wanted to get a better sense of
> what our options were here.

Thanks for the review and discussion.

bug#62426: [PATCH] eshell: Add 'rgrep' builtin.

[Jim Porter]

On 4/10/2023 7:12 PM, Antero Mejr via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> I just see one issue with the patch: the NEWS entry and docstring are a bit
>> misleading. I know these are just copying from the existing Eshell functions,
>> but this isn't really accurate: "Use Emacs grep facility instead of calling
>> external rgrep." It *always* calls the external rgrep; it's just that sometimes
>> the output goes into a compilation buffer.
> 
> I don't think that's always the case. In the "eshell-grep" function it
> checks if external grep is available, and if not then it uses a slow
> elisp-only implementation. Then it checks to see if the output is being
> redirected, and so on.
> 
> IMO that entire process constitutes the "emacs grep facility" as
> described in the docstring.

Good point. There's probably a way to indicate that more clearly, but 
I'll think it over. It shouldn't hold up merging this. Therefore, I've 
merged your patch as ebac67129e8.

This change is pretty close to the limit for changes that you can make 
without assigning copyright to the FSF (15 lines). I'm not sure if 
you've filled out the paperwork (I don't think I have the permissions to 
check directly), but if not, do you want to fill it out so you can make 
more changes to Emacs in the future? I think Eli (CCed) can get you 
started if so.

bug#62426: (no subject)

[Antero Mejr via Bug reports for GNU Emacs, the Swiss army knife of text editors]