bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Thierry Volpiatto]

This is a followup of this report on reddit:
https://www.reddit.com/r/emacs/comments/yha104/merging_directories_in_dired_am_i_doing_it_wrong/

When using dired-copy to copy a directory to another directory
containing a directory with the same name overwriting fails.
e.g. copy ~/tmp/test/foo/ to ~/tmp/test1/ fails when test1 contain foo/

The bug is IMHO in copy-directory 3th clause of this cond:

(cond ((not (directory-name-p newname))
       ;; If NEWNAME is not a directory name, create it;
       ;; that is where we will copy the files of DIRECTORY.
       (make-directory newname parents))
      ;; NEWNAME is a directory name.  If COPY-CONTENTS is non-nil,
      ;; create NEWNAME if it is not already a directory;
      ;; otherwise, create NEWNAME/[DIRECTORY-BASENAME].
      ((if copy-contents
	   (or parents (not (file-directory-p newname)))
	 (setq newname (concat newname
			       (file-name-nondirectory directory))))
       (make-directory (directory-file-name newname) parents))
      (t (setq follow t)))

This change was introduced here:

commit 047f02f00f602b9aef63ae8938e12f3f0ab481eb
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Wed Sep 20 11:49:12 2017 -0700

    Fix new copy-directory bug with empty dirs
    
    Problem reported by Afdam Plaice (Bug#28520) and by Eli Zaretskii
    (Bug#28483#34).  This is another bug that I introduced in my
    recent copy-directory changes.
    * lisp/files.el (copy-directory): Work with empty subdirectories, too.
    * test/lisp/files-tests.el (files-tests--copy-directory):
    Test for this bug.

Reverting this change fix the bug.

Using this should also fix the bug (based on my 2012 changes), not sure
though if it fix the bug described in the above commit.

(cond ((not (directory-name-p newname))
       ;; If NEWNAME is not a directory name, create it;
       ;; that is where we will copy the files of DIRECTORY.
       (make-directory newname parents))
      ;; NEWNAME is a directory name.  If COPY-CONTENTS is non-nil,
      ;; create NEWNAME if it is not already a directory;
      ;; otherwise, create NEWNAME/[DIRECTORY-BASENAME].
      ((and copy-contents
            (or parents (not (file-directory-p newname))))
       (make-directory (directory-file-name newname) parents))
      ((not copy-contents)
       (setq newname (concat newname
			     (file-name-nondirectory directory)))
       (and (file-exists-p newname)
	    (not (file-directory-p newname))
	    (error "Cannot overwrite non-directory %s with a directory"
		   newname))
       (make-directory newname t))
      (t (setq follow t)))

It also make readable the clauses handling copy-contents...



In GNU Emacs 28.2 (build 1, x86_64-pc-linux-gnu, Motif Version 2.3.8, cairo version 1.16.0)
 of 2022-09-12 built on IPad-S340
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: Linux Mint 20.3

Configured using:
 'configure CFLAGS=-O8 --with-mailutils --with-cairo --without-dbus
 --without-gconf --without-gsettings --with-x-toolkit=motif'

Configured features:
ACL CAIRO FREETYPE GIF GLIB GMP GNUTLS GPM HARFBUZZ JPEG JSON LCMS2
LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY
PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE
XIM XPM MOTIF ZLIB

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

Major mode: ƐĽ

Minor modes in effect:
  bug-reference-prog-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  psession-mode: t
  psession-savehist-mode: t
  global-git-gutter-mode: t
  git-gutter-mode: t
  display-time-mode: t
  winner-mode: t
  helm-epa-mode: t
  helm-descbinds-mode: t
  helm-adaptive-mode: t
  helm-mode: t
  helm-minibuffer-history-mode: t
  helm-ff-icon-mode: t
  shell-dirtrack-mode: t
  helm-popup-tip-mode: t
  async-bytecomp-package-mode: t
  minibuffer-depth-indicate-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  auto-fill-function: do-auto-fill
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow epa-mail face-remap emacsbug vc-annotate sort gnus-cite w3m-form
w3m-symbol w3m doc-view jka-compr timezone w3m-hist w3m-fb bookmark-w3m
w3m-ems w3m-favicon w3m-image tab-line w3m-proc w3m-util mm-archive qp
smiley mail-extr addressbook-bookmark tv-mu4e-config mu4e-contrib eshell
esh-cmd esh-ext esh-opt esh-proc esh-io esh-arg esh-module esh-groups
esh-util mu4e-patch mu4e mu4e-org mu4e-main mu4e-view gnus-art mm-uu
mml2015 mm-view mml-smime smime dig gnus-sum gnus-group gnus-undo
gnus-start gnus-dbus gnus-cloud nnimap nnmail mail-source utf7 netrc
nnoo gnus-spec gnus-int gnus-range gnus-win mu4e-headers mu4e-compose
mu4e-draft mu4e-actions smtpmail sendmail mu4e-search mu4e-lists
mu4e-bookmarks mu4e-mark mu4e-message shr kinsoku svg flow-fill hl-line
mu4e-contacts mu4e-update mu4e-folders mu4e-server mu4e-context
mu4e-obsolete mu4e-vars mu4e-helpers mu4e-config ido helm-firefox
helm-dabbrev help-fns radix-tree debug cl-print cus-start helm-command
helm-x-files helm-for-files dired-x image-file image-converter char-fold
tramp-archive tramp-gvfs dbus bug-reference naquadah-theme view solar
cal-dst holidays hol-loaddefs tv-utils osm dom yaml-mode undo-tree diff
queue psession frameset log-view pcvs-util bash-completion cl-indent
pcase ffap thingatpt autocrypt-message message rmc puny rfc822 mml
mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev
gmm-utils mailheader autocrypt-gnus gnus nnheader gnus-util rmail
rmail-loaddefs rfc2047 rfc2045 mail-utils mm-util mail-prsvr
autocrypt-mu4e autocrypt ietf-drums config-w3m git-gutter mule-util appt
diary-lib diary-loaddefs gud wdired dired-extension org-config
ob-gnuplot org-crypt net-utils time winner autotest-mode autoconf-mode
woman man ediff ediff-merg ediff-mult ediff-wind ediff-diff ediff-help
ediff-init ediff-util init-helm helm-ls-git vc-git diff-mode vc
vc-dispatcher helm-fd epa derived epg rfc6068 epg-config helm-epa
helm-imenu imenu helm-elisp-package helm-find helm-org org ob ob-tangle
ob-ref ob-lob ob-table ob-exp org-macro org-footnote org-src ob-comint
org-pcomplete org-list org-faces org-entities noutline outline
org-version ob-emacs-lisp ob-core ob-eval org-table oc-basic bibtex ol
rx org-keys oc org-compat advice org-macs org-loaddefs cal-menu calendar
cal-loaddefs helm-external isl helm-descbinds helm-wikipedia
all-the-icons all-the-icons-faces data-material data-weathericons
data-octicons data-fileicons data-faicons data-alltheicons wfnames
cus-edit wid-edit helm-ipython helm-elisp helm-eval edebug backtrace
find-func python tramp-sh popup helm-bookmark helm-net xml helm-info
bookmark pp helm-adaptive helm-mode helm-misc helm-files image-dired
image-mode exif filenotify tramp tramp-loaddefs trampver
tramp-integration files-x tramp-compat shell pcomplete parse-time
iso8601 time-date ls-lisp helm-buffers helm-occur helm-tags helm-locate
helm-grep wgrep-helm wgrep grep compile text-property-search comint ring
helm-regexp format-spec ansi-color helm-utils helm-help helm-types
helm-extensions-autoloads helm-config helm-autoloads helm
helm-global-bindings helm-easymenu helm-core async-bytecomp helm-source
helm-multi-match helm-lib dired-async dired-aux dired dired-loaddefs
async diminish cl-extra help-mode mb-depth server edmacro kmacro avoid
cus-load use-package use-package-ensure use-package-delight
use-package-diminish use-package-bind-key bind-key easy-mmode
use-package-core finder-inf package browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
mailcap url-handlers url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json subr-x map url-vars seq
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib info w3m-load
iso-transl tooltip 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 cl-generic
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 simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads inotify
lcms2 dynamic-setting font-render-setting cairo motif x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 824596 90080)
 (symbols 48 43448 5)
 (strings 32 266727 16954)
 (string-bytes 1 8033636)
 (vectors 16 86864)
 (vector-slots 8 1908060 113478)
 (floats 8 2769 951)
 (intervals 56 23817 13089)
 (buffers 992 107))
<#secure method=pgpmime mode=sign>

-- 
Thierry

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Eli Zaretskii]

merge 58919 58918
thanks

> From: Thierry Volpiatto <thievol@posteo.net>
> Date: Mon, 31 Oct 2022 08:54:28 +0000
> 
> 
> This is a followup of this report on reddit:
> https://www.reddit.com/r/emacs/comments/yha104/merging_directories_in_dired_am_i_doing_it_wrong/

Which was already reported as bug#58918...

> When using dired-copy to copy a directory to another directory
> containing a directory with the same name overwriting fails.
> e.g. copy ~/tmp/test/foo/ to ~/tmp/test1/ fails when test1 contain foo/
> 
> The bug is IMHO in copy-directory 3th clause of this cond:
> 
> (cond ((not (directory-name-p newname))
>        ;; If NEWNAME is not a directory name, create it;
>        ;; that is where we will copy the files of DIRECTORY.
>        (make-directory newname parents))
>       ;; NEWNAME is a directory name.  If COPY-CONTENTS is non-nil,
>       ;; create NEWNAME if it is not already a directory;
>       ;; otherwise, create NEWNAME/[DIRECTORY-BASENAME].
>       ((if copy-contents
> 	   (or parents (not (file-directory-p newname)))
> 	 (setq newname (concat newname
> 			       (file-name-nondirectory directory))))
>        (make-directory (directory-file-name newname) parents))
>       (t (setq follow t)))
> 
> This change was introduced here:
> 
> commit 047f02f00f602b9aef63ae8938e12f3f0ab481eb
> Author: Paul Eggert <eggert@cs.ucla.edu>
> Date:   Wed Sep 20 11:49:12 2017 -0700
> 
>     Fix new copy-directory bug with empty dirs
>     
>     Problem reported by Afdam Plaice (Bug#28520) and by Eli Zaretskii
>     (Bug#28483#34).  This is another bug that I introduced in my
>     recent copy-directory changes.
>     * lisp/files.el (copy-directory): Work with empty subdirectories, too.
>     * test/lisp/files-tests.el (files-tests--copy-directory):
>     Test for this bug.
> 
> Reverting this change fix the bug.

Paul, could you please look into this?

I think this also affects bug#58721.

Thanks.

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Thierry Volpiatto]

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


For people using dired-async.el (from emacs-async package) here how to
fix this bug temporarily until fixed:

https://github.com/jwiegley/emacs-async/issues/158

Eli Zaretskii <eliz@gnu.org> writes:

> merge 58919 58918
> thanks
>
>> From: Thierry Volpiatto <thievol@posteo.net>
>> Date: Mon, 31 Oct 2022 08:54:28 +0000
>> 
>> 
>> This is a followup of this report on reddit:
>> https://www.reddit.com/r/emacs/comments/yha104/merging_directories_in_dired_am_i_doing_it_wrong/
>
> Which was already reported as bug#58918...
>
>> When using dired-copy to copy a directory to another directory
>> containing a directory with the same name overwriting fails.
>> e.g. copy ~/tmp/test/foo/ to ~/tmp/test1/ fails when test1 contain foo/
>> 
>> The bug is IMHO in copy-directory 3th clause of this cond:
>> 
>> (cond ((not (directory-name-p newname))
>>        ;; If NEWNAME is not a directory name, create it;
>>        ;; that is where we will copy the files of DIRECTORY.
>>        (make-directory newname parents))
>>       ;; NEWNAME is a directory name.  If COPY-CONTENTS is non-nil,
>>       ;; create NEWNAME if it is not already a directory;
>>       ;; otherwise, create NEWNAME/[DIRECTORY-BASENAME].
>>       ((if copy-contents
>> 	   (or parents (not (file-directory-p newname)))
>> 	 (setq newname (concat newname
>> 			       (file-name-nondirectory directory))))
>>        (make-directory (directory-file-name newname) parents))
>>       (t (setq follow t)))
>> 
>> This change was introduced here:
>> 
>> commit 047f02f00f602b9aef63ae8938e12f3f0ab481eb
>> Author: Paul Eggert <eggert@cs.ucla.edu>
>> Date:   Wed Sep 20 11:49:12 2017 -0700
>> 
>>     Fix new copy-directory bug with empty dirs
>>     
>>     Problem reported by Afdam Plaice (Bug#28520) and by Eli Zaretskii
>>     (Bug#28483#34).  This is another bug that I introduced in my
>>     recent copy-directory changes.
>>     * lisp/files.el (copy-directory): Work with empty subdirectories, too.
>>     * test/lisp/files-tests.el (files-tests--copy-directory):
>>     Test for this bug.
>> 
>> Reverting this change fix the bug.
>
> Paul, could you please look into this?
>
> I think this also affects bug#58721.
>
> Thanks.


-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Paul Eggert]

On 2022-10-31 06:01, Eli Zaretskii wrote:
> Paul, could you please look into this?

I am taking a look. Sigh, what a mess this code is; even the proposed 
fix has a TOCTOU bug.

The simplest fix I can see is to enhance make-directory so that it 
returns t if the directory already existed and PARENTS was given, nil on 
any other successful return. This would require changes to Tramp to 
avoid races there. I plan to post a proposed patch for comment.

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Eli Zaretskii]

> Date: Tue, 1 Nov 2022 11:04:22 -0700
> Cc: 58919@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> The simplest fix I can see is to enhance make-directory so that it 
> returns t if the directory already existed and PARENTS was given, nil on 
> any other successful return. This would require changes to Tramp to 
> avoid races there. I plan to post a proposed patch for comment.

SGTM: make-directory didn't return any meaningful value until now, so
this change should be safe, I think.

Thanks.

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> The simplest fix I can see is to enhance make-directory so that it
> returns t if the directory already existed and PARENTS was given, nil
> on any other successful return. This would require changes to Tramp to
> avoid races there. I plan to post a proposed patch for comment.

To make it more fun, there are several Tramp implementations of that
function, and also other ones. Xref, running in the lisp/ tree for
"defun.*make-directory", returns

--8<---------------cut here---------------start------------->8---
lisp/dired.el
1911: (defun dired--make-directory-clickable ()
lisp/files.el
6203: (defun make-directory (dir &optional parents)
lisp/gnus/gnus-group.el
3120: (defun gnus-group-make-directory-group (dir)
lisp/gnus/gnus-util.el
726: (defun gnus-make-directory (directory)
lisp/htmlfontify.el
1844: (defun hfy-make-directory (dir)
lisp/ido.el
2994: (defun ido-make-directory (&optional dir)
lisp/net/ange-ftp.el
4125: (defun ange-ftp-make-directory (dir &optional parents)
4530: (defun ange-ftp-real-make-directory (&rest args)
lisp/net/tramp-adb.el
411: (defun tramp-adb-handle-make-directory (dir &optional parents)
lisp/net/tramp-crypt.el
800: (defun tramp-crypt-handle-make-directory (dir &optional parents)
lisp/net/tramp-fuse.el
128: (defun tramp-fuse-handle-make-directory (dir &optional parents)
lisp/net/tramp-gvfs.el
1560: (defun tramp-gvfs-handle-make-directory (dir &optional parents)
lisp/net/tramp-sh.el
2559: (defun tramp-sh-handle-make-directory (dir &optional parents)
lisp/net/tramp-smb.el
1172: (defun tramp-smb-handle-make-directory (dir &optional parents)
1192: (defun tramp-smb-handle-make-directory-internal (directory)
lisp/net/tramp-sudoedit.el
626: (defun tramp-sudoedit-handle-make-directory (dir &optional parents)
lisp/obsolete/autoload.el
725: (defun make-directory-autoloads (dir output-file)
lisp/url/url-dav.el
761: (defun url-dav-make-directory (url &optional _parents)
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Eli Zaretskii]

Ping!

Paul, did you have an opportunity to come up with the patch you
mentioned in this discussion?  I'd like to solve this bug for Emacs
29, please.

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Tue, 01 Nov 2022 20:21:32 +0100
> Cc: Thierry Volpiatto <thievol@posteo.net>, 58919@debbugs.gnu.org,
>  Eli Zaretskii <eliz@gnu.org>
> 
> Paul Eggert <eggert@cs.ucla.edu> writes:
> 
> Hi Paul,
> 
> > The simplest fix I can see is to enhance make-directory so that it
> > returns t if the directory already existed and PARENTS was given, nil
> > on any other successful return. This would require changes to Tramp to
> > avoid races there. I plan to post a proposed patch for comment.
> 
> To make it more fun, there are several Tramp implementations of that
> function, and also other ones. Xref, running in the lisp/ tree for
> "defun.*make-directory", returns
> 
> --8<---------------cut here---------------start------------->8---
> lisp/dired.el
> 1911: (defun dired--make-directory-clickable ()
> lisp/files.el
> 6203: (defun make-directory (dir &optional parents)
> lisp/gnus/gnus-group.el
> 3120: (defun gnus-group-make-directory-group (dir)
> lisp/gnus/gnus-util.el
> 726: (defun gnus-make-directory (directory)
> lisp/htmlfontify.el
> 1844: (defun hfy-make-directory (dir)
> lisp/ido.el
> 2994: (defun ido-make-directory (&optional dir)
> lisp/net/ange-ftp.el
> 4125: (defun ange-ftp-make-directory (dir &optional parents)
> 4530: (defun ange-ftp-real-make-directory (&rest args)
> lisp/net/tramp-adb.el
> 411: (defun tramp-adb-handle-make-directory (dir &optional parents)
> lisp/net/tramp-crypt.el
> 800: (defun tramp-crypt-handle-make-directory (dir &optional parents)
> lisp/net/tramp-fuse.el
> 128: (defun tramp-fuse-handle-make-directory (dir &optional parents)
> lisp/net/tramp-gvfs.el
> 1560: (defun tramp-gvfs-handle-make-directory (dir &optional parents)
> lisp/net/tramp-sh.el
> 2559: (defun tramp-sh-handle-make-directory (dir &optional parents)
> lisp/net/tramp-smb.el
> 1172: (defun tramp-smb-handle-make-directory (dir &optional parents)
> 1192: (defun tramp-smb-handle-make-directory-internal (directory)
> lisp/net/tramp-sudoedit.el
> 626: (defun tramp-sudoedit-handle-make-directory (dir &optional parents)
> lisp/obsolete/autoload.el
> 725: (defun make-directory-autoloads (dir output-file)
> lisp/url/url-dav.el
> 761: (defun url-dav-make-directory (url &optional _parents)
> --8<---------------cut here---------------end--------------->8---
> 
> Best regards, Michael.
> 
> 
> 
> 

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Paul Eggert]

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

On 12/11/22 02:46, Eli Zaretskii wrote:
> Paul, did you have an opportunity to come up with the patch you
> mentioned in this discussion?  I'd like to solve this bug for Emacs
> 29, please.

I hacked on it for a bit and came up with the attached proposed patches 
to the emacs-29 branch. I have not installed them.

These patches address the issues raised by Michael by passing only 
single arguments to make-directory handlers. That way, we don't need to 
worry about whether the handlers follow the new convention. At our 
leisure, perhaps in Emacs 30, we can upgrade the make-directory handlers 
to support the new convention.

[-- Attachment #2: 0001-Use-make-directory-handlers-uniformly.patch --]
[-- Type: text/x-patch, Size: 6565 bytes --]

From bfbb463c6e1b74c96211e454c80d8588f187dcfb Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 16 Dec 2022 14:55:47 -0800
Subject: [PATCH 1/3] Use make-directory handlers uniformly

Formerly, the code supported both make-directory and
make-directory-internal handlers. This led to confusion and meant than
in a few cases (nnmaildir, ido) remote directories could not be used in
some cases.  Fix this by using only make-directory handlers.

Perhaps there used to be a reason for why there were both
make-directory and make-directory-internal handlers, but whatever that
reason was, it seems to have vanished even before now.

There is no longer any need for make-directory-internal handlers, as
the few remaining callers that use make-directory-internal do so only
when there are no handlers.  However, this change keeps the existing
make-directory-internal handlers for now, in case this code is ever
used in older Emacs versions that still call those handlers.

* lisp/gnus/nnmaildir.el (nnmaildir--mkdir):
* lisp/ido.el (ido-file-internal):
* lisp/net/tramp-smb.el (tramp-smb-handle-make-directory):
Use make-directory, not make-directory-internal.
* lisp/net/tramp-smb.el (tramp-smb-handle-make-directory-internal):
Now obsolete.
* src/fileio.c (Fmake_directory_internal): Do not look for or
use a make-directory-internal handler.
* test/lisp/files-tests.el:
(files-tests-file-name-non-special-make-directory-internal):
Remove, as this test incorrectly assumes that make-directory-internal
must support handlers.
---
 doc/lispref/files.texi   |  2 --
 etc/NEWS                 |  4 ++++
 lisp/gnus/nnmaildir.el   |  2 +-
 lisp/ido.el              |  2 +-
 lisp/net/tramp-smb.el    |  3 ++-
 src/fileio.c             |  5 -----
 test/lisp/files-tests.el | 11 -----------
 7 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 4b45d89f9d0..5bcbac6ceb8 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -3378,7 +3378,6 @@ Magic File Names
 @code{load}, @code{lock-file},
 @code{make-auto-save-file-name},
 @code{make-directory},
-@code{make-directory-internal},
 @code{make-lock-file-name},
 @code{make-nearby-temp-file},
 @code{make-process},
@@ -3440,7 +3439,6 @@ Magic File Names
 @code{load}, @code{lock-file},
 @code{make-auto-save-file-name},
 @code{make-direc@discretionary{}{}{}tory},
-@code{make-direc@discretionary{}{}{}tory-internal},
 @code{make-lock-file-name},
 @code{make-nearby-temp-file},
 @code{make-process},
diff --git a/etc/NEWS b/etc/NEWS
index 157fe98c983..dbf1642206b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -4477,6 +4477,10 @@ set is too big to transfer to Emacs every time a completion is
 needed.  The table uses new 'external' completion style exclusively
 and cannot work with regular styles such as 'basic' or 'flex'.
 
++++
+** Magic file handlers for make-directory-internal are no longer needed.
+Instead, Emacs uses the already-existing make-directory handlers.
+
 \f
 * Changes in Emacs 29.1 on Non-Free Operating Systems
 
diff --git a/lisp/gnus/nnmaildir.el b/lisp/gnus/nnmaildir.el
index faa288934d1..3fb87f3a712 100644
--- a/lisp/gnus/nnmaildir.el
+++ b/lisp/gnus/nnmaildir.el
@@ -296,7 +296,7 @@ nnmaildir--unlink
      (if (file-attributes file) (delete-file file))))
 (defun nnmaildir--mkdir (dir)
   (or (file-exists-p (file-name-as-directory dir))
-      (make-directory-internal (directory-file-name dir))))
+      (make-directory (directory-file-name dir))))
 (defun nnmaildir--mkfile (file)
   (write-region "" nil file nil 'no-message))
 (defun nnmaildir--delete-dir-files (dir ls)
diff --git a/lisp/ido.el b/lisp/ido.el
index 77e4dd447d8..92b4370cb45 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -2435,7 +2435,7 @@ ido-file-internal
                                 filename))
 	      (ido-record-command method dirname)
 	      (ido-record-work-directory dirname)
-	      (make-directory-internal dirname)
+	      (make-directory dirname)
 	      (funcall method dirname))
 	     (t
 	      ;; put make-directory command on history
diff --git a/lisp/net/tramp-smb.el b/lisp/net/tramp-smb.el
index c720b33b5f2..24fff9bb495 100644
--- a/lisp/net/tramp-smb.el
+++ b/lisp/net/tramp-smb.el
@@ -1186,12 +1186,13 @@ tramp-smb-handle-make-directory
 	(make-directory ldir parents))
       ;; Just do it.
       (when (file-directory-p ldir)
-	(make-directory-internal dir))
+	(make-directory dir))
       (unless (file-directory-p dir)
 	(tramp-error v 'file-error "Couldn't make directory %s" dir)))))
 
 (defun tramp-smb-handle-make-directory-internal (directory)
   "Like `make-directory-internal' for Tramp files."
+  (declare (obsolete nil "29.1"))
   (setq directory (directory-file-name (expand-file-name directory)))
   (unless (file-name-absolute-p directory)
     (setq directory (expand-file-name directory default-directory)))
diff --git a/src/fileio.c b/src/fileio.c
index 92335b639cd..835c42cc0a4 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2427,16 +2427,11 @@ DEFUN ("make-directory-internal", Fmake_directory_internal,
   (Lisp_Object directory)
 {
   const char *dir;
-  Lisp_Object handler;
   Lisp_Object encoded_dir;
 
   CHECK_STRING (directory);
   directory = Fexpand_file_name (directory, Qnil);
 
-  handler = Ffind_file_name_handler (directory, Qmake_directory_internal);
-  if (!NILP (handler))
-    return call2 (handler, Qmake_directory_internal, directory);
-
   encoded_dir = ENCODE_FILE (directory);
 
   dir = SSDATA (encoded_dir);
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 682b5cdb449..efafb5583ac 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1038,17 +1038,6 @@ files-tests-file-name-non-special-make-directory
     (let ((default-directory nospecial-dir))
       (should-error (make-directory "dir")))))
 
-(ert-deftest files-tests-file-name-non-special-make-directory-internal ()
-  (files-tests--with-temp-non-special (tmpdir nospecial-dir t)
-    (let ((default-directory nospecial-dir))
-      (make-directory-internal "dir")
-      (should (file-directory-p "dir"))
-      (delete-directory "dir")))
-  (files-tests--with-temp-non-special-and-file-name-handler
-      (tmpdir nospecial-dir t)
-    (let ((default-directory nospecial-dir))
-      (should-error (make-directory-internal "dir")))))
-
 (ert-deftest files-tests-file-name-non-special-make-nearby-temp-file ()
   (let* ((default-directory (file-name-quote temporary-file-directory))
          (near-tmpfile (make-nearby-temp-file "file")))
-- 
2.38.1


[-- Attachment #3: 0002-make-directory-now-returns-t-if-dir-already-exists.patch --]
[-- Type: text/x-patch, Size: 6734 bytes --]

From b671e175372825c077ab4a1810e353845626c26a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 16 Dec 2022 14:55:48 -0800
Subject: [PATCH 2/3] make-directory now returns t if dir already exists

This new feature will help fix a copy-directory bug (Bug#58919).
Its implementation does not rely on make-directory handlers
supporting the new feature, as it no longer uses a make-directory
handler H in any way other than (funcall H DIR), thus using
only the intersection of the old and new behavior for handlers.
This will give us time to fix handlers at our leisure.
* lisp/files.el (files--ensure-directory): New arg MKDIR.
All uses changed.
(files--ensure-directory, make-directory):
Return non-nil if DIR is already a directory.  All uses changed.
* test/lisp/files-tests.el (files-tests-make-directory):
Test new return-value convention.
---
 doc/lispref/files.texi   |  3 +++
 etc/NEWS                 |  5 ++++
 lisp/files.el            | 58 +++++++++++++++++++++-------------------
 test/lisp/files-tests.el |  6 ++---
 4 files changed, 41 insertions(+), 31 deletions(-)

diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 5bcbac6ceb8..29d9e514c72 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -3205,6 +3205,9 @@ Create/Delete Dirs
 @var{parents} is non-@code{nil}, as is always the case in an
 interactive call, that means to create the parent directories first,
 if they don't already exist.
+As a function, @code{make-directory} returns non-@code{nil} if @var{dirname}
+already exists as a directory and @var{parents} is non-@code{nil},
+and returns @code{nil} if it successfully created @var{dirname}.
 @code{mkdir} is an alias for this.
 @end deffn
 
diff --git a/etc/NEWS b/etc/NEWS
index dbf1642206b..d59d61a9d25 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -4481,6 +4481,11 @@ and cannot work with regular styles such as 'basic' or 'flex'.
 ** Magic file handlers for make-directory-internal are no longer needed.
 Instead, Emacs uses the already-existing make-directory handlers.
 
++++
+** (make-directory DIR t) returns non-nil if DIR already exists.
+This can let a caller know whether it created DIR.  Formerly,
+make-directory's return value was unspecified.
+
 \f
 * Changes in Emacs 29.1 on Non-Free Operating Systems
 
diff --git a/lisp/files.el b/lisp/files.el
index c74e7e808e4..235eacee704 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6193,18 +6193,17 @@ rename-uniquely
       (rename-buffer (generate-new-buffer-name base-name))
       (force-mode-line-update))))
 
-(defun files--ensure-directory (dir)
-  "Make directory DIR if it is not already a directory.  Return nil."
+(defun files--ensure-directory (mkdir dir)
+  "Use function MKDIR to make directory DIR if it is not already a directory.
+Return non-nil if DIR is already a directory."
   (condition-case err
-      (make-directory-internal dir)
+      (funcall mkdir dir)
     (error
-     (unless (file-directory-p dir)
-       (signal (car err) (cdr err))))))
+     (or (file-directory-p dir)
+	 (signal (car err) (cdr err))))))
 
 (defun make-directory (dir &optional parents)
   "Create the directory DIR and optionally any nonexistent parent dirs.
-If DIR already exists as a directory, signal an error, unless
-PARENTS is non-nil.
 
 Interactively, the default choice of directory to create is the
 current buffer's default directory.  That is useful when you have
@@ -6214,8 +6213,9 @@ make-directory
 non-nil, says whether to create parent directories that don't
 exist.  Interactively, this happens by default.
 
-If creating the directory or directories fail, an error will be
-raised."
+Return non-nil if PARENTS is non-nil and DIR already exists as a
+directory, and nil if DIR did not already exist but was created.
+Signal an error if unsuccessful."
   (interactive
    (list (read-file-name "Make directory: " default-directory default-directory
 			 nil nil)
@@ -6223,25 +6223,27 @@ make-directory
   ;; If default-directory is a remote directory,
   ;; make sure we find its make-directory handler.
   (setq dir (expand-file-name dir))
-  (let ((handler (find-file-name-handler dir 'make-directory)))
-    (if handler
-	(funcall handler 'make-directory dir parents)
-      (if (not parents)
-	  (make-directory-internal dir)
-	(let ((dir (directory-file-name (expand-file-name dir)))
-	      create-list parent)
-	  (while (progn
-		   (setq parent (directory-file-name
-				 (file-name-directory dir)))
-		   (condition-case ()
-		       (files--ensure-directory dir)
-		     (file-missing
-		      ;; Do not loop if root does not exist (Bug#2309).
-		      (not (string= dir parent)))))
-	    (setq create-list (cons dir create-list)
-		  dir parent))
-	  (dolist (dir create-list)
-            (files--ensure-directory dir)))))))
+  (let ((mkdir (if-let ((handler (find-file-name-handler dir 'make-directory)))
+                   #'(lambda (dir) (funcall handler 'make-directory dir))
+                 #'make-directory-internal)))
+    (if (not parents)
+        (funcall mkdir dir)
+      (let ((dir (directory-file-name (expand-file-name dir)))
+            already-dir create-list parent)
+        (while (progn
+                 (setq parent (directory-file-name
+                               (file-name-directory dir)))
+                 (condition-case ()
+                     (ignore (setq already-dir
+                                   (files--ensure-directory mkdir dir)))
+                   (error
+                    ;; Do not loop if root does not exist (Bug#2309).
+                    (not (string= dir parent)))))
+          (setq create-list (cons dir create-list)
+                dir parent))
+        (dolist (dir create-list)
+          (setq already-dir (files--ensure-directory mkdir dir)))
+        already-dir))))
 
 (defun make-empty-file (filename &optional parents)
   "Create an empty file FILENAME.
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index efafb5583ac..b9fbeb8a4e0 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1261,11 +1261,11 @@ files-tests-make-directory
            (a/b (concat dirname "a/b")))
       (write-region "" nil file)
       (should-error (make-directory "/"))
-      (should-not (make-directory "/" t))
+      (should (make-directory "/" t))
       (should-error (make-directory dir))
-      (should-not (make-directory dir t))
+      (should (make-directory dir t))
       (should-error (make-directory dirname))
-      (should-not (make-directory dirname t))
+      (should (make-directory dirname t))
       (should-error (make-directory file))
       (should-error (make-directory file t))
       (should-not (make-directory subdir1))
-- 
2.38.1


[-- Attachment #4: 0003-Fix-copy-directory-bug-when-dest-dir-exists.patch --]
[-- Type: text/x-patch, Size: 3944 bytes --]

From 5b6435e1e57ae0c20ce3078ac1fe97a7757c4ba7 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@penguin.cs.ucla.edu>
Date: Fri, 16 Dec 2022 14:55:48 -0800
Subject: [PATCH 3/3] Fix copy-directory bug when dest dir exists
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/files.el (copy-directory): Set ‘follow’ depending on
whether we made the directory, not based on a guess that is
sometimes wrong.  When NEWNAME is a directory name and
COPY-CONTENTS is nil, do not object merely because the adjusted
NEWNAME is already a directory.  (Bug#58919).
* test/lisp/files-tests.el (files-tests-copy-directory):
Test for the bug.
---
 lisp/files.el            | 19 ++++++++++++-------
 test/lisp/files-tests.el |  9 ++++++++-
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 235eacee704..3cf7833ae02 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6437,7 +6437,7 @@ copy-directory
   ;; copy-directory handler.
   (let ((handler (or (find-file-name-handler directory 'copy-directory)
 		     (find-file-name-handler newname 'copy-directory)))
-	(follow parents))
+	follow)
     (if handler
 	(funcall handler 'copy-directory directory
                  newname keep-time parents copy-contents)
@@ -6457,19 +6457,24 @@ copy-directory
 				    t)
 	      (make-symbolic-link target newname t)))
         ;; Else proceed to copy as a regular directory
-        (cond ((not (directory-name-p newname))
+	;; first by creating the destination directory if needed,
+	;; preparing to follow any symlink to a directory we did not create.
+	(setq follow
+	    (if (not (directory-name-p newname))
 	       ;; If NEWNAME is not a directory name, create it;
 	       ;; that is where we will copy the files of DIRECTORY.
-	       (make-directory newname parents))
+	       (make-directory newname parents)
 	      ;; NEWNAME is a directory name.  If COPY-CONTENTS is non-nil,
 	      ;; create NEWNAME if it is not already a directory;
 	      ;; otherwise, create NEWNAME/[DIRECTORY-BASENAME].
-	      ((if copy-contents
-		   (or parents (not (file-directory-p newname)))
+	      (unless copy-contents
 	         (setq newname (concat newname
 				       (file-name-nondirectory directory))))
-	       (make-directory (directory-file-name newname) parents))
-	      (t (setq follow t)))
+	      (condition-case err
+		  (make-directory (directory-file-name newname) parents)
+		(error
+		 (or (file-directory-p newname)
+		     (signal (car err) (cdr err)))))))
 
         ;; Copy recursively.
         (dolist (file
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index b9fbeb8a4e0..011bfa67cc2 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1346,7 +1346,9 @@ files-tests-copy-directory
            (dest (concat dirname "dest/new/directory/"))
            (file (concat (file-name-as-directory source) "file"))
            (source2 (concat dirname "source2"))
-           (dest2 (concat dirname "dest/new2")))
+           (dest2 (concat dirname "dest/new2"))
+           (source3 (concat dirname "source3/d"))
+           (dest3 (concat dirname "dest3/d")))
       (make-directory source)
       (write-region "" nil file)
       (copy-directory source dest t t t)
@@ -1354,6 +1356,11 @@ files-tests-copy-directory
       (make-directory (concat (file-name-as-directory source2) "a") t)
       (copy-directory source2 dest2)
       (should (file-directory-p (concat (file-name-as-directory dest2) "a")))
+      (make-directory source3 t)
+      (write-region "x\n" nil (concat (file-name-as-directory source3) "file"))
+      (make-directory dest3 t)
+      (write-region "y\n" nil (concat (file-name-as-directory dest3) "file"))
+      (copy-directory source3 (file-name-directory dest3) t)
       (delete-directory dir 'recursive))))
 
 (ert-deftest files-tests-abbreviate-file-name-homedir ()
-- 
2.38.1

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Eli Zaretskii]

> Date: Fri, 16 Dec 2022 15:22:16 -0800
> Cc: thievol@posteo.net, 58919@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 12/11/22 02:46, Eli Zaretskii wrote:
> > Paul, did you have an opportunity to come up with the patch you
> > mentioned in this discussion?  I'd like to solve this bug for Emacs
> > 29, please.
> 
> I hacked on it for a bit and came up with the attached proposed patches 
> to the emacs-29 branch. I have not installed them.
> 
> These patches address the issues raised by Michael by passing only 
> single arguments to make-directory handlers. That way, we don't need to 
> worry about whether the handlers follow the new convention. At our 
> leisure, perhaps in Emacs 30, we can upgrade the make-directory handlers 
> to support the new convention.

Thanks.  Looks non-trivial, but I guess we cannot hope for a simpler
fix.

Michael, are you okay with this? do you see any problems, real or
potential, that could endanger the release of Emacs 29?

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli & Paul,

>> These patches address the issues raised by Michael by passing only
>> single arguments to make-directory handlers. That way, we don't need to
>> worry about whether the handlers follow the new convention. At our
>> leisure, perhaps in Emacs 30, we can upgrade the make-directory handlers
>> to support the new convention.
>
> Michael, are you okay with this? do you see any problems, real or
> potential, that could endanger the release of Emacs 29?

I've reviewed them, and in general it looks OK. Needs some testing, of
course.

Since file name handlers still raise an error in case DIR exists and
PARENTS is nil, we might see surprises in code assuming the new
behavior. I guess I'll add a change in tramp-*-handle-make-directory
like

--8<---------------cut here---------------start------------->8---
    (if (and (null parents) (file-exists-p dir))
	(if (>= emacs-major-version 29)
            t
	  (tramp-error v 'file-already-exists dir)))
--8<---------------cut here---------------end--------------->8---

And, of course, the return value (nil or t) must be added. But this
doesn't break compatibility, because until now no return value is
specified.

Similar changes to ange-ftp-make-directory.

tramp-test13-make-directory of tramp-tests.el must be adapted as well,
but this is minor. Will do.

These changes must be applied anyway, for Tramp's compatibility over
several Emacs versions.

In short, I guess we could add the patch to the emacs-29 branch.

Best regards, Michael.

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  thievol@posteo.net,
>   58919@debbugs.gnu.org
> Date: Sat, 17 Dec 2022 10:52:25 +0100
> 
> In short, I guess we could add the patch to the emacs-29 branch.

Thanks.

Paul, please go ahead and install this on the emacs-29 branch at your
earliest convenience.

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Paul Eggert]

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

On 12/17/22 01:52, Michael Albinus wrote:
> Since file name handlers still raise an error in case DIR exists and
> PARENTS is nil, we might see surprises in code assuming the new
> behavior. I guess I'll add a change in tramp-*-handle-make-directory
> like
>
> --8<---------------cut here---------------start------------->8---
>      (if (and (null parents) (file-exists-p dir))
> 	(if (>= emacs-major-version 29)
>              t
> 	  (tramp-error v 'file-already-exists dir)))
> --8<---------------cut here---------------end--------------->8---

That doesn't look right, as there is no change as to whether 
make-directory signals an error. Nor is there any change in behavior 
when PARENTS is nil. The only change in behavior is when PARENTS is 
non-nil and DIRECTORY already exists as a directory: in this case, Emacs 
29 returns non-nil whereas earlier Emacs returns (undocumented) nil.

So I think all that it would be nice to do is make sure the handlers 
ordinarily return nil, except that they return non-nil in the 
abovementioned special case. If a handler currently signals an error, it 
should continue to do so in the same way as it did before. Strictly 
speaking, modifying the handlers in this way won't affect whether they 
are compatible with Emacs 28- (since their return value is undocumented 
there) nor will it affect whether they work in Emacs 29 (since Emacs 29 
ignores their return value). But it might affect whether the handlers 
will work with Emacs 30, which might start assuming the Emacs 29 API for 
make-directory handlers.

Come to think of it, if an existing make-directory handler returns 
non-nil now (which it is allowed to in Emacs 28 as the return value is 
undocumented), then the proposed changes would sometimes have caused 
make-directory to return that non-nil value to its caller, even when the 
Emacs 29 doc says make-directory should return nil. As far as I know no 
such make-directory handler does so now, but to be safe I installed the 
attached additional patch to make sure Emacss 29 make-directory returns 
nil in this situation. As the combined set of patches should fix the 
original bug report I'm marking it as done.

In Emacs 30 we could remove this additional patch once we've fixed all 
the handlers, along with omitting some of the other code that supports 
calling Emacs 28-style handlers in Emacs 30 environments. Or we could 
leave it in; there's no rush, I imagine.

[-- Attachment #2: 0001-Don-t-assume-make-directory-handler-returns-nil.patch --]
[-- Type: text/x-patch, Size: 1381 bytes --]

From 4a8ff671b0e93e96f7fca4204cdbc83f99a3387c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 17 Dec 2022 14:09:03 -0800
Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20assume=20make-directory=20handl?=
 =?UTF-8?q?er=20returns=20nil?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/files.el (make-directory): Ignore what the make-directory
handler returns, as its return value was not documented in Emacs 28.
---
 lisp/files.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index 3cf7833ae02..cc7d7e2af94 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6224,7 +6224,12 @@ make-directory
   ;; make sure we find its make-directory handler.
   (setq dir (expand-file-name dir))
   (let ((mkdir (if-let ((handler (find-file-name-handler dir 'make-directory)))
-                   #'(lambda (dir) (funcall handler 'make-directory dir))
+		   #'(lambda (dir)
+		       ;; Use 'ignore' since the handler might be designed for
+		       ;; Emacs 28-, so it might return an (undocumented)
+		       ;; non-nil value, whereas the Emacs 29+ convention is
+		       ;; to return nil here.
+		       (ignore (funcall handler 'make-directory dir)))
                  #'make-directory-internal)))
     (if (not parents)
         (funcall mkdir dir)
-- 
2.38.1

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> On 12/17/22 01:52, Michael Albinus wrote:
>> Since file name handlers still raise an error in case DIR exists and
>> PARENTS is nil, we might see surprises in code assuming the new
>> behavior. I guess I'll add a change in tramp-*-handle-make-directory
>> like
>>
>> --8<---------------cut here---------------start------------->8---
>>      (if (and (null parents) (file-exists-p dir))
>> 	(if (>= emacs-major-version 29)
>>              t
>> 	  (tramp-error v 'file-already-exists dir)))
>> --8<---------------cut here---------------end--------------->8---
>
> That doesn't look right, as there is no change as to whether
> make-directory signals an error. Nor is there any change in behavior
> when PARENTS is nil. The only change in behavior is when PARENTS is
> non-nil and DIRECTORY already exists as a directory: in this case,
> Emacs 29 returns non-nil whereas earlier Emacs returns (undocumented)
> nil.

Hmm, you're right. I re-read your make-directory code, it looks like
PARENTS isn't propagated any longer to the file name handlers. So this
must be handled documented there, at least.

> So I think all that it would be nice to do is make sure the handlers
> ordinarily return nil, except that they return non-nil in the
> abovementioned special case. If a handler currently signals an error,
> it should continue to do so in the same way as it did before. Strictly
> speaking, modifying the handlers in this way won't affect whether they
> are compatible with Emacs 28- (since their return value is
> undocumented there) nor will it affect whether they work in Emacs 29
> (since Emacs 29 ignores their return value). But it might affect
> whether the handlers will work with Emacs 30, which might start
> assuming the Emacs 29 API for make-directory handlers.

Yep. Using the return value of the handlers shall be re-enabled in Emacs 30.

> Come to think of it, if an existing make-directory handler returns
> non-nil now (which it is allowed to in Emacs 28 as the return value is
> undocumented), then the proposed changes would sometimes have caused
> make-directory to return that non-nil value to its caller, even when
> the Emacs 29 doc says make-directory should return nil. As far as I
> know no such make-directory handler does so now, but to be safe I
> installed the attached additional patch to make sure Emacss 29
> make-directory returns nil in this situation. As the combined set of
> patches should fix the original bug report I'm marking it as done.
>
> In Emacs 30 we could remove this additional patch once we've fixed all
> the handlers, along with omitting some of the other code that supports
> calling Emacs 28-style handlers in Emacs 30 environments. Or we could
> leave it in; there's no rush, I imagine.

Yes, I will adapt the Tramp and ange-ftp handlers accordingly. Since the
return value is either undocumented, or (in Emacs 29) suppressed,
there's no harm.

For the time being, I have applied a small patch fixing an issue from
your last tramp-smb.el change.

Best regards, Michael.

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Paul Eggert]

On 12/18/22 11:35, Michael Albinus wrote:
> I re-read your make-directory code, it looks like
> PARENTS isn't propagated any longer to the file name handlers. So this
> must be handled documented there, at least.

Yes, the idea is that in Emacs 29, make-directory handlers never are 
passed a non-nil PARENTS flag, and their return values are always 
ignored. That way, Emacs 28 style make-directory handlers should work 
fine in Emacs 29 since only the intersection of the Emacs 28 and 29 
make-directory APIs is used when calling a make-directory handler.

In Emacs 30, once we've updated make-directory handlers to support the 
Emacs 29 make-directory API, we can simplify the code that calls these 
handlers.

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

>> I re-read your make-directory code, it looks like
>> PARENTS isn't propagated any longer to the file name handlers. So this
>> must be handled documented there, at least.
>
> Yes, the idea is that in Emacs 29, make-directory handlers never are
> passed a non-nil PARENTS flag, and their return values are always
> ignored. That way, Emacs 28 style make-directory handlers should work
> fine in Emacs 29 since only the intersection of the Emacs 28 and 29
> make-directory APIs is used when calling a make-directory handler.
>
> In Emacs 30, once we've updated make-directory handlers to support the
> Emacs 29 make-directory API, we can simplify the code that calls these
> handlers.

In Emacs 30, I have adapted tramp-*-make-directory and
ange-ftp-make-directory accordingly.

There's also url-dav-make-directory, but I don't know whether it is
still used. At least, I haven't been able to trigger it by a respective
"dav://..." URL. And it looks strange, because it ignores PARENTS, and
it doesn't raise an error in case the directory exists already. Hmmm.

There don't seem to be other file name handlers for make-directory in
core Emacs and in GNU ELPA packages. Do you want to simplify
make-directory accordingly?

Best regards, Michael.

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Paul Eggert]

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

On 12/23/22 02:26, Michael Albinus wrote:
> Do you want to simplify
> make-directory accordingly?

Sure, I installed the attached.

[-- Attachment #2: 0001-Assume-make-directory-handler-follows-new-API.patch --]
[-- Type: text/x-patch, Size: 3710 bytes --]

From cc2cc0c2971bf867283d1478bd0d99c2f420f982 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 24 Dec 2022 01:08:21 -0800
Subject: [PATCH] Assume make-directory handler follows new API

Suggested by Michael Albinus (Bug#58919#56).
* lisp/files.el (files--ensure-directory): Omit recently-added arg
MKDIR, since it is now always make-directory again.  All uses
changed.
(make-directory): Assume the make-directory handler follows the
new API where it yields non-nil if DIR already exists.  This
reverts some of the recent changes in this area, and simplifies
this funciton.
---
 lisp/files.el | 53 +++++++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index f352d3a9a7..0fb080b53c 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6193,11 +6193,11 @@ rename-uniquely
       (rename-buffer (generate-new-buffer-name base-name))
       (force-mode-line-update))))
 
-(defun files--ensure-directory (mkdir dir)
-  "Use function MKDIR to make directory DIR if it is not already a directory.
+(defun files--ensure-directory (dir)
+  "Make directory DIR if it is not already a directory.
 Return non-nil if DIR is already a directory."
   (condition-case err
-      (funcall mkdir dir)
+      (make-directory-internal dir)
     (error
      (or (file-directory-p dir)
 	 (signal (car err) (cdr err))))))
@@ -6223,32 +6223,27 @@ make-directory
   ;; If default-directory is a remote directory,
   ;; make sure we find its make-directory handler.
   (setq dir (expand-file-name dir))
-  (let ((mkdir (if-let ((handler (find-file-name-handler dir 'make-directory)))
-		   #'(lambda (dir)
-		       ;; Use 'ignore' since the handler might be designed for
-		       ;; Emacs 28-, so it might return an (undocumented)
-		       ;; non-nil value, whereas the Emacs 29+ convention is
-		       ;; to return nil here.
-		       (ignore (funcall handler 'make-directory dir)))
-                 #'make-directory-internal)))
-    (if (not parents)
-        (funcall mkdir dir)
-      (let ((dir (directory-file-name (expand-file-name dir)))
-            already-dir create-list parent)
-        (while (progn
-                 (setq parent (directory-file-name
-                               (file-name-directory dir)))
-                 (condition-case ()
-                     (ignore (setq already-dir
-                                   (files--ensure-directory mkdir dir)))
-                   (error
-                    ;; Do not loop if root does not exist (Bug#2309).
-                    (not (string= dir parent)))))
-          (setq create-list (cons dir create-list)
-                dir parent))
-        (dolist (dir create-list)
-          (setq already-dir (files--ensure-directory mkdir dir)))
-        already-dir))))
+  (let ((handler (find-file-name-handler dir 'make-directory)))
+    (if handler
+	(funcall handler 'make-directory dir parents)
+      (if (not parents)
+	  (make-directory-internal dir)
+	(let ((dir (directory-file-name (expand-file-name dir)))
+	      already-dir create-list parent)
+	  (while (progn
+		   (setq parent (directory-file-name
+				 (file-name-directory dir)))
+		   (condition-case ()
+		       (ignore (setq already-dir
+				     (files--ensure-directory dir)))
+		     (error
+		      ;; Do not loop if root does not exist (Bug#2309).
+		      (not (string= dir parent)))))
+	    (setq create-list (cons dir create-list)
+		  dir parent))
+	  (dolist (dir create-list)
+	    (setq already-dir (files--ensure-directory dir)))
+	  already-dir)))))
 
 (defun make-empty-file (filename &optional parents)
   "Create an empty file FILENAME.
-- 
2.38.1

bug#58919: 28.2; dired-copy-file-recursive fails to overwrite directory

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

>> Do you want to simplify
>> make-directory accordingly?
>
> Sure, I installed the attached.

Thanks. tramp-test13-make-directory (which I did change accordingly)
still succeeds, so I guess we're done.

Best regards, Michael.