bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

1. emacs -Q
2. Eval:
(setq uniquify-buffer-name-style 'forward
      uniquify-trailing-separator-p t)
3. Open "Makefile" in the Emacs source tree
4. M-x rename-buffer lisp RET
5. Observe as it is renamed to lisp/ because there happens to be a
directory named lisp in the same directory.

This seems surprising, and also uniquify-trailing-separator-p is
documented to only affect dired buffers, so seems like a bug.

I have a patch which fixes this (and adds tests!) which I'll send in a
followup.


In GNU Emacs 29.0.60 (build 2, x86_64-pc-linux-gnu, X toolkit, cairo
version 1.16.0, Xaw3d scroll bars)
Repository revision: 4b6f2a7028b91128934a19f83572f24106782225
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: NixOS 21.11 (Porcupine)

Configured using:
 'configure
 --prefix=/nix/store/6d12l6xgg6bdqbv2l0k1nkpbixh93ib7-emacs-git-20220225.0
 --disable-build-details --with-modules --with-x-toolkit=lucid
 --with-xft --with-cairo'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY
PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XAW3D XDBE XIM XPM LUCID ZLIB

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

Major mode: ELisp/l

Minor modes in effect:
  bug-reference-prog-mode: t
  envrc-global-mode: t
  envrc-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  auto-revert-mode: t
  shell-dirtrack-mode: t
  server-mode: t
  windmove-mode: t
  tracking-mode: t
  savehist-mode: t
  save-place-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/sbaugh/.emacs.d/elpa/transient-0.3.7/transient hides /home/sbaugh/.local/src/emacs29/lisp/transient

Features:
(emacs-news-mode shadow emacsbug vc-annotate vc-dir vc-filewise
mode-local pcvs pcvs-defs pcvs-parse pcvs-info conf-mode magit-bundle
magit-gitignore magit-subtree ibuf-ext ibuf-macs mule-diag dos-w32
find-cmd apropos finder autoinsert pcmpl-unix pcmpl-gnu make-mode ido
benchmark term ehelp eshell esh-cmd esh-ext esh-opt esh-proc esh-io
esh-arg esh-module esh-groups esh-util ibuffer ibuffer-loaddefs
package-x eat term/xterm xterm eat-autoloads tramp-adb tramp-container
tramp-ftp loadhist timezone rect ediff-vers debbugs-browse time
flow-fill qp sort smiley gnus-cite mail-extr gnus-async gnus-bcklg
gnus-agent gnus-srvr gnus-score score-mode nnvirtual nntp gnus-ml
gnus-msg disp-table nndoc gnus-cache gnus-dup debbugs-gnu debbugs-compat
debbugs soap-client rng-xsd xsd-regexp debbugs-autoloads tar-mode
arc-mode archive-mode forge-list forge-commands forge-semi
forge-bitbucket buck forge-gogs gogs forge-gitea gtea forge-gitlab glab
forge-github ghub-graphql treepy gsexp ghub forge-notify forge-revnote
forge-pullreq forge-issue forge-topic yaml forge-post let-alist
markdown-mode forge-repo forge forge-core forge-db closql emacsql-sqlite
emacsql emacsql-compiler emoji-labels emoji multisession sqlite
org-attach tramp-cmds tramp-cache time-stamp tramp-sh shr-color textsec
uni-scripts idna-mapping ucs-normalize uni-confusable textsec-check
skeleton mhtml-mode css-mode js c-ts-common sgml-mode facemenu nix-mode
nix-repl nix-shell nix-store nix-log nix-instantiate nix-shebang
nix-format nix ediff-ptch magit-patch tramp-archive tramp-gvfs tramp
tramp-loaddefs trampver tramp-integration tramp-compat ls-lisp
cursor-sensor magit-bookmark bookmark man find-dired grep org-capture
ob-ditaa ob-plantuml org-clock org-colview org-crypt org-ctags org-habit
org-mouse org-plot org-protocol novice pulse color git-rebase canlock
view image-file image-converter rmail goto-addr whitespace eglot
external-completion array jsonrpc ert flymake-proc flymake warnings
diary-lib diary-loaddefs cal-iso cus-dep loaddefs-gen cus-theme oc-basic
ol-eww eww url-queue mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus
nnselect gnus-art mm-uu mml2015 mm-view mml-smime smime dig gnus-sum shr
pixel-fill kinsoku url-file svg dom gnus-group gnus-undo gnus-start
gnus-dbus dbus xml gnus-cloud nnimap nnmail mail-source utf7 nnoo
parse-time gnus-spec gnus-int gnus-range gnus-win gnus nnheader range
ol-docview doc-view jka-compr image-mode exif ol-bibtex bibtex iso8601
ol-bbdb ol-w3m ol-doi org-link-doi dired-aux sh-script smie executable
files-x tabify ggtags etags fileloop xref compile ewoc cc-mode cc-fonts
cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs
debug backtrace cus-edit cus-start cus-load wid-edit lisp-mnt dabbrev pp
cl-print shortdoc completion help-fns radix-tree mm-archive
network-stream url-cache url-http url-auth url-gw nsm
display-line-numbers misc tmm mule-util misearch multi-isearch vc-hg
vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs log-view vc bug-reference
magit-ediff ediff ediff-merg ediff-mult ediff-wind ediff-diff ediff-help
ediff-init ediff-util vc-git vc-dispatcher face-remap ob-python python
pcase treesit agda2 envrc inheritenv page-ext dired-x magit-extras
project magit-submodule magit-obsolete magit-blame magit-stash
magit-reflog magit-bisect magit-push magit-pull magit-fetch magit-clone
magit-remote magit-commit magit-sequence magit-notes magit-worktree
magit-tag magit-merge magit-branch magit-reset magit-files magit-refs
magit-status magit magit-repos magit-apply magit-wip magit-log
which-func imenu magit-diff smerge-mode diff git-commit log-edit message
sendmail yank-media dired dired-loaddefs rfc822 mml mml-sec epa derived
epg rfc6068 epg-config gnus-util text-property-search mm-decode
mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils mailheader
pcvs-util add-log magit-core magit-autorevert autorevert filenotify
magit-margin magit-transient magit-process with-editor shell server
magit-mode transient edmacro kmacro magit-git magit-section magit-utils
crm dash cl-extra windmove lui-autopaste circe advice diff-mode
lui-irc-colors irc gnutls puny lcs lui-logging lui-format lui tracking
shorten help-mode flyspell ispell circe-compat ox-odt rng-loc rng-uri
rng-parse rng-match rng-dt rng-util rng-pttrn nxml-parse nxml-ns
nxml-enc xmltok nxml-util ox-latex ox-icalendar org-agenda ox-html table
ox-ascii ox-publish ox org-element org-persist xdg org-id org-refile
avl-tree generator org ob ob-tangle ob-ref ob-lob ob-table ob-exp
org-macro org-src ob-comint org-pcomplete pcomplete org-list
org-footnote org-faces org-entities time-date noutline outline icons
ob-emacs-lisp ob-core ob-eval org-cycle org-table ol rx org-fold
org-fold-core org-keys oc org-loaddefs find-func cal-menu calendar
cal-loaddefs org-version org-compat org-macs format-spec gdb-mi bindat
gud easy-mmode comint ansi-osc ansi-color ring ffap thingatpt
cyberpunk-theme savehist saveplace finder-inf envrc-autoloads
nix-mode-autoloads forge-autoloads htmlize-autoloads
slime-volleyball-autoloads graphviz-dot-mode-autoloads yaml-autoloads
auctex-autoloads tex-site notmuch-autoloads csv-mode-autoloads
ghub-autoloads treepy-autoloads circe-autoloads inheritenv-autoloads
mentor-autoloads url-scgi-autoloads xml-rpc-autoloads async-autoloads
ggtags-autoloads closql-autoloads emacsql-sqlite-autoloads
emacsql-autoloads magit-autoloads magit-section-autoloads
git-commit-autoloads with-editor-autoloads transient-autoloads
cyberpunk-theme-autoloads info dash-autoloads markdown-mode-autoloads
package browse-url url url-proxy url-privacy url-expand url-methods
url-history url-cookie generate-lisp-file url-domsuf url-util mailcap
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
password-cache json subr-x map byte-opt gv bytecomp byte-compile
url-vars cl-loaddefs cl-lib rmc iso-transl tooltip cconv eldoc paren
electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-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 x-toolkit
x multi-tty make-network-process emacs)

Memory information:
((conses 16 5991041 619218)
 (symbols 48 79996 40)
 (strings 32 571790 315988)
 (string-bytes 1 40038796)
 (vectors 16 186257)
 (vector-slots 8 3516735 1014181)
 (floats 8 10132 8490)
 (intervals 56 582407 12855)
 (buffers 976 373))

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

This patch takes the approach of pulling uniquify-trailing-separator-p
out of uniquify and putting it into dired; now the trailing separator is
specified when the dired buffer is created.  This is incidentally also
vastly more efficient: The old way did n² filesystem accesses which is
not something we should be doing on every buffer creation/rename.

Also, this approach is in line with other simplifications of uniquify
that I'd like to make (as part of implementing bug#62621).

Also, now there are tests for uniquify.

diff --git a/lisp/dired.el b/lisp/dired.el
index 4a4ecc901c4..12629dbbd87 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -490,6 +490,17 @@ dired-guess-shell-znew-switches
                  (string :tag "Switches"))
   :version "29.1")
 
+(defcustom dired-trailing-separator nil
+  "If non-nil, add a file name separator to dired buffer names.
+For example, \"dir/\" instead of \"dir\".
+
+Normally the separator is added at the end and matches the
+platform convention for path separators.  If
+`uniquify-buffer-name-style' is `reverse', add the separator at
+the beginning, and use `uniquify-separator' for the separator."
+  :type 'boolean
+  :group 'dired)
+
 \f
 ;;; Internal variables
 
@@ -1285,6 +1296,19 @@ dired--align-all-files
               (insert-char ?\s distance 'inherit))
             (forward-line)))))))
 
+(defun dired--create-buffer (dirname)
+  "Create a buffer with an appropriate name for visiting this directory.
+
+Obeys `dired-trailing-separator'."
+  (let* ((filename (directory-file-name dirname))
+         (base (file-name-nondirectory filename)))
+    (create-file-buffer filename
+                        (if dired-trailing-separator
+                            (cond ((eq uniquify-buffer-name-style 'forward)
+	                           (file-name-as-directory base))
+	                          ((eq uniquify-buffer-name-style 'reverse)
+	                           (concat (or uniquify-separator "\\") base)))))))
+
 (defun dired-internal-noselect (dir-or-list &optional switches mode)
   ;; If DIR-OR-LIST is a string and there is an existing dired buffer
   ;; for it, just leave buffer as it is (don't even call dired-revert).
@@ -1306,7 +1330,7 @@ dired-internal-noselect
 	 ;; Note that buffer already is in dired-mode, if found.
 	 (new-buffer-p (null buffer)))
     (or buffer
-        (setq buffer (create-file-buffer (directory-file-name dirname))))
+        (setq buffer (dired--create-buffer dirname)))
     (set-buffer buffer)
     (if (not new-buffer-p)		; existing buffer ...
 	(cond (switches			; ... but new switches
diff --git a/lisp/files.el b/lisp/files.el
index d325729bf4d..75495ab608e 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2062,22 +2062,27 @@ find-alternate-file
 	  (kill-buffer obuf))))))
 \f
 ;; FIXME we really need to fold the uniquify stuff in here by default,
-;; not using advice, and add it to the doc string.
-(defun create-file-buffer (filename)
+(defun create-file-buffer (filename &optional basename)
   "Create a suitably named buffer for visiting FILENAME, and return it.
 FILENAME (sans directory) is used unchanged if that name is free;
-otherwise a string <2> or <3> or ... is appended to get an unused name.
+otherwise the buffer is renamed according to
+`uniquify-buffer-name-style' to get an unused name.
 
 Emacs treats buffers whose names begin with a space as internal buffers.
 To avoid confusion when visiting a file whose name begins with a space,
-this function prepends a \"|\" to the final result if necessary."
+this function prepends a \"|\" to the final result if necessary.
+
+If BASENAME is non-nil, it will be used as the buffer name.
+FILENAME will only be used to rename the buffer according to
+`uniquify-buffer-name-style' to get an unused name.
+"
   (let* ((lastname (file-name-nondirectory filename))
 	 (lastname (if (string= lastname "")
 	               filename lastname))
-	 (buf (generate-new-buffer (if (string-prefix-p " " lastname)
+	 (buf (generate-new-buffer (or basename (if (string-prefix-p " " lastname)
 			               (concat "|" lastname)
-			             lastname))))
-    (uniquify--create-file-buffer-advice buf filename)
+			             lastname)))))
+    (uniquify--create-file-buffer-advice buf filename basename)
     buf))
 
 (defvar abbreviated-home-dir nil
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index dee9ecba2ea..6c0f5468faa 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -147,12 +147,14 @@ uniquify-separator
 file name components (default \"\\\")."
   :type '(choice (const nil) string))
 
-(defcustom uniquify-trailing-separator-p nil
-  "If non-nil, add a file name separator to dired buffer names.
-If `uniquify-buffer-name-style' is `forward', add the separator at the end;
-if it is `reverse', add the separator at the beginning; otherwise, this
-variable is ignored."
-  :type 'boolean)
+(defvaralias
+ 'uniquify-trailing-separator-p
+ 'dired-trailing-separator)
+
+(make-obsolete-variable
+ 'uniquify-trailing-separator-p
+ 'dired-trailing-separator
+ "30.1")
 
 (defcustom uniquify-strip-common-suffix
   ;; Using it when uniquify-min-dir-content>0 doesn't make much sense.
@@ -174,8 +176,8 @@ uniquify-list-buffers-directory-modes
 (cl-defstruct (uniquify-item
 	    (:constructor nil) (:copier nil)
 	    (:constructor uniquify-make-item
-	     (base dirname buffer &optional proposed original-dirname)))
-  base dirname buffer proposed original-dirname)
+	     (base dirname buffer &optional proposed)))
+  base dirname buffer proposed)
 
 ;; Internal variables used free
 (defvar uniquify-possibly-resolvable nil)
@@ -211,7 +213,7 @@ uniquify-rationalize-file-buffer-names
   (when dirname
     (setq dirname (expand-file-name (directory-file-name dirname)))
     (let ((fix-list (list (uniquify-make-item base dirname newbuf
-                                              nil dirname)))
+                                              nil)))
 	  items)
       (dolist (buffer (buffer-list))
 	(when (and (not (and uniquify-ignore-buffers-re
@@ -292,8 +294,7 @@ uniquify-rationalize
       (setf (uniquify-item-proposed item)
 	    (uniquify-get-proposed-name (uniquify-item-base item)
 					(uniquify-item-dirname item)
-                                        nil
-                                        (uniquify-item-original-dirname item)))
+                                        nil))
       (setq uniquify-managed fix-list)))
   ;; Strip any shared last directory names of the dirname.
   (when (and (cdr fix-list) uniquify-strip-common-suffix)
@@ -316,8 +317,7 @@ uniquify-rationalize
 					      (uniquify-item-dirname item))))
 				      (and f (directory-file-name f)))
 				    (uniquify-item-buffer item)
-				    (uniquify-item-proposed item)
-                                    (uniquify-item-original-dirname item))
+				    (uniquify-item-proposed item))
 		fix-list)))))
   ;; If uniquify-min-dir-content is 0, this will end up just
   ;; passing fix-list to uniquify-rationalize-conflicting-sublist.
@@ -345,21 +345,10 @@ uniquify-rationalize-a-list
     (uniquify-rationalize-conflicting-sublist conflicting-sublist
 					      old-proposed depth)))
 
-(defun uniquify-get-proposed-name (base dirname &optional depth
-                                        original-dirname)
+(defun uniquify-get-proposed-name (base dirname &optional depth)
   (unless depth (setq depth uniquify-min-dir-content))
   (cl-assert (equal (directory-file-name dirname) dirname)) ;No trailing slash.
 
-  ;; Distinguish directories by adding extra separator.
-  (if (and uniquify-trailing-separator-p
-	   (file-directory-p (expand-file-name base original-dirname))
-	   (not (string-equal base "")))
-      (cond ((eq uniquify-buffer-name-style 'forward)
-	     (setq base (file-name-as-directory base)))
-	    ;; (setq base (concat base "/")))
-	    ((eq uniquify-buffer-name-style 'reverse)
-	     (setq base (concat (or uniquify-separator "\\") base)))))
-
   (let ((extra-string nil)
 	(n depth))
     (while (and (> n 0) dirname)
@@ -421,8 +410,7 @@ uniquify-rationalize-conflicting-sublist
 		  (uniquify-get-proposed-name
 		   (uniquify-item-base item)
 		   (uniquify-item-dirname item)
-		   depth
-                   (uniquify-item-original-dirname item))))
+		   depth)))
 	  (uniquify-rationalize-a-list conf-list depth))
       (unless (string= old-name "")
 	(uniquify-rename-buffer (car conf-list) old-name)))))
@@ -492,15 +480,14 @@ uniquify--rename-buffer-advice
 
 
 ;; (advice-add 'create-file-buffer :around #'uniquify--create-file-buffer-advice)
-(defun uniquify--create-file-buffer-advice (buf filename)
+(defun uniquify--create-file-buffer-advice (buf filename basename)
   ;; BEWARE: This is called directly from `files.el'!
   "Uniquify buffer names with parts of directory name."
   (when uniquify-buffer-name-style
-    (let ((filename (expand-file-name (directory-file-name filename))))
-      (uniquify-rationalize-file-buffer-names
-       (file-name-nondirectory filename)
-       (file-name-directory filename)
-       buf))))
+    (uniquify-rationalize-file-buffer-names
+     (or basename (file-name-nondirectory filename))
+     (file-name-directory (expand-file-name (directory-file-name filename)))
+     buf)))
 
 (defun uniquify-unload-function ()
   "Unload the uniquify library."
diff --git a/test/lisp/uniquify-tests.el b/test/lisp/uniquify-tests.el
new file mode 100644
index 00000000000..70f7125cbe9
--- /dev/null
+++ b/test/lisp/uniquify-tests.el
@@ -0,0 +1,111 @@
+;;; uniquify-tests.el --- Tests for uniquify         -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; Author: Spencer Baugh <sbaugh@janestreet.com>
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest uniquify-basic ()
+  (let (bufs old-names)
+    (cl-flet ((names-are (current-names &optional nosave)
+                (should (equal (mapcar #'buffer-name bufs) current-names))
+                (unless nosave (push current-names old-names))))
+      (should (eq (get-buffer "z") nil))
+      (push (find-file-noselect "a/b/z") bufs)
+      (names-are '("z"))
+      (push (find-file-noselect "a/b/c/z") bufs)
+      (names-are '("z<c>" "z<b>"))
+      (push (find-file-noselect "a/b/d/z") bufs)
+      (names-are '("z<d>" "z<c>" "z<b>"))
+      (push (find-file-noselect "e/b/z") bufs)
+      (names-are '("z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; buffers without a buffer-file-name don't get uniquified by uniquify
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; but they do get uniquified by the C code which uses <n>
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      (save-excursion
+        ;; uniquify will happily work with file-visiting buffers whose names don't match buffer-file-name
+        (find-file "f/y")
+        (push (current-buffer) bufs)
+        (rename-buffer "z" t)
+        (names-are '("z<f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave)
+        ;; somewhat confusing behavior results if a buffer is renamed to match an already-uniquified buffer
+        (rename-buffer "z<a/b>" t)
+        (names-are '("z<a/b><f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave))
+      (while bufs
+        (kill-buffer (pop bufs))
+        (names-are (pop old-names) 'nosave)))))
+
+(ert-deftest uniquify-dirs ()
+  "Check strip-common-suffix and trailing-separator-p work together; bug#47132"
+  (let* ((root (make-temp-file "emacs-uniquify-tests" 'dir))
+         (a-path (file-name-concat root "a/x/y/dir"))
+         (b-path (file-name-concat root "b/x/y/dir")))
+    (make-directory a-path 'parents)
+    (make-directory b-path 'parents)
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p nil))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir" "b/dir")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix nil)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/x/y/dir/" "b/x/y/dir/")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir/" "b/dir/")))
+        (mapc #'kill-buffer bufs)))))
+
+(ert-deftest uniquify-rename-to-dir ()
+  "Giving a buffer a name which matches a directory doesn't rename the buffer"
+  (let ((uniquify-buffer-name-style 'forward)
+        (uniquify-trailing-separator-p t))
+      (save-excursion
+        (find-file "../README")
+        (rename-buffer "lisp" t)
+        (should (equal (buffer-name) "lisp"))
+        (kill-buffer))))
+
+(ert-deftest uniquify-separator-style-reverse ()
+  (let ((uniquify-buffer-name-style 'reverse)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "\\lib-src"))
+      (kill-buffer))))
+
+(provide 'uniquify-tests)
+;;; uniquify-tests.el ends here

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

Ah, while I'm at it, here's a fix (based on the patch in the preceding
mail) for a different bug which I just noticed: create-file-buffer's
documentations states:

>Emacs treats buffers whose names begin with a space as internal buffers.
>To avoid confusion when visiting a file whose name begins with a space,
>this function prepends a "|" to the final result if necessary.

But uniquify renames the buffer away from having that "|".  This patch
fixes that bug.

diff --git a/lisp/files.el b/lisp/files.el
index c9433938729..e1e8e905fb0 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2079,9 +2079,10 @@ create-file-buffer
   (let* ((lastname (or basename (file-name-nondirectory filename)))
 	 (lastname (if (string= lastname "")
 	               filename lastname))
-	 (buf (generate-new-buffer (if (string-prefix-p " " lastname)
-			               (concat "|" lastname)
-			             lastname))))
+         (basename (if (string-prefix-p " " lastname)
+		       (concat "|" lastname)
+		     lastname))
+	 (buf (generate-new-buffer basename)))
     (uniquify--create-file-buffer-advice buf filename basename)
     buf))
 
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index 6c0f5468faa..ad6f9797381 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -485,7 +485,7 @@ uniquify--create-file-buffer-advice
   "Uniquify buffer names with parts of directory name."
   (when uniquify-buffer-name-style
     (uniquify-rationalize-file-buffer-names
-     (or basename (file-name-nondirectory filename))
+     basename
      (file-name-directory (expand-file-name (directory-file-name filename)))
      buf)))
 

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

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


Simplified and combined the previous two patches, now with a nice commit
message and changelog.  Also, stopped moving this functionality into
dired, that's not really necessary.

Most of this change is tests, and most of the remainder is moving the
uniquify-trailing-separator-p code without changes from uniquify.el into
create-file-buffer.  Hopefully it is fairly easy to review.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-recalculate-the-buffer-basename-inside-uniquif.patch --]
[-- Type: text/x-patch, Size: 13664 bytes --]

From ebd49cb05f5c0db643e4a616bad23565eef53b75 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Sun, 9 Apr 2023 08:10:52 -0400
Subject: [PATCH] Don't recalculate the buffer basename inside uniquify

Previously, uniquify--create-file-buffer-advice would use the filename
of the buffer to calculate what the buffer's basename should be.  Now
that gets passed in from create-file-buffer, which lets us fix several
bugs:

1. Before this patch, if a buffer happened to be named the same thing
as directory in its default-directory, the buffer would get renamed
with a directory separator according to uniquify-trailing-separator-p.

2. Buffers with a leading space should get a leading |, as described
by create-file-buffer's docstring; before this patch, uniquify would
remove that leading |.

* lisp/dired.el (dired-internal-noselect): Pass t to
create-file-buffer for the new directory argument.
* lisp/files.el (create-file-buffer): Add a new directory argument to
handle uniquify-trailing-separator-p, and pass the desired basename to
uniquify directly.
* lisp/uniquify.el (uniquify-item):
(uniquify-rationalize-file-buffer-names, uniquify-rationalize,
uniquify-get-proposed-name, uniquify-rationalize-conflicting-sublist):
Remove uniquify-trailing-separator-p handling.
(uniquify--create-file-buffer-advice): Take new basename argument and
use it, instead of recalculating the basename from the filename.
---
 lisp/dired.el               |   2 +-
 lisp/files.el               |  26 +++++---
 lisp/uniquify.el            |  32 +++-------
 test/lisp/uniquify-tests.el | 118 ++++++++++++++++++++++++++++++++++++
 4 files changed, 146 insertions(+), 32 deletions(-)
 create mode 100644 test/lisp/uniquify-tests.el

diff --git a/lisp/dired.el b/lisp/dired.el
index 4a4ecc901c4..62ff98c5279 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1306,7 +1306,7 @@ dired-internal-noselect
 	 ;; Note that buffer already is in dired-mode, if found.
 	 (new-buffer-p (null buffer)))
     (or buffer
-        (setq buffer (create-file-buffer (directory-file-name dirname))))
+        (setq buffer (create-file-buffer (directory-file-name dirname) t)))
     (set-buffer buffer)
     (if (not new-buffer-p)		; existing buffer ...
 	(cond (switches			; ... but new switches
diff --git a/lisp/files.el b/lisp/files.el
index d325729bf4d..ada3d19442f 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2062,22 +2062,32 @@ find-alternate-file
 	  (kill-buffer obuf))))))
 \f
 ;; FIXME we really need to fold the uniquify stuff in here by default,
-;; not using advice, and add it to the doc string.
-(defun create-file-buffer (filename)
+(defun create-file-buffer (filename &optional directory)
   "Create a suitably named buffer for visiting FILENAME, and return it.
 FILENAME (sans directory) is used unchanged if that name is free;
-otherwise a string <2> or <3> or ... is appended to get an unused name.
+otherwise the buffer is renamed according to
+`uniquify-buffer-name-style' to get an unused name.
 
 Emacs treats buffers whose names begin with a space as internal buffers.
 To avoid confusion when visiting a file whose name begins with a space,
-this function prepends a \"|\" to the final result if necessary."
+this function prepends a \"|\" to the final result if necessary.
+
+If DIRECTORY is non-nil, a file name separator will be added to
+the buffer name according to `uniquify-trailing-separator-p'."
   (let* ((lastname (file-name-nondirectory filename))
 	 (lastname (if (string= lastname "")
 	               filename lastname))
-	 (buf (generate-new-buffer (if (string-prefix-p " " lastname)
-			               (concat "|" lastname)
-			             lastname))))
-    (uniquify--create-file-buffer-advice buf filename)
+         (lastname (if (and directory uniquify-trailing-separator-p)
+                       (cond ((eq uniquify-buffer-name-style 'forward)
+	                      (file-name-as-directory lastname))
+	                     ((eq uniquify-buffer-name-style 'reverse)
+	                      (concat (or uniquify-separator "\\") lastname)))
+                     lastname))
+         (basename (if (string-prefix-p " " lastname)
+		       (concat "|" lastname)
+		     lastname))
+	 (buf (generate-new-buffer basename)))
+    (uniquify--create-file-buffer-advice buf filename basename)
     buf))
 
 (defvar abbreviated-home-dir nil
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index dee9ecba2ea..bfb61eca16d 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -174,8 +174,8 @@ uniquify-list-buffers-directory-modes
 (cl-defstruct (uniquify-item
 	    (:constructor nil) (:copier nil)
 	    (:constructor uniquify-make-item
-	     (base dirname buffer &optional proposed original-dirname)))
-  base dirname buffer proposed original-dirname)
+	     (base dirname buffer &optional proposed)))
+  base dirname buffer proposed)
 
 ;; Internal variables used free
 (defvar uniquify-possibly-resolvable nil)
@@ -211,7 +211,7 @@ uniquify-rationalize-file-buffer-names
   (when dirname
     (setq dirname (expand-file-name (directory-file-name dirname)))
     (let ((fix-list (list (uniquify-make-item base dirname newbuf
-                                              nil dirname)))
+                                              nil)))
 	  items)
       (dolist (buffer (buffer-list))
 	(when (and (not (and uniquify-ignore-buffers-re
@@ -292,8 +292,7 @@ uniquify-rationalize
       (setf (uniquify-item-proposed item)
 	    (uniquify-get-proposed-name (uniquify-item-base item)
 					(uniquify-item-dirname item)
-                                        nil
-                                        (uniquify-item-original-dirname item)))
+                                        nil))
       (setq uniquify-managed fix-list)))
   ;; Strip any shared last directory names of the dirname.
   (when (and (cdr fix-list) uniquify-strip-common-suffix)
@@ -316,8 +315,7 @@ uniquify-rationalize
 					      (uniquify-item-dirname item))))
 				      (and f (directory-file-name f)))
 				    (uniquify-item-buffer item)
-				    (uniquify-item-proposed item)
-                                    (uniquify-item-original-dirname item))
+				    (uniquify-item-proposed item))
 		fix-list)))))
   ;; If uniquify-min-dir-content is 0, this will end up just
   ;; passing fix-list to uniquify-rationalize-conflicting-sublist.
@@ -345,21 +343,10 @@ uniquify-rationalize-a-list
     (uniquify-rationalize-conflicting-sublist conflicting-sublist
 					      old-proposed depth)))
 
-(defun uniquify-get-proposed-name (base dirname &optional depth
-                                        original-dirname)
+(defun uniquify-get-proposed-name (base dirname &optional depth)
   (unless depth (setq depth uniquify-min-dir-content))
   (cl-assert (equal (directory-file-name dirname) dirname)) ;No trailing slash.
 
-  ;; Distinguish directories by adding extra separator.
-  (if (and uniquify-trailing-separator-p
-	   (file-directory-p (expand-file-name base original-dirname))
-	   (not (string-equal base "")))
-      (cond ((eq uniquify-buffer-name-style 'forward)
-	     (setq base (file-name-as-directory base)))
-	    ;; (setq base (concat base "/")))
-	    ((eq uniquify-buffer-name-style 'reverse)
-	     (setq base (concat (or uniquify-separator "\\") base)))))
-
   (let ((extra-string nil)
 	(n depth))
     (while (and (> n 0) dirname)
@@ -421,8 +408,7 @@ uniquify-rationalize-conflicting-sublist
 		  (uniquify-get-proposed-name
 		   (uniquify-item-base item)
 		   (uniquify-item-dirname item)
-		   depth
-                   (uniquify-item-original-dirname item))))
+		   depth)))
 	  (uniquify-rationalize-a-list conf-list depth))
       (unless (string= old-name "")
 	(uniquify-rename-buffer (car conf-list) old-name)))))
@@ -492,13 +478,13 @@ uniquify--rename-buffer-advice
 
 
 ;; (advice-add 'create-file-buffer :around #'uniquify--create-file-buffer-advice)
-(defun uniquify--create-file-buffer-advice (buf filename)
+(defun uniquify--create-file-buffer-advice (buf filename basename)
   ;; BEWARE: This is called directly from `files.el'!
   "Uniquify buffer names with parts of directory name."
   (when uniquify-buffer-name-style
     (let ((filename (expand-file-name (directory-file-name filename))))
       (uniquify-rationalize-file-buffer-names
-       (file-name-nondirectory filename)
+       basename
        (file-name-directory filename)
        buf))))
 
diff --git a/test/lisp/uniquify-tests.el b/test/lisp/uniquify-tests.el
new file mode 100644
index 00000000000..89976add164
--- /dev/null
+++ b/test/lisp/uniquify-tests.el
@@ -0,0 +1,118 @@
+;;; uniquify-tests.el --- Tests for uniquify         -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; Author: Spencer Baugh <sbaugh@janestreet.com>
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest uniquify-basic ()
+  (let (bufs old-names)
+    (cl-flet ((names-are (current-names &optional nosave)
+                (should (equal (mapcar #'buffer-name bufs) current-names))
+                (unless nosave (push current-names old-names))))
+      (should (eq (get-buffer "z") nil))
+      (push (find-file-noselect "a/b/z") bufs)
+      (names-are '("z"))
+      (push (find-file-noselect "a/b/c/z") bufs)
+      (names-are '("z<c>" "z<b>"))
+      (push (find-file-noselect "a/b/d/z") bufs)
+      (names-are '("z<d>" "z<c>" "z<b>"))
+      (push (find-file-noselect "e/b/z") bufs)
+      (names-are '("z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; buffers without a buffer-file-name don't get uniquified by uniquify
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; but they do get uniquified by the C code which uses <n>
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      (save-excursion
+        ;; uniquify will happily work with file-visiting buffers whose names don't match buffer-file-name
+        (find-file "f/y")
+        (push (current-buffer) bufs)
+        (rename-buffer "z" t)
+        (names-are '("z<f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave)
+        ;; somewhat confusing behavior results if a buffer is renamed to match an already-uniquified buffer
+        (rename-buffer "z<a/b>" t)
+        (names-are '("z<a/b><f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave))
+      (while bufs
+        (kill-buffer (pop bufs))
+        (names-are (pop old-names) 'nosave)))))
+
+(ert-deftest uniquify-dirs ()
+  "Check strip-common-suffix and trailing-separator-p work together; bug#47132"
+  (let* ((root (make-temp-file "emacs-uniquify-tests" 'dir))
+         (a-path (file-name-concat root "a/x/y/dir"))
+         (b-path (file-name-concat root "b/x/y/dir")))
+    (make-directory a-path 'parents)
+    (make-directory b-path 'parents)
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p nil))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir" "b/dir")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix nil)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/x/y/dir/" "b/x/y/dir/")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir/" "b/dir/")))
+        (mapc #'kill-buffer bufs)))))
+
+(ert-deftest uniquify-rename-to-dir ()
+  "Giving a buffer a name which matches a directory doesn't rename the buffer"
+  (let ((uniquify-buffer-name-style 'forward)
+        (uniquify-trailing-separator-p t))
+      (save-excursion
+        (find-file "../README")
+        (rename-buffer "lisp" t)
+        (should (equal (buffer-name) "lisp"))
+        (kill-buffer))))
+
+(ert-deftest uniquify-separator-style-reverse ()
+  (let ((uniquify-buffer-name-style 'reverse)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "\\lib-src"))
+      (kill-buffer))))
+
+(ert-deftest uniquify-space-prefix ()
+  "If a buffer starts with a space, | is added at the start"
+  (save-excursion
+    (find-file " foo")
+    (should (equal (buffer-name) "| foo"))
+    (kill-buffer)))
+
+(provide 'uniquify-tests)
+;;; uniquify-tests.el ends here
-- 
2.38.0

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

Stefan, Lars: any comments?

> From: sbaugh@catern.com
> Date: Fri, 21 Apr 2023 20:59:30 +0000 (UTC)
> 
> Simplified and combined the previous two patches, now with a nice commit
> message and changelog.  Also, stopped moving this functionality into
> dired, that's not really necessary.
> 
> Most of this change is tests, and most of the remainder is moving the
> uniquify-trailing-separator-p code without changes from uniquify.el into
> create-file-buffer.  Hopefully it is fairly easy to review.
> 
> 
> >From ebd49cb05f5c0db643e4a616bad23565eef53b75 Mon Sep 17 00:00:00 2001
> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sun, 9 Apr 2023 08:10:52 -0400
> Subject: [PATCH] Don't recalculate the buffer basename inside uniquify
> 
> Previously, uniquify--create-file-buffer-advice would use the filename
> of the buffer to calculate what the buffer's basename should be.  Now
> that gets passed in from create-file-buffer, which lets us fix several
> bugs:
> 
> 1. Before this patch, if a buffer happened to be named the same thing
> as directory in its default-directory, the buffer would get renamed
> with a directory separator according to uniquify-trailing-separator-p.
> 
> 2. Buffers with a leading space should get a leading |, as described
> by create-file-buffer's docstring; before this patch, uniquify would
> remove that leading |.
> 
> * lisp/dired.el (dired-internal-noselect): Pass t to
> create-file-buffer for the new directory argument.
> * lisp/files.el (create-file-buffer): Add a new directory argument to
> handle uniquify-trailing-separator-p, and pass the desired basename to
> uniquify directly.
> * lisp/uniquify.el (uniquify-item):
> (uniquify-rationalize-file-buffer-names, uniquify-rationalize,
> uniquify-get-proposed-name, uniquify-rationalize-conflicting-sublist):
> Remove uniquify-trailing-separator-p handling.
> (uniquify--create-file-buffer-advice): Take new basename argument and
> use it, instead of recalculating the basename from the filename.
> ---
>  lisp/dired.el               |   2 +-
>  lisp/files.el               |  26 +++++---
>  lisp/uniquify.el            |  32 +++-------
>  test/lisp/uniquify-tests.el | 118 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 146 insertions(+), 32 deletions(-)
>  create mode 100644 test/lisp/uniquify-tests.el
> 
> diff --git a/lisp/dired.el b/lisp/dired.el
> index 4a4ecc901c4..62ff98c5279 100644
> --- a/lisp/dired.el
> +++ b/lisp/dired.el
> @@ -1306,7 +1306,7 @@ dired-internal-noselect
>  	 ;; Note that buffer already is in dired-mode, if found.
>  	 (new-buffer-p (null buffer)))
>      (or buffer
> -        (setq buffer (create-file-buffer (directory-file-name dirname))))
> +        (setq buffer (create-file-buffer (directory-file-name dirname) t)))
>      (set-buffer buffer)
>      (if (not new-buffer-p)		; existing buffer ...
>  	(cond (switches			; ... but new switches
> diff --git a/lisp/files.el b/lisp/files.el
> index d325729bf4d..ada3d19442f 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -2062,22 +2062,32 @@ find-alternate-file
>  	  (kill-buffer obuf))))))
>  \f
>  ;; FIXME we really need to fold the uniquify stuff in here by default,
> -;; not using advice, and add it to the doc string.
> -(defun create-file-buffer (filename)
> +(defun create-file-buffer (filename &optional directory)
>    "Create a suitably named buffer for visiting FILENAME, and return it.
>  FILENAME (sans directory) is used unchanged if that name is free;
> -otherwise a string <2> or <3> or ... is appended to get an unused name.
> +otherwise the buffer is renamed according to
> +`uniquify-buffer-name-style' to get an unused name.
>  
>  Emacs treats buffers whose names begin with a space as internal buffers.
>  To avoid confusion when visiting a file whose name begins with a space,
> -this function prepends a \"|\" to the final result if necessary."
> +this function prepends a \"|\" to the final result if necessary.
> +
> +If DIRECTORY is non-nil, a file name separator will be added to
> +the buffer name according to `uniquify-trailing-separator-p'."
>    (let* ((lastname (file-name-nondirectory filename))
>  	 (lastname (if (string= lastname "")
>  	               filename lastname))
> -	 (buf (generate-new-buffer (if (string-prefix-p " " lastname)
> -			               (concat "|" lastname)
> -			             lastname))))
> -    (uniquify--create-file-buffer-advice buf filename)
> +         (lastname (if (and directory uniquify-trailing-separator-p)
> +                       (cond ((eq uniquify-buffer-name-style 'forward)
> +	                      (file-name-as-directory lastname))
> +	                     ((eq uniquify-buffer-name-style 'reverse)
> +	                      (concat (or uniquify-separator "\\") lastname)))
> +                     lastname))
> +         (basename (if (string-prefix-p " " lastname)
> +		       (concat "|" lastname)
> +		     lastname))
> +	 (buf (generate-new-buffer basename)))
> +    (uniquify--create-file-buffer-advice buf filename basename)
>      buf))
>  
>  (defvar abbreviated-home-dir nil
> diff --git a/lisp/uniquify.el b/lisp/uniquify.el
> index dee9ecba2ea..bfb61eca16d 100644
> --- a/lisp/uniquify.el
> +++ b/lisp/uniquify.el
> @@ -174,8 +174,8 @@ uniquify-list-buffers-directory-modes
>  (cl-defstruct (uniquify-item
>  	    (:constructor nil) (:copier nil)
>  	    (:constructor uniquify-make-item
> -	     (base dirname buffer &optional proposed original-dirname)))
> -  base dirname buffer proposed original-dirname)
> +	     (base dirname buffer &optional proposed)))
> +  base dirname buffer proposed)
>  
>  ;; Internal variables used free
>  (defvar uniquify-possibly-resolvable nil)
> @@ -211,7 +211,7 @@ uniquify-rationalize-file-buffer-names
>    (when dirname
>      (setq dirname (expand-file-name (directory-file-name dirname)))
>      (let ((fix-list (list (uniquify-make-item base dirname newbuf
> -                                              nil dirname)))
> +                                              nil)))
>  	  items)
>        (dolist (buffer (buffer-list))
>  	(when (and (not (and uniquify-ignore-buffers-re
> @@ -292,8 +292,7 @@ uniquify-rationalize
>        (setf (uniquify-item-proposed item)
>  	    (uniquify-get-proposed-name (uniquify-item-base item)
>  					(uniquify-item-dirname item)
> -                                        nil
> -                                        (uniquify-item-original-dirname item)))
> +                                        nil))
>        (setq uniquify-managed fix-list)))
>    ;; Strip any shared last directory names of the dirname.
>    (when (and (cdr fix-list) uniquify-strip-common-suffix)
> @@ -316,8 +315,7 @@ uniquify-rationalize
>  					      (uniquify-item-dirname item))))
>  				      (and f (directory-file-name f)))
>  				    (uniquify-item-buffer item)
> -				    (uniquify-item-proposed item)
> -                                    (uniquify-item-original-dirname item))
> +				    (uniquify-item-proposed item))
>  		fix-list)))))
>    ;; If uniquify-min-dir-content is 0, this will end up just
>    ;; passing fix-list to uniquify-rationalize-conflicting-sublist.
> @@ -345,21 +343,10 @@ uniquify-rationalize-a-list
>      (uniquify-rationalize-conflicting-sublist conflicting-sublist
>  					      old-proposed depth)))
>  
> -(defun uniquify-get-proposed-name (base dirname &optional depth
> -                                        original-dirname)
> +(defun uniquify-get-proposed-name (base dirname &optional depth)
>    (unless depth (setq depth uniquify-min-dir-content))
>    (cl-assert (equal (directory-file-name dirname) dirname)) ;No trailing slash.
>  
> -  ;; Distinguish directories by adding extra separator.
> -  (if (and uniquify-trailing-separator-p
> -	   (file-directory-p (expand-file-name base original-dirname))
> -	   (not (string-equal base "")))
> -      (cond ((eq uniquify-buffer-name-style 'forward)
> -	     (setq base (file-name-as-directory base)))
> -	    ;; (setq base (concat base "/")))
> -	    ((eq uniquify-buffer-name-style 'reverse)
> -	     (setq base (concat (or uniquify-separator "\\") base)))))
> -
>    (let ((extra-string nil)
>  	(n depth))
>      (while (and (> n 0) dirname)
> @@ -421,8 +408,7 @@ uniquify-rationalize-conflicting-sublist
>  		  (uniquify-get-proposed-name
>  		   (uniquify-item-base item)
>  		   (uniquify-item-dirname item)
> -		   depth
> -                   (uniquify-item-original-dirname item))))
> +		   depth)))
>  	  (uniquify-rationalize-a-list conf-list depth))
>        (unless (string= old-name "")
>  	(uniquify-rename-buffer (car conf-list) old-name)))))
> @@ -492,13 +478,13 @@ uniquify--rename-buffer-advice
>  
>  
>  ;; (advice-add 'create-file-buffer :around #'uniquify--create-file-buffer-advice)
> -(defun uniquify--create-file-buffer-advice (buf filename)
> +(defun uniquify--create-file-buffer-advice (buf filename basename)
>    ;; BEWARE: This is called directly from `files.el'!
>    "Uniquify buffer names with parts of directory name."
>    (when uniquify-buffer-name-style
>      (let ((filename (expand-file-name (directory-file-name filename))))
>        (uniquify-rationalize-file-buffer-names
> -       (file-name-nondirectory filename)
> +       basename
>         (file-name-directory filename)
>         buf))))
>  
> diff --git a/test/lisp/uniquify-tests.el b/test/lisp/uniquify-tests.el
> new file mode 100644
> index 00000000000..89976add164
> --- /dev/null
> +++ b/test/lisp/uniquify-tests.el
> @@ -0,0 +1,118 @@
> +;;; uniquify-tests.el --- Tests for uniquify         -*- lexical-binding: t; -*-
> +
> +;; Copyright (C) 2023 Free Software Foundation, Inc.
> +
> +;; Author: Spencer Baugh <sbaugh@janestreet.com>
> +
> +;; This program is free software; you can redistribute it and/or modify
> +;; it under the terms of the GNU General Public License as published by
> +;; the Free Software Foundation, either version 3 of the License, or
> +;; (at your option) any later version.
> +
> +;; This program is distributed in the hope that it will be useful,
> +;; but WITHOUT ANY WARRANTY; without even the implied warranty of
> +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;; GNU General Public License for more details.
> +
> +;; You should have received a copy of the GNU General Public License
> +;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
> +
> +;;; Commentary:
> +
> +;;; Code:
> +
> +(require 'ert)
> +
> +(ert-deftest uniquify-basic ()
> +  (let (bufs old-names)
> +    (cl-flet ((names-are (current-names &optional nosave)
> +                (should (equal (mapcar #'buffer-name bufs) current-names))
> +                (unless nosave (push current-names old-names))))
> +      (should (eq (get-buffer "z") nil))
> +      (push (find-file-noselect "a/b/z") bufs)
> +      (names-are '("z"))
> +      (push (find-file-noselect "a/b/c/z") bufs)
> +      (names-are '("z<c>" "z<b>"))
> +      (push (find-file-noselect "a/b/d/z") bufs)
> +      (names-are '("z<d>" "z<c>" "z<b>"))
> +      (push (find-file-noselect "e/b/z") bufs)
> +      (names-are '("z<e/b>" "z<d>" "z<c>" "z<a/b>"))
> +      ;; buffers without a buffer-file-name don't get uniquified by uniquify
> +      (push (generate-new-buffer "z") bufs)
> +      (names-are '("z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
> +      ;; but they do get uniquified by the C code which uses <n>
> +      (push (generate-new-buffer "z") bufs)
> +      (names-are '("z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
> +      (save-excursion
> +        ;; uniquify will happily work with file-visiting buffers whose names don't match buffer-file-name
> +        (find-file "f/y")
> +        (push (current-buffer) bufs)
> +        (rename-buffer "z" t)
> +        (names-are '("z<f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave)
> +        ;; somewhat confusing behavior results if a buffer is renamed to match an already-uniquified buffer
> +        (rename-buffer "z<a/b>" t)
> +        (names-are '("z<a/b><f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave))
> +      (while bufs
> +        (kill-buffer (pop bufs))
> +        (names-are (pop old-names) 'nosave)))))
> +
> +(ert-deftest uniquify-dirs ()
> +  "Check strip-common-suffix and trailing-separator-p work together; bug#47132"
> +  (let* ((root (make-temp-file "emacs-uniquify-tests" 'dir))
> +         (a-path (file-name-concat root "a/x/y/dir"))
> +         (b-path (file-name-concat root "b/x/y/dir")))
> +    (make-directory a-path 'parents)
> +    (make-directory b-path 'parents)
> +    (let ((uniquify-buffer-name-style 'forward)
> +          (uniquify-strip-common-suffix t)
> +          (uniquify-trailing-separator-p nil))
> +      (let ((bufs (list (find-file-noselect a-path)
> +                       (find-file-noselect b-path))))
> +        (should (equal (mapcar #'buffer-name bufs)
> +                       '("a/dir" "b/dir")))
> +        (mapc #'kill-buffer bufs)))
> +    (let ((uniquify-buffer-name-style 'forward)
> +          (uniquify-strip-common-suffix nil)
> +          (uniquify-trailing-separator-p t))
> +      (let ((bufs (list (find-file-noselect a-path)
> +                       (find-file-noselect b-path))))
> +        (should (equal (mapcar #'buffer-name bufs)
> +                       '("a/x/y/dir/" "b/x/y/dir/")))
> +        (mapc #'kill-buffer bufs)))
> +    (let ((uniquify-buffer-name-style 'forward)
> +          (uniquify-strip-common-suffix t)
> +          (uniquify-trailing-separator-p t))
> +      (let ((bufs (list (find-file-noselect a-path)
> +                       (find-file-noselect b-path))))
> +        (should (equal (mapcar #'buffer-name bufs)
> +                       '("a/dir/" "b/dir/")))
> +        (mapc #'kill-buffer bufs)))))
> +
> +(ert-deftest uniquify-rename-to-dir ()
> +  "Giving a buffer a name which matches a directory doesn't rename the buffer"
> +  (let ((uniquify-buffer-name-style 'forward)
> +        (uniquify-trailing-separator-p t))
> +      (save-excursion
> +        (find-file "../README")
> +        (rename-buffer "lisp" t)
> +        (should (equal (buffer-name) "lisp"))
> +        (kill-buffer))))
> +
> +(ert-deftest uniquify-separator-style-reverse ()
> +  (let ((uniquify-buffer-name-style 'reverse)
> +        (uniquify-trailing-separator-p t))
> +    (save-excursion
> +      (should (file-directory-p "../lib-src"))
> +      (find-file "../lib-src")
> +      (should (equal (buffer-name) "\\lib-src"))
> +      (kill-buffer))))
> +
> +(ert-deftest uniquify-space-prefix ()
> +  "If a buffer starts with a space, | is added at the start"
> +  (save-excursion
> +    (find-file " foo")
> +    (should (equal (buffer-name) "| foo"))
> +    (kill-buffer)))
> +
> +(provide 'uniquify-tests)
> +;;; uniquify-tests.el ends here
> -- 
> 2.38.0
> 

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

Ping again on this latest version of my patch.  I have some actual
features I'd like to add to uniquify, but they're on top of this patch.
Plus it would be nice to land the tests added in my patch.

Eli Zaretskii <eliz@gnu.org> writes:
> Stefan, Lars: any comments?

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: sbaugh@catern.com
> Date: Mon, 03 Jul 2023 18:54:58 +0000 (UTC)
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Lars Ingebrigtsen
> 	<larsi@gnus.org>, 62732@debbugs.gnu.org
> 
> 
> Ping again on this latest version of my patch.  I have some actual
> features I'd like to add to uniquify, but they're on top of this patch.
> Plus it would be nice to land the tests added in my patch.
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > Stefan, Lars: any comments?

I still want to hear someone else's opinion, because the changeset
looks waaay larger and more complex than can be justified by the tiny
problem in rare corner situations that triggered it.  If no one else
is interested enough to voice any opinion, I tend to "wontfix" it,
TBH.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

> Ah, while I'm at it, here's a fix (based on the patch in the preceding
> mail) for a different bug which I just noticed: create-file-buffer's
> documentations states:
>
>>Emacs treats buffers whose names begin with a space as internal buffers.
>>To avoid confusion when visiting a file whose name begins with a space,
>>this function prepends a "|" to the final result if necessary.
>
> But uniquify renames the buffer away from having that "|".  This patch
> fixes that bug.

How 'bout the patch below (based on the current code), instead?


        Stefan


diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index dee9ecba2ea..c252d5461aa 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -498,7 +498,7 @@ uniquify--create-file-buffer-advice
   (when uniquify-buffer-name-style
     (let ((filename (expand-file-name (directory-file-name filename))))
       (uniquify-rationalize-file-buffer-names
-       (file-name-nondirectory filename)
+       (buffer-name buf)
        (file-name-directory filename)
        buf))))
 

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

> Ping again on this latest version of my patch.  I have some actual
> features I'd like to add to uniquify, but they're on top of this patch.
> Plus it would be nice to land the tests added in my patch.

Sorry for not following up further, but I was waiting for a reaction to
my proposal to replace the additional arg to `create-file-buffer`
by the patch below.

If we can't avoid changing the API of `create-file-buffer`, I'd like
a comment explaining clearly why.  As it stands the patch is a bit vague
about that, if not confusing:

     ;; FIXME we really need to fold the uniquify stuff in here by default,
    -;; not using advice, and add it to the doc string.
    -(defun create-file-buffer (filename)
    +(defun create-file-buffer (filename &optional directory)
       "Create a suitably named buffer for visiting FILENAME, and return it.
     FILENAME (sans directory) is used unchanged if that name is free;
    -otherwise a string <2> or <3> or ... is appended to get an unused name.
    +otherwise the buffer is renamed according to
    +`uniquify-buffer-name-style' to get an unused name.
     
     Emacs treats buffers whose names begin with a space as internal buffers.
     To avoid confusion when visiting a file whose name begins with a space,
    -this function prepends a \"|\" to the final result if necessary."
    +this function prepends a \"|\" to the final result if necessary.
    +
    +If DIRECTORY is non-nil, a file name separator will be added to
    +the buffer name according to `uniquify-trailing-separator-p'."

Where will that separator be added?  And why is the arg called `directory`?
And why/when is that arg needed, since the separator will often be
introduced anyway by uniquify even with a nil arg?


        Stefan


Stefan Monnier [2023-05-05 16:30:28] wrote:

>> Ah, while I'm at it, here's a fix (based on the patch in the preceding
>> mail) for a different bug which I just noticed: create-file-buffer's
>> documentations states:
>>
>>>Emacs treats buffers whose names begin with a space as internal buffers.
>>>To avoid confusion when visiting a file whose name begins with a space,
>>>this function prepends a "|" to the final result if necessary.
>>
>> But uniquify renames the buffer away from having that "|".  This patch
>> fixes that bug.
>
> How 'bout the patch below (based on the current code), instead?
>
>
>         Stefan
>
>
> diff --git a/lisp/uniquify.el b/lisp/uniquify.el
> index dee9ecba2ea..c252d5461aa 100644
> --- a/lisp/uniquify.el
> +++ b/lisp/uniquify.el
> @@ -498,7 +498,7 @@ uniquify--create-file-buffer-advice
>    (when uniquify-buffer-name-style
>      (let ((filename (expand-file-name (directory-file-name filename))))
>        (uniquify-rationalize-file-buffer-names
> -       (file-name-nondirectory filename)
> +       (buffer-name buf)
>         (file-name-directory filename)
>         buf))))
>  

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Ping again on this latest version of my patch.  I have some actual
>> features I'd like to add to uniquify, but they're on top of this patch.
>> Plus it would be nice to land the tests added in my patch.
>
> Sorry for not following up further, but I was waiting for a reaction to
> my proposal to replace the additional arg to `create-file-buffer`
> by the patch below.

Ah, that unfortunately doesn't work, at least on its own.  It causes the
tests I added for uniquify to immediately fail.  The reason it doesn't
work, I believe, is that it makes us try to uniquify buffers with their
<n> numeric suffix included (which is the initial name the buffer gets).
And such buffers are of course already unique, so uniquify stops doing
anything.

We could remove the <n> suffix, but what we really want is just "the
name we wanted to give the buffer, before any uniquifying", aka the
basename.  And we have that in create-file-buffer, so it seems better to
just pass it down directly.

> If we can't avoid changing the API of `create-file-buffer`, I'd like
> a comment explaining clearly why.

So, the thing we want to communicate to uniquify is, the basename of the
buffer: the name we want for the buffer before it's made unique.  For
regular files this is (file-name-nondirectory file), the last component
of the file name.

Before this patch, the basename (as passed to uniquify) was always (file-name-nondirectory
(directory-file-name dir)) for directories.  So directories and regular
files looked the same to uniquify.  So it needed to add the trailing
separator some other way (by checking file-directory-p).

After this patch, the trailing separator is added *as part of the
basename* as passed to uniquify.  So uniquify doesn't need to do
anything, just uniquify the buffer like it would any other, and indeed
uniquify.el no longer uses uniquify-trailing-separator-p at all.

> As it stands the patch is a bit vague
> about that, if not confusing:
>
>      ;; FIXME we really need to fold the uniquify stuff in here by default,
>     -;; not using advice, and add it to the doc string.
>     -(defun create-file-buffer (filename)
>     +(defun create-file-buffer (filename &optional directory)
>        "Create a suitably named buffer for visiting FILENAME, and return it.
>      FILENAME (sans directory) is used unchanged if that name is free;
>     -otherwise a string <2> or <3> or ... is appended to get an unused name.
>     +otherwise the buffer is renamed according to
>     +`uniquify-buffer-name-style' to get an unused name.
>      
>      Emacs treats buffers whose names begin with a space as internal buffers.
>      To avoid confusion when visiting a file whose name begins with a space,
>     -this function prepends a \"|\" to the final result if necessary."
>     +this function prepends a \"|\" to the final result if necessary.
>     +
>     +If DIRECTORY is non-nil, a file name separator will be added to
>     +the buffer name according to `uniquify-trailing-separator-p'."
>
> Where will that separator be added?

It is added in create-file-buffer, in the calculation of basename.

> And why is the arg called `directory`?

Because we are indicating that this filename is a directory, or at least
that we want to visit this filename as if it was a directory.

> And why/when is that arg needed, since the separator will often be
> introduced anyway by uniquify even with a nil arg?

This is now the only place that uniquify-trailing-separator-p is used,
so the separator won't be introduced if DIRECTORY is nil.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

> This patch takes the approach of pulling uniquify-trailing-separator-p
> out of uniquify and putting it into dired; now the trailing separator is
> specified when the dired buffer is created.  This is incidentally also
> vastly more efficient: The old way did n² filesystem accesses which is
> not something we should be doing on every buffer creation/rename.

It's indeed a better approach, thanks.
I'm a bit annoyed at the need to add an argument to `create-file-buffer`
and I wonder if we could avoid that by replacing:

> +(defun dired--create-buffer (dirname)
> +  "Create a buffer with an appropriate name for visiting this directory.
> +
> +Obeys `dired-trailing-separator'."
> +  (let* ((filename (directory-file-name dirname))
> +         (base (file-name-nondirectory filename)))
> +    (create-file-buffer filename
> +                        (if dired-trailing-separator
> +                            (cond ((eq uniquify-buffer-name-style 'forward)
> +	                           (file-name-as-directory base))
> +	                          ((eq uniquify-buffer-name-style 'reverse)
> +	                           (concat (or uniquify-separator "\\") base)))))))

with

    (defun dired--create-buffer (dirname)
      "Create a buffer with an appropriate name for visiting this directory.
    Obeys `dired-trailing-separator'."
      (let* ((filename (directory-file-name dirname)))
        (create-file-buffer (if dired-trailing-separator
                                (file-name-as-directory filename)
                              filename))))

or even just

    (defun dired--create-buffer (dirname)
      "Create a buffer with an appropriate name for visiting this directory."
      (create-file-buffer (file-name-as-directory dirname)))

and then do the rest inside `uniquify.el`.


        Stefan

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

>     (defun dired--create-buffer (dirname)
>       "Create a buffer with an appropriate name for visiting this directory."
>       (create-file-buffer (file-name-as-directory dirname)))
>
> and then do the rest inside `uniquify.el`.

Or inside `create-file-buffer`.


        Stefan

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Spencer Baugh]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> This patch takes the approach of pulling uniquify-trailing-separator-p
>> out of uniquify and putting it into dired; now the trailing separator is
>> specified when the dired buffer is created.  This is incidentally also
>> vastly more efficient: The old way did n² filesystem accesses which is
>> not something we should be doing on every buffer creation/rename.
>
> It's indeed a better approach, thanks.
> I'm a bit annoyed at the need to add an argument to `create-file-buffer`
> and I wonder if we could avoid that by replacing:
>
>> +(defun dired--create-buffer (dirname)
>> +  "Create a buffer with an appropriate name for visiting this directory.
>> +
>> +Obeys `dired-trailing-separator'."
>> +  (let* ((filename (directory-file-name dirname))
>> +         (base (file-name-nondirectory filename)))
>> +    (create-file-buffer filename
>> +                        (if dired-trailing-separator
>> +                            (cond ((eq uniquify-buffer-name-style 'forward)
>> +	                           (file-name-as-directory base))
>> +	                          ((eq uniquify-buffer-name-style 'reverse)
>> +	                           (concat (or uniquify-separator "\\") base)))))))
>
> with
>
>     (defun dired--create-buffer (dirname)
>       "Create a buffer with an appropriate name for visiting this directory.
>     Obeys `dired-trailing-separator'."
>       (let* ((filename (directory-file-name dirname)))
>         (create-file-buffer (if dired-trailing-separator
>                                 (file-name-as-directory filename)
>                               filename))))
>
> or even just
>
>     (defun dired--create-buffer (dirname)
>       "Create a buffer with an appropriate name for visiting this directory."
>       (create-file-buffer (file-name-as-directory dirname)))
>
> and then do the rest inside `uniquify.el`.
>
>
>         Stefan

Ah, check my most recently sent patch, I revised it a fair bit from the
initial version.  It still adds an argument to create-file-buffer, but I
think it is a much more reasonable one.  Plus it's independent of dired,
so probably useful for other dired-like packages which want to create
buffers which view directories, if there are any such...

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This patch takes the approach of pulling uniquify-trailing-separator-p
>> out of uniquify and putting it into dired; now the trailing separator is
>> specified when the dired buffer is created.  This is incidentally also
>> vastly more efficient: The old way did n² filesystem accesses which is
>> not something we should be doing on every buffer creation/rename.
>
> It's indeed a better approach, thanks.
> I'm a bit annoyed at the need to add an argument to `create-file-buffer`
> and I wonder if we could avoid that by replacing:
>
>> +(defun dired--create-buffer (dirname)
>> +  "Create a buffer with an appropriate name for visiting this directory.
>> +
>> +Obeys `dired-trailing-separator'."
>> +  (let* ((filename (directory-file-name dirname))
>> +         (base (file-name-nondirectory filename)))
>> +    (create-file-buffer filename
>> +                        (if dired-trailing-separator
>> +                            (cond ((eq uniquify-buffer-name-style 'forward)
>> +	                           (file-name-as-directory base))
>> +	                          ((eq uniquify-buffer-name-style 'reverse)
>> +	                           (concat (or uniquify-separator "\\") base)))))))
>
> with
>
>     (defun dired--create-buffer (dirname)
>       "Create a buffer with an appropriate name for visiting this directory.
>     Obeys `dired-trailing-separator'."
>       (let* ((filename (directory-file-name dirname)))
>         (create-file-buffer (if dired-trailing-separator
>                                 (file-name-as-directory filename)
>                               filename))))
>
> or even just
>
>     (defun dired--create-buffer (dirname)
>       "Create a buffer with an appropriate name for visiting this directory."
>       (create-file-buffer (file-name-as-directory dirname)))
>
> and then do the rest inside `uniquify.el`.

This inspired me to do something not exactly this but which also gets
rid of the new argument to create-file-buffer.  How about this:

diff --git a/lisp/dired.el b/lisp/dired.el
index d14cf47ffd5..3c9e6e40f9b 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1306,7 +1306,7 @@ dired-internal-noselect
 	 ;; Note that buffer already is in dired-mode, if found.
 	 (new-buffer-p (null buffer)))
     (or buffer
-        (setq buffer (create-file-buffer (directory-file-name dirname))))
+        (setq buffer (create-file-buffer dirname)))
     (set-buffer buffer)
     (if (not new-buffer-p)		; existing buffer ...
 	(cond (switches			; ... but new switches
diff --git a/lisp/files.el b/lisp/files.el
index d325729bf4d..4b5a877d1e3 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2062,22 +2062,30 @@ find-alternate-file
 	  (kill-buffer obuf))))))
 \f
 ;; FIXME we really need to fold the uniquify stuff in here by default,
-;; not using advice, and add it to the doc string.
 (defun create-file-buffer (filename)
   "Create a suitably named buffer for visiting FILENAME, and return it.
 FILENAME (sans directory) is used unchanged if that name is free;
-otherwise a string <2> or <3> or ... is appended to get an unused name.
+otherwise the buffer is renamed according to
+`uniquify-buffer-name-style' to get an unused name.
 
 Emacs treats buffers whose names begin with a space as internal buffers.
 To avoid confusion when visiting a file whose name begins with a space,
 this function prepends a \"|\" to the final result if necessary."
-  (let* ((lastname (file-name-nondirectory filename))
-	 (lastname (if (string= lastname "")
-	               filename lastname))
-	 (buf (generate-new-buffer (if (string-prefix-p " " lastname)
-			               (concat "|" lastname)
-			             lastname))))
-    (uniquify--create-file-buffer-advice buf filename)
+  (let* ((lastname (if (directory-name-p filename)
+                       (file-name-nondirectory (directory-file-name filename))
+                     (file-name-nondirectory filename)))
+         (lastname (if (and (directory-name-p filename) uniquify-trailing-separator-p)
+                       (cond ((eq uniquify-buffer-name-style 'forward)
+	                      (file-name-as-directory lastname))
+	                     ((eq uniquify-buffer-name-style 'reverse)
+	                      (concat (or uniquify-separator "\\") lastname))
+                             (t lastname))
+                     lastname))
+         (basename (if (string-prefix-p " " lastname)
+		       (concat "|" lastname)
+		     lastname))
+	 (buf (generate-new-buffer basename)))
+    (uniquify--create-file-buffer-advice buf filename basename)
     buf))
 
 (defvar abbreviated-home-dir nil
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index dee9ecba2ea..bfb61eca16d 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -174,8 +174,8 @@ uniquify-list-buffers-directory-modes
 (cl-defstruct (uniquify-item
 	    (:constructor nil) (:copier nil)
 	    (:constructor uniquify-make-item
-	     (base dirname buffer &optional proposed original-dirname)))
-  base dirname buffer proposed original-dirname)
+	     (base dirname buffer &optional proposed)))
+  base dirname buffer proposed)
 
 ;; Internal variables used free
 (defvar uniquify-possibly-resolvable nil)
@@ -211,7 +211,7 @@ uniquify-rationalize-file-buffer-names
   (when dirname
     (setq dirname (expand-file-name (directory-file-name dirname)))
     (let ((fix-list (list (uniquify-make-item base dirname newbuf
-                                              nil dirname)))
+                                              nil)))
 	  items)
       (dolist (buffer (buffer-list))
 	(when (and (not (and uniquify-ignore-buffers-re
@@ -292,8 +292,7 @@ uniquify-rationalize
       (setf (uniquify-item-proposed item)
 	    (uniquify-get-proposed-name (uniquify-item-base item)
 					(uniquify-item-dirname item)
-                                        nil
-                                        (uniquify-item-original-dirname item)))
+                                        nil))
       (setq uniquify-managed fix-list)))
   ;; Strip any shared last directory names of the dirname.
   (when (and (cdr fix-list) uniquify-strip-common-suffix)
@@ -316,8 +315,7 @@ uniquify-rationalize
 					      (uniquify-item-dirname item))))
 				      (and f (directory-file-name f)))
 				    (uniquify-item-buffer item)
-				    (uniquify-item-proposed item)
-                                    (uniquify-item-original-dirname item))
+				    (uniquify-item-proposed item))
 		fix-list)))))
   ;; If uniquify-min-dir-content is 0, this will end up just
   ;; passing fix-list to uniquify-rationalize-conflicting-sublist.
@@ -345,21 +343,10 @@ uniquify-rationalize-a-list
     (uniquify-rationalize-conflicting-sublist conflicting-sublist
 					      old-proposed depth)))
 
-(defun uniquify-get-proposed-name (base dirname &optional depth
-                                        original-dirname)
+(defun uniquify-get-proposed-name (base dirname &optional depth)
   (unless depth (setq depth uniquify-min-dir-content))
   (cl-assert (equal (directory-file-name dirname) dirname)) ;No trailing slash.
 
-  ;; Distinguish directories by adding extra separator.
-  (if (and uniquify-trailing-separator-p
-	   (file-directory-p (expand-file-name base original-dirname))
-	   (not (string-equal base "")))
-      (cond ((eq uniquify-buffer-name-style 'forward)
-	     (setq base (file-name-as-directory base)))
-	    ;; (setq base (concat base "/")))
-	    ((eq uniquify-buffer-name-style 'reverse)
-	     (setq base (concat (or uniquify-separator "\\") base)))))
-
   (let ((extra-string nil)
 	(n depth))
     (while (and (> n 0) dirname)
@@ -421,8 +408,7 @@ uniquify-rationalize-conflicting-sublist
 		  (uniquify-get-proposed-name
 		   (uniquify-item-base item)
 		   (uniquify-item-dirname item)
-		   depth
-                   (uniquify-item-original-dirname item))))
+		   depth)))
 	  (uniquify-rationalize-a-list conf-list depth))
       (unless (string= old-name "")
 	(uniquify-rename-buffer (car conf-list) old-name)))))
@@ -492,13 +478,13 @@ uniquify--rename-buffer-advice
 
 
 ;; (advice-add 'create-file-buffer :around #'uniquify--create-file-buffer-advice)
-(defun uniquify--create-file-buffer-advice (buf filename)
+(defun uniquify--create-file-buffer-advice (buf filename basename)
   ;; BEWARE: This is called directly from `files.el'!
   "Uniquify buffer names with parts of directory name."
   (when uniquify-buffer-name-style
     (let ((filename (expand-file-name (directory-file-name filename))))
       (uniquify-rationalize-file-buffer-names
-       (file-name-nondirectory filename)
+       basename
        (file-name-directory filename)
        buf))))
 

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

> This inspired me to do something not exactly this but which also gets
> rid of the new argument to create-file-buffer.  How about this:

Now you're talking!  :-)
LGTM!  Thank you very much,


        Stefan

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> This inspired me to do something not exactly this but which also gets
>> rid of the new argument to create-file-buffer.  How about this:
>
> Now you're talking!  :-)
> LGTM!  Thank you very much,

Great!  Here's that as a complete patch again.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-recalculate-the-buffer-basename-inside-uniquif.patch --]
[-- Type: text/x-patch, Size: 14210 bytes --]

From 46804cd388d3ca05c0ad2054a0fcf3aba80b651c Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Sun, 9 Jul 2023 10:24:33 -0400
Subject: [PATCH] Don't recalculate the buffer basename inside uniquify

Previously, uniquify--create-file-buffer-advice would use the filename
of the buffer to calculate what the buffer's basename should be.  Now
that gets passed in from create-file-buffer, which lets us fix several
bugs:

1. before this patch, if a buffer happened to be named the same thing
as directory in its default-directory, the buffer would get renamed
with a directory separator according to uniquify-trailing-separator-p.

2. buffers with a leading space should get a leading |, as described
by create-file-buffer's docstring; before this patch, uniquify would
remove that leading |.

* lisp/dired.el (dired-internal-noselect): Pass a directory name to
create-file-buffer.
* lisp/files.el (create-file-buffer): Do uniquify-trailing-separator-p
handling if passed a directory filename.
* lisp/uniquify.el (uniquify-item):
(uniquify-rationalize-file-buffer-names, uniquify-rationalize,
uniquify-get-proposed-name, uniquify-rationalize-conflicting-sublist):
Remove uniquify-trailing-separator-p handling.
(uniquify--create-file-buffer-advice): Take new basename argument and
use it, instead of recalculating the basename from the filename.
---
 lisp/dired.el               |   2 +-
 lisp/files.el               |  26 +++++---
 lisp/uniquify.el            |  39 ++++-------
 test/lisp/uniquify-tests.el | 129 ++++++++++++++++++++++++++++++++++++
 4 files changed, 159 insertions(+), 37 deletions(-)
 create mode 100644 test/lisp/uniquify-tests.el

diff --git a/lisp/dired.el b/lisp/dired.el
index d14cf47ffd5..3c9e6e40f9b 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1306,7 +1306,7 @@ dired-internal-noselect
 	 ;; Note that buffer already is in dired-mode, if found.
 	 (new-buffer-p (null buffer)))
     (or buffer
-        (setq buffer (create-file-buffer (directory-file-name dirname))))
+        (setq buffer (create-file-buffer dirname)))
     (set-buffer buffer)
     (if (not new-buffer-p)		; existing buffer ...
 	(cond (switches			; ... but new switches
diff --git a/lisp/files.el b/lisp/files.el
index d325729bf4d..4b5a877d1e3 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2062,22 +2062,30 @@ find-alternate-file
 	  (kill-buffer obuf))))))
 \f
 ;; FIXME we really need to fold the uniquify stuff in here by default,
-;; not using advice, and add it to the doc string.
 (defun create-file-buffer (filename)
   "Create a suitably named buffer for visiting FILENAME, and return it.
 FILENAME (sans directory) is used unchanged if that name is free;
-otherwise a string <2> or <3> or ... is appended to get an unused name.
+otherwise the buffer is renamed according to
+`uniquify-buffer-name-style' to get an unused name.
 
 Emacs treats buffers whose names begin with a space as internal buffers.
 To avoid confusion when visiting a file whose name begins with a space,
 this function prepends a \"|\" to the final result if necessary."
-  (let* ((lastname (file-name-nondirectory filename))
-	 (lastname (if (string= lastname "")
-	               filename lastname))
-	 (buf (generate-new-buffer (if (string-prefix-p " " lastname)
-			               (concat "|" lastname)
-			             lastname))))
-    (uniquify--create-file-buffer-advice buf filename)
+  (let* ((lastname (if (directory-name-p filename)
+                       (file-name-nondirectory (directory-file-name filename))
+                     (file-name-nondirectory filename)))
+         (lastname (if (and (directory-name-p filename) uniquify-trailing-separator-p)
+                       (cond ((eq uniquify-buffer-name-style 'forward)
+	                      (file-name-as-directory lastname))
+	                     ((eq uniquify-buffer-name-style 'reverse)
+	                      (concat (or uniquify-separator "\\") lastname))
+                             (t lastname))
+                     lastname))
+         (basename (if (string-prefix-p " " lastname)
+		       (concat "|" lastname)
+		     lastname))
+	 (buf (generate-new-buffer basename)))
+    (uniquify--create-file-buffer-advice buf filename basename)
     buf))
 
 (defvar abbreviated-home-dir nil
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index dee9ecba2ea..d1ca455b673 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -174,8 +174,8 @@ uniquify-list-buffers-directory-modes
 (cl-defstruct (uniquify-item
 	    (:constructor nil) (:copier nil)
 	    (:constructor uniquify-make-item
-	     (base dirname buffer &optional proposed original-dirname)))
-  base dirname buffer proposed original-dirname)
+	     (base dirname buffer &optional proposed)))
+  base dirname buffer proposed)
 
 ;; Internal variables used free
 (defvar uniquify-possibly-resolvable nil)
@@ -211,7 +211,7 @@ uniquify-rationalize-file-buffer-names
   (when dirname
     (setq dirname (expand-file-name (directory-file-name dirname)))
     (let ((fix-list (list (uniquify-make-item base dirname newbuf
-                                              nil dirname)))
+                                              nil)))
 	  items)
       (dolist (buffer (buffer-list))
 	(when (and (not (and uniquify-ignore-buffers-re
@@ -292,8 +292,7 @@ uniquify-rationalize
       (setf (uniquify-item-proposed item)
 	    (uniquify-get-proposed-name (uniquify-item-base item)
 					(uniquify-item-dirname item)
-                                        nil
-                                        (uniquify-item-original-dirname item)))
+                                        nil))
       (setq uniquify-managed fix-list)))
   ;; Strip any shared last directory names of the dirname.
   (when (and (cdr fix-list) uniquify-strip-common-suffix)
@@ -316,8 +315,7 @@ uniquify-rationalize
 					      (uniquify-item-dirname item))))
 				      (and f (directory-file-name f)))
 				    (uniquify-item-buffer item)
-				    (uniquify-item-proposed item)
-                                    (uniquify-item-original-dirname item))
+				    (uniquify-item-proposed item))
 		fix-list)))))
   ;; If uniquify-min-dir-content is 0, this will end up just
   ;; passing fix-list to uniquify-rationalize-conflicting-sublist.
@@ -345,21 +343,10 @@ uniquify-rationalize-a-list
     (uniquify-rationalize-conflicting-sublist conflicting-sublist
 					      old-proposed depth)))
 
-(defun uniquify-get-proposed-name (base dirname &optional depth
-                                        original-dirname)
+(defun uniquify-get-proposed-name (base dirname &optional depth)
   (unless depth (setq depth uniquify-min-dir-content))
   (cl-assert (equal (directory-file-name dirname) dirname)) ;No trailing slash.
 
-  ;; Distinguish directories by adding extra separator.
-  (if (and uniquify-trailing-separator-p
-	   (file-directory-p (expand-file-name base original-dirname))
-	   (not (string-equal base "")))
-      (cond ((eq uniquify-buffer-name-style 'forward)
-	     (setq base (file-name-as-directory base)))
-	    ;; (setq base (concat base "/")))
-	    ((eq uniquify-buffer-name-style 'reverse)
-	     (setq base (concat (or uniquify-separator "\\") base)))))
-
   (let ((extra-string nil)
 	(n depth))
     (while (and (> n 0) dirname)
@@ -421,8 +408,7 @@ uniquify-rationalize-conflicting-sublist
 		  (uniquify-get-proposed-name
 		   (uniquify-item-base item)
 		   (uniquify-item-dirname item)
-		   depth
-                   (uniquify-item-original-dirname item))))
+		   depth)))
 	  (uniquify-rationalize-a-list conf-list depth))
       (unless (string= old-name "")
 	(uniquify-rename-buffer (car conf-list) old-name)))))
@@ -492,15 +478,14 @@ uniquify--rename-buffer-advice
 
 
 ;; (advice-add 'create-file-buffer :around #'uniquify--create-file-buffer-advice)
-(defun uniquify--create-file-buffer-advice (buf filename)
+(defun uniquify--create-file-buffer-advice (buf filename basename)
   ;; BEWARE: This is called directly from `files.el'!
   "Uniquify buffer names with parts of directory name."
   (when uniquify-buffer-name-style
-    (let ((filename (expand-file-name (directory-file-name filename))))
-      (uniquify-rationalize-file-buffer-names
-       (file-name-nondirectory filename)
-       (file-name-directory filename)
-       buf))))
+    (uniquify-rationalize-file-buffer-names
+     basename
+     (file-name-directory (expand-file-name (directory-file-name filename)))
+     buf)))
 
 (defun uniquify-unload-function ()
   "Unload the uniquify library."
diff --git a/test/lisp/uniquify-tests.el b/test/lisp/uniquify-tests.el
new file mode 100644
index 00000000000..abd61fa3504
--- /dev/null
+++ b/test/lisp/uniquify-tests.el
@@ -0,0 +1,129 @@
+;;; uniquify-tests.el --- Tests for uniquify         -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; Author: Spencer Baugh <sbaugh@janestreet.com>
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest uniquify-basic ()
+  (let (bufs old-names)
+    (cl-flet ((names-are (current-names &optional nosave)
+                (should (equal (mapcar #'buffer-name bufs) current-names))
+                (unless nosave (push current-names old-names))))
+      (should (eq (get-buffer "z") nil))
+      (push (find-file-noselect "a/b/z") bufs)
+      (names-are '("z"))
+      (push (find-file-noselect "a/b/c/z") bufs)
+      (names-are '("z<c>" "z<b>"))
+      (push (find-file-noselect "a/b/d/z") bufs)
+      (names-are '("z<d>" "z<c>" "z<b>"))
+      (push (find-file-noselect "e/b/z") bufs)
+      (names-are '("z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; buffers without a buffer-file-name don't get uniquified by uniquify
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; but they do get uniquified by the C code which uses <n>
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      (save-excursion
+        ;; uniquify will happily work with file-visiting buffers whose names don't match buffer-file-name
+        (find-file "f/y")
+        (push (current-buffer) bufs)
+        (rename-buffer "z" t)
+        (names-are '("z<f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave)
+        ;; somewhat confusing behavior results if a buffer is renamed to match an already-uniquified buffer
+        (rename-buffer "z<a/b>" t)
+        (names-are '("z<a/b><f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave))
+      (while bufs
+        (kill-buffer (pop bufs))
+        (names-are (pop old-names) 'nosave)))))
+
+(ert-deftest uniquify-dirs ()
+  "Check strip-common-suffix and trailing-separator-p work together; bug#47132"
+  (let* ((root (make-temp-file "emacs-uniquify-tests" 'dir))
+         (a-path (file-name-concat root "a/x/y/dir"))
+         (b-path (file-name-concat root "b/x/y/dir")))
+    (make-directory a-path 'parents)
+    (make-directory b-path 'parents)
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p nil))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir" "b/dir")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix nil)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/x/y/dir/" "b/x/y/dir/")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir/" "b/dir/")))
+        (mapc #'kill-buffer bufs)))))
+
+(ert-deftest uniquify-rename-to-dir ()
+  "Giving a buffer a name which matches a directory doesn't rename the buffer"
+  (let ((uniquify-buffer-name-style 'forward)
+        (uniquify-trailing-separator-p t))
+      (save-excursion
+        (find-file "../README")
+        (rename-buffer "lisp" t)
+        (should (equal (buffer-name) "lisp"))
+        (kill-buffer))))
+
+(ert-deftest uniquify-separator-style-reverse ()
+  (let ((uniquify-buffer-name-style 'reverse)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "\\lib-src"))
+      (kill-buffer))))
+
+(ert-deftest uniquify-separator-ignored ()
+  "If uniquify-buffer-name-style isn't forward or reverse,
+uniquify-trailing-separator-p is ignored"
+  (let ((uniquify-buffer-name-style 'post-forward-angle-brackets)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "lib-src"))
+      (kill-buffer))))
+
+(ert-deftest uniquify-space-prefix ()
+  "If a buffer starts with a space, | is added at the start"
+  (save-excursion
+    (find-file " foo")
+    (should (equal (buffer-name) "| foo"))
+    (kill-buffer)))
+
+(provide 'uniquify-tests)
+;;; uniquify-tests.el ends here
-- 
2.41.0

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

>>> This inspired me to do something not exactly this but which also gets
>>> rid of the new argument to create-file-buffer.  How about this:
>>
>> Now you're talking!  :-)
>> LGTM!  Thank you very much,
>
> Great!  Here's that as a complete patch again.

Eli, OK to install on `master`?


        Stefan


PS: Nitpicks:

> +  (let* ((lastname (if (directory-name-p filename)
> +                       (file-name-nondirectory (directory-file-name filename))
> +                     (file-name-nondirectory filename)))

Aka

     (let* ((lastname (file-name-nondirectory
                       (if (directory-name-p filename)
                           (directory-file-name filename)
                         filename)))

or even just

     (let* ((lastname (file-name-nondirectory
                       (directory-file-name filename)))

> +         (lastname (if (and (directory-name-p filename) uniquify-trailing-separator-p)
> +                       (cond ((eq uniquify-buffer-name-style 'forward)
> +	                      (file-name-as-directory lastname))
> +	                     ((eq uniquify-buffer-name-style 'reverse)
> +	                      (concat (or uniquify-separator "\\") lastname))
> +                             (t lastname))
> +                     lastname))

Here you can merge the `if` into the `cond` and I'd test
`uniquify-trailing-separator-p` first (cheaper and nil by default)
and since we know (directory-name-p filename), I'd be tempted to replace
(file-name-as-directory lastname) with just `filename`.  Also I'd argue
that when `uniquify-trailing-separator-p` and (directory-name-p
filename) are both true we never want to use just `lastname` so the
default should be to return `filename`:

            (lastname (cond
                       ((not (and uniquify-trailing-separator-p (directory-name-p filename)))
                        lastname)
                       ((eq uniquify-buffer-name-style 'reverse)
                        (concat (or uniquify-separator "\\") lastname))
                       (t filename)))

so for `post-forward` (my personal favorite) /foo/bar/mumble/name/ would
turn into name/|bar/mumble.
WDYT?


        Stefan

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> This inspired me to do something not exactly this but which also gets
>>>> rid of the new argument to create-file-buffer.  How about this:
>>>
>>> Now you're talking!  :-)
>>> LGTM!  Thank you very much,
>>
>> Great!  Here's that as a complete patch again.
>
> Eli, OK to install on `master`?
>
>
>         Stefan
>
>
> PS: Nitpicks:
>
>> +  (let* ((lastname (if (directory-name-p filename)
>> +                       (file-name-nondirectory (directory-file-name filename))
>> +                     (file-name-nondirectory filename)))
>
> Aka
>
>      (let* ((lastname (file-name-nondirectory
>                        (if (directory-name-p filename)
>                            (directory-file-name filename)
>                          filename)))
>
> or even just
>
>      (let* ((lastname (file-name-nondirectory
>                        (directory-file-name filename)))

Absolutely, makes sense.

>> +         (lastname (if (and (directory-name-p filename) uniquify-trailing-separator-p)
>> +                       (cond ((eq uniquify-buffer-name-style 'forward)
>> +	                      (file-name-as-directory lastname))
>> +	                     ((eq uniquify-buffer-name-style 'reverse)
>> +	                      (concat (or uniquify-separator "\\") lastname))
>> +                             (t lastname))
>> +                     lastname))
>
> Here you can merge the `if` into the `cond` and I'd test
> `uniquify-trailing-separator-p` first (cheaper and nil by default)

Yes, makes sense.  Revised patch for these two below.

> and since we know (directory-name-p filename), I'd be tempted to replace
> (file-name-as-directory lastname) with just `filename`.

(file-name-as-directory lastname) and filename are different though: the
former is only the last component of filename, but as a directory name.
The latter is the full filename, which is not what the basename of the
buffer should be.  (since it gets no further processing)

> Also I'd argue that when `uniquify-trailing-separator-p` and
> (directory-name-p filename) are both true we never want to use just
> `lastname` so the default should be to return `filename`:
>
>             (lastname (cond
>                        ((not (and uniquify-trailing-separator-p (directory-name-p filename)))
>                         lastname)
>                        ((eq uniquify-buffer-name-style 'reverse)
>                         (concat (or uniquify-separator "\\") lastname))
>                        (t filename)))
>
> so for `post-forward` (my personal favorite) /foo/bar/mumble/name/ would
> turn into name/|bar/mumble.
> WDYT?

I agree that would be good (modulo that it does need to be
(file-name-as-directory lastname) rather than filename), but it would
change the existing behavior and not comply with the docstring of
`uniquify-trailing-separator-p'.  (I think it would be a reasonable
change though, but probably as a subsequent change)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-recalculate-the-buffer-basename-inside-uniquif.patch --]
[-- Type: text/x-patch, Size: 14093 bytes --]

From 023b8e7a715374e59a5456075b98d1422659cfe6 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Sun, 9 Jul 2023 10:24:33 -0400
Subject: [PATCH] Don't recalculate the buffer basename inside uniquify

Previously, uniquify--create-file-buffer-advice would use the filename
of the buffer to calculate what the buffer's basename should be.  Now
that gets passed in from create-file-buffer, which lets us fix several
bugs:

1. before this patch, if a buffer happened to be named the same thing
as directory in its default-directory, the buffer would get renamed
with a directory separator according to uniquify-trailing-separator-p.

2. buffers with a leading space should get a leading |, as described
by create-file-buffer's docstring; before this patch, uniquify would
remove that leading |.

* lisp/dired.el (dired-internal-noselect): Pass a directory name to
create-file-buffer.
* lisp/files.el (create-file-buffer): Do uniquify-trailing-separator-p
handling if passed a directory filename. (bug#62732)
* lisp/uniquify.el (uniquify-item):
(uniquify-rationalize-file-buffer-names, uniquify-rationalize,
uniquify-get-proposed-name, uniquify-rationalize-conflicting-sublist):
Remove uniquify-trailing-separator-p handling.
(uniquify--create-file-buffer-advice): Take new basename argument and
use it, instead of recalculating the basename from the filename.
---
 lisp/dired.el               |   2 +-
 lisp/files.el               |  25 ++++---
 lisp/uniquify.el            |  39 ++++-------
 test/lisp/uniquify-tests.el | 129 ++++++++++++++++++++++++++++++++++++
 4 files changed, 158 insertions(+), 37 deletions(-)
 create mode 100644 test/lisp/uniquify-tests.el

diff --git a/lisp/dired.el b/lisp/dired.el
index d14cf47ffd5..3c9e6e40f9b 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1306,7 +1306,7 @@ dired-internal-noselect
 	 ;; Note that buffer already is in dired-mode, if found.
 	 (new-buffer-p (null buffer)))
     (or buffer
-        (setq buffer (create-file-buffer (directory-file-name dirname))))
+        (setq buffer (create-file-buffer dirname)))
     (set-buffer buffer)
     (if (not new-buffer-p)		; existing buffer ...
 	(cond (switches			; ... but new switches
diff --git a/lisp/files.el b/lisp/files.el
index d325729bf4d..f1b3b6be4f4 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2062,22 +2062,29 @@ find-alternate-file
 	  (kill-buffer obuf))))))
 \f
 ;; FIXME we really need to fold the uniquify stuff in here by default,
-;; not using advice, and add it to the doc string.
 (defun create-file-buffer (filename)
   "Create a suitably named buffer for visiting FILENAME, and return it.
 FILENAME (sans directory) is used unchanged if that name is free;
-otherwise a string <2> or <3> or ... is appended to get an unused name.
+otherwise the buffer is renamed according to
+`uniquify-buffer-name-style' to get an unused name.
 
 Emacs treats buffers whose names begin with a space as internal buffers.
 To avoid confusion when visiting a file whose name begins with a space,
 this function prepends a \"|\" to the final result if necessary."
-  (let* ((lastname (file-name-nondirectory filename))
-	 (lastname (if (string= lastname "")
-	               filename lastname))
-	 (buf (generate-new-buffer (if (string-prefix-p " " lastname)
-			               (concat "|" lastname)
-			             lastname))))
-    (uniquify--create-file-buffer-advice buf filename)
+  (let* ((lastname (file-name-nondirectory (directory-file-name filename)))
+         (lastname (cond
+                    ((not (and uniquify-trailing-separator-p (directory-name-p filename)))
+                     lastname)
+                    ((eq uniquify-buffer-name-style 'forward)
+	             (file-name-as-directory lastname))
+	            ((eq uniquify-buffer-name-style 'reverse)
+	             (concat (or uniquify-separator "\\") lastname))
+                    (t lastname)))
+         (basename (if (string-prefix-p " " lastname)
+		       (concat "|" lastname)
+		     lastname))
+	 (buf (generate-new-buffer basename)))
+    (uniquify--create-file-buffer-advice buf filename basename)
     buf))
 
 (defvar abbreviated-home-dir nil
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index dee9ecba2ea..d1ca455b673 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -174,8 +174,8 @@ uniquify-list-buffers-directory-modes
 (cl-defstruct (uniquify-item
 	    (:constructor nil) (:copier nil)
 	    (:constructor uniquify-make-item
-	     (base dirname buffer &optional proposed original-dirname)))
-  base dirname buffer proposed original-dirname)
+	     (base dirname buffer &optional proposed)))
+  base dirname buffer proposed)
 
 ;; Internal variables used free
 (defvar uniquify-possibly-resolvable nil)
@@ -211,7 +211,7 @@ uniquify-rationalize-file-buffer-names
   (when dirname
     (setq dirname (expand-file-name (directory-file-name dirname)))
     (let ((fix-list (list (uniquify-make-item base dirname newbuf
-                                              nil dirname)))
+                                              nil)))
 	  items)
       (dolist (buffer (buffer-list))
 	(when (and (not (and uniquify-ignore-buffers-re
@@ -292,8 +292,7 @@ uniquify-rationalize
       (setf (uniquify-item-proposed item)
 	    (uniquify-get-proposed-name (uniquify-item-base item)
 					(uniquify-item-dirname item)
-                                        nil
-                                        (uniquify-item-original-dirname item)))
+                                        nil))
       (setq uniquify-managed fix-list)))
   ;; Strip any shared last directory names of the dirname.
   (when (and (cdr fix-list) uniquify-strip-common-suffix)
@@ -316,8 +315,7 @@ uniquify-rationalize
 					      (uniquify-item-dirname item))))
 				      (and f (directory-file-name f)))
 				    (uniquify-item-buffer item)
-				    (uniquify-item-proposed item)
-                                    (uniquify-item-original-dirname item))
+				    (uniquify-item-proposed item))
 		fix-list)))))
   ;; If uniquify-min-dir-content is 0, this will end up just
   ;; passing fix-list to uniquify-rationalize-conflicting-sublist.
@@ -345,21 +343,10 @@ uniquify-rationalize-a-list
     (uniquify-rationalize-conflicting-sublist conflicting-sublist
 					      old-proposed depth)))
 
-(defun uniquify-get-proposed-name (base dirname &optional depth
-                                        original-dirname)
+(defun uniquify-get-proposed-name (base dirname &optional depth)
   (unless depth (setq depth uniquify-min-dir-content))
   (cl-assert (equal (directory-file-name dirname) dirname)) ;No trailing slash.
 
-  ;; Distinguish directories by adding extra separator.
-  (if (and uniquify-trailing-separator-p
-	   (file-directory-p (expand-file-name base original-dirname))
-	   (not (string-equal base "")))
-      (cond ((eq uniquify-buffer-name-style 'forward)
-	     (setq base (file-name-as-directory base)))
-	    ;; (setq base (concat base "/")))
-	    ((eq uniquify-buffer-name-style 'reverse)
-	     (setq base (concat (or uniquify-separator "\\") base)))))
-
   (let ((extra-string nil)
 	(n depth))
     (while (and (> n 0) dirname)
@@ -421,8 +408,7 @@ uniquify-rationalize-conflicting-sublist
 		  (uniquify-get-proposed-name
 		   (uniquify-item-base item)
 		   (uniquify-item-dirname item)
-		   depth
-                   (uniquify-item-original-dirname item))))
+		   depth)))
 	  (uniquify-rationalize-a-list conf-list depth))
       (unless (string= old-name "")
 	(uniquify-rename-buffer (car conf-list) old-name)))))
@@ -492,15 +478,14 @@ uniquify--rename-buffer-advice
 
 
 ;; (advice-add 'create-file-buffer :around #'uniquify--create-file-buffer-advice)
-(defun uniquify--create-file-buffer-advice (buf filename)
+(defun uniquify--create-file-buffer-advice (buf filename basename)
   ;; BEWARE: This is called directly from `files.el'!
   "Uniquify buffer names with parts of directory name."
   (when uniquify-buffer-name-style
-    (let ((filename (expand-file-name (directory-file-name filename))))
-      (uniquify-rationalize-file-buffer-names
-       (file-name-nondirectory filename)
-       (file-name-directory filename)
-       buf))))
+    (uniquify-rationalize-file-buffer-names
+     basename
+     (file-name-directory (expand-file-name (directory-file-name filename)))
+     buf)))
 
 (defun uniquify-unload-function ()
   "Unload the uniquify library."
diff --git a/test/lisp/uniquify-tests.el b/test/lisp/uniquify-tests.el
new file mode 100644
index 00000000000..abd61fa3504
--- /dev/null
+++ b/test/lisp/uniquify-tests.el
@@ -0,0 +1,129 @@
+;;; uniquify-tests.el --- Tests for uniquify         -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; Author: Spencer Baugh <sbaugh@janestreet.com>
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest uniquify-basic ()
+  (let (bufs old-names)
+    (cl-flet ((names-are (current-names &optional nosave)
+                (should (equal (mapcar #'buffer-name bufs) current-names))
+                (unless nosave (push current-names old-names))))
+      (should (eq (get-buffer "z") nil))
+      (push (find-file-noselect "a/b/z") bufs)
+      (names-are '("z"))
+      (push (find-file-noselect "a/b/c/z") bufs)
+      (names-are '("z<c>" "z<b>"))
+      (push (find-file-noselect "a/b/d/z") bufs)
+      (names-are '("z<d>" "z<c>" "z<b>"))
+      (push (find-file-noselect "e/b/z") bufs)
+      (names-are '("z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; buffers without a buffer-file-name don't get uniquified by uniquify
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; but they do get uniquified by the C code which uses <n>
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      (save-excursion
+        ;; uniquify will happily work with file-visiting buffers whose names don't match buffer-file-name
+        (find-file "f/y")
+        (push (current-buffer) bufs)
+        (rename-buffer "z" t)
+        (names-are '("z<f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave)
+        ;; somewhat confusing behavior results if a buffer is renamed to match an already-uniquified buffer
+        (rename-buffer "z<a/b>" t)
+        (names-are '("z<a/b><f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave))
+      (while bufs
+        (kill-buffer (pop bufs))
+        (names-are (pop old-names) 'nosave)))))
+
+(ert-deftest uniquify-dirs ()
+  "Check strip-common-suffix and trailing-separator-p work together; bug#47132"
+  (let* ((root (make-temp-file "emacs-uniquify-tests" 'dir))
+         (a-path (file-name-concat root "a/x/y/dir"))
+         (b-path (file-name-concat root "b/x/y/dir")))
+    (make-directory a-path 'parents)
+    (make-directory b-path 'parents)
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p nil))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir" "b/dir")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix nil)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/x/y/dir/" "b/x/y/dir/")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir/" "b/dir/")))
+        (mapc #'kill-buffer bufs)))))
+
+(ert-deftest uniquify-rename-to-dir ()
+  "Giving a buffer a name which matches a directory doesn't rename the buffer"
+  (let ((uniquify-buffer-name-style 'forward)
+        (uniquify-trailing-separator-p t))
+      (save-excursion
+        (find-file "../README")
+        (rename-buffer "lisp" t)
+        (should (equal (buffer-name) "lisp"))
+        (kill-buffer))))
+
+(ert-deftest uniquify-separator-style-reverse ()
+  (let ((uniquify-buffer-name-style 'reverse)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "\\lib-src"))
+      (kill-buffer))))
+
+(ert-deftest uniquify-separator-ignored ()
+  "If uniquify-buffer-name-style isn't forward or reverse,
+uniquify-trailing-separator-p is ignored"
+  (let ((uniquify-buffer-name-style 'post-forward-angle-brackets)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "lib-src"))
+      (kill-buffer))))
+
+(ert-deftest uniquify-space-prefix ()
+  "If a buffer starts with a space, | is added at the start"
+  (save-excursion
+    (find-file " foo")
+    (should (equal (buffer-name) "| foo"))
+    (kill-buffer)))
+
+(provide 'uniquify-tests)
+;;; uniquify-tests.el ends here
-- 
2.41.0

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

>> and since we know (directory-name-p filename), I'd be tempted to replace
>> (file-name-as-directory lastname) with just `filename`.
>
> (file-name-as-directory lastname) and filename are different though: the
> former is only the last component of filename, but as a directory name.

Duh!  You're right.

> I agree that would be good (modulo that it does need to be
> (file-name-as-directory lastname) rather than filename), but it would
> change the existing behavior and not comply with the docstring of
> `uniquify-trailing-separator-p'.  (I think it would be a reasonable
> change though, but probably as a subsequent change)

Good point.


        Stefan

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 62732@debbugs.gnu.org, sbaugh@catern.com
> Date: Sun, 09 Jul 2023 22:04:54 -0400
> 
> >>> This inspired me to do something not exactly this but which also gets
> >>> rid of the new argument to create-file-buffer.  How about this:
> >>
> >> Now you're talking!  :-)
> >> LGTM!  Thank you very much,
> >
> > Great!  Here's that as a complete patch again.
> 
> Eli, OK to install on `master`?

Yes, but see my comments to the patch.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> Cc: 62732@debbugs.gnu.org
> From: sbaugh@catern.com
> Date: Mon, 10 Jul 2023 01:36:00 +0000 (UTC)
> 
> Great!  Here's that as a complete patch again.

> --- a/lisp/dired.el
> +++ b/lisp/dired.el
> @@ -1306,7 +1306,7 @@ dired-internal-noselect
>  	 ;; Note that buffer already is in dired-mode, if found.
>  	 (new-buffer-p (null buffer)))
>      (or buffer
> -        (setq buffer (create-file-buffer (directory-file-name dirname))))
> +        (setq buffer (create-file-buffer dirname)))

This seems to imply that callers of create-file-buffer will now have
to remember to ensure the argument ends in a slash if it is the name
of a directory.  If so, I'd prefer that create-file-buffer did that
internally, when its argument is a directory.  Callers shouldn't know
to much about the internals of the callee.

Does this changeset have any user-facing behavior changes?  If so,
they should be at least in NEWS.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 62732@debbugs.gnu.org
>> From: sbaugh@catern.com
>> Date: Mon, 10 Jul 2023 01:36:00 +0000 (UTC)
>> 
>> Great!  Here's that as a complete patch again.
>
>> --- a/lisp/dired.el
>> +++ b/lisp/dired.el
>> @@ -1306,7 +1306,7 @@ dired-internal-noselect
>>  	 ;; Note that buffer already is in dired-mode, if found.
>>  	 (new-buffer-p (null buffer)))
>>      (or buffer
>> -        (setq buffer (create-file-buffer (directory-file-name dirname))))
>> +        (setq buffer (create-file-buffer dirname)))
>
> This seems to imply that callers of create-file-buffer will now have
> to remember to ensure the argument ends in a slash if it is the name
> of a directory.  If so, I'd prefer that create-file-buffer did that
> internally, when its argument is a directory.  Callers shouldn't know
> to much about the internals of the callee.

I can (and should) add this to the docstring of create-file-buffer.  It
seems intuitive to me that the last non-empty component of the filename
passed in by the caller is what create-file-buffer uses, including if
that "last component" ends in a slash.  (It's a nice way to avoid the
additional DIRECTORY argument which says whether the filename is
intended to refer to a directory)

By doing this internally in create-file-buffer, you mean running
file-directory-p to see if the filename actually points to an existing
directory?  I'm hesitant to do that:

- That prevents running create-file-buffer to create a buffer to visit a
directory which does not yet exist (in the same way you can visit a file
which does not yet exist).  dired doesn't currently support that but
other packages might want to.

- Checking file-directory-p is what uniquify did which caused these bugs
in the first place, and I think this could partially recreate the same
bug, where we add a trailing slash just because there happens to be a
directory of the right name.  (Although I'm not sure, just worried)

- It adds filesystem access to what is currently a pure function.

> Does this changeset have any user-facing behavior changes?  If so,
> they should be at least in NEWS.

The only user-facing behavior change is fixing the two bugs mentioned in
the commit message.  Is that appropriate to include in NEWS?

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: sbaugh@catern.com,  monnier@iro.umontreal.ca,  62732@debbugs.gnu.org
> Date: Mon, 10 Jul 2023 09:39:10 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> -        (setq buffer (create-file-buffer (directory-file-name dirname))))
> >> +        (setq buffer (create-file-buffer dirname)))
> >
> > This seems to imply that callers of create-file-buffer will now have
> > to remember to ensure the argument ends in a slash if it is the name
> > of a directory.  If so, I'd prefer that create-file-buffer did that
> > internally, when its argument is a directory.  Callers shouldn't know
> > to much about the internals of the callee.
> 
> I can (and should) add this to the docstring of create-file-buffer.  It
> seems intuitive to me that the last non-empty component of the filename
> passed in by the caller is what create-file-buffer uses, including if
> that "last component" ends in a slash.  (It's a nice way to avoid the
> additional DIRECTORY argument which says whether the filename is
> intended to refer to a directory)

If the only reason is to avoid an additional argument, I'd prefer an
additional argument.  Documenting a problematic design doesn't mean
the design ceases to be problematic, and relies too heavily on people
paying attention to subtle aspects of the documented behavior.

> By doing this internally in create-file-buffer, you mean running
> file-directory-p to see if the filename actually points to an existing
> directory?  I'm hesitant to do that:
> 
> - That prevents running create-file-buffer to create a buffer to visit a
> directory which does not yet exist (in the same way you can visit a file
> which does not yet exist).  dired doesn't currently support that but
> other packages might want to.

Didn't that problem exist before your changes?

And anyway, if we want to support that, adding an extra variable, or
even requiring the trailing slash only for non-existing directories,
would be a better solution.

> - Checking file-directory-p is what uniquify did which caused these bugs
> in the first place, and I think this could partially recreate the same
> bug, where we add a trailing slash just because there happens to be a
> directory of the right name.  (Although I'm not sure, just worried)

I don't see why that would follow.  It is a very minor change in the
code, and doesn't affect the logic, AFAICT.

> - It adds filesystem access to what is currently a pure function.

But create-file-buffer is not documented as not hitting the disk, so I
don't see a problem here.

> > Does this changeset have any user-facing behavior changes?  If so,
> > they should be at least in NEWS.
> 
> The only user-facing behavior change is fixing the two bugs mentioned in
> the commit message.  Is that appropriate to include in NEWS?

Does the fix result in different buffer names?  If so, it is not just
a bugfix, it changes user-facing behavior.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

>> --- a/lisp/dired.el
>> +++ b/lisp/dired.el
>> @@ -1306,7 +1306,7 @@ dired-internal-noselect
>>  	 ;; Note that buffer already is in dired-mode, if found.
>>  	 (new-buffer-p (null buffer)))
>>      (or buffer
>> -        (setq buffer (create-file-buffer (directory-file-name dirname))))
>> +        (setq buffer (create-file-buffer dirname)))
>
> This seems to imply that callers of create-file-buffer will now have
> to remember to ensure the argument ends in a slash if it is the name
> of a directory.

Which callers are you thinking of?

I think the fact that the callers get to control this regardless of
whether there is a file or directory by that name is one of the best
part of this change.

> If so, I'd prefer that create-file-buffer did that internally, when
> its argument is a directory.

What would be the benefit?

> Callers shouldn't know to much about the internals of the callee.

Indeed: currently `create-file-buffer` doesn't pay attention to the file
system at all, it just creates a buffer with a name based on the
FILENAME that's passed.  Spencer's patch just offers more control to the
callers by making `create-file-buffer` respect the choice of the callers
(whether they used a file name or a dire name, which is an important
distinction in Emacs's file name APIs, not just here).

There's no need for the callers to know about the internals of
the callee.  If they call `create-file-buffer` with /foo/bar/baz the
buffer will be called "baz" and if they call it with /foo/bar/baz/ the
buffer will be called "baz/" (depending on
`uniquify-trailing-separator-p`, of course).
It's the most natural/obvious semantics.


        Stefan

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: sbaugh@catern.com,  62732@debbugs.gnu.org
> Date: Mon, 10 Jul 2023 12:53:00 -0400
> 
> > Callers shouldn't know to much about the internals of the callee.
> 
> Indeed: currently `create-file-buffer` doesn't pay attention to the file
> system at all, it just creates a buffer with a name based on the
> FILENAME that's passed.  Spencer's patch just offers more control to the
> callers by making `create-file-buffer` respect the choice of the callers
> (whether they used a file name or a dire name, which is an important
> distinction in Emacs's file name APIs, not just here).
> 
> There's no need for the callers to know about the internals of
> the callee.  If they call `create-file-buffer` with /foo/bar/baz the
> buffer will be called "baz" and if they call it with /foo/bar/baz/ the
> buffer will be called "baz/" (depending on
> `uniquify-trailing-separator-p`, of course).
> It's the most natural/obvious semantics.

Wasn't the fact that the trailing slash was absent part of the reason
for the bug this tries to fix?  If so, then this is not just "if you
want it, use it", is it?

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

>> > Callers shouldn't know to much about the internals of the callee.
>> 
>> Indeed: currently `create-file-buffer` doesn't pay attention to the file
>> system at all, it just creates a buffer with a name based on the
>> FILENAME that's passed.  Spencer's patch just offers more control to the
>> callers by making `create-file-buffer` respect the choice of the callers
>> (whether they used a file name or a dire name, which is an important
>> distinction in Emacs's file name APIs, not just here).
>> 
>> There's no need for the callers to know about the internals of
>> the callee.  If they call `create-file-buffer` with /foo/bar/baz the
>> buffer will be called "baz" and if they call it with /foo/bar/baz/ the
>> buffer will be called "baz/" (depending on
>> `uniquify-trailing-separator-p`, of course).
>> It's the most natural/obvious semantics.
>
> Wasn't the fact that the trailing slash was absent part of the reason
> for the bug this tries to fix?  If so, then this is not just "if you
> want it, use it", is it?

No, `create-file-buffer` used to throw away the trailing slash, rather
than make use of this information.  Not sure why Dired bothered to
remove the tailing slash when calling it, maybe because a long time ago
`create-file-buffer` had a bug if the name had a trailing slash.


        Stefan

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: sbaugh@catern.com,  62732@debbugs.gnu.org
> Date: Mon, 10 Jul 2023 15:18:48 -0400
> 
> >> > Callers shouldn't know to much about the internals of the callee.
> >> 
> >> Indeed: currently `create-file-buffer` doesn't pay attention to the file
> >> system at all, it just creates a buffer with a name based on the
> >> FILENAME that's passed.  Spencer's patch just offers more control to the
> >> callers by making `create-file-buffer` respect the choice of the callers
> >> (whether they used a file name or a dire name, which is an important
> >> distinction in Emacs's file name APIs, not just here).
> >> 
> >> There's no need for the callers to know about the internals of
> >> the callee.  If they call `create-file-buffer` with /foo/bar/baz the
> >> buffer will be called "baz" and if they call it with /foo/bar/baz/ the
> >> buffer will be called "baz/" (depending on
> >> `uniquify-trailing-separator-p`, of course).
> >> It's the most natural/obvious semantics.
> >
> > Wasn't the fact that the trailing slash was absent part of the reason
> > for the bug this tries to fix?  If so, then this is not just "if you
> > want it, use it", is it?
> 
> No, `create-file-buffer` used to throw away the trailing slash, rather
> than make use of this information.  Not sure why Dired bothered to
> remove the tailing slash when calling it, maybe because a long time ago
> `create-file-buffer` had a bug if the name had a trailing slash.

So why the need for the change in dired.el?

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

>> No, `create-file-buffer` used to throw away the trailing slash, rather
>> than make use of this information.

[ And instead uniquify had to try and recover that information by checking
  the file-system.  ]

>> Not sure why Dired bothered to remove the tailing slash when calling
>> it, maybe because a long time ago `create-file-buffer` had a bug if
>> the name had a trailing slash.
> So why the need for the change in dired.el?

Because we do want Dired to tell `create-file-buffer` that this is
a directory and it should thus obey `uniquify-trailing-separator-p`.

Otherwise `uniquify-trailing-separator-p` would end up never used (since
Dired is AFAIK the only package that creates "directory file buffers").


        Stefan

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: sbaugh@catern.com,  62732@debbugs.gnu.org
> Date: Mon, 10 Jul 2023 22:55:23 -0400
> 
> >> No, `create-file-buffer` used to throw away the trailing slash, rather
> >> than make use of this information.
> 
> [ And instead uniquify had to try and recover that information by checking
>   the file-system.  ]
> 
> >> Not sure why Dired bothered to remove the tailing slash when calling
> >> it, maybe because a long time ago `create-file-buffer` had a bug if
> >> the name had a trailing slash.
> > So why the need for the change in dired.el?
> 
> Because we do want Dired to tell `create-file-buffer` that this is
> a directory and it should thus obey `uniquify-trailing-separator-p`.

When will we NOT want to tell create-file-buffer that the file is a
directory?  Your original response, viz.:

> I think the fact that the callers get to control this regardless of
> whether there is a file or directory by that name is one of the best
> part of this change.

seemed to indicate that there are cases where we would not want
create-file-buffer to know that, but I suspect that we will always
want, because otherwise uniquify will not work in those cases, and
Spencer will report a bug.

My comments assumed that indeed we will (almost) always want to tell
create-file-buffer this is a directory.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: sbaugh@catern.com,  62732@debbugs.gnu.org
>> Date: Mon, 10 Jul 2023 22:55:23 -0400
>> 
>> >> No, `create-file-buffer` used to throw away the trailing slash, rather
>> >> than make use of this information.
>> 
>> [ And instead uniquify had to try and recover that information by checking
>>   the file-system.  ]
>> 
>> >> Not sure why Dired bothered to remove the tailing slash when calling
>> >> it, maybe because a long time ago `create-file-buffer` had a bug if
>> >> the name had a trailing slash.
>> > So why the need for the change in dired.el?
>> 
>> Because we do want Dired to tell `create-file-buffer` that this is
>> a directory and it should thus obey `uniquify-trailing-separator-p`.
>
> When will we NOT want to tell create-file-buffer that the file is a
> directory?  Your original response, viz.:
>
>> I think the fact that the callers get to control this regardless of
>> whether there is a file or directory by that name is one of the best
>> part of this change.
>
> seemed to indicate that there are cases where we would not want
> create-file-buffer to know that, but I suspect that we will always
> want, because otherwise uniquify will not work in those cases, and
> Spencer will report a bug.
>
> My comments assumed that indeed we will (almost) always want to tell
> create-file-buffer this is a directory.

One contribution, not intended to be exhaustive of all use cases, and
not intended to be definitively a good idea: a user could want opened
tar files with their file listing view to have a trailing slash, even
though they aren't actually directories.

And with my approach that is possibly just by running
file-name-as-directory over the name before passing it to
create-file-buffer.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  sbaugh@catern.com,
>    62732@debbugs.gnu.org
> Date: Tue, 11 Jul 2023 08:31:51 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > My comments assumed that indeed we will (almost) always want to tell
> > create-file-buffer this is a directory.
> 
> One contribution, not intended to be exhaustive of all use cases, and
> not intended to be definitively a good idea: a user could want opened
> tar files with their file listing view to have a trailing slash, even
> though they aren't actually directories.

But users don't call create-file-buffer, do they?  So this is not
user-level option, at least not directly so.

> And with my approach that is possibly just by running
> file-name-as-directory over the name before passing it to
> create-file-buffer.

If you worry about users, they can be told to append a slash by hand,
when they mean a directory and that directory does not yet exist.  We
do this elsewhere in Emacs.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

I'm not sure what to make of this discussion.

The issue at hand is the following: `create-file-buffer` needs to know
if the filename it receives is for a directory or not so it can decide
whether the buffer name should end in / or not according to
`uniquify-trailing-separator-p`.

I can see 3 ways to provide this info:

1- use `file-directory-p`.
2- add a boolean `directory` argument to `create-file-buffer`.
3- use the presence of a trailing directory separator in the filename.

Those 3 are very close to each other, in practice, so we're pretty much
in bikeshed territory.

My preference is (3) first, (2) second, and (1) last.

(1) is my least favorite because it makes it impossible/difficult to
create a "directory buffer" if `file-directory-p` returns nil and
vice versa, even tho I can imagine scenarios where this could be useful
(such as the scenario mentioned by Spencer where we want to create
a "directory buffer" for an archive that Emacs's file-name-handlers
don't understand).

(3) is my favorite because it doesn't need an extra argument and instead
uses an existing piece of information: if I pass "/a/b/c/" then it seems
clear that I mean this to be a "directory buffer" rather than a "file
buffer".  Representing the information "this is meant to be a directory"
in the file name via a trailing / is a standard practice in ELisp (and
POSIX in general), embodied by things like `file-name-as-directory` and
`directory-file-name`, so it seems only natural (or even a mere a bug
fix) to let `create-file-buffer` make use of that info instead of
throwing it away.

But I prefer any one of those 3 choices over the status quo, so I'll
stop arguing here.  Just let me know which one I should install.


        Stefan


Eli Zaretskii [2023-07-11 18:31:07] wrote:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  sbaugh@catern.com,
>>    62732@debbugs.gnu.org
>> Date: Tue, 11 Jul 2023 08:31:51 -0400
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> > My comments assumed that indeed we will (almost) always want to tell
>> > create-file-buffer this is a directory.
>> 
>> One contribution, not intended to be exhaustive of all use cases, and
>> not intended to be definitively a good idea: a user could want opened
>> tar files with their file listing view to have a trailing slash, even
>> though they aren't actually directories.
>
> But users don't call create-file-buffer, do they?  So this is not
> user-level option, at least not directly so.
>
>> And with my approach that is possibly just by running
>> file-name-as-directory over the name before passing it to
>> create-file-buffer.
>
> If you worry about users, they can be told to append a slash by hand,
> when they mean a directory and that directory does not yet exist.  We
> do this elsewhere in Emacs.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Spencer Baugh <sbaugh@janestreet.com>,  sbaugh@catern.com,
>   62732@debbugs.gnu.org
> Date: Wed, 12 Jul 2023 09:04:40 -0400
> 
> I'm not sure what to make of this discussion.
> 
> The issue at hand is the following: `create-file-buffer` needs to know
> if the filename it receives is for a directory or not so it can decide
> whether the buffer name should end in / or not according to
> `uniquify-trailing-separator-p`.
> 
> I can see 3 ways to provide this info:
> 
> 1- use `file-directory-p`.
> 2- add a boolean `directory` argument to `create-file-buffer`.
> 3- use the presence of a trailing directory separator in the filename.
> 
> Those 3 are very close to each other, in practice, so we're pretty much
> in bikeshed territory.
> 
> My preference is (3) first, (2) second, and (1) last.

I prefer (1), because it avoids requesting the callers to remember to
ensure that every directory ends in a slash.

The trailing-slash semantics is indeed pretty much standard, but only
in interactive usage (where it is made easier by the file-name
completion machinery, both in Emacs and in other programs that ask
users to type file names).  And even in interactive usage it is
problematic: recall the many complaints when we started requiring the
slash in copy-file and such likes.  Here we are talking about a
low-level function, not an interactive command, which then places this
burden on the callers, and I worry that many of them will not pay
attention to this subtlety, and will cause subtle bugs, because AFAIK
the uniquify modes where that is important are rarely used, and thus
such problems could go undetected for many years.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Spencer Baugh <sbaugh@janestreet.com>,  sbaugh@catern.com,
>>   62732@debbugs.gnu.org
>> Date: Wed, 12 Jul 2023 09:04:40 -0400
>> 
>> I'm not sure what to make of this discussion.
>> 
>> The issue at hand is the following: `create-file-buffer` needs to know
>> if the filename it receives is for a directory or not so it can decide
>> whether the buffer name should end in / or not according to
>> `uniquify-trailing-separator-p`.
>> 
>> I can see 3 ways to provide this info:
>> 
>> 1- use `file-directory-p`.
>> 2- add a boolean `directory` argument to `create-file-buffer`.
>> 3- use the presence of a trailing directory separator in the filename.
>> 
>> Those 3 are very close to each other, in practice, so we're pretty much
>> in bikeshed territory.
>> 
>> My preference is (3) first, (2) second, and (1) last.
>
> I prefer (1), because it avoids requesting the callers to remember to
> ensure that every directory ends in a slash.
>
> The trailing-slash semantics is indeed pretty much standard, but only
> in interactive usage (where it is made easier by the file-name
> completion machinery, both in Emacs and in other programs that ask
> users to type file names).  And even in interactive usage it is
> problematic: recall the many complaints when we started requiring the
> slash in copy-file and such likes.  Here we are talking about a
> low-level function, not an interactive command, which then places this
> burden on the callers, and I worry that many of them will not pay
> attention to this subtlety, and will cause subtle bugs, because AFAIK
> the uniquify modes where that is important are rarely used, and thus
> such problems could go undetected for many years.

Not to prelong the discussion any further, but one more detail: This
uniquify-trailing-separator-p variable really IMO should be a dired
variable, since it only affects dired buffer naming (at least in core
Emacs, for now).  This behavior has really nothing to do with uniquify.
So IMO it should never have been a uniquify variable in the first place,
and in an earlier version of my patch I moved the variable to dired and
obsoleted the old one.

I dropped that move because it wasn't particularly necessary and could
be done in a followup, but of these three options, 2 and 3 work much
better with moving this variable to dired, because only 2 and 3 let
dired control this without affecting other packages.  Option 1 forces
the same behavior on every package.  I guess we could still have that as
a dired variable, but it seems weirder.

(As a dired variable, it could control just "does dired pass a directory
name or a file name to create-file-buffer?", and create-file-buffer
could always include the trailing slash when it sees a directory name)

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

> Not to prelong the discussion any further, but one more detail: This
> uniquify-trailing-separator-p variable really IMO should be a dired
> variable, since it only affects dired buffer naming (at least in core
> Emacs, for now).  This behavior has really nothing to do with uniquify.
> So IMO it should never have been a uniquify variable in the first place,
> and in an earlier version of my patch I moved the variable to dired and
> obsoleted the old one.

FWIW, I agree, and I'd also recommend to make this variable default to t.


        Stefan

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> Cc: sbaugh@janestreet.com, 62732@debbugs.gnu.org, sbaugh@catern.com
> Date: Wed, 12 Jul 2023 16:42:01 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > I can see 3 ways to provide this info:
> > 
> > 1- use `file-directory-p`.
> > 2- add a boolean `directory` argument to `create-file-buffer`.
> > 3- use the presence of a trailing directory separator in the filename.
> > 
> > Those 3 are very close to each other, in practice, so we're pretty much
> > in bikeshed territory.
> > 
> > My preference is (3) first, (2) second, and (1) last.
> 
> I prefer (1), because it avoids requesting the callers to remember to
> ensure that every directory ends in a slash.

So how about compromising on a variant of (2): we add an optional
DIRECTORY-P argument, and if FILENAME doesn't end in a slash, but
DIRECTORY-P is non-nil, create-file-buffer will append a slash?

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: sbaugh@janestreet.com, 62732@debbugs.gnu.org, sbaugh@catern.com
>> Date: Wed, 12 Jul 2023 16:42:01 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>> > I can see 3 ways to provide this info:
>> > 
>> > 1- use `file-directory-p`.
>> > 2- add a boolean `directory` argument to `create-file-buffer`.
>> > 3- use the presence of a trailing directory separator in the filename.
>> > 
>> > Those 3 are very close to each other, in practice, so we're pretty much
>> > in bikeshed territory.
>> > 
>> > My preference is (3) first, (2) second, and (1) last.
>> 
>> I prefer (1), because it avoids requesting the callers to remember to
>> ensure that every directory ends in a slash.
>
> So how about compromising on a variant of (2): we add an optional
> DIRECTORY-P argument, and if FILENAME doesn't end in a slash, but
> DIRECTORY-P is non-nil, create-file-buffer will append a slash?

Okay, so like this?

BTW, would you be okay with moving uniquify-trailing-separator-p into
dired, as I described in my other recent email?  Then create-file-buffer
wouldn't need to check it, which would simplify its docstring slightly;
instead dired would just decide whether to pass a directory name or file
name based on uniquify-trailing-separator-p.  Since I'm changing this
area anyway, now would be the time to make that change, as a nice
cleanup which Stefan also likes.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-recalculate-the-buffer-basename-inside-uniquif.patch --]
[-- Type: text/x-patch, Size: 14093 bytes --]

From 023b8e7a715374e59a5456075b98d1422659cfe6 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Sun, 9 Jul 2023 10:24:33 -0400
Subject: [PATCH] Don't recalculate the buffer basename inside uniquify

Previously, uniquify--create-file-buffer-advice would use the filename
of the buffer to calculate what the buffer's basename should be.  Now
that gets passed in from create-file-buffer, which lets us fix several
bugs:

1. before this patch, if a buffer happened to be named the same thing
as directory in its default-directory, the buffer would get renamed
with a directory separator according to uniquify-trailing-separator-p.

2. buffers with a leading space should get a leading |, as described
by create-file-buffer's docstring; before this patch, uniquify would
remove that leading |.

* lisp/dired.el (dired-internal-noselect): Pass a directory name to
create-file-buffer.
* lisp/files.el (create-file-buffer): Do uniquify-trailing-separator-p
handling if passed a directory filename. (bug#62732)
* lisp/uniquify.el (uniquify-item):
(uniquify-rationalize-file-buffer-names, uniquify-rationalize,
uniquify-get-proposed-name, uniquify-rationalize-conflicting-sublist):
Remove uniquify-trailing-separator-p handling.
(uniquify--create-file-buffer-advice): Take new basename argument and
use it, instead of recalculating the basename from the filename.
---
 lisp/dired.el               |   2 +-
 lisp/files.el               |  25 ++++---
 lisp/uniquify.el            |  39 ++++-------
 test/lisp/uniquify-tests.el | 129 ++++++++++++++++++++++++++++++++++++
 4 files changed, 158 insertions(+), 37 deletions(-)
 create mode 100644 test/lisp/uniquify-tests.el

diff --git a/lisp/dired.el b/lisp/dired.el
index d14cf47ffd5..3c9e6e40f9b 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1306,7 +1306,7 @@ dired-internal-noselect
 	 ;; Note that buffer already is in dired-mode, if found.
 	 (new-buffer-p (null buffer)))
     (or buffer
-        (setq buffer (create-file-buffer (directory-file-name dirname))))
+        (setq buffer (create-file-buffer dirname)))
     (set-buffer buffer)
     (if (not new-buffer-p)		; existing buffer ...
 	(cond (switches			; ... but new switches
diff --git a/lisp/files.el b/lisp/files.el
index d325729bf4d..f1b3b6be4f4 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2062,22 +2062,29 @@ find-alternate-file
 	  (kill-buffer obuf))))))
 \f
 ;; FIXME we really need to fold the uniquify stuff in here by default,
-;; not using advice, and add it to the doc string.
 (defun create-file-buffer (filename)
   "Create a suitably named buffer for visiting FILENAME, and return it.
 FILENAME (sans directory) is used unchanged if that name is free;
-otherwise a string <2> or <3> or ... is appended to get an unused name.
+otherwise the buffer is renamed according to
+`uniquify-buffer-name-style' to get an unused name.
 
 Emacs treats buffers whose names begin with a space as internal buffers.
 To avoid confusion when visiting a file whose name begins with a space,
 this function prepends a \"|\" to the final result if necessary."
-  (let* ((lastname (file-name-nondirectory filename))
-	 (lastname (if (string= lastname "")
-	               filename lastname))
-	 (buf (generate-new-buffer (if (string-prefix-p " " lastname)
-			               (concat "|" lastname)
-			             lastname))))
-    (uniquify--create-file-buffer-advice buf filename)
+  (let* ((lastname (file-name-nondirectory (directory-file-name filename)))
+         (lastname (cond
+                    ((not (and uniquify-trailing-separator-p (directory-name-p filename)))
+                     lastname)
+                    ((eq uniquify-buffer-name-style 'forward)
+	             (file-name-as-directory lastname))
+	            ((eq uniquify-buffer-name-style 'reverse)
+	             (concat (or uniquify-separator "\\") lastname))
+                    (t lastname)))
+         (basename (if (string-prefix-p " " lastname)
+		       (concat "|" lastname)
+		     lastname))
+	 (buf (generate-new-buffer basename)))
+    (uniquify--create-file-buffer-advice buf filename basename)
     buf))
 
 (defvar abbreviated-home-dir nil
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index dee9ecba2ea..d1ca455b673 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -174,8 +174,8 @@ uniquify-list-buffers-directory-modes
 (cl-defstruct (uniquify-item
 	    (:constructor nil) (:copier nil)
 	    (:constructor uniquify-make-item
-	     (base dirname buffer &optional proposed original-dirname)))
-  base dirname buffer proposed original-dirname)
+	     (base dirname buffer &optional proposed)))
+  base dirname buffer proposed)
 
 ;; Internal variables used free
 (defvar uniquify-possibly-resolvable nil)
@@ -211,7 +211,7 @@ uniquify-rationalize-file-buffer-names
   (when dirname
     (setq dirname (expand-file-name (directory-file-name dirname)))
     (let ((fix-list (list (uniquify-make-item base dirname newbuf
-                                              nil dirname)))
+                                              nil)))
 	  items)
       (dolist (buffer (buffer-list))
 	(when (and (not (and uniquify-ignore-buffers-re
@@ -292,8 +292,7 @@ uniquify-rationalize
       (setf (uniquify-item-proposed item)
 	    (uniquify-get-proposed-name (uniquify-item-base item)
 					(uniquify-item-dirname item)
-                                        nil
-                                        (uniquify-item-original-dirname item)))
+                                        nil))
       (setq uniquify-managed fix-list)))
   ;; Strip any shared last directory names of the dirname.
   (when (and (cdr fix-list) uniquify-strip-common-suffix)
@@ -316,8 +315,7 @@ uniquify-rationalize
 					      (uniquify-item-dirname item))))
 				      (and f (directory-file-name f)))
 				    (uniquify-item-buffer item)
-				    (uniquify-item-proposed item)
-                                    (uniquify-item-original-dirname item))
+				    (uniquify-item-proposed item))
 		fix-list)))))
   ;; If uniquify-min-dir-content is 0, this will end up just
   ;; passing fix-list to uniquify-rationalize-conflicting-sublist.
@@ -345,21 +343,10 @@ uniquify-rationalize-a-list
     (uniquify-rationalize-conflicting-sublist conflicting-sublist
 					      old-proposed depth)))
 
-(defun uniquify-get-proposed-name (base dirname &optional depth
-                                        original-dirname)
+(defun uniquify-get-proposed-name (base dirname &optional depth)
   (unless depth (setq depth uniquify-min-dir-content))
   (cl-assert (equal (directory-file-name dirname) dirname)) ;No trailing slash.
 
-  ;; Distinguish directories by adding extra separator.
-  (if (and uniquify-trailing-separator-p
-	   (file-directory-p (expand-file-name base original-dirname))
-	   (not (string-equal base "")))
-      (cond ((eq uniquify-buffer-name-style 'forward)
-	     (setq base (file-name-as-directory base)))
-	    ;; (setq base (concat base "/")))
-	    ((eq uniquify-buffer-name-style 'reverse)
-	     (setq base (concat (or uniquify-separator "\\") base)))))
-
   (let ((extra-string nil)
 	(n depth))
     (while (and (> n 0) dirname)
@@ -421,8 +408,7 @@ uniquify-rationalize-conflicting-sublist
 		  (uniquify-get-proposed-name
 		   (uniquify-item-base item)
 		   (uniquify-item-dirname item)
-		   depth
-                   (uniquify-item-original-dirname item))))
+		   depth)))
 	  (uniquify-rationalize-a-list conf-list depth))
       (unless (string= old-name "")
 	(uniquify-rename-buffer (car conf-list) old-name)))))
@@ -492,15 +478,14 @@ uniquify--rename-buffer-advice
 
 
 ;; (advice-add 'create-file-buffer :around #'uniquify--create-file-buffer-advice)
-(defun uniquify--create-file-buffer-advice (buf filename)
+(defun uniquify--create-file-buffer-advice (buf filename basename)
   ;; BEWARE: This is called directly from `files.el'!
   "Uniquify buffer names with parts of directory name."
   (when uniquify-buffer-name-style
-    (let ((filename (expand-file-name (directory-file-name filename))))
-      (uniquify-rationalize-file-buffer-names
-       (file-name-nondirectory filename)
-       (file-name-directory filename)
-       buf))))
+    (uniquify-rationalize-file-buffer-names
+     basename
+     (file-name-directory (expand-file-name (directory-file-name filename)))
+     buf)))
 
 (defun uniquify-unload-function ()
   "Unload the uniquify library."
diff --git a/test/lisp/uniquify-tests.el b/test/lisp/uniquify-tests.el
new file mode 100644
index 00000000000..abd61fa3504
--- /dev/null
+++ b/test/lisp/uniquify-tests.el
@@ -0,0 +1,129 @@
+;;; uniquify-tests.el --- Tests for uniquify         -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; Author: Spencer Baugh <sbaugh@janestreet.com>
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest uniquify-basic ()
+  (let (bufs old-names)
+    (cl-flet ((names-are (current-names &optional nosave)
+                (should (equal (mapcar #'buffer-name bufs) current-names))
+                (unless nosave (push current-names old-names))))
+      (should (eq (get-buffer "z") nil))
+      (push (find-file-noselect "a/b/z") bufs)
+      (names-are '("z"))
+      (push (find-file-noselect "a/b/c/z") bufs)
+      (names-are '("z<c>" "z<b>"))
+      (push (find-file-noselect "a/b/d/z") bufs)
+      (names-are '("z<d>" "z<c>" "z<b>"))
+      (push (find-file-noselect "e/b/z") bufs)
+      (names-are '("z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; buffers without a buffer-file-name don't get uniquified by uniquify
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; but they do get uniquified by the C code which uses <n>
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      (save-excursion
+        ;; uniquify will happily work with file-visiting buffers whose names don't match buffer-file-name
+        (find-file "f/y")
+        (push (current-buffer) bufs)
+        (rename-buffer "z" t)
+        (names-are '("z<f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave)
+        ;; somewhat confusing behavior results if a buffer is renamed to match an already-uniquified buffer
+        (rename-buffer "z<a/b>" t)
+        (names-are '("z<a/b><f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave))
+      (while bufs
+        (kill-buffer (pop bufs))
+        (names-are (pop old-names) 'nosave)))))
+
+(ert-deftest uniquify-dirs ()
+  "Check strip-common-suffix and trailing-separator-p work together; bug#47132"
+  (let* ((root (make-temp-file "emacs-uniquify-tests" 'dir))
+         (a-path (file-name-concat root "a/x/y/dir"))
+         (b-path (file-name-concat root "b/x/y/dir")))
+    (make-directory a-path 'parents)
+    (make-directory b-path 'parents)
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p nil))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir" "b/dir")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix nil)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/x/y/dir/" "b/x/y/dir/")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir/" "b/dir/")))
+        (mapc #'kill-buffer bufs)))))
+
+(ert-deftest uniquify-rename-to-dir ()
+  "Giving a buffer a name which matches a directory doesn't rename the buffer"
+  (let ((uniquify-buffer-name-style 'forward)
+        (uniquify-trailing-separator-p t))
+      (save-excursion
+        (find-file "../README")
+        (rename-buffer "lisp" t)
+        (should (equal (buffer-name) "lisp"))
+        (kill-buffer))))
+
+(ert-deftest uniquify-separator-style-reverse ()
+  (let ((uniquify-buffer-name-style 'reverse)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "\\lib-src"))
+      (kill-buffer))))
+
+(ert-deftest uniquify-separator-ignored ()
+  "If uniquify-buffer-name-style isn't forward or reverse,
+uniquify-trailing-separator-p is ignored"
+  (let ((uniquify-buffer-name-style 'post-forward-angle-brackets)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "lib-src"))
+      (kill-buffer))))
+
+(ert-deftest uniquify-space-prefix ()
+  "If a buffer starts with a space, | is added at the start"
+  (save-excursion
+    (find-file " foo")
+    (should (equal (buffer-name) "| foo"))
+    (kill-buffer)))
+
+(provide 'uniquify-tests)
+;;; uniquify-tests.el ends here
-- 
2.41.0

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: sbaugh@catern.com
> Date: Thu, 13 Jul 2023 15:52:54 +0000 (UTC)
> Cc: monnier@iro.umontreal.ca, sbaugh@janestreet.com, 62732@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So how about compromising on a variant of (2): we add an optional
> > DIRECTORY-P argument, and if FILENAME doesn't end in a slash, but
> > DIRECTORY-P is non-nil, create-file-buffer will append a slash?
> 
> Okay, so like this?

Looks like you sent an incorrect patch or something?

> BTW, would you be okay with moving uniquify-trailing-separator-p into
> dired, as I described in my other recent email?  Then create-file-buffer
> wouldn't need to check it, which would simplify its docstring slightly;
> instead dired would just decide whether to pass a directory name or file
> name based on uniquify-trailing-separator-p.  Since I'm changing this
> area anyway, now would be the time to make that change, as a nice
> cleanup which Stefan also likes.

I don't quite understand how can uniquify-trailing-separator-p be in
dired.el when the code which supports it is in uniquify.el.  What am I
missing?

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[sbaugh]

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

Eli Zaretskii <eliz@gnu.org> writes:
>> From: sbaugh@catern.com
>> Date: Thu, 13 Jul 2023 15:52:54 +0000 (UTC)
>> Cc: monnier@iro.umontreal.ca, sbaugh@janestreet.com, 62732@debbugs.gnu.org
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > So how about compromising on a variant of (2): we add an optional
>> > DIRECTORY-P argument, and if FILENAME doesn't end in a slash, but
>> > DIRECTORY-P is non-nil, create-file-buffer will append a slash?
>> 
>> Okay, so like this?
>
> Looks like you sent an incorrect patch or something?

Oops again, here's the correct patch.

>> BTW, would you be okay with moving uniquify-trailing-separator-p into
>> dired, as I described in my other recent email?  Then create-file-buffer
>> wouldn't need to check it, which would simplify its docstring slightly;
>> instead dired would just decide whether to pass a directory name or file
>> name based on uniquify-trailing-separator-p.  Since I'm changing this
>> area anyway, now would be the time to make that change, as a nice
>> cleanup which Stefan also likes.
>
> I don't quite understand how can uniquify-trailing-separator-p be in
> dired.el when the code which supports it is in uniquify.el.  What am I
> missing?

After this patch the only reference to uniquify-trailing-separator-p in
uniquify.el is its defcustom.  (As I mentioned in the other email, it
doesn't really have anything to do with uniquify)
              

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-recalculate-the-buffer-basename-inside-uniquif.patch --]
[-- Type: text/x-patch, Size: 14840 bytes --]

From ac884054ec824a04c87313cb0c57616d6082c36a Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Sun, 9 Jul 2023 10:24:33 -0400
Subject: [PATCH] Don't recalculate the buffer basename inside uniquify

Previously, uniquify--create-file-buffer-advice would use the filename
of the buffer to calculate what the buffer's basename should be.  Now
that gets passed in from create-file-buffer, which lets us fix several
bugs:

1. before this patch, if a buffer happened to be named the same thing
as directory in its default-directory, the buffer would get renamed
with a directory separator according to uniquify-trailing-separator-p.

2. buffers with a leading space should get a leading |, as described
by create-file-buffer's docstring; before this patch, uniquify would
remove that leading |.

* lisp/dired.el (dired-internal-noselect): Pass a directory name to
create-file-buffer.
* lisp/files.el (create-file-buffer): Do uniquify-trailing-separator-p
handling if passed a directory filename. (bug#62732)
* lisp/uniquify.el (uniquify-item):
(uniquify-rationalize-file-buffer-names, uniquify-rationalize,
uniquify-get-proposed-name, uniquify-rationalize-conflicting-sublist):
Remove uniquify-trailing-separator-p handling.
(uniquify--create-file-buffer-advice): Take new basename argument and
use it, instead of recalculating the basename from the filename.
---
 lisp/dired.el               |   2 +-
 lisp/files.el               |  48 +++++++++-----
 lisp/uniquify.el            |  39 ++++-------
 test/lisp/uniquify-tests.el | 129 ++++++++++++++++++++++++++++++++++++
 4 files changed, 175 insertions(+), 43 deletions(-)
 create mode 100644 test/lisp/uniquify-tests.el

diff --git a/lisp/dired.el b/lisp/dired.el
index d14cf47ffd5..3c9e6e40f9b 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1306,7 +1306,7 @@ dired-internal-noselect
 	 ;; Note that buffer already is in dired-mode, if found.
 	 (new-buffer-p (null buffer)))
     (or buffer
-        (setq buffer (create-file-buffer (directory-file-name dirname))))
+        (setq buffer (create-file-buffer dirname)))
     (set-buffer buffer)
     (if (not new-buffer-p)		; existing buffer ...
 	(cond (switches			; ... but new switches
diff --git a/lisp/files.el b/lisp/files.el
index d325729bf4d..c87a9bc8d22 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2062,22 +2062,40 @@ find-alternate-file
 	  (kill-buffer obuf))))))
 \f
 ;; FIXME we really need to fold the uniquify stuff in here by default,
-;; not using advice, and add it to the doc string.
-(defun create-file-buffer (filename)
+(defun create-file-buffer (filename &optional directory-p)
   "Create a suitably named buffer for visiting FILENAME, and return it.
-FILENAME (sans directory) is used unchanged if that name is free;
-otherwise a string <2> or <3> or ... is appended to get an unused name.
-
-Emacs treats buffers whose names begin with a space as internal buffers.
-To avoid confusion when visiting a file whose name begins with a space,
-this function prepends a \"|\" to the final result if necessary."
-  (let* ((lastname (file-name-nondirectory filename))
-	 (lastname (if (string= lastname "")
-	               filename lastname))
-	 (buf (generate-new-buffer (if (string-prefix-p " " lastname)
-			               (concat "|" lastname)
-			             lastname))))
-    (uniquify--create-file-buffer-advice buf filename)
+
+Either a file name or a directory name can be passed as FILENAME.
+In either case, the last non-empty component of FILENAME is used
+as the buffer name.
+
+If `uniquify-trailing-separator-p' is non-nil, then if FILENAME
+is a directory name, a file name separator is included in the
+buffer name.  If DIRECTORY-P is non-nil, this will happen even if
+FILENAME is a file name.
+
+Emacs treats buffers whose names begin with a space as internal
+buffers.  To avoid confusion when visiting a file whose name
+begins with a space, this function prepends a \"|\" to the buffer
+name if necessary.
+
+If the buffer name is already in use, the buffer will be renamed
+according to `uniquify-buffer-name-style' to get an unused name."
+  (let* ((lastname (file-name-nondirectory (directory-file-name filename)))
+         (lastname (cond
+                    ((not (and uniquify-trailing-separator-p
+                               (or (directory-name-p filename) directory-p)))
+                     lastname)
+                    ((eq uniquify-buffer-name-style 'forward)
+	             (file-name-as-directory lastname))
+	            ((eq uniquify-buffer-name-style 'reverse)
+	             (concat (or uniquify-separator "\\") lastname))
+                    (t lastname)))
+         (basename (if (string-prefix-p " " lastname)
+		       (concat "|" lastname)
+		     lastname))
+	 (buf (generate-new-buffer basename)))
+    (uniquify--create-file-buffer-advice buf filename basename)
     buf))
 
 (defvar abbreviated-home-dir nil
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index dee9ecba2ea..d1ca455b673 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -174,8 +174,8 @@ uniquify-list-buffers-directory-modes
 (cl-defstruct (uniquify-item
 	    (:constructor nil) (:copier nil)
 	    (:constructor uniquify-make-item
-	     (base dirname buffer &optional proposed original-dirname)))
-  base dirname buffer proposed original-dirname)
+	     (base dirname buffer &optional proposed)))
+  base dirname buffer proposed)
 
 ;; Internal variables used free
 (defvar uniquify-possibly-resolvable nil)
@@ -211,7 +211,7 @@ uniquify-rationalize-file-buffer-names
   (when dirname
     (setq dirname (expand-file-name (directory-file-name dirname)))
     (let ((fix-list (list (uniquify-make-item base dirname newbuf
-                                              nil dirname)))
+                                              nil)))
 	  items)
       (dolist (buffer (buffer-list))
 	(when (and (not (and uniquify-ignore-buffers-re
@@ -292,8 +292,7 @@ uniquify-rationalize
       (setf (uniquify-item-proposed item)
 	    (uniquify-get-proposed-name (uniquify-item-base item)
 					(uniquify-item-dirname item)
-                                        nil
-                                        (uniquify-item-original-dirname item)))
+                                        nil))
       (setq uniquify-managed fix-list)))
   ;; Strip any shared last directory names of the dirname.
   (when (and (cdr fix-list) uniquify-strip-common-suffix)
@@ -316,8 +315,7 @@ uniquify-rationalize
 					      (uniquify-item-dirname item))))
 				      (and f (directory-file-name f)))
 				    (uniquify-item-buffer item)
-				    (uniquify-item-proposed item)
-                                    (uniquify-item-original-dirname item))
+				    (uniquify-item-proposed item))
 		fix-list)))))
   ;; If uniquify-min-dir-content is 0, this will end up just
   ;; passing fix-list to uniquify-rationalize-conflicting-sublist.
@@ -345,21 +343,10 @@ uniquify-rationalize-a-list
     (uniquify-rationalize-conflicting-sublist conflicting-sublist
 					      old-proposed depth)))
 
-(defun uniquify-get-proposed-name (base dirname &optional depth
-                                        original-dirname)
+(defun uniquify-get-proposed-name (base dirname &optional depth)
   (unless depth (setq depth uniquify-min-dir-content))
   (cl-assert (equal (directory-file-name dirname) dirname)) ;No trailing slash.
 
-  ;; Distinguish directories by adding extra separator.
-  (if (and uniquify-trailing-separator-p
-	   (file-directory-p (expand-file-name base original-dirname))
-	   (not (string-equal base "")))
-      (cond ((eq uniquify-buffer-name-style 'forward)
-	     (setq base (file-name-as-directory base)))
-	    ;; (setq base (concat base "/")))
-	    ((eq uniquify-buffer-name-style 'reverse)
-	     (setq base (concat (or uniquify-separator "\\") base)))))
-
   (let ((extra-string nil)
 	(n depth))
     (while (and (> n 0) dirname)
@@ -421,8 +408,7 @@ uniquify-rationalize-conflicting-sublist
 		  (uniquify-get-proposed-name
 		   (uniquify-item-base item)
 		   (uniquify-item-dirname item)
-		   depth
-                   (uniquify-item-original-dirname item))))
+		   depth)))
 	  (uniquify-rationalize-a-list conf-list depth))
       (unless (string= old-name "")
 	(uniquify-rename-buffer (car conf-list) old-name)))))
@@ -492,15 +478,14 @@ uniquify--rename-buffer-advice
 
 
 ;; (advice-add 'create-file-buffer :around #'uniquify--create-file-buffer-advice)
-(defun uniquify--create-file-buffer-advice (buf filename)
+(defun uniquify--create-file-buffer-advice (buf filename basename)
   ;; BEWARE: This is called directly from `files.el'!
   "Uniquify buffer names with parts of directory name."
   (when uniquify-buffer-name-style
-    (let ((filename (expand-file-name (directory-file-name filename))))
-      (uniquify-rationalize-file-buffer-names
-       (file-name-nondirectory filename)
-       (file-name-directory filename)
-       buf))))
+    (uniquify-rationalize-file-buffer-names
+     basename
+     (file-name-directory (expand-file-name (directory-file-name filename)))
+     buf)))
 
 (defun uniquify-unload-function ()
   "Unload the uniquify library."
diff --git a/test/lisp/uniquify-tests.el b/test/lisp/uniquify-tests.el
new file mode 100644
index 00000000000..abd61fa3504
--- /dev/null
+++ b/test/lisp/uniquify-tests.el
@@ -0,0 +1,129 @@
+;;; uniquify-tests.el --- Tests for uniquify         -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; Author: Spencer Baugh <sbaugh@janestreet.com>
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest uniquify-basic ()
+  (let (bufs old-names)
+    (cl-flet ((names-are (current-names &optional nosave)
+                (should (equal (mapcar #'buffer-name bufs) current-names))
+                (unless nosave (push current-names old-names))))
+      (should (eq (get-buffer "z") nil))
+      (push (find-file-noselect "a/b/z") bufs)
+      (names-are '("z"))
+      (push (find-file-noselect "a/b/c/z") bufs)
+      (names-are '("z<c>" "z<b>"))
+      (push (find-file-noselect "a/b/d/z") bufs)
+      (names-are '("z<d>" "z<c>" "z<b>"))
+      (push (find-file-noselect "e/b/z") bufs)
+      (names-are '("z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; buffers without a buffer-file-name don't get uniquified by uniquify
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      ;; but they do get uniquified by the C code which uses <n>
+      (push (generate-new-buffer "z") bufs)
+      (names-are '("z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>"))
+      (save-excursion
+        ;; uniquify will happily work with file-visiting buffers whose names don't match buffer-file-name
+        (find-file "f/y")
+        (push (current-buffer) bufs)
+        (rename-buffer "z" t)
+        (names-are '("z<f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave)
+        ;; somewhat confusing behavior results if a buffer is renamed to match an already-uniquified buffer
+        (rename-buffer "z<a/b>" t)
+        (names-are '("z<a/b><f>" "z<2>" "z" "z<e/b>" "z<d>" "z<c>" "z<a/b>") 'nosave))
+      (while bufs
+        (kill-buffer (pop bufs))
+        (names-are (pop old-names) 'nosave)))))
+
+(ert-deftest uniquify-dirs ()
+  "Check strip-common-suffix and trailing-separator-p work together; bug#47132"
+  (let* ((root (make-temp-file "emacs-uniquify-tests" 'dir))
+         (a-path (file-name-concat root "a/x/y/dir"))
+         (b-path (file-name-concat root "b/x/y/dir")))
+    (make-directory a-path 'parents)
+    (make-directory b-path 'parents)
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p nil))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir" "b/dir")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix nil)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/x/y/dir/" "b/x/y/dir/")))
+        (mapc #'kill-buffer bufs)))
+    (let ((uniquify-buffer-name-style 'forward)
+          (uniquify-strip-common-suffix t)
+          (uniquify-trailing-separator-p t))
+      (let ((bufs (list (find-file-noselect a-path)
+                       (find-file-noselect b-path))))
+        (should (equal (mapcar #'buffer-name bufs)
+                       '("a/dir/" "b/dir/")))
+        (mapc #'kill-buffer bufs)))))
+
+(ert-deftest uniquify-rename-to-dir ()
+  "Giving a buffer a name which matches a directory doesn't rename the buffer"
+  (let ((uniquify-buffer-name-style 'forward)
+        (uniquify-trailing-separator-p t))
+      (save-excursion
+        (find-file "../README")
+        (rename-buffer "lisp" t)
+        (should (equal (buffer-name) "lisp"))
+        (kill-buffer))))
+
+(ert-deftest uniquify-separator-style-reverse ()
+  (let ((uniquify-buffer-name-style 'reverse)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "\\lib-src"))
+      (kill-buffer))))
+
+(ert-deftest uniquify-separator-ignored ()
+  "If uniquify-buffer-name-style isn't forward or reverse,
+uniquify-trailing-separator-p is ignored"
+  (let ((uniquify-buffer-name-style 'post-forward-angle-brackets)
+        (uniquify-trailing-separator-p t))
+    (save-excursion
+      (should (file-directory-p "../lib-src"))
+      (find-file "../lib-src")
+      (should (equal (buffer-name) "lib-src"))
+      (kill-buffer))))
+
+(ert-deftest uniquify-space-prefix ()
+  "If a buffer starts with a space, | is added at the start"
+  (save-excursion
+    (find-file " foo")
+    (should (equal (buffer-name) "| foo"))
+    (kill-buffer)))
+
+(provide 'uniquify-tests)
+;;; uniquify-tests.el ends here
-- 
2.41.0

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Michael Heerdegen]

sbaugh@catern.com writes:

> Subject: [PATCH] Don't recalculate the buffer basename inside uniquify

Sorry but: Is it this patch's fault that it is not possible to visit the root
directory "/" any more?

| Debugger entered--Lisp error: (error "Empty string for buffer name is not allowed")
|   get-buffer-create("" nil)
|   generate-new-buffer("")
|   create-file-buffer("/")
|   dired-internal-noselect("/" nil)
|   dired-noselect("/" nil)
|   dired("/" nil)


TIA,

Michael.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  sbaugh@janestreet.com,
>   monnier@iro.umontreal.ca,  62732@debbugs.gnu.org
> Date: Mon, 17 Jul 2023 07:03:28 +0200
> 
> sbaugh@catern.com writes:
> 
> > Subject: [PATCH] Don't recalculate the buffer basename inside uniquify
> 
> Sorry but: Is it this patch's fault that it is not possible to visit the root
> directory "/" any more?
> 
> | Debugger entered--Lisp error: (error "Empty string for buffer name is not allowed")
> |   get-buffer-create("" nil)
> |   generate-new-buffer("")
> |   create-file-buffer("/")
> |   dired-internal-noselect("/" nil)
> |   dired-noselect("/" nil)
> |   dired("/" nil)

Yes.  Should be fixed now.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > |   get-buffer-create("" nil)
> > |   generate-new-buffer("")
> > |   create-file-buffer("/")
> > |   dired-internal-noselect("/" nil)
> > |   dired-noselect("/" nil)
> > |   dired("/" nil)
>
> Yes.  Should be fixed now.

Indeed.  Thank you.

Michael.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 62732@debbugs.gnu.org
> Date: Tue, 18 Jul 2023 06:13:17 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > |   get-buffer-create("" nil)
> > > |   generate-new-buffer("")
> > > |   create-file-buffer("/")
> > > |   dired-internal-noselect("/" nil)
> > > |   dired-noselect("/" nil)
> > > |   dired("/" nil)
> >
> > Yes.  Should be fixed now.
> 
> Indeed.  Thank you.

Thanks for testing.

bug#62732: 29.0.60; uniquify-trailing-separator-p affects any buffer whose name matches a dir in CWD

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

> I prefer (1), because it avoids requesting the callers to remember to
> ensure that every directory ends in a slash.

OK, pushed.  It is admittedly closer to the current semantics, so in
a sense it's a step in the right direction and we could still change it
later if/when needed.

Thank you very much, Spencer,


        Stefan