GNU bug report logs - #62426
[PATCH] eshell: Add 'rgrep' builtin.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 62426 in the body.
You can then email your comments to 62426 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] eshell: Add 'rgrep' builtin.
Date: Fri, 24 Mar 2023 21:02:18 +0000

[Message part 1 (text/plain, inline)]

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.

[0001-eshell-Add-rgrep-builtin.patch (text/x-patch, attachment)]

Message #8 received at 62426 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Antero Mejr <antero <at> mailbox.org>, 62426 <at> debbugs.gnu.org
Subject: Re: bug#62426: [PATCH] eshell: Add 'rgrep' builtin.
Date: Fri, 24 Mar 2023 17:04:56 -0700

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...

Message #11 received at 62426 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Antero Mejr <antero <at> mailbox.org>, 62426 <at> debbugs.gnu.org
Subject: Re: bug#62426: [PATCH] eshell: Add 'rgrep' builtin.
Date: Sat, 25 Mar 2023 10:22:07 -0700

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

Message #14 received at 62426 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: Antero Mejr <antero <at> mailbox.org>, 62426 <at> debbugs.gnu.org
Subject: Re: bug#62426: [PATCH] eshell: Add 'rgrep' builtin.
Date: Sat, 25 Mar 2023 12:07:37 -0700

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.)

Message #17 received at 62426 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Antero Mejr <antero <at> mailbox.org>, 62426 <at> debbugs.gnu.org
Subject: Re: bug#62426: [PATCH] eshell: Add 'rgrep' builtin.
Date: Thu, 30 Mar 2023 14:19:08 -0700

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

Message #20 received at 62426 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: Antero Mejr <antero <at> mailbox.org>, 62426 <at> debbugs.gnu.org
Subject: Re: bug#62426: [PATCH] eshell: Add 'rgrep' builtin.
Date: Thu, 30 Mar 2023 17:14:49 -0700

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

Message #23 received at 62426 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: Antero Mejr <antero <at> mailbox.org>, 62426 <at> debbugs.gnu.org
Subject: Re: bug#62426: [PATCH] eshell: Add 'rgrep' builtin.
Date: Sat, 8 Apr 2023 18:55:06 -0700

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.

Message #26 received at 62426 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 62426 <at> debbugs.gnu.org, Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#62426: [PATCH] eshell: Add 'rgrep' builtin.
Date: Tue, 11 Apr 2023 02:12:52 +0000

Jim Porter <jporterbugs <at> 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.

Message #29 received at 62426 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Antero Mejr <antero <at> mailbox.org>
Cc: eliz <at> gnu.org, 62426 <at> debbugs.gnu.org,
 Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#62426: [PATCH] eshell: Add 'rgrep' builtin.
Date: Mon, 10 Apr 2023 21:01:49 -0700

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 <at> 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.

Message #34 received at 62426-done <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: 62426-done <at> debbugs.gnu.org
Date: Sat, 03 Jun 2023 01:41:00 +0000

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.