GNU bug report logs - #34621
[PATCH] lisp/progmodes/grep.el (grep-read-files): Add file-directory-p check

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 34621 in the body.
You can then email your comments to 34621 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: Christopher Thorne <c.thorne <at> reckondigital.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] lisp/progmodes/grep.el (grep-read-files): Add
 file-directory-p check
Date: Fri, 22 Feb 2019 17:29:10 +0000

Hello,

This patch fixes a bug in the rgrep command which causes certain
directory names to be mistaken for files with extensions. For example,
when running rgrep in a directory called "django-1.11", rgrep will
prompt with 'Search for "x" in files (default *.11):', under the
assumption that .11 is a file extension. Similarly, creating a
directory called "test.c" and running rgrep inside it results in the
prompt 'Search for "x" in files (default *.[ch])'. With this patch,
the default file extension for directories is either taken from the
rgrep history or set to "all".

Changelog entry:
* lisp/progmodes/grep.el (grep-read-files): Add file-directory-p check

Patch:

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 3fd2a7e701..fe0fb5b30c 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -963,6 +963,7 @@ grep-read-files
                  (file-name-nondirectory bn)))
         (default-alias
           (and fn
+               (not (file-directory-p (concat "../" fn)))
                (let ((aliases (remove (assoc "all" grep-files-aliases)
                                       grep-files-aliases))
                      alias)
@@ -979,6 +980,7 @@ grep-read-files
                  (cdr alias))))
         (default-extension
           (and fn
+               (not (file-directory-p (concat "../" fn)))
                (let ((ext (file-name-extension fn)))
                  (and ext (concat "*." ext)))))
         (default

Regards,
Christopher Thorne

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

From: c.thorne <at> reckondigital.com <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Subject: Update
Date: Mon, 25 Feb 2019 10:13:36 +0100

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

The initial patch misses an edge case where a non-file buffer has the same name as a directory. I'll send a new patch with a more robust solution in the next couple of days.

[Message part 2 (text/html, inline)]

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

From: Christopher Thorne <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Subject: Patch Update
Date: Mon, 04 Mar 2019 11:13:07 +0000

I've changed the patch to check if the buffer is in dired mode. I think 
this makes more sense than looking at the file system and addresses the 
edge case described in the previous email.

Also, I think I could have been clearer in explaining how to reproduce:
1. Create directory called django-1.11 on the filesystem
2. Open this directory in dired buffer
3. Run rgrep with this dired buffer open
4. Search for any string
5. Observe unexpected default extension suggestion of (*.11)

Patch below:

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 3fd2a7e701..22e7ec11d8 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -963,6 +963,7 @@ grep-read-files
                  (file-name-nondirectory bn)))
         (default-alias
           (and fn
+               (not (eq major-mode 'dired-mode))
                (let ((aliases (remove (assoc "all" grep-files-aliases)
                                       grep-files-aliases))
                      alias)
@@ -979,6 +980,7 @@ grep-read-files
                  (cdr alias))))
         (default-extension
           (and fn
+               (not (eq major-mode 'dired-mode))
                (let ((ext (file-name-extension fn)))
                  (and ext (concat "*." ext)))))
         (default

Changelog entry:
* lisp/progmodes/grep.el (grep-read-files): Disable default extension in 
dired buffers

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Christopher Thorne <c.thorne <at> reckondigital.com>, 34621 <at> debbugs.gnu.org
Subject: RE: bug#34621: Patch Update
Date: Mon, 4 Mar 2019 07:26:27 -0800 (PST)

> +               (not (eq major-mode 'dired-mode))
> +               (not (eq major-mode 'dired-mode))

I haven't followed this thread at all - dunno
what the problem is that you're trying to solve.
But I'm a bit surprised that Dired needs to be
special-cased for grep in any way.

Anyway, the reasom I'm writing is to suggest
that you probably don't want
(eq major-mode 'dired-mode).  You probably
want (derived-mode-p 'dired-mode).

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

From: Christopher Thorne <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Cc: Drew Adams <drew.adams <at> oracle.com>
Subject: RE: bug#34621: Patch Update
Date: Tue, 05 Mar 2019 10:49:18 +0000

Thanks Drew, I agree derived-mode-p is better.

The reason for special casing dired is to stop rgrep taking extension 
suggestions from dired buffers, e.g. suggesting "*.11" as the rgrep file 
pattern in a dired buffer called "django-1.11".

Here is the same fix but using derived-mode-p:

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 3fd2a7e701..83154872aa 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -963,6 +963,7 @@ grep-read-files
                  (file-name-nondirectory bn)))
         (default-alias
           (and fn
+               (not (derived-mode-p 'dired-mode))
                (let ((aliases (remove (assoc "all" grep-files-aliases)
                                       grep-files-aliases))
                      alias)
@@ -979,6 +980,7 @@ grep-read-files
                  (cdr alias))))
         (default-extension
           (and fn
+               (not (derived-mode-p 'dired-mode))
                (let ((ext (file-name-extension fn)))
                  (and ext (concat "*." ext)))))
         (default

Changelog entry:
* lisp/progmodes/grep.el (grep-read-files): Disable default extension in
dired buffers


On 2019-03-04 15:26, Drew Adams wrote:
>> +               (not (eq major-mode 'dired-mode))
>> +               (not (eq major-mode 'dired-mode))
> 
> I haven't followed this thread at all - dunno
> what the problem is that you're trying to solve.
> But I'm a bit surprised that Dired needs to be
> special-cased for grep in any way.
> 
> Anyway, the reasom I'm writing is to suggest
> that you probably don't want
> (eq major-mode 'dired-mode).  You probably
> want (derived-mode-p 'dired-mode).

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Christopher Thorne <c.thorne <at> reckondigital.com>, 34621 <at> debbugs.gnu.org
Subject: RE: bug#34621: Patch Update
Date: Tue, 5 Mar 2019 09:48:45 -0800 (PST)

> The reason for special casing dired is to stop rgrep taking extension
> suggestions from dired buffers, e.g. suggesting "*.11" as the rgrep
> file pattern in a dired buffer called "django-1.11".

I see.  It sounds like `rgrep' should itself be changed
to be more flexible or smarter, or to let users more
easily control the default file-name pattern.

It doesn't sound to me like this has anything, per se,
to do with Dired.  It has to do with how the default is
determined, and that's apparently now being picked up
from the buffer name.

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

From: Christopher Thorne <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Cc: Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#34621: Patch Update
Date: Tue, 05 Mar 2019 18:22:15 +0000

> It doesn't sound to me like this has anything, per se,
> to do with Dired.  It has to do with how the default is
> determined, and that's apparently now being picked up
> from the buffer name.

Hmm, I think you're right that this isn't just isolated to dired. For 
example, I can start a shell-mode buffer, rename it to shell.txt and 
rgrep will now suggest "*.txt" as the default extension even though my 
buffer is unrelated to .txt files.

An alternative I considered is only showing extension suggestions for 
buffers that are associated with a file (i.e. buffer-file-name is a 
non-empty string). Can you think of any cases where this would fall 
down?

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Christopher Thorne <c.thorne <at> reckondigital.com>, 34621 <at> debbugs.gnu.org
Subject: RE: bug#34621: Patch Update
Date: Tue, 5 Mar 2019 10:44:10 -0800 (PST)

> > It doesn't sound to me like this has anything, per se,
> > to do with Dired.  It has to do with how the default is
> > determined, and that's apparently now being picked up
> > from the buffer name.
> 
> Hmm, I think you're right that this isn't just isolated to dired. For
> example, I can start a shell-mode buffer, rename it to shell.txt and
> rgrep will now suggest "*.txt" as the default extension even though my
> buffer is unrelated to .txt files.
> 
> An alternative I considered is only showing extension suggestions for
> buffers that are associated with a file (i.e. buffer-file-name is a
> non-empty string). Can you think of any cases where this would fall
> down?

I don't think it's a question of falling down.

It's not obvious what a reasonable or smart default
filename pattern is in most cases.  Just because
your current buffer is visiting a file does not at
all imply that you want to search files with the
same extension.

I think you need to (for yourself) specify just
what relation (if any) you want between the current
buffer and the default filename pattern.

If it's a question of improving `rgrep` then the
determination of the default needs to be such that
it's useful generally and typically, and perhaps
user configurable.

As an example, but for the search pattern only
(not the filename pattern(s)), in `grep+.el' there
is user option `grepp-default-regexp-fn` and a
default function of the same name, as follows.
(If the option value is not a function then the
default function is used.)

https://www.emacswiki.org/emacs/grep%2b.el

I mention this only as an example of how you can
provide some flexibility and user control.  For
filename pattern(s) obviously the ways of picking
a default would be different.

------------8<--------------

grepp-default-regexp-fn is a variable defined in `grep+.el'.
Its value is non-nil-symbol-name-nearest-point

Documentation:
Function of 0 args called to provide default search regexp to \\[grep].
Some reasonable choices are defined in `thingatpt+.el':
`word-nearest-point', `non-nil-symbol-name-nearest-point',
`region-or-non-nil-symbol-name-nearest-point', `sexp-nearest-point'.

This is ignored if Transient Mark mode is on and the region is active
and non-empty.  In that case, the quoted (") region text is used as
the default regexp.

If `grepp-default-regexp-fn' is nil and no prefix arg is given to
`grep', then no defaulting is done.

Otherwise, if the value is not a function, then function
`grepp-default-regexp-fn' does the defaulting.

You can customize this variable.

------------8<--------------

(defun grepp-default-regexp-fn ()
  "*Function of 0 args called to provide default search regexp to \\[grep].
This is used only if both of the following are true:
- Transient Mark mode is off or the region is inactive or empty.
- The value of option `grepp-default-regexp-fn' is
  `grepp-default-regexp-fn'.

When this is used, the default regexp is provided by calling the
first of these that references a defined function:
  - variable `grepp-default-regexp-fn'
  - variable `find-tag-default-function'
  - the `find-tag-default-function' property of the `major-mode'
  - function `non-nil-symbol-name-nearest-point', if bound
  - function `grep-tag-default'"
  (cond ((functionp grepp-default-regexp-fn) grepp-default-regexp-fn)
        (find-tag-default-function)
        ((get major-mode 'find-tag-default-function))
        ((fboundp 'non-nil-symbol-name-nearest-point) 'non-nil-symbol-name-nearest-point)
        (t 'find-tag-default)))

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

From: Christopher Thorne <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Cc: Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#34621: Patch Update
Date: Wed, 06 Mar 2019 11:10:03 +0000

Thanks for the input, Drew.

> It's not obvious what a reasonable or smart default
> filename pattern is in most cases.  Just because
> your current buffer is visiting a file does not at
> all imply that you want to search files with the
> same extension.
> 
> I think you need to (for yourself) specify just
> what relation (if any) you want between the current
> buffer and the default filename pattern.

Personally I think the current behaviour of searching files with the 
same extension works quite well in most cases. There is also some 
flexibility with aliases which means that if you're in a "*.c" file the 
default pattern becomes "*.[ch]".

However, I like the idea of providing more flexibility like 
`grepp-default-regexp-fn`. This would mean a buffer in c-mode could 
always suggest *.[ch] regardless of its buffer/file name. More 
importantly, dired could implement this so that no default patterns are 
suggested for dired buffers.

I will consider both (1) using only buffer-file-name instead of 
buffer-name (2) enabling rgrep to delegate extension suggestions to 
major modes. (1) would fix the case I'm personally encountering with 
dired, but (2) may be better in the long term and could cover more cases 
that I'm currently imagining.

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

From: Juri Linkov <juri <at> linkov.net>
To: Christopher Thorne <c.thorne <at> reckondigital.com>
Cc: 34621 <at> debbugs.gnu.org
Subject: Re: bug#34621: [PATCH] lisp/progmodes/grep.el (grep-read-files): Add
 file-directory-p check
Date: Sun, 17 Mar 2019 23:28:51 +0200

> More importantly, dired could implement this so that no default
> patterns are suggested for dired buffers.

For Dired, it would make more sense to use a default based on the
filename under point by using dired-get-filename, so for example
when in the Dired buffer in a directory called "django-1.11"
the cursor is on a file called "test.c" then the default could be
*.[ch]

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

From: Christopher Thorne <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Subject: [PATCH] Fix rgrep in dired taking default search file pattern from
 directory name (e.g. *.11 for django-1.11)
Date: Mon, 08 Apr 2019 11:41:20 +0100

I've updated the patch following input from Drew and Juri.
This now adds 'grep-default-file-pattern-function' which major modes can 
implement.
Dired implements this to take the extension of the file at point, with a 
default of 'all',
thus solving the issue I was having with the django-1.11 directory 
mentioned earlier in the thread.
Behaviour for major modes that don't implement 
'grep-default-file-pattern-function' will remain as before.

diff --git a/lisp/dired.el b/lisp/dired.el
index 3cb645eea7..219acbf148 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -4138,6 +4138,19 @@ dired-restore-desktop-buffer
 (add-to-list 'desktop-buffer-mode-handlers
             '(dired-mode . dired-restore-desktop-buffer))

+(defun dired-grep-default-file-pattern ()
+  "Use extension of file at point as the default file pattern for grep.
+If a directory or nothing is found at point, fallback to 'all'."
+  (let* ((dired-file-name (ignore-errors (dired-get-filename)))
+        (dired-file-extension (if (and dired-file-name
+                                       (not (file-directory-p 
dired-file-name)))
+                                  (file-name-extension 
dired-file-name))))
+    (if dired-file-extension
+       (concat "*." dired-file-extension)
+       "all")))
+(put 'dired-mode 'grep-default-file-pattern-function 
'dired-grep-default-file-pattern)
+
+
 (provide 'dired)

 (run-hooks 'dired-load-hook)           ; for your customizations
diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 3fd2a7e701..54d1412a66 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -961,6 +961,8 @@ grep-read-files
         (fn (and bn
                  (stringp bn)
                  (file-name-nondirectory bn)))
+        (default-file-pattern-function
+          (get major-mode 'grep-default-file-pattern-function))
         (default-alias
           (and fn
                (let ((aliases (remove (assoc "all" grep-files-aliases)
@@ -982,10 +984,12 @@ grep-read-files
                (let ((ext (file-name-extension fn)))
                  (and ext (concat "*." ext)))))
         (default
-          (or default-alias
-              default-extension
-              (car grep-files-history)
-              (car (car grep-files-aliases))))
+          (if default-file-pattern-function
+            (funcall default-file-pattern-function)
+            (or default-alias
+                default-extension
+                (car grep-files-history)
+                (car (car grep-files-aliases)))))
         (files (completing-read
                 (concat "Search for \"" regexp
                         "\" in files matching wildcard"

Changelog entry:
* lisp/progmodes/grep.el (grep-read-files): Allow major modes to
define default search file pattern
* lisp/dired.el (dired-grep-default-file-pattern): Define default search
file pattern for grep

Thanks for the input so far. Any further suggestions are welcome.

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

From: Juri Linkov <juri <at> linkov.net>
To: Christopher Thorne <c.thorne <at> reckondigital.com>
Cc: 34621 <at> debbugs.gnu.org
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Mon, 08 Apr 2019 22:44:20 +0300

> I've updated the patch following input from Drew and Juri.
> This now adds 'grep-default-file-pattern-function' which major modes
> can implement.
> Dired implements this to take the extension of the file at point, with
> a default of 'all',
> thus solving the issue I was having with the django-1.11 directory
> mentioned earlier in the thread.
> Behaviour for major modes that don't implement
> 'grep-default-file-pattern-function' will remain as before.

Thanks, this is better than previous versions.

Even more, you don't need any changes in dired.el.  Just use in grep.el

  (run-hook-with-args-until-success 'file-name-at-point-functions)

because 'file-name-at-point-functions' already has the right value in Dired.

You can call it at the very beginning of grep-read-files in the same 'or'
before buffer-file-name, so the existing code will take care about getting
its file extension.

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

From: Christopher Thorne <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Cc: juri <at> linkov.net
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Tue, 09 Apr 2019 12:09:27 +0100

> Even more, you don't need any changes in dired.el.  Just use in grep.el
> 
>   (run-hook-with-args-until-success 'file-name-at-point-functions)
> 
> because 'file-name-at-point-functions' already has the right value in 
> Dired.
> 
> You can call it at the very beginning of grep-read-files in the same 
> 'or'
> before buffer-file-name, so the existing code will take care about 
> getting
> its file extension.

Thanks, Juri. I tried this:

diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 3fd2a7e701..4fb4490104 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -956,11 +956,17 @@ grep-read-files
 The pattern can include shell wildcards.  As whitespace triggers
 completion when entering a pattern, including it requires
 quoting, e.g. `\\[quoted-insert]<space>'."
-  (let* ((bn (or (buffer-file-name)
+  (let* ((file-name-at-point (run-hook-with-args-until-success 
'file-name-at-point-functions))
+        (bn (or (if (and (stringp file-name-at-point)
+                         (not (file-directory-p file-name-at-point)))
+                    file-name-at-point)
+                (buffer-file-name)
                 (replace-regexp-in-string "<[0-9]+>\\'" "" 
(buffer-name))))
         (fn (and bn

However, it doesn't cover the case where emacs is in a dired buffer 
called django-1.11 with nothing at point. In this case, *.11 will be 
suggested because buffer-name is used. I'm not sure how to fix this in 
grep.el without an "if is dired" check.
That said, I do like the fact that this approach preserves the existing 
aliasing behaviour (e.g. *.c -> *.[ch]), so I've changed the approach of 
the previous patch to let major modes define the file-name grep should 
use rather than extension:

diff --git a/lisp/dired.el b/lisp/dired.el
index 3cb645eea7..d66f12b3a6 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -4138,6 +4138,16 @@ dired-restore-desktop-buffer
 (add-to-list 'desktop-buffer-mode-handlers
             '(dired-mode . dired-restore-desktop-buffer))

+(defun dired-grep-file-name-for-default-pattern ()
+  "Use file at point as the file for grep's default file-name pattern 
suggestion.
+If a directory or nothing is found at point, return nil."
+  (let ((dired-file-name (run-hook-with-args-until-success 
'file-name-at-point-functions)))
+    (if (and dired-file-name
+            (not (file-directory-p dired-file-name)))
+       dired-file-name)))
+(put 'dired-mode 'grep-file-name-for-default-pattern-function 
'dired-grep-file-name-for-default-pattern)
+
+
 (provide 'dired)

 (run-hooks 'dired-load-hook)           ; for your customizations
diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
index 3fd2a7e701..5fb56a33be 100644
--- a/lisp/progmodes/grep.el
+++ b/lisp/progmodes/grep.el
@@ -956,8 +956,12 @@ grep-read-files
 The pattern can include shell wildcards.  As whitespace triggers
 completion when entering a pattern, including it requires
 quoting, e.g. `\\[quoted-insert]<space>'."
-  (let* ((bn (or (buffer-file-name)
-                (replace-regexp-in-string "<[0-9]+>\\'" "" 
(buffer-name))))
+  (let* ((file-name-for-default-pattern-function
+          (get major-mode 
'grep-file-name-for-default-pattern-function))
+        (bn (if file-name-for-default-pattern-function
+                (funcall file-name-for-default-pattern-function)
+              (or (buffer-file-name)
+                (replace-regexp-in-string "<[0-9]+>\\'" "" 
(buffer-name)))))
         (fn (and bn
                  (stringp bn)
                  (file-name-nondirectory bn)))

Changelog entry:
* lisp/progmodes/grep.el (grep-read-files): Allow major modes to
define file name to use for default search pattern
* lisp/dired.el (dired-grep-file-name-for-default-pattern): Use file at 
point
for grep file name pattern

Let me know if there are any further improvements I can make.

Regards,
Chris

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

From: Noam Postavsky <npostavs <at> gmail.com>
To: Christopher Thorne <c.thorne <at> reckondigital.com>
Cc: 34621 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Tue, 09 Apr 2019 07:52:24 -0400

> +  (let* ((file-name-at-point (run-hook-with-args-until-success
> 'file-name-at-point-functions))

Just a minor suggestion meta suggestion: post your patches by attaching
the output from 'git format-patch', rather than inlining the output of
git diff.  That will avoid patch corruption due to line-wrapping, like
in the above quote (although that line is also a bit long, and you
should break it manually).

> Changelog entry:
> * lisp/progmodes/grep.el (grep-read-files): Allow major modes to
> define file name to use for default search pattern
> * lisp/dired.el (dired-grep-file-name-for-default-pattern): Use file
> at point
> for grep file name pattern

And the ChangeLog entry then goes in the git commit message.  It's much
easier for other people to apply your patch that way.

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

From: Christopher Thorne <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Cc: Noam Postavsky <npostavs <at> gmail.com>
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Tue, 09 Apr 2019 13:23:11 +0100

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

> Just a minor suggestion meta suggestion: post your patches by attaching
> the output from 'git format-patch', rather than inlining the output of
> git diff.  That will avoid patch corruption due to line-wrapping, like
> in the above quote (although that line is also a bit long, and you
> should break it manually).

Thanks, Noam. Trying this now. Patch should be attached.

[0001-Fix-rgrep-in-dired-using-directory-for-search-file-p.patch (text/x-diff, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christopher Thorne <c.thorne <at> reckondigital.com>
Cc: 34621 <at> debbugs.gnu.org, npostavs <at> gmail.com
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Tue, 09 Apr 2019 17:18:54 +0300

> Date: Tue, 09 Apr 2019 13:23:11 +0100
> From: Christopher Thorne <c.thorne <at> reckondigital.com>
> Cc: Noam Postavsky <npostavs <at> gmail.com>
> 
> > Just a minor suggestion meta suggestion: post your patches by attaching
> > the output from 'git format-patch', rather than inlining the output of
> > git diff.  That will avoid patch corruption due to line-wrapping, like
> > in the above quote (although that line is also a bit long, and you
> > should break it manually).
> 
> Thanks, Noam. Trying this now. Patch should be attached.

One more nit to a well-formatted patch:

> * lisp/progmodes/grep.el (grep-read-files): Allow major modes to
> define file name to use for default search pattern
> * lisp/dired.el (dired-grep-file-name-for-default-pattern): Use file
> at point for grep file name pattern

Please end each sentence with a period.

Also, it is better to have the bug number as part of the detailed
descriptions, not as part of the header line (in Subject), because
that leaves you more space for the header.

Thanks.

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

From: Christopher Thorne <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Tue, 09 Apr 2019 15:32:31 +0100

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

> One more nit to a well-formatted patch:

Thanks, Eli. I've updated the commit message as requested.

[0001-Fix-rgrep-in-dired-using-directory-for-search-file-p.patch (text/x-diff, attachment)]

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

From: Juri Linkov <juri <at> linkov.net>
To: Christopher Thorne <c.thorne <at> reckondigital.com>
Cc: 34621 <at> debbugs.gnu.org
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Tue, 09 Apr 2019 23:32:01 +0300

> However, it doesn't cover the case where emacs is in a dired buffer called
> django-1.11 with nothing at point. In this case, *.11 will be suggested
> because buffer-name is used. I'm not sure how to fix this in grep.el
> without an "if is dired" check.

When I tried the change that I suggested to use, I forgot that I have
this customization:

  (add-hook 'dired-after-readin-hook
            (lambda ()
              ;; Set name of dired buffers to absolute directory names.
              (when (stringp dired-directory)
                (rename-buffer (generate-new-buffer-name dired-directory)))))

that renames dired buffers to e.g. "django-1.11/", and due to the
trailing slash, grep-read-files doesn't use it's buffer name.
But since by default there is no trailing slash in dired buffer names,
this is unsuitable.  So I agree with your approach to allow modes
to override using buffer names as candidates.

> diff --git a/lisp/dired.el b/lisp/dired.el
> index fc0b71238b..33abca550a 100644
> --- a/lisp/dired.el
> +++ b/lisp/dired.el
> @@ -4162,6 +4162,17 @@ dired-restore-desktop-buffer
>  (add-to-list 'desktop-buffer-mode-handlers
>  	     '(dired-mode . dired-restore-desktop-buffer))
>  
> +(defun dired-grep-file-name-for-default-pattern ()
> +  "Use file at point as the file for grep's default file-name pattern suggestion.
> +If a directory or nothing is found at point, return nil."
> +  (let ((dired-file-name
> +	  (run-hook-with-args-until-success 'file-name-at-point-functions)))
> +    (if (and dired-file-name
> +	     (not (file-directory-p dired-file-name)))
> +	dired-file-name)))
> +(put 'dired-mode 'grep-file-name-for-default-pattern-function 'dired-grep-file-name-for-default-pattern)

This is an overwhelmingly long name, please use the same name as
the function that uses this feature, i.e. just 'grep-read-files'.
Then when someone reading dired.el will find a line

  (put 'dired-mode 'grep-read-files 'dired-grep-read-files)

it will be clear for them the purpose of this feature.

Please place this change in dired.el after the definition of
'dired-file-name-at-point' that you can use in your dired function
instead of (run-hook-with-args-until-success 'file-name-at-point-functions)

> diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el
> index c0f47159c9..b0bb8e6924 100644
> --- a/lisp/progmodes/grep.el
> +++ b/lisp/progmodes/grep.el
> @@ -959,8 +959,12 @@ grep-read-files
>  The pattern can include shell wildcards.  As whitespace triggers
>  completion when entering a pattern, including it requires
>  quoting, e.g. `\\[quoted-insert]<space>'."
> -  (let* ((bn (or (buffer-file-name)
> -		 (replace-regexp-in-string "<[0-9]+>\\'" "" (buffer-name))))
> +  (let* ((file-name-for-default-pattern-function
> +	   (get major-mode 'grep-file-name-for-default-pattern-function))
> +	 (bn (if file-name-for-default-pattern-function
> +		 (funcall file-name-for-default-pattern-function)
> +	       (or (buffer-file-name)
> +		 (replace-regexp-in-string "<[0-9]+>\\'" "" (buffer-name)))))

If you want you could also add as a 4th option additionally
(run-hook-with-args-until-success 'file-name-at-point-functions)
to automatically support modes other than dired, i.e. other modes
that set file-name-at-point-functions.

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

From: Christopher Thorne <c.thorne <at> reckondigital.com>
To: 34621 <at> debbugs.gnu.org
Cc: Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Wed, 10 Apr 2019 11:42:50 +0100

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

> This is an overwhelmingly long name, please use the same name as
> the function that uses this feature, i.e. just 'grep-read-files'.
> Then when someone reading dired.el will find a line
> 
>   (put 'dired-mode 'grep-read-files 'dired-grep-read-files)
> 
> it will be clear for them the purpose of this feature.

Good point, I've changed it to grep-read-files.

> Please place this change in dired.el after the definition of
> 'dired-file-name-at-point' that you can use in your dired function
> instead of (run-hook-with-args-until-success 
> 'file-name-at-point-functions)

Thanks, also done this.

> If you want you could also add as a 4th option additionally
> (run-hook-with-args-until-success 'file-name-at-point-functions)
> to automatically support modes other than dired, i.e. other modes
> that set file-name-at-point-functions.

Sounds like a sensible default, added this as well.

Patch attached.

[0001-Fix-rgrep-in-dired-using-directory-for-search-file-p.patch (text/x-diff, attachment)]

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

From: Juri Linkov <juri <at> linkov.net>
To: Christopher Thorne <c.thorne <at> reckondigital.com>
Cc: 34621 <at> debbugs.gnu.org
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Wed, 10 Apr 2019 23:37:13 +0300

> Patch attached.
>
> From bbf584e0c5f836c55579b9804e091c736bcf7a23 Mon Sep 17 00:00:00 2001
> From: Christopher Thorne <c.thorne <at> reckondigital.com>
> Date: Tue, 9 Apr 2019 13:12:39 +0100
> Subject: [PATCH] Fix rgrep in dired using directory for search file pattern
>
> * lisp/progmodes/grep.el (grep-read-files): Allow major modes to
> define file name to use for default search pattern.
> * lisp/progmodes/grep.el (grep-read-files): Add non-directory file at
> point as default search pattern candidate.
> * lisp/dired.el (dired-grep-read-files): Use non-directory file at
> point for grep file name pattern.  (Bug#34621)

Thanks, I tested your patch, and it works without problems.

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

From: Juri Linkov <juri <at> linkov.net>
To: Christopher Thorne <c.thorne <at> reckondigital.com>
Cc: 34621-done <at> debbugs.gnu.org
Subject: Re: bug#34621: [PATCH] Fix rgrep in dired taking default search file
 pattern from directory name (e.g. *.11 for django-1.11)
Date: Thu, 11 Apr 2019 23:51:46 +0300

> Patch attached.

Your patch is pushed to master, thanks.

Previous Next

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