bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Tino Calancha]

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


When a buffer visiting the file on disk exists, the matching is performed 
on this buffer: if the buffer is out of sync with the file, the
result may be wrong.

emacs -Q /tmp/foo --eval='(progn (with-current-buffer "foo" (insert "baz")) (save-buffer))'
C-x d RET
M-! printf baz >> bar; for f in foo bar; do printf qux >> $f ; done RET
g
% g \`bazqux$ RET
;; Just match bar.  It may signal an error if trying to read a non regular file.


*) Files satisfying predicate `file-regular-p'.
*) A buffer visiting the file on disk need to be updated before matching the input regexp.

In GNU Emacs 25.0.91.1 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.23)
Repository revision: b1a3ebedba88689d26f44cf7db338104b849ae99

[-- Attachment #2: Type: text/plain, Size: 832 bytes --]

diff --git a/lisp/dired.el b/lisp/dired.el
index 24b128f..f42040d 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -3355,8 +3355,8 @@ dired-mark-files-containing-regexp
      (and (not (looking-at-p dired-re-dot))
 	  (not (eolp))			; empty line
 	  (let ((fn (dired-get-filename nil t)))
-	    (when (and fn (file-readable-p fn)
-		       (not (file-directory-p fn)))
+	    (when (and fn (file-regular-p fn)
+		       (file-readable-p fn))
 	      (let ((prebuf (get-file-buffer fn)))
 		(message "Checking %s" fn)
 		;; For now we do it inside emacs
@@ -3364,6 +3364,7 @@ dired-mark-files-containing-regexp
 		(if prebuf
 		    (with-current-buffer prebuf
 		      (save-excursion
+            (revert-buffer t t t)
 			(goto-char (point-min))
 			(re-search-forward regexp nil t)))
 		  (with-temp-buffer

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Eli Zaretskii]

> Date: Tue, 16 Feb 2016 21:47:28 +0900 (JST)
> From: Tino Calancha <f92capac@gmail.com>
> 
> When a buffer visiting the file on disk exists, the matching is performed 
> on this buffer: if the buffer is out of sync with the file, the
> result may be wrong.
> 
> emacs -Q /tmp/foo --eval='(progn (with-current-buffer "foo" (insert "baz")) (save-buffer))'
> C-x d RET
> M-! printf baz >> bar; for f in foo bar; do printf qux >> $f ; done RET
> g
> % g \`bazqux$ RET
> ;; Just match bar.  It may signal an error if trying to read a non regular file.
> 
> 
> *) Files satisfying predicate `file-regular-p'.
> *) A buffer visiting the file on disk need to be updated before matching the input regexp.

Isn't this by design?  Dired doesn't by default re-sync with the disk;
if you don't like this, you can turn on auto-revert in the Dired
buffer.

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Tino Calancha]

> Isn't this by design?  Dired doesn't by default re-sync with the disk;
> if you don't like this, you can turn on auto-revert in the Dired
> buffer.
I don't like reverting the buffer either.
I found better to ignore the buffers visiting and handle all files with
with-temp-buffer. Using grep is also nice, but it would change the type of 
regexp.

Regardless of whether auto-revert is enabled or not, the doc. string 
of this function says that it marks files whose contain match a regexp,
so, it should check the file instead of relying in the visiting 
buffer.

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Tino Calancha]

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


I think is better to not use the buffer and always read the file.
This way we always mark all the files containing the regexp,
regardless of the user customization.

[-- Attachment #2: Type: text/plain, Size: 1226 bytes --]

diff --git a/lisp/dired.el b/lisp/dired.el
index 6c7445c..199e6ff 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -3359,23 +3359,16 @@ dired-mark-files-containing-regexp
      (and (not (looking-at-p dired-re-dot))
 	  (not (eolp))			; empty line
 	  (let ((fn (dired-get-filename nil t)))
-	    (when (and fn (file-readable-p fn)
-		       (not (file-directory-p fn)))
-	      (let ((prebuf (get-file-buffer fn)))
+	    (when (and fn (file-regular-p fn)
+                   (file-readable-p fn))
 		(message "Checking %s" fn)
 		;; For now we do it inside emacs
 		;; Grep might be better if there are a lot of files
-		(if prebuf
-		    (with-current-buffer prebuf
-		      (save-excursion
-			(goto-char (point-min))
-			(re-search-forward regexp nil t)))
-		  (with-temp-buffer
-		    (insert-file-contents fn)
-		    (goto-char (point-min))
-		    (re-search-forward regexp nil t))))
-		      )))
-     "matching file")))
+		(with-temp-buffer
+		  (insert-file-contents fn)
+		  (goto-char (point-min))
+		  (re-search-forward regexp nil t)))))
+	"matching file")))
 
 (defun dired-flag-files-regexp (regexp)
   "In Dired, flag all files containing the specified REGEXP for deletion.

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Tino Calancha]

Let me argue more about why i found more useful if we change the current 
behaviour:

Let's suppose the following case:

1) An user submit several hundred of jobs to a batch server.

2) The output consist of just log files (1 per job) which are written
    under the submission directory until the jobs succeded of fail.
    When the job fails, the logfile contains the word 'aborting'.

3) To resubmit the failed jobs, the user may put all logfiles together in a
    dired buffer using `find-name-dired'.  From time to time, he/she call
    `dired-mark-files-containing-regexp' using 'aborting' as regexp: from this,
    he/she obtain directly the list of failed jobs...

4) ...But if the user has opened some of the logfiles from failed jobs, the word
    'aborting' may not be in the buffer (the user need to revert it first), so the
    list of failed jobs at 3) will not be exhaustive.  This behaviour is not
    consistent with the doc. string of the function:

"Mark all files with contents containing REGEXP for use in later commands.
A prefix argument means to unmark them instead.
`.' and `..' are never marked."

  - As mentioned in 4) this function may not mark all files containing
    REGEXP and this is not obvious unless you read the source code.

  - Just from the doc. string i would expect this function behaves as in
    the patch within this thread.

  - Using the buffer when available seems like an optimization but
    in fact it's not because it may produce different results.

  I suggest in order of preference:

A) Apply the patch in this thread: then, the funcion behaves as described in the doc. string.

B) Modify the doc. string to account for the case when there are buffers visiting the files.

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Eli Zaretskii]

> Date: Sun, 10 Apr 2016 16:02:59 +0900 (JST)
> From: Tino Calancha <f92capac@gmail.com>
> 
> 1) An user submit several hundred of jobs to a batch server.
> 
> 2) The output consist of just log files (1 per job) which are written
>    under the submission directory until the jobs succeded of fail.
>    When the job fails, the logfile contains the word 'aborting'.
> 
> 3) To resubmit the failed jobs, the user may put all logfiles together in a
>    dired buffer using `find-name-dired'.  From time to time, he/she call
>    `dired-mark-files-containing-regexp' using 'aborting' as regexp: from this,
>    he/she obtain directly the list of failed jobs...
> 
> 4) ...But if the user has opened some of the logfiles from failed jobs, the word
>    'aborting' may not be in the buffer (the user need to revert it first), so the
>    list of failed jobs at 3) will not be exhaustive.  This behaviour is not
>    consistent with the doc. string of the function:

Making the doc string more explicit about this aspect is indeed a good
idea.  I've just did it on the emacs-25 branch.

As for your scenario: when you work with logfiles, or any other kind
of files that get updated regularly behind Emacs's back, you should
turn on auto-revert-mode or auto-revert-tail-mode in the buffers of
those files.  Then the buffer's contents will be synchronized with the
relevant files on disk, and the problem you describe would not exist.

Bottom line, I don't think I agree with permanently changing the
implementation along the lines you suggest, as that would be against
the general principles (AFAIK them) of Dired's design, and actually
also against the general principles of Emacs design vis-a-vis files
and buffers that visit them: we don't automatically sync a buffer with
the file it visits, and we don't automatically look on disk when the
file's buffer differs from what's on disk.

I guess we could have an option to switch to the behavior you would
like to see, but such an option, if we introduce it, IMO should not be
specific to this command, it should affect all the Dired commands
which might produce different results when buffers are not
auto-reverted.  Another alternative is to have an optional feature
that would ask whether to revert a buffer if Emacs finds that it was
changed on disk since last visited.

Thanks.

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Tino Calancha]

Thank you very much for your response.

> Making the doc string more explicit about this aspect is indeed a good
> idea.  I've just did it on the emacs-25 branch.
Thanks.  The doc. string is very clear now.
Certainly the function name is very suggestive about what the function
should do, so may be people who will not read the doc and still suffer
the issue... (i know, they should read the doc.).

> As for your scenario: when you work with logfiles, or any other kind
> of files that get updated regularly behind Emacs's back, you should
> turn on auto-revert-mode or auto-revert-tail-mode in the buffers of
> those files.  Then the buffer's contents will be synchronized with the
> relevant files on disk, and the problem you describe would not exist.
Thank you for the advise.  Definitely i need to use one of these options.

> Bottom line, I don't think I agree with permanently changing the
> implementation along the lines you suggest, as that would be against
> the general principles (AFAIK them) of Dired's design, and actually
> also against the general principles of Emacs design vis-a-vis files
> and buffers that visit them: we don't automatically sync a buffer with
> the file it visits, and we don't automatically look on disk when the
> file's buffer differs from what's on disk.
I have no comment on this: i am not expert enough.

> I guess we could have an option to switch to the behavior you would
> like to see, but such an option, if we introduce it, IMO should not be
> specific to this command, it should affect all the Dired commands
> which might produce different results when buffers are not
> auto-reverted.
I like this way.  I may work on implement such option if you want.

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Eli Zaretskii]

> Date: Thu, 21 Apr 2016 00:31:14 +0900 (JST)
> From: Tino Calancha <f92capac@gmail.com>
> cc: Tino Calancha <f92capac@gmail.com>, 22694@debbugs.gnu.org
> 
> Thank you very much for your response.

Thanks for raising the issue and for perseverance.

> > I guess we could have an option to switch to the behavior you would
> > like to see, but such an option, if we introduce it, IMO should not be
> > specific to this command, it should affect all the Dired commands
> > which might produce different results when buffers are not
> > auto-reverted.
> I like this way.  I may work on implement such option if you want.

I suggest to wait for a couple of days in case someone objects to such
an option, and if no comments are posted, please go ahead and work on
it.

Thanks.

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Tino Calancha]

On Wed, 20 Apr 2016, Eli Zaretskii wrote:

>I guess we could have an option to switch to the behavior you would
>like to see, but such an option, if we introduce it, IMO should not be
>specific to this command, it should affect all the Dired commands
>which might produce different results when buffers are not
>auto-reverted.

I have only found another Dired command which might require the new 
option:
`dired-do-query-replace-regexp'.
But it seems unnecessary because this command already alert the user
if the file has being modified 'externally':

./emacs -Q -eval '(progn (with-temp-file "/tmp/foo" (insert "foobar")) (find-file "/tmp/foo") (dired "/tmp"))'
% g \`foobar$ RET
M-! echo foo > foo
M-x dired-do-query-replace-regexp RET \`foobar$ RET bar RET
;; File foo changed on disk.  Reread from disk? (yes or no)

;;;
So i propose a patch which:
1) Adds a new option `dired-always-read-filesystem' (default value nil).
2) Use it just in `dired-mark-files-containing-regexp'.

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

From e6449456c023e86511cd09dbcacefba471c5f1d5 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Mon, 27 Jun 2016 00:10:57 +0900
Subject: [PATCH] Dired always read file system

* dired.el (dired-always-read-filesystem): Add new option.
(dired-mark-files-containing-regexp): Use it (Bug#22694).
* doc/emacs/dired.texi: Mention it in the manual.
; * etc/NEWS: Add entry for this change.
---
  doc/emacs/dired.texi |  7 +++++--
  etc/NEWS             |  3 +++
  lisp/dired.el        | 14 ++++++++++++--
  3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/doc/emacs/dired.texi b/doc/emacs/dired.texi
index 486e92a..763c1f5 100644
--- a/doc/emacs/dired.texi
+++ b/doc/emacs/dired.texi
@@ -550,13 +550,16 @@ Marks vs Flags
  the regular expression @var{regexp}
  (@code{dired-mark-files-containing-regexp}).  This command is like
  @kbd{% m}, except that it searches the file contents instead of the file
-name.  Note that if a file is visited in an Emacs buffer, this command
+name.  Note that if a file is visited in an Emacs buffer,
+and @code{dired-always-read-filesystem} evaluates @code{nil}, this 
command
  will look in the buffer without revisiting the file, so the results
  might be inconsistent with the file on disk if its contents has changed
  since it was last visited.  If you don't want this, you may wish
  reverting the files you have visited in your buffers, or turning on
  the @code{auto-revert} mode in those buffers, before invoking this
-command.  @xref{Reverting}.
+command.  @xref{Reverting}.  If you prefer that this command always 
revisit
+the file, without having to revert the file or enable @code{auto-revert}
+mode, you might want to set @code{dired-always-read-filesystem} to 
non-@code{nil}.

  @item C-/
  @itemx C-x u
diff --git a/etc/NEWS b/etc/NEWS
index b3a044d..6683199 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -198,6 +198,9 @@ questions, with a handy way to display help texts.
  ** Dired

  +++
+*** New option 'dired-always-read-filesystem'.
+
++++
  *** In wdired, when editing files to contain slash characters,
  the resulting directories are automatically created.  Whether to do
  this is controlled by the 'wdired-create-parent-directories' variable.
diff --git a/lisp/dired.el b/lisp/dired.el
index 38979b5..7eb6216 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -255,6 +255,15 @@ dired-hide-details-hide-information-lines
    :version "24.4"
    :group 'dired)

+(defcustom dired-always-read-filesystem nil
+  "Non-nil means commands like `dired-mark-files-containing-regexp' which
+may use a buffer visiting the file or read the file on disk, always
+read the file system.  Otherwise, if do exist a buffer visiting the file,
+then use that buffer."
+  :type 'boolean
+  :version "25.2"
+  :group 'dired)
+
  ;; Internal variables

  (defvar dired-marker-char ?*		; the answer is 42
@@ -3359,7 +3368,8 @@ dired-mark-files-containing-regexp
  A prefix argument means to unmark them instead.
  `.' and `..' are never marked.

-Note that if a file is visited in an Emacs buffer, this command will
+Note that if a file is visited in an Emacs buffer, and
+`dired-always-read-filesystem' evaluates nil, this command will
  look in the buffer without revisiting the file, so the results might
  be inconsistent with the file on disk if its contents has changed
  since it was last visited."
@@ -3379,7 +3389,7 @@ dired-mark-files-containing-regexp
  		(message "Checking %s" fn)
  		;; For now we do it inside emacs
  		;; Grep might be better if there are a lot of files
-		(if prebuf
+		(if (and prebuf (not dired-always-read-filesystem))
  		    (with-current-buffer prebuf
  		      (save-excursion
  			(goto-char (point-min))
-- 
2.8.1


In GNU Emacs 25.1.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
Repository revision: 8419f0d166cf5107062ff74f120c591f3fce35d9

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Eli Zaretskii]

> From: Tino Calancha <f92capac@gmail.com>
> Date: Mon, 27 Jun 2016 00:32:18 +0900 (JST)
> cc: Tino Calancha <f92capac@gmail.com>, 22694@debbugs.gnu.org
> 
> >I guess we could have an option to switch to the behavior you would
> >like to see, but such an option, if we introduce it, IMO should not be
> >specific to this command, it should affect all the Dired commands
> >which might produce different results when buffers are not
> >auto-reverted.
> 
> I have only found another Dired command which might require the new 
> option:
> `dired-do-query-replace-regexp'.
> But it seems unnecessary because this command already alert the user
> if the file has being modified 'externally':
> 
> ./emacs -Q -eval '(progn (with-temp-file "/tmp/foo" (insert "foobar")) (find-file "/tmp/foo") (dired "/tmp"))'
> % g \`foobar$ RET
> M-! echo foo > foo
> M-x dired-do-query-replace-regexp RET \`foobar$ RET bar RET
> ;; File foo changed on disk.  Reread from disk? (yes or no)
> 
> ;;;
> So i propose a patch which:
> 1) Adds a new option `dired-always-read-filesystem' (default value nil).
> 2) Use it just in `dired-mark-files-containing-regexp'.

Thanks, please push to master, after taking care of the following
issues:

> --- a/doc/emacs/dired.texi
> +++ b/doc/emacs/dired.texi
> @@ -550,13 +550,16 @@ Marks vs Flags
>   the regular expression @var{regexp}
>   (@code{dired-mark-files-containing-regexp}).  This command is like
>   @kbd{% m}, except that it searches the file contents instead of the file
> -name.  Note that if a file is visited in an Emacs buffer, this command
> +name.  Note that if a file is visited in an Emacs buffer,
> +and @code{dired-always-read-filesystem} evaluates @code{nil}, this 

Our usual style for what you want to say in the last sentence is like
this:

  Note that if a file is visited in an Emacs buffer, and
  @code{dired-always-read-filesystem} is @code{nil} (the default), ...

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -198,6 +198,9 @@ questions, with a handy way to display help texts.
>   ** Dired
> 
>   +++
> +*** New option 'dired-always-read-filesystem'.

Please tell in a sentence or 2 what does this option do.

> +(defcustom dired-always-read-filesystem nil
> +  "Non-nil means commands like `dired-mark-files-containing-regexp' which
> +may use a buffer visiting the file or read the file on disk, always
> +read the file system.  Otherwise, if do exist a buffer visiting the file,
> +then use that buffer."

The first line of the doc string must be a complete sentence.  So I
suggest the following alternative wording (which also clarifies the
text in other places):

   "Non-nil means revert buffers visiting files before searching them.
 By default,  commands like `dired-mark-files-containing-regexp' will
 search any buffers visiting the marked files without reverting them,
 even if they were changed on disk.  When this option is non-nil, such
 buffers are always reverted before searching them."

> +Note that if a file is visited in an Emacs buffer, and
> +`dired-always-read-filesystem' evaluates nil, this command will
                                  ^^^^^^^^^^^^^
"is nil"

Thanks.

How about a test for this functionality?

bug#22694: 25.0.91; dired-mark-files-containing-regexp read file disk

[Tino Calancha]

On Sat, 9 Jul 2016, Eli Zaretskii wrote:

> Thanks, please push to master, after taking care of the following
> issues:
Thank you very much for your help.
Addressed all the issues and pushed to master.

> How about a test for this functionality?
Good idea.  Added one test.

bug#22694: (no subject)

[Tino Calancha]

Pushed to master branch