dired.el

dired.el

[Luc Teirlinck]

While fixing bugs in Dired, locate and find-dired, I noticed the
following two problems, which were both present before my changes.
The small patch below corrects both.  I will install if it seems OK. 

Make a directory. say ~/subdir, say using + in Dired and then
make files ~/testfile and ~/subdir/testfile.  (It is very common to
have files with the same name in different directories.)

Visit ~/ using Dired.  Then insert ~/subdir using `i'.  "Accidentally"
kill it using C-u k on the subdirectory header line.  Reinsert it using
`C-_'.  First (minor) problem: occasionally the mode line marks the
Dired buffer as writable for some period before correcting it.  The
Dired buffer is not really writable during that period, the modeline
was just updated at the wrong time.  Now the real problem.  Delete
the "testfile" entry in the inserted subdir.  Now ~/testfile and not
~/subdir/testfile is deleted.  All kind of other serious damage can
occur in the given situation, the above is just an example.

It can be argued that this is not a bug.  It is mentioned in the
docstring of `dired-undo':

    Undo in a dired buffer.
    This doesn't recover lost files, it just undoes changes in the buffer
    itself.
    You can use it to recover marks, killed lines or subdirs.
    In the latter case, you have to do M-x dired-build-subdir-alist to
    parse the buffer again.

But not everybody reads the docstring of every single command before
using it.  `dired-undo' is documented in the Emacs manual and _there_
`dired-build-subdir-alist' does not seem to be mentioned at all.
Given the danger involved in failing to call
`dired-build-subdir-alist' in the given situation, why not simply make
`dired-undo' call `dired-build-subdir-alist'?  The patch below does this.

As an added bonus, it also corrects the modeline problem.  (To be
honest, I do not fully understand why, but it does.  The patch changes
the scope of the buffer-read-only binding.  Does message update modelines?)

===File ~/dired-diff========================================
*** dired.el	05 Jun 2004 19:54:59 -0500	1.287
--- dired.el	06 Jun 2004 12:20:04 -0500	
***************
*** 1453,1461 ****
  parse the buffer again."
    (interactive)
    (let (buffer-read-only)
!     (undo)
!     (message "Change in Dired buffer undone.
! Actual changes in files cannot be undone by Emacs.")))
  
  (defun dired-next-line (arg)
    "Move down lines then position at filename.
--- 1453,1462 ----
  parse the buffer again."
    (interactive)
    (let (buffer-read-only)
!     (undo))
!   (dired-build-subdir-alist)
!   (message "Change in Dired buffer undone.
! Actual changes in files cannot be undone by Emacs."))
  
  (defun dired-next-line (arg)
    "Move down lines then position at filename.
============================================================

Re: dired.el

[Luc Teirlinck]

>From my previous message:

   As an added bonus, it also corrects the modeline problem.  (To be
   honest, I do not fully understand why, but it does.  The patch changes
   the scope of the buffer-read-only binding.  Does message update modelines?)

At second thought, it seems obvious:

!   (message "Change in Dired buffer undone.
! Actual changes in files cannot be undone by Emacs."))

That message has two lines.  The echo area needs to grow and hence the
mode line _needs_ to get redisplayed.

Sincerely,

Luc.

Re: dired.el

[David Kastrup]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> >From my previous message:
> 
>    As an added bonus, it also corrects the modeline problem.  (To be
>    honest, I do not fully understand why, but it does.  The patch changes
>    the scope of the buffer-read-only binding.  Does message update modelines?)
> 
> At second thought, it seems obvious:
> 
> !   (message "Change in Dired buffer undone.
> ! Actual changes in files cannot be undone by Emacs."))
> 
> That message has two lines.  The echo area needs to grow and hence the
> mode line _needs_ to get redisplayed.

redraw-modeline is an alias for `force-mode-line-update' in `subr'.
(redraw-modeline &optional all)

Force redisplay of the current buffer's mode line and header line.
With optional non-nil all, force redisplay of all mode lines and
header lines.  This function also forces recomputation of the
menu bar menus and the frame title.


-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: dired.el

[Stefan Monnier]

> !   (message "Change in Dired buffer undone.
> ! Actual changes in files cannot be undone by Emacs."))

> That message has two lines.  The echo area needs to grow and hence the
> mode line _needs_ to get redisplayed.

let-binding inhibit-read-only would probably avoid this kind of problem and
would also be more correct, most likely.
Moving the `message' call outside the let scope is also a good change, in
either case.


        Stefan

Re: dired.el

[Luc Teirlinck]

Stefan Monnier wrote:

   let-binding inhibit-read-only would probably avoid this kind of problem and
   would also be more correct, most likely.

You mean let-binding inhibit-read-only to t around the call to message?
That would not seem to be OK, because, one never knows, the user might
have, deliberately or accidentally, made the dired buffer writable.

   Moving the `message' call outside the let scope is also a good change, in
   either case.

It would seem to solve the problem, unless some other function called
dired-undo with inhibit-read-only bound to nil, in which case that
other function would need to be fixed.  But dired-undo is not likely
to be called from other Lisp functions anyway.

Sincerely,

Luc.

Re: dired.el

[Stefan Monnier]

>    let-binding inhibit-read-only would probably avoid this kind of problem
>    and would also be more correct, most likely.

> You mean let-binding inhibit-read-only to t around the call to message?
> That would not seem to be OK, because, one never knows, the user might
> have, deliberately or accidentally, made the dired buffer writable.

We seem to miscommunicate: most let-bindings of buffer-read-only (99.9% of
which are to nil) are due to people not knowing that they should bind
inhibit-read-only to t instead.
So I suggested to replace one let-binding with the other.
It would most likely fix the minibuffer thing because it would leave the
buffer-read-only variable alone, thus not triggering a minibuffer-refresh.
I have no idea what kind of problem you´re referring to w.r.t writable
dired buffers.


        Stefan

Re: dired.el

[Luc Teirlinck]

Stefan Monnier wrote:

   We seem to miscommunicate: most let-bindings of buffer-read-only (99.9% of
   which are to nil) are due to people not knowing that they should bind
   inhibit-read-only to t instead.

Sorry, I somehow failed to notice that you were talking about a
different variable.  I was still thinking about buffer-read-only, even
though I might have written inhibit-read-only.

dired.el and dired-aux.el bind buffer-read-only to nil countless
times, but there is only one single inhibit-read-only binding in the
two files combined.  I have the impression that this is not a result
of "not knowing", but rather a result of the fact that the code is
old, probably from before inhibit-read-only existed.

Prolonged bindings of {buffer,inhibit}-read-only both have their own
dangers.  For buffer-read-only it is inappropriate mode line updates,
for inhibit-read-only it is the fact that it applies to all buffers,
including the minibuffer (unless one goes through the trouble of
making a buffer-local binding).  So one can not just blindly replace
either one by the other.

In the case of undo, I actually believe that _both_ are safe.

Sincerely,

Luc.

Re: dired.el

[Richard Stallman]

    Given the danger involved in failing to call
    `dired-build-subdir-alist' in the given situation, why not simply make
    `dired-undo' call `dired-build-subdir-alist'?  The patch below does this.

It sounds good to me.  Thanks for thinking of it.

dired.el

[Luc Teirlinck]

My previous patch to dired-aux.el turned out to still require several
changes, for instance to be able to handle the `-R' switch to `ls'.

My patches now require some changes to dired.el as well.  If they look
OK, I will install all four patches.

The variable `dired-subdir-switches' allows to get rid of two ugly
alternative functions and bindings for `dired-maybe-insert-subdir' in
locate.el and find-dired.el.  It allows to specify different default
switches for the main directory and for inserted subdirectories.  It
could potentially be used for user customization as well.  But I am
hesitating whether that is true to a sufficient degree to use
defcustom instead of defvar.  The patch below uses defvar, but, of
course, it would be trivial to change that.

`dired-switches-alist' already occurred in my previous dired-aux.el
patch.  I just had to move it to dired.el.

The change to `dired-insert-old-subdirs' corrects a bug when reverting
buffers containing a subdirectory inserted using the "-R" switch.
The previous version first correctly inserted all subdirectories using
`ls -R' and then went through a lot of work getting things (sometimes)
wrong, by systematically erasing every single subdirectory and
re-inserting it.  Subdirectories several levels down could get
inserted, deleted and re-inserted countless times.  My patch corrects
this.

In as far as the final change is concerned, it would make
`dired-build-subdir-alist' only print the "Buffer includes %d directories"
message when called interactively.  This is yet another auto-revert
noise control issue.  I believe the user is only interested in the
info if the function was called interactively.  If it was called for
internal reasons by some other command, then I believe that the message
is likely to confuse the user, regardless of the auto-revert issue.
It certainly is confusing when `dired-build-subdir-alist' is indirectly
called by auto-revert.

===File ~/dired-diff========================================
*** dired.el	30 May 2004 15:58:40 -0500	1.286
--- dired.el	02 Jun 2004 16:01:52 -0500	
***************
*** 60,65 ****
--- 60,69 ----
    :type 'string
    :group 'dired)
  
+ (defvar dired-subdir-switches nil
+   "If non-nil, switches passed to `ls' for inserting subdirectories.
+ If nil, `dired-listing-switches' is used.")
+ 
  ; Don't use absolute file names as /bin should be in any PATH and people
  ; may prefer /usr/local/gnu/bin or whatever.  However, chown is
  ; usually not in PATH.
***************
*** 274,286 ****
  (defvar dired-re-perms "[-bcdlps][-r][-w].[-r][-w].[-r][-w].")
  (defvar dired-re-dot "^.* \\.\\.?/?$")
  
! ;; The subdirectory names in this list are expanded.
  (defvar dired-subdir-alist nil
    "Association list of subdirectories and their buffer positions.
  Each subdirectory has an element: (DIRNAME . STARTMARKER).
  The order of elements is the reverse of the order in the buffer.
  In simple cases, this list contains one element.")
  
  (defvar dired-subdir-regexp "^. \\([^\n\r]+\\)\\(:\\)[\n\r]"
    "Regexp matching a maybe hidden subdirectory line in `ls -lR' output.
  Subexpression 1 is the subdirectory proper, no trailing colon.
--- 278,294 ----
  (defvar dired-re-perms "[-bcdlps][-r][-w].[-r][-w].[-r][-w].")
  (defvar dired-re-dot "^.* \\.\\.?/?$")
  
! ;; The subdirectory names in the next two lists are expanded.
  (defvar dired-subdir-alist nil
    "Association list of subdirectories and their buffer positions.
  Each subdirectory has an element: (DIRNAME . STARTMARKER).
  The order of elements is the reverse of the order in the buffer.
  In simple cases, this list contains one element.")
  
+ (defvar dired-switches-alist nil
+   "Keeps track of which switches to use for inserted subdirectories.
+ This is an alist of the form (SUBDIR . SWITCHES).")
+ 
  (defvar dired-subdir-regexp "^. \\([^\n\r]+\\)\\(:\\)[\n\r]"
    "Regexp matching a maybe hidden subdirectory line in `ls -lR' output.
  Subexpression 1 is the subdirectory proper, no trailing colon.
***************
*** 961,967 ****
  	  (condition-case ()
  	      (progn
  		(dired-uncache dir)
! 		(dired-insert-subdir dir))
  	    (error nil))))))
  
  (defun dired-uncache (dir)
--- 969,982 ----
  	  (condition-case ()
  	      (progn
  		(dired-uncache dir)
! 		(dired-insert-subdir dir)
! 		(let ((switches (cdr (assoc-string dir dired-switches-alist))))
! 		  (and switches
! 		       (string-match "R" switches)
! 		       (dolist (cur-ass (copy-sequence old-subdir-alist))
! 			 (when (string-match (concat "\\`" dir) (car cur-ass))
! 			   (setq old-subdir-alist
! 				 (delete cur-ass old-subdir-alist)))))))
  	    (error nil))))))
  
  (defun dired-uncache (dir)
***************
*** 1406,1411 ****
--- 1421,1427 ----
         '(dired-font-lock-keywords t nil nil beginning-of-line))
    (set (make-local-variable 'desktop-save-buffer)
         'dired-desktop-buffer-misc-data)
+   (set (make-local-variable 'dired-switches-alist) nil)
    (dired-sort-other dired-actual-switches t)
    (run-mode-hooks 'dired-mode-hook)
    (when (featurep 'x-dnd)
***************
*** 2073,2079 ****
  			     (goto-char (match-beginning 0))
  			     (beginning-of-line)
  			     (point-marker))))
!       (if (> count 1)
  	  (message "Buffer includes %d directories" count))
        ;; We don't need to sort it because it is in buffer order per
        ;; constructionem.  Return new alist:
--- 2089,2095 ----
  			     (goto-char (match-beginning 0))
  			     (beginning-of-line)
  			     (point-marker))))
!       (if (and (> count 1) (interactive-p))
  	  (message "Buffer includes %d directories" count))
        ;; We don't need to sort it because it is in buffer order per
        ;; constructionem.  Return new alist:
============================================================

Re: dired.el

[Luc Teirlinck]

>From my previous message:

   The change to `dired-insert-old-subdirs' corrects a bug when reverting
   buffers containing a subdirectory inserted using the "-R" switch.

Meant was a bug that remained after my previous patch.  After using,
say `C-u i RET "-ARl"' on directory DIR, all subdirectories of DIR
would originally be inserted using "-Al".  Before my first patch,
reverting would override this for _all_ directories with the default
switches.  After my first patch, reverting would keep the "Al" for
DIR, but override it with the default for the subdirectories of DIR.
This is hard to correct without my proposed change to
`dired-insert-old-subdirs'.  Moreover, the change seems necessitated
by efficiency considerations anyway, regardless of any bugs.

Sincerely,

Luc.