bug#54636: dired fails to update subdirs when files are created/deleted/renamed

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Andreas Schwab]

dired-to-delete, dired-do-rename, dired-do-copy or
dired-create-directory fail to update the dired buffer when performed in
a subdirectory.

This is a regression from Emacs 27.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Eli Zaretskii]

> Resent-From: Andreas Schwab <schwab@linux-m68k.org>
> Original-Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
> Resent-CC: bug-gnu-emacs@gnu.org
> Resent-Sender: help-debbugs@gnu.org
> From: Andreas Schwab <schwab@linux-m68k.org>
> Date: Wed, 30 Mar 2022 13:15:11 +0200
> 
> dired-to-delete, dired-do-rename, dired-do-copy or
> dired-create-directory fail to update the dired buffer when performed in
> a subdirectory.
> 
> This is a regression from Emacs 27.

Thanks.  A bisection will be appreciated.

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Andreas Schwab]

Broken by 7b50ed553f Ask if dir and subdir dired buffers be killed when
deleting dir.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Tassilo Horn]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Broken by 7b50ed553f Ask if dir and subdir dired buffers be killed
> when deleting dir.

Oh, sorry, I'll look into it ASAP.  I guess this is to be corrected in
emacs-28?

Bye,
Tassilo

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: 54636@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 30 Mar 2022 14:43:22 +0200
> 
> Andreas Schwab <schwab@linux-m68k.org> writes:
> 
> > Broken by 7b50ed553f Ask if dir and subdir dired buffers be killed
> > when deleting dir.

Thanks for the bisection, Andreas, it made finding the culprit very
easy.

> Oh, sorry, I'll look into it ASAP.  I guess this is to be corrected in
> emacs-28?

Yes.

I think I just fixed it there, please take a look.

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> Oh, sorry, I'll look into it ASAP.  I guess this is to be corrected
>> in emacs-28?
>
> Yes.
>
> I think I just fixed it there, please take a look.

That's what I was about to do, too, but that's not right, too.  Say you
have dired buffers for

  ~/
  ~/foo/
  ~/foo/bar/

and then delete ~/foo from inside the ~/ dired buffer.  You'll be asked
if the buffers for foo should be delete, too, and when you confirm, what
is deleted are the buffers ~/ and ~/foo but ~/foo/bar persists.  So the
buffers are deleted downwards (to the root) instead of upwards which is
wrong.

I guess I recognized that in 7b50ed553f and switched the arguments for
that very reason.  (file-in-directory-p has the same argument order as
dired-in-this-tree.)

Bye,
Tassilo

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
> Date: Wed, 30 Mar 2022 15:29:36 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Oh, sorry, I'll look into it ASAP.  I guess this is to be corrected
> >> in emacs-28?
> >
> > Yes.
> >
> > I think I just fixed it there, please take a look.
> 
> That's what I was about to do, too, but that's not right, too.

Well, it fixes the regression.

> Say you have dired buffers for
> 
>   ~/
>   ~/foo/
>   ~/foo/bar/
> 
> and then delete ~/foo from inside the ~/ dired buffer.  You'll be asked
> if the buffers for foo should be delete, too, and when you confirm, what
> is deleted are the buffers ~/ and ~/foo but ~/foo/bar persists.  So the
> buffers are deleted downwards (to the root) instead of upwards which is
> wrong.

This means your fix for the "ask" part is incomplete, and should be
improved.  But that is a new feature in Emacs 28, so it is not a
catastrophe if it is imperfect.  Regressions in previously correct
behavior are much worse.

Of course, if you can come up with a fix for the question-asking part
that makes it delete all the relevant buffers, and if that fix is safe
enough (a high bar at this late stage of the pretest), we can install
that on the release branch.  Failing that, the fix for that will have
to wait till Emacs 28.2 at the very least.

> I guess I recognized that in 7b50ed553f and switched the arguments for
> that very reason.  (file-in-directory-p has the same argument order as
> dired-in-this-tree.)

But reversing the arguments breaks dired-in-this-tree completely,
because it doesn't treat the arguments symmetrically.  In the scenario
described by Andreas, dired-in-this-tree consistently returned nil for
a subdirectory inserted into a Dired buffer of its parent with 'i'.
Specifically, DIR will _never_ match DIR/FILE inside
dired-in-this-tree, only the other way around.

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Tassilo Horn]

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

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> > I think I just fixed it there, please take a look.
>> 
>> That's what I was about to do, too, but that's not right, too.
>
> Well, it fixes the regression.

That's true.

>> Say you have dired buffers for
>> 
>>   ~/
>>   ~/foo/
>>   ~/foo/bar/
>> 
>> and then delete ~/foo from inside the ~/ dired buffer.  You'll be
>> asked if the buffers for foo should be delete, too, and when you
>> confirm, what is deleted are the buffers ~/ and ~/foo but ~/foo/bar
>> persists.  So the buffers are deleted downwards (to the root) instead
>> of upwards which is wrong.
>
> This means your fix for the "ask" part is incomplete, and should be
> improved.  But that is a new feature in Emacs 28, so it is not a
> catastrophe if it is imperfect.

It's still a major annoyance since the feature is not opt-in.

> Regressions in previously correct behavior are much worse.

Of course.

> Of course, if you can come up with a fix for the question-asking part
> that makes it delete all the relevant buffers, and if that fix is safe
> enough (a high bar at this late stage of the pretest), we can install
> that on the release branch.  Failing that, the fix for that will have
> to wait till Emacs 28.2 at the very least.

Here's a patch.  I've created a separate cond-arm for the SUBDIRS case,
so any callers which are not dired-clean-up-after-deletion (the only one
setting SUBDIRS) are not affected.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dired.patch --]
[-- Type: text/x-patch, Size: 1723 bytes --]

diff --git a/lisp/dired.el b/lisp/dired.el
index 75dcd33e67..972a0865f4 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2874,8 +2874,9 @@ dired-buffers-for-dir
   "Return a list of buffers for DIR (top level or in-situ subdir).
 If FILE is non-nil, include only those whose wildcard pattern (if any)
 matches FILE.
-If SUBDIRS is non-nil, also include the dired buffers of
-directories below DIR.
+If SUBDIRS is non-nil, include the dired buffers of DIR and the
+directories below DIR instead (but no dired buffers with in-situ
+subdir DIR).
 The list is in reverse order of buffer creation, most recent last.
 As a side effect, killed dired buffers for DIR are removed from
 `dired-buffers'."
@@ -2887,10 +2888,9 @@ dired-buffers-for-dir
        ((null (buffer-name buf))
 	;; Buffer is killed - clean up:
 	(setq dired-buffers (delq elt dired-buffers)))
-       ((dired-in-this-tree-p dir (car elt))
+       ((and (null subdirs) (dired-in-this-tree-p dir (car elt)))
 	(with-current-buffer buf
-          (when (and (or subdirs
-                         (assoc dir dired-subdir-alist))
+          (when (and (assoc dir dired-subdir-alist)
 	             (or (null file)
 		         (if (stringp dired-directory)
 		             (let ((wildcards (file-name-nondirectory
@@ -2900,7 +2900,9 @@ dired-buffers-for-dir
                                                    file)))
 		           (member (expand-file-name file dir)
 			           (cdr dired-directory)))))
-            (setq result (cons buf result)))))))
+            (setq result (cons buf result)))))
+       ((and subdirs (dired-in-this-tree-p (car elt) dir))
+        (setq result (cons buf result)))))
     result))
 
 (defun dired-glob-regexp (pattern)

[-- Attachment #3: Type: text/plain, Size: 47 bytes --]


Can we agree it is safe enough?

Bye,
Tassilo

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
> Date: Wed, 30 Mar 2022 16:30:22 +0200
> 
> Here's a patch.  I've created a separate cond-arm for the SUBDIRS case,
> so any callers which are not dired-clean-up-after-deletion (the only one
> setting SUBDIRS) are not affected.

Thanks.

So when will SUBDIRS be non-nil?  dired-clean-up-after-deletion is
called not only when a directory is deleted, I think.

Andreas, could you perhaps also try this patch and see if you have any
problems in your use cases?

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tassilo Horn <tsdh@gnu.org>
>> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
>> Date: Wed, 30 Mar 2022 16:30:22 +0200
>> 
>> Here's a patch.  I've created a separate cond-arm for the SUBDIRS case,
>> so any callers which are not dired-clean-up-after-deletion (the only one
>> setting SUBDIRS) are not affected.
>
> Thanks.
>
> So when will SUBDIRS be non-nil?  dired-clean-up-after-deletion is
> called not only when a directory is deleted, I think.

Yes, it's also called when deleting a file but in that case
dired-buffers-for-dir will return nil, so no buffer will be deleted.
Probably, I shouldn't even bother to call it if a file has been deleted.

I think my patch is valid but now I think I should just create a
separate function dired-buffers-for-cleanup and use that from
dired-clean-up-after-deletion (or just collect the buffers in that
function) and remove the SUBDIRS argument from dired-buffers-for-dir as
it changes its logic too drastically.

I'll prepare a patch in that direction later.

Bye,
Tassilo

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Tassilo Horn]

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

Tassilo Horn <tsdh@gnu.org> writes:

> I think my patch is valid but now I think I should just create a
> separate function dired-buffers-for-cleanup and use that from
> dired-clean-up-after-deletion (or just collect the buffers in that
> function) and remove the SUBDIRS argument from dired-buffers-for-dir
> as it changes its logic too drastically.
>
> I'll prepare a patch in that direction later.

Here it is:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dired2.patch --]
[-- Type: text/x-patch, Size: 3210 bytes --]

diff --git a/lisp/dired.el b/lisp/dired.el
index 75dcd33e67..89b81ca0db 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2870,12 +2870,10 @@ dired-copy-filename-as-kill
 \f
 ;;; Keeping Dired buffers in sync with the filesystem and with each other
 
-(defun dired-buffers-for-dir (dir &optional file subdirs)
+(defun dired-buffers-for-dir (dir &optional file)
   "Return a list of buffers for DIR (top level or in-situ subdir).
 If FILE is non-nil, include only those whose wildcard pattern (if any)
 matches FILE.
-If SUBDIRS is non-nil, also include the dired buffers of
-directories below DIR.
 The list is in reverse order of buffer creation, most recent last.
 As a side effect, killed dired buffers for DIR are removed from
 `dired-buffers'."
@@ -2889,8 +2887,7 @@ dired-buffers-for-dir
 	(setq dired-buffers (delq elt dired-buffers)))
        ((dired-in-this-tree-p dir (car elt))
 	(with-current-buffer buf
-          (when (and (or subdirs
-                         (assoc dir dired-subdir-alist))
+          (when (and (assoc dir dired-subdir-alist)
 	             (or (null file)
 		         (if (stringp dired-directory)
 		             (let ((wildcards (file-name-nondirectory
@@ -2903,6 +2900,22 @@ dired-buffers-for-dir
             (setq result (cons buf result)))))))
     result))
 
+(defun dired-buffers-for-dir-or-subdir (dir)
+  "Return a list of buffers for DIR or a subdirectory thereof.
+As a side effect, killed dired buffers for DIR are removed from
+`dired-buffers'."
+  (setq dir (file-name-as-directory dir))
+  (let (result buf)
+    (dolist (elt dired-buffers)
+      (setq buf (cdr elt))
+      (cond
+       ((null (buffer-name buf))
+	;; Buffer is killed - clean up:
+	(setq dired-buffers (delq elt dired-buffers)))
+       ((dired-in-this-tree-p (car elt) dir)
+        (setq result (cons buf result)))))
+    result))
+
 (defun dired-glob-regexp (pattern)
   "Convert glob-pattern PATTERN to a regular expression."
   (let ((matched-in-pattern 0)  ;; How many chars of PATTERN we've handled.
@@ -3479,15 +3492,16 @@ dired-clean-up-after-deletion
                                      (file-name-nondirectory fn))))
                (not dired-clean-confirm-killing-deleted-buffers))
            (kill-buffer buf)))
-    (let ((buf-list (dired-buffers-for-dir (expand-file-name fn)
-                                           nil 'subdirs)))
+    (let ((buf-list (dired-buffers-for-dir-or-subdir
+                     (expand-file-name fn))))
       (and buf-list
            (or (and dired-clean-confirm-killing-deleted-buffers
                     (y-or-n-p
                      (format
-                      (ngettext "Kill Dired buffer of %s, too? "
-                                "Kill Dired buffers of %s, too? "
-                                (length buf-list))
+                      (ngettext
+                       "Kill Dired buffer of %s, too? "
+                       "Kill Dired buffers of %s and its sub-directories, too? "
+                       (length buf-list))
                       (file-name-nondirectory
                        ;; FN may end in a / if `dired-listing-switches'
                        ;; contains -p, so we need to strip that

[-- Attachment #3: Type: text/plain, Size: 132 bytes --]


I think that's a better approach than changing dired-buffers-for-dir
which I shouldn't have done in the first place.

Bye,
Tassilo

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
> Date: Wed, 30 Mar 2022 18:02:15 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So when will SUBDIRS be non-nil?  dired-clean-up-after-deletion is
> > called not only when a directory is deleted, I think.
> 
> Yes, it's also called when deleting a file but in that case
> dired-buffers-for-dir will return nil, so no buffer will be deleted.

What about moving/renaming a directory?

> I think my patch is valid but now I think I should just create a
> separate function dired-buffers-for-cleanup and use that from
> dired-clean-up-after-deletion (or just collect the buffers in that
> function) and remove the SUBDIRS argument from dired-buffers-for-dir as
> it changes its logic too drastically.
> 
> I'll prepare a patch in that direction later.

At this point, I'd prefer the safest, simplest patch for the release
branch, because I intend to release Emacs 28.1 VSN.  We can make a
cleaner fix on master later.

Do you think this patch you posted is safe enough for Emacs 28.1?

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tassilo Horn <tsdh@gnu.org>
>> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
>> Date: Wed, 30 Mar 2022 18:02:15 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > So when will SUBDIRS be non-nil?  dired-clean-up-after-deletion is
>> > called not only when a directory is deleted, I think.
>> 
>> Yes, it's also called when deleting a file but in that case
>> dired-buffers-for-dir will return nil, so no buffer will be deleted.
>
> What about moving/renaming a directory?

No, it's only called from dired-delete-entry which is only called from
dired-internal-do-deletions which is only called from dired-do-delete
and dired-do-flagged-delete.  So only real deletions.

>> I think my patch is valid but now I think I should just create a
>> separate function dired-buffers-for-cleanup and use that from
>> dired-clean-up-after-deletion (or just collect the buffers in that
>> function) and remove the SUBDIRS argument from dired-buffers-for-dir as
>> it changes its logic too drastically.
>> 
>> I'll prepare a patch in that direction later.
>
> At this point, I'd prefer the safest, simplest patch for the release
> branch, because I intend to release Emacs 28.1 VSN.  We can make a
> cleaner fix on master later.

The other patch with the separate function only called by
dired-clean-up-after-deletion is equivalent and safer than the
modification of dired-buffers-for-dir which is called in many places.
In the worst case, we have a cleanup problem after directory deletion.

> Do you think this patch you posted is safe enough for Emacs 28.1?

Yes, but I'd very much prefer to use the newer patch from my mail some
minutes ago for the reasons explained above.

Bye,
Tassilo

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
> Date: Wed, 30 Mar 2022 18:29:29 +0200
> 
> > I'll prepare a patch in that direction later.
> 
> Here it is:
> [...]
> I think that's a better approach than changing dired-buffers-for-dir
> which I shouldn't have done in the first place.

If so, can we restore dired-buffers-for-dir to what it was in Emacs
27?  AFAICT, the only difference now is that you use 'when' instead of
just 'and' -- is there a reason for that?

Thanks.

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Tassilo Horn]

30.03.2022 18:42:48 Eli Zaretskii <eliz@gnu.org>:

>> From: Tassilo Horn <tsdh@gnu.org>
>> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
>> Date: Wed, 30 Mar 2022 18:29:29 +0200
>>
>>> I'll prepare a patch in that direction later.
>>
>> Here it is:
>> [...]
>> I think that's a better approach than changing dired-buffers-for-dir
>> which I shouldn't have done in the first place.
>
> If so, can we restore dired-buffers-for-dir to what it was in Emacs
> 27?  AFAICT, the only difference now is that you use 'when' instead of
> just 'and' -- is there a reason for that?

No reason except stylistic preference.
I can change that if you want.

Bye,
Tassilo

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Eli Zaretskii]

> Date: Wed, 30 Mar 2022 18:47:49 +0200 (GMT+02:00)
> From: Tassilo Horn <tsdh@gnu.org>
> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
> 
> 
> 30.03.2022 18:42:48 Eli Zaretskii <eliz@gnu.org>:
> 
> >> From: Tassilo Horn <tsdh@gnu.org>
> >> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
> >> Date: Wed, 30 Mar 2022 18:29:29 +0200
> >>
> >>> I'll prepare a patch in that direction later.
> >>
> >> Here it is:
> >> [...]
> >> I think that's a better approach than changing dired-buffers-for-dir
> >> which I shouldn't have done in the first place.
> >
> > If so, can we restore dired-buffers-for-dir to what it was in Emacs
> > 27?  AFAICT, the only difference now is that you use 'when' instead of
> > just 'and' -- is there a reason for that?
> 
> No reason except stylistic preference.
> I can change that if you want.

Yes, please.  And then please install on the emacs-28 branch.

Thanks.

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

Hi all,

sorry, this message has sit in my drafts folder since March, 30th but
apparently wasn't sent.  The fix was committed to emacs-28 on that day.

>> > If so, can we restore dired-buffers-for-dir to what it was in Emacs
>> > 27?  AFAICT, the only difference now is that you use 'when' instead
>> > of just 'and' -- is there a reason for that?
>> 
>> No reason except stylistic preference.
>> I can change that if you want.
>
> Yes, please.  And then please install on the emacs-28 branch.

Done, thank you.

Andreas, could you please test if the bug it fixed for you (not that I
did't try myself) and close the report if it is?

Bye,
Tassilo

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: schwab@linux-m68k.org, 54636@debbugs.gnu.org
> Date: Wed, 30 Mar 2022 19:51:48 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> Hi all,
> 
> sorry, this message has sit in my drafts folder since March, 30th but
> apparently wasn't sent.  The fix was committed to emacs-28 on that day.

Yes, thanks.  (I wouldn't have released Emacs 28.1 if I wasn't aware
of your commit that day.)

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Jakub Ječmínek]

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

Hello,
I’m going through bug reports to see where I can help and it seems that is
particular issue has been resolved but never closed? Is there something
else waiting to be done?

Best, Kuba

[-- Attachment #2: Type: text/html, Size: 261 bytes --]

bug#54636: dired fails to update subdirs when files are created/deleted/renamed

[Eli Zaretskii]

> From: Jakub Ječmínek <jecminek.k@gmail.com>
> Date: Sat, 8 Jul 2023 20:31:18 +0200
> 
> I’m going through bug reports to see where I can help and it seems that is particular issue has been
> resolved but never closed? Is there something else waiting to be done?

Nothing more to do; closing.

Thanks for the heads-up.