GNU bug report logs - #58919
28.2; dired-copy-file-recursive fails to overwrite directory

Previous Next

Package: emacs;

Reported by: Thierry Volpiatto <thievol <at> posteo.net>

Date: Mon, 31 Oct 2022 09:13:02 UTC

Severity: normal

Merged with 58918

Found in version 28.2

Done: Paul Eggert <eggert <at> cs.ucla.edu>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 58919 in the body.
You can then email your comments to 58919 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Mon, 31 Oct 2022 09:13:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Thierry Volpiatto <thievol <at> posteo.net>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Mon, 31 Oct 2022 09:13:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thierry Volpiatto <thievol <at> posteo.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.2; dired-copy-file-recursive fails to overwrite directory
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/

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 <at> 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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Mon, 31 Oct 2022 13:03:01 GMT) Full text and rfc822 format available.

Message #8 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thierry Volpiatto <thievol <at> posteo.net>, Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 58919 <at> debbugs.gnu.org
Subject: Re: bug#58919: 28.2;
 dired-copy-file-recursive fails to overwrite directory
Date: Mon, 31 Oct 2022 15:01:47 +0200
merge 58919 58918
thanks

> From: Thierry Volpiatto <thievol <at> 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 <at> 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.




Merged 58918 58919. Request was from Eli Zaretskii <eliz <at> gnu.org> to control <at> debbugs.gnu.org. (Mon, 31 Oct 2022 13:03:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Tue, 01 Nov 2022 18:00:03 GMT) Full text and rfc822 format available.

Message #13 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Thierry Volpiatto <thievol <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 58919 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Tue, 01 Nov 2022 17:34:08 +0000
[Message part 1 (text/plain, inline)]
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 <at> gnu.org> writes:

> merge 58919 58918
> thanks
>
>> From: Thierry Volpiatto <thievol <at> 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 <at> 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
[signature.asc (application/pgp-signature, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Tue, 01 Nov 2022 18:05:02 GMT) Full text and rfc822 format available.

Message #16 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>, Thierry Volpiatto <thievol <at> posteo.net>
Cc: 58919 <at> debbugs.gnu.org
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Tue, 1 Nov 2022 11:04:22 -0700
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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Tue, 01 Nov 2022 18:10:02 GMT) Full text and rfc822 format available.

Message #19 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Tue, 01 Nov 2022 20:09:13 +0200
> Date: Tue, 1 Nov 2022 11:04:22 -0700
> Cc: 58919 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Tue, 01 Nov 2022 19:28:01 GMT) Full text and rfc822 format available.

Message #22 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Thierry Volpiatto <thievol <at> posteo.net>, 58919 <at> debbugs.gnu.org,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Tue, 01 Nov 2022 20:21:32 +0100
Paul Eggert <eggert <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Sun, 11 Dec 2022 10:47:02 GMT) Full text and rfc822 format available.

Message #25 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Sun, 11 Dec 2022 12:46:22 +0200
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 <at> gmx.de>
> Date: Tue, 01 Nov 2022 20:21:32 +0100
> Cc: Thierry Volpiatto <thievol <at> posteo.net>, 58919 <at> debbugs.gnu.org,
>  Eli Zaretskii <eliz <at> gnu.org>
> 
> Paul Eggert <eggert <at> 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.
> 
> 
> 
> 




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Fri, 16 Dec 2022 23:23:01 GMT) Full text and rfc822 format available.

Message #28 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>, Michael Albinus <michael.albinus <at> gmx.de>
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Fri, 16 Dec 2022 15:22:16 -0800
[Message part 1 (text/plain, inline)]
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.
[0001-Use-make-directory-handlers-uniformly.patch (text/x-patch, attachment)]
[0002-make-directory-now-returns-t-if-dir-already-exists.patch (text/x-patch, attachment)]
[0003-Fix-copy-directory-bug-when-dest-dir-exists.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Sat, 17 Dec 2022 08:05:01 GMT) Full text and rfc822 format available.

Message #31 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>, michael.albinus <at> gmx.de
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Sat, 17 Dec 2022 10:04:51 +0200
> Date: Fri, 16 Dec 2022 15:22:16 -0800
> Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> 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?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Sat, 17 Dec 2022 09:53:02 GMT) Full text and rfc822 format available.

Message #34 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Sat, 17 Dec 2022 10:52:25 +0100
Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Sat, 17 Dec 2022 10:41:02 GMT) Full text and rfc822 format available.

Message #37 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Sat, 17 Dec 2022 12:40:19 +0200
> From: Michael Albinus <michael.albinus <at> gmx.de>
> Cc: Paul Eggert <eggert <at> cs.ucla.edu>,  thievol <at> posteo.net,
>   58919 <at> 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.




Reply sent to Paul Eggert <eggert <at> cs.ucla.edu>:
You have taken responsibility. (Sat, 17 Dec 2022 22:41:02 GMT) Full text and rfc822 format available.

Notification sent to Thierry Volpiatto <thievol <at> posteo.net>:
bug acknowledged by developer. (Sat, 17 Dec 2022 22:41:02 GMT) Full text and rfc822 format available.

Message #42 received at 58919-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Albinus <michael.albinus <at> gmx.de>, Eli Zaretskii <eliz <at> gnu.org>
Cc: thievol <at> posteo.net, 58919-done <at> debbugs.gnu.org
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Sat, 17 Dec 2022 14:40:09 -0800
[Message part 1 (text/plain, inline)]
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.
[0001-Don-t-assume-make-directory-handler-returns-nil.patch (text/x-patch, attachment)]

Reply sent to Paul Eggert <eggert <at> cs.ucla.edu>:
You have taken responsibility. (Sat, 17 Dec 2022 22:41:02 GMT) Full text and rfc822 format available.

Notification sent to Skyler Mayfield <skyler544 <at> gmail.com>:
bug acknowledged by developer. (Sat, 17 Dec 2022 22:41:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Sun, 18 Dec 2022 19:36:01 GMT) Full text and rfc822 format available.

Message #50 received at 58919-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: thievol <at> posteo.net, Eli Zaretskii <eliz <at> gnu.org>,
 58919-done <at> debbugs.gnu.org
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Sun, 18 Dec 2022 20:35:34 +0100
Paul Eggert <eggert <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Sun, 18 Dec 2022 20:56:01 GMT) Full text and rfc822 format available.

Message #53 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Sun, 18 Dec 2022 12:54:59 -0800
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.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Fri, 23 Dec 2022 10:27:01 GMT) Full text and rfc822 format available.

Message #56 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Fri, 23 Dec 2022 11:26:30 +0100
Paul Eggert <eggert <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Sat, 24 Dec 2022 09:12:02 GMT) Full text and rfc822 format available.

Message #59 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Sat, 24 Dec 2022 01:11:39 -0800
[Message part 1 (text/plain, inline)]
On 12/23/22 02:26, Michael Albinus wrote:
> Do you want to simplify
> make-directory accordingly?

Sure, I installed the attached.
[0001-Assume-make-directory-handler-follows-new-API.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#58919; Package emacs. (Sat, 24 Dec 2022 10:14:02 GMT) Full text and rfc822 format available.

Message #62 received at 58919 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: thievol <at> posteo.net, 58919 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#58919: 28.2; dired-copy-file-recursive fails to overwrite
 directory
Date: Sat, 24 Dec 2022 11:13:35 +0100
Paul Eggert <eggert <at> 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.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 21 Jan 2023 12:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 67 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.