bug#28834: 25.1; dired-do-copy: allow to copy into a nonexistent directory

[Eli Zaretskii <eliz@gnu.org>]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 28834@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 16 Oct 2017 12:47:06 +0900
> 
> Thank you for the explanation.  Let's discuss the updated patch:

Thanks, I have a few comments:

> diff --git a/doc/emacs/dired.texi b/doc/emacs/dired.texi
> index db5dea329b..0f37ac60ac 100644
> --- a/doc/emacs/dired.texi
> +++ b/doc/emacs/dired.texi
> @@ -646,6 +646,16 @@ Operating on Files
>  Copy the specified files (@code{dired-do-copy}).  The argument @var{new}
>  is the directory to copy into, or (if copying a single file) the new
>  name.  This is like the shell command @code{cp}.
> +The option @var{dired-create-destination-dirs} controls whether Dired
> +should create non-existent directories in @var{new}.

This sentence is redundant (repeated right after it), and also uses
@var incorrectly.

> +@videnx dired-create-destination-dirs
   ^^^^^^^
A typo.

> +The option @code{dired-create-destination-dirs} controls whether Dired
> +should create non-existent directories in the destination while
> +copying/renaming files.  The default value @code{never} means Dired
> +never creates such missing directories;  the value @code{always},
> +means Dired automatically creates them; the value @code{prompt}
> +means Dired asks you for confirmation before creating them.

I think we generally use 'ask', not 'prompt' in these cases.

> diff --git a/etc/NEWS b/etc/NEWS
> index 716b0309a5..e5cec45426 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -56,6 +56,13 @@ whether '"' is also replaced in 'electric-quote-mode'.  If non-nil,
>  \f
>  * Changes in Specialized Modes and Packages in Emacs 27.1
>  
> +** Dired
> +
> +---

Since you've updated the manual, this should be "+++", not "---".

> +*** The new user option 'dired-create-destination-dirs' controls whether
> +'dired-do-copy' and 'dired-rename-file' must create non-existent
                                           ^^^^
"Should", not "must".

> +directories in the destination.
> +
>  ** Edebug
>  
>  +++
> diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
> index 7e2252fcf1..0e415b7738 100644
> --- a/lisp/dired-aux.el
> +++ b/lisp/dired-aux.el
> @@ -1548,6 +1548,26 @@ dired-copy-file
>  
>  (declare-function make-symbolic-link "fileio.c")
>  
> +(defcustom dired-create-destination-dirs 'never
> +  "Whether Dired should create destination dirs when copying/removing files.
> +If never, don't create them.
> +If always, create them without ask.
> +If prompt, ask for user confirmation."

The symbols should be quoted: `never', `always', etc.

Btw, perhaps it's better to use nil instead of 'never'.

> +(defun dired--create-dirs (dir)
> +  (unless (file-exists-p dir)
> +    (pcase dired-create-destination-dirs
> +      ('never nil)
> +      ('always (dired-create-directory dir))
> +      ('prompt
> +       (when (yes-or-no-p (format "Create destination dir '%s'? " dir))
> +         (dired-create-directory dir))))))

Is use of pcase really justified here?

> +(ert-deftest dired-test-bug28834 ()
> +  "test for https://debbugs.gnu.org/28834 ."
> +  (let* ((from (make-temp-file "from"))
> +         (foo (make-temp-file "foo" 'dir))
> +         (bar (file-name-as-directory (expand-file-name "bar" foo)))
> +         (qux (file-name-as-directory (expand-file-name "qux" foo)))
> +         (tmpdir temporary-file-directory)
> +         (to-cp (expand-file-name "foo-cp" bar))
> +         (to-mv (expand-file-name "foo-mv" qux))
> +         (dired-create-destination-dirs 'always))
> +    (unwind-protect
> +        (progn
> +          (dired-copy-file-recursive from to-cp nil)
> +          (should (file-exists-p to-cp))
> +          (dired-rename-file from to-mv nil)
> +          (should (file-exists-p to-mv))
> +          ;; Repeat the same with `dired-create-destination-dirs' set to 'never.
> +          (dired-rename-file to-mv from nil)
> +          (delete-file to-cp)
> +          (delete-directory bar)
> +          (delete-directory qux)
> +          (let ((dired-create-destination-dirs 'never))
> +            (should-error (dired-copy-file-recursive from to-cp nil))
> +            (should-error (dired-rename-file from to-mv nil))))

This doesn't test the 3rd value.  Why is that?