bug#67856: Dired navigation via directory line does not respect dired-kill-when-opening-new-dired-buffer

bug#67856: Dired navigation via directory line does not respect dired-kill-when-opening-new-dired-buffer

[Jared Finder via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

The option dired-kill-when-opening-new-dired-buffer is not respected
when clicking on parent directories in the directory line at the top of 
dired
buffers.  This can be fixed by calling
dired--find-possibly-alternate-file instead of dired in the callback, as
my attached patch does.

I believe changing to dired--find-possibly-alternate-file is safe 
because from
playing around with dired, the directory being clicked on must be a 
directory
and can not contain wildcards at this point.  Therefore, at this point 
it is
known that the directory is just a plain directory (no wildcards) and so 
going
through find-file machinery will have the intended result.

Steps to reproduce:

emacs -Q
M-x load-library RET dired RET
M-x set-variable RET dired-kill-when-opening-new-dired-buffer RET t RET
C-x d ~/ RET
Click something in the directory line like "home" in "/home/jared" for 
myself.
C-x C-b
See that there are two directory buffers instead of just one.

In GNU Emacs 29.1 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.37,
  cairo version 1.16.0) of 2023-09-11, modified by Debian built on melete
System Description: Debian GNU/Linux 12 (bookworm)

Configured using:
  'configure --build x86_64-linux-gnu --prefix=/usr
  --sharedstatedir=/var/lib --libexecdir=/usr/libexec
  --localstatedir=/var/lib --infodir=/usr/share/info
  --mandir=/usr/share/man --with-libsystemd --with-pop=yes
  
--enable-locallisppath=/etc/emacs:/usr/local/share/emacs/29.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/29.1/site-lisp:/usr/share/emacs/site-lisp
  --with-sound=alsa --without-gconf --with-mailutils
  --with-native-compilation --build x86_64-linux-gnu --prefix=/usr
  --sharedstatedir=/var/lib --libexecdir=/usr/libexec
  --localstatedir=/var/lib --infodir=/usr/share/info
  --mandir=/usr/share/man --with-libsystemd --with-pop=yes
  
--enable-locallisppath=/etc/emacs:/usr/local/share/emacs/29.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/29.1/site-lisp:/usr/share/emacs/site-lisp
  --with-sound=alsa --without-gconf --with-mailutils
  --with-native-compilation --with-pgtk 'CFLAGS=-g -O2
  -ffile-prefix-map=/build/emacs-N816CI/emacs-29.1+1=. 
-fstack-protector-strong
  -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
  -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP
NOTIFY INOTIFY PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM GTK3 ZLIB

Important settings:
   value of $LANG: en_US.UTF-8
   locale-coding-system: utf-8-unix

Major mode: Dired by name

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-mode: t
   show-paren-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   buffer-read-only: t
   line-number-mode: t
   indent-tabs-mode: t
   transient-mark-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny rfc822
mml mml-sec password-cache epa derived epg rfc6068 epg-config gnus-util
text-property-search time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils mule-util display-line-numbers
dired-aux cus-edit pp cus-start cus-load wid-edit help-fns radix-tree
dired dired-loaddefs thingatpt cl-loaddefs comp comp-cstr warnings icons
subr-x rx cl-seq cl-macs gv cl-extra help-mode bytecomp byte-compile
cl-lib find-func rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/pgtk-win pgtk-win term/common-win pgtk-dnd tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo gtk pgtk
lcms2 multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 105919 13638)
  (symbols 48 9060 0)
  (strings 32 24805 2351)
  (string-bytes 1 725024)
  (vectors 16 17316)
  (vector-slots 8 354985 19128)
  (floats 8 37 33)
  (intervals 56 711 0)
  (buffers 984 14))

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Clicks-on-directory-line-kill-prev-buffer-if-option-.patch --]
[-- Type: text/x-diff; name=0001-Clicks-on-directory-line-kill-prev-buffer-if-option-.patch, Size: 1225 bytes --]

From 45d2601a8219ef392bb0191693b5785160d223fc Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Sat, 16 Dec 2023 12:10:03 -0800
Subject: [PATCH] Clicks on directory line kill prev buffer if option is set

The option dired-kill-when-opening-new-dired-buffer should be checked
in this code path as well.

* lisp/dired.el (dired--make-directory-clickable): Call
dired--find-possibly-alternative-file instead of dired in click
callback.
---
 lisp/dired.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/dired.el b/lisp/dired.el
index 33e38ed2c1c..03f673c5e22 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2012,7 +2012,7 @@ dired--make-directory-clickable
                                       (interactive)
                                       (if (assoc current-dir dired-subdir-alist)
                                           (dired-goto-subdir current-dir)
-                                        (dired current-dir)))))
+                                        (dired--find-possibly-alternative-file current-dir)))))
                         (define-keymap
                           "<mouse-2>" click
                           "<follow-link>" 'mouse-face
-- 
2.39.2

bug#67856: Dired navigation via directory line does not respect dired-kill-when-opening-new-dired-buffer

[Eli Zaretskii]

> Date: Sat, 16 Dec 2023 12:35:10 -0800
> From:  Jared Finder via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> The option dired-kill-when-opening-new-dired-buffer is not respected
> when clicking on parent directories in the directory line at the top
> of dired buffers.  This can be fixed by calling
> dired--find-possibly-alternate-file instead of dired in the
> callback, as my attached patch does.
> 
> I believe changing to dired--find-possibly-alternate-file is safe
> because from playing around with dired, the directory being clicked
> on must be a directory and can not contain wildcards at this point.
> Therefore, at this point it is known that the directory is just a
> plain directory (no wildcards) and so going through find-file
> machinery will have the intended result.

If we want to install this on the emacs-29 release branch, I'd prefer
a safer variant, which actually verified that we don't call
dired--find-possibly-alternate-file with a wildcard.  That's because
we have quite a few features that place buffers in Dired mode, and we
could easily miss one that does have wildcards there.

So would you mind modifying the patch a little so that it checks
whether current-dir includes wildcards, and if so, calls 'dired'
instead of dired--find-possibly-alternate-file?

Thanks.

bug#67856: Dired navigation via directory line does not respect dired-kill-when-opening-new-dired-buffer

[Jared Finder via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

On 2023-12-21 05:18, Eli Zaretskii wrote:
>> Date: Sat, 16 Dec 2023 12:35:10 -0800
>> From:  Jared Finder via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> The option dired-kill-when-opening-new-dired-buffer is not respected
>> when clicking on parent directories in the directory line at the top
>> of dired buffers.  This can be fixed by calling
>> dired--find-possibly-alternate-file instead of dired in the
>> callback, as my attached patch does.
>> 
>> I believe changing to dired--find-possibly-alternate-file is safe
>> because from playing around with dired, the directory being clicked
>> on must be a directory and can not contain wildcards at this point.
>> Therefore, at this point it is known that the directory is just a
>> plain directory (no wildcards) and so going through find-file
>> machinery will have the intended result.
> 
> If we want to install this on the emacs-29 release branch, I'd prefer
> a safer variant, which actually verified that we don't call
> dired--find-possibly-alternate-file with a wildcard.  That's because
> we have quite a few features that place buffers in Dired mode, and we
> could easily miss one that does have wildcards there.
> 
> So would you mind modifying the patch a little so that it checks
> whether current-dir includes wildcards, and if so, calls 'dired'
> instead of dired--find-possibly-alternate-file?

Done, new patch attached.  Luckily, there already is a function that 
does exactly the right check.

Feel free to alter the comment explaining why this third code path 
exists.  I wasn't sure the right style to indicate this type of 
defensive code path that I am not certain is needed.

   -- MJF

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Clicks-on-directory-line-kill-prev-buffer-if-option-.patch --]
[-- Type: text/x-diff; name=0001-Clicks-on-directory-line-kill-prev-buffer-if-option-.patch, Size: 2131 bytes --]

From 30ad2b8acef36a3ab14f045bca94d7987ecd96e8 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Sat, 16 Dec 2023 12:10:03 -0800
Subject: [PATCH] Clicks on directory line kill prev buffer if option is set

The option dired-kill-when-opening-new-dired-buffer should be checked
in this code path as well.

* lisp/dired.el (dired--make-directory-clickable): Call
dired--find-possibly-alternative-file instead of dired in click
callback.
---
 lisp/dired.el | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/lisp/dired.el b/lisp/dired.el
index 33e38ed2c1c..fc91b58ecad 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2010,9 +2010,18 @@ dired--make-directory-clickable
               keymap ,(let* ((current-dir dir)
                              (click (lambda ()
                                       (interactive)
-                                      (if (assoc current-dir dired-subdir-alist)
-                                          (dired-goto-subdir current-dir)
-                                        (dired current-dir)))))
+                                      (cond
+                                       ((assoc current-dir dired-subdir-alist)
+                                        (dired-goto-subdir current-dir))
+                                       ;; If there is a wildcard character in the directory, don't
+                                       ;; use the alternate file machinery which tries to keep only
+                                       ;; one dired buffer open at once.
+                                       ;;
+                                       ;; FIXME: Is this code path reachable?
+                                       ((insert-directory-wildcard-in-dir-p current-dir)
+                                        (dired current-dir))
+                                       (t
+                                        (dired--find-possibly-alternative-file current-dir))))))
                         (define-keymap
                           "<mouse-2>" click
                           "<follow-link>" 'mouse-face
-- 
2.39.2

bug#67856: Dired navigation via directory line does not respect dired-kill-when-opening-new-dired-buffer

[Eli Zaretskii]

> Date: Sun, 24 Dec 2023 12:11:49 -0800
> From: Jared Finder <jared@finder.org>
> Cc: 67856@debbugs.gnu.org
> 
> On 2023-12-21 05:18, Eli Zaretskii wrote:
> >> Date: Sat, 16 Dec 2023 12:35:10 -0800
> >> From:  Jared Finder via "Bug reports for GNU Emacs,
> >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >> 
> >> The option dired-kill-when-opening-new-dired-buffer is not respected
> >> when clicking on parent directories in the directory line at the top
> >> of dired buffers.  This can be fixed by calling
> >> dired--find-possibly-alternate-file instead of dired in the
> >> callback, as my attached patch does.
> >> 
> >> I believe changing to dired--find-possibly-alternate-file is safe
> >> because from playing around with dired, the directory being clicked
> >> on must be a directory and can not contain wildcards at this point.
> >> Therefore, at this point it is known that the directory is just a
> >> plain directory (no wildcards) and so going through find-file
> >> machinery will have the intended result.
> > 
> > If we want to install this on the emacs-29 release branch, I'd prefer
> > a safer variant, which actually verified that we don't call
> > dired--find-possibly-alternate-file with a wildcard.  That's because
> > we have quite a few features that place buffers in Dired mode, and we
> > could easily miss one that does have wildcards there.
> > 
> > So would you mind modifying the patch a little so that it checks
> > whether current-dir includes wildcards, and if so, calls 'dired'
> > instead of dired--find-possibly-alternate-file?
> 
> Done, new patch attached.  Luckily, there already is a function that 
> does exactly the right check.
> 
> Feel free to alter the comment explaining why this third code path 
> exists.  I wasn't sure the right style to indicate this type of 
> defensive code path that I am not certain is needed.

Thanks, installed on the emacs-29 branch with some minor reformatting,
and closing the bug.