bug in copy-directory

bug in copy-directory

[Thierry Volpiatto]

Hi,
Actually on 23.2.92.1, copy-directory, called interactively or not copy
the files of directory A to existing directory B instead of copying
directory A inside directory B.(as a subdirectory of B).
e.g:

directory A:
foo
bar
baz

directory B exists and is empty.

M-x copy-directory A RET B RET

now in directory B:
foo
bar
baz

instead of
A

Note that the dired command work as expected `C'.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Chong Yidong]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Actually on 23.2.92.1, copy-directory, called interactively or not copy
> the files of directory A to existing directory B instead of copying
> directory A inside directory B.(as a subdirectory of B).

Hmm, this is a bit problematic.

The attached patch should fix the problem---Thierry, could you test?

The trouble is that Lisp callers might depend on the old behavior.  In
particular, I don't understand how Tramp interacts with copy-directory.
Michael, could you take a look and see if there is any problem?

Also, I noticed that the command doesn't prompt for overwriting files.
It probably ought to, at least when called interactively.

*** lisp/files.el	2011-01-24 20:34:44 +0000
--- lisp/files.el	2011-01-28 01:05:58 +0000
***************
*** 4756,4762 ****
        ;; Compute target name.
        (setq directory (directory-file-name (expand-file-name directory))
  	    newname   (directory-file-name (expand-file-name newname)))
!       (if (not (file-directory-p newname)) (make-directory newname parents))
  
        ;; Copy recursively.
        (mapc
--- 4756,4771 ----
        ;; Compute target name.
        (setq directory (directory-file-name (expand-file-name directory))
  	    newname   (directory-file-name (expand-file-name newname)))
! 
!       (if (not (file-directory-p newname))
! 	  (make-directory newname parents)
! 	;; If NEWNAME is an existing directory, we want to copy into
! 	;; NEWNAME/DIRECTORY.
! 	(setq newname (expand-file-name
! 		       (file-name-nondirectory
! 			(directory-file-name directory))
! 		       newname))
! 	(make-directory newname t))
  
        ;; Copy recursively.
        (mapc

Re: bug in copy-directory

[Thierry Volpiatto]

Chong Yidong <cyd@stupidchicken.com> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Actually on 23.2.92.1, copy-directory, called interactively or not copy
>> the files of directory A to existing directory B instead of copying
>> directory A inside directory B.(as a subdirectory of B).
>
> Hmm, this is a bit problematic.
>
> The attached patch should fix the problem---Thierry, could you test?
Yes it works, thanks.

> The trouble is that Lisp callers might depend on the old behavior.  In
> particular, I don't understand how Tramp interacts with copy-directory.


This part of code call the tramp handler

,----[ files.el copy-directory ]
|   ;; If default-directory is a remote directory, make sure we find its
|   ;; copy-directory handler.
|   (let ((handler (or (find-file-name-handler directory 'copy-directory)
| 		     (find-file-name-handler newname 'copy-directory))))
|     (if handler
| 	(funcall handler 'copy-directory directory newname keep-time parents)
`----

[EVAL] (assoc 'copy-directory tramp-file-name-handler-alist)


> Michael, could you take a look and see if there is any problem?


> Also, I noticed that the command doesn't prompt for overwriting files.
> It probably ought to, at least when called interactively.
It should prompt also to copy recursively dir.


Please could you send git-style patchs?

---
 lisp/files.el |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index ee77975..9eac9a3 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4756,7 +4756,16 @@ this happens by default."
       ;; Compute target name.
       (setq directory (directory-file-name (expand-file-name directory))
 	    newname   (directory-file-name (expand-file-name newname)))
-      (if (not (file-directory-p newname)) (make-directory newname parents))
+
+      (if (not (file-directory-p newname))
+	  (make-directory newname parents)
+	;; If NEWNAME is an existing directory, we want to copy into
+	;; NEWNAME/DIRECTORY.
+	(setq newname (expand-file-name
+		       (file-name-nondirectory
+			(directory-file-name directory))
+		       newname))
+	(make-directory newname t))
 
       ;; Copy recursively.
       (mapc

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Stefan Monnier]

>> Actually on 23.2.92.1, copy-directory, called interactively or not copy
>> the files of directory A to existing directory B instead of copying
>> directory A inside directory B.(as a subdirectory of B).

> Hmm, this is a bit problematic.
[...]
> The trouble is that Lisp callers might depend on the old behavior.

Couldn't/shouldn't we distinguish between interactive and
non-interactive calls, then?  I.e. have the interactive spec turn the
B into (expand-file-name (file-name-nondirectory A)) when B is an
existing directory.


        Stefan

Re: bug in copy-directory

[Chong Yidong]

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

>>> Actually on 23.2.92.1, copy-directory, called interactively or not copy
>>> the files of directory A to existing directory B instead of copying
>>> directory A inside directory B.(as a subdirectory of B).
>
>> Hmm, this is a bit problematic.
> [...]
>> The trouble is that Lisp callers might depend on the old behavior.
>
> Couldn't/shouldn't we distinguish between interactive and
> non-interactive calls, then?  I.e. have the interactive spec turn the
> B into (expand-file-name (file-name-nondirectory A)) when B is an
> existing directory.

Wouldn't that be confusing?  "Copy directory A to directory B" really
ought to mean the same as "cp -r a b".

Re: bug in copy-directory

[Lennart Borgman]

On Fri, Jan 28, 2011 at 5:30 PM, Chong Yidong <cyd@stupidchicken.com> wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>> Actually on 23.2.92.1, copy-directory, called interactively or not copy
>>>> the files of directory A to existing directory B instead of copying
>>>> directory A inside directory B.(as a subdirectory of B).
>>
>>> Hmm, this is a bit problematic.
>> [...]
>>> The trouble is that Lisp callers might depend on the old behavior.
>>
>> Couldn't/shouldn't we distinguish between interactive and
>> non-interactive calls, then?  I.e. have the interactive spec turn the
>> B into (expand-file-name (file-name-nondirectory A)) when B is an
>> existing directory.
>
> Wouldn't that be confusing?  "Copy directory A to directory B" really
> ought to mean the same as "cp -r a b".

Isn't the semantics of "cp" broken + undescribed?

I had a directory x1, but no x2. Doing

  cp -r x1 x2

works as I expect it to, i.e. x1 and x2 are identical.

However after a second

  cp -r x1 x2

there is suddenly a directory x1 inside x2.

I really dislike this kind of context specific semantics that is both
unintuitive and undescribed.

Re: bug in copy-directory

[Andreas Schwab]

Lennart Borgman <lennart.borgman@gmail.com> writes:

> I really dislike this kind of context specific semantics that is both
> unintuitive and undescribed.

Except that it's long-standing tradition and specified by POSIX.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: bug in copy-directory

[Lennart Borgman]

On Fri, Jan 28, 2011 at 6:05 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Lennart Borgman <lennart.borgman@gmail.com> writes:
>
>> I really dislike this kind of context specific semantics that is both
>> unintuitive and undescribed.
>
> Except that it's long-standing tradition and specified by POSIX.

I am not very impressed that POSIX specified it this way ;-)

Re: bug in copy-directory

[Andreas Schwab]

Lennart Borgman <lennart.borgman@gmail.com> writes:

> On Fri, Jan 28, 2011 at 6:05 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>> Lennart Borgman <lennart.borgman@gmail.com> writes:
>>
>>> I really dislike this kind of context specific semantics that is both
>>> unintuitive and undescribed.
>>
>> Except that it's long-standing tradition and specified by POSIX.
>
> I am not very impressed that POSIX specified it this way ;-)

POSIX just specifies existing practice.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: bug in copy-directory

[Thierry Volpiatto]

Lennart Borgman <lennart.borgman@gmail.com> writes:

> On Fri, Jan 28, 2011 at 5:30 PM, Chong Yidong <cyd@stupidchicken.com> wrote:
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>>>> Actually on 23.2.92.1, copy-directory, called interactively or not copy
>>>>> the files of directory A to existing directory B instead of copying
>>>>> directory A inside directory B.(as a subdirectory of B).
>>>
>>>> Hmm, this is a bit problematic.
>>> [...]
>>>> The trouble is that Lisp callers might depend on the old behavior.
>>>
>>> Couldn't/shouldn't we distinguish between interactive and
>>> non-interactive calls, then?  I.e. have the interactive spec turn the
>>> B into (expand-file-name (file-name-nondirectory A)) when B is an
>>> existing directory.
>>
>> Wouldn't that be confusing?  "Copy directory A to directory B" really
>> ought to mean the same as "cp -r a b".
>
> Isn't the semantics of "cp" broken + undescribed?
No

> I had a directory x1, but no x2. Doing
>
>   cp -r x1 x2
>
> works as I expect it to, i.e. x1 and x2 are identical.
>
> However after a second
>
>   cp -r x1 x2
>
> there is suddenly a directory x1 inside x2.
That's what is expected, imagine with what you expect, x2 is your home
directory or /etc, and you copy x1 to it by error...

> I really dislike this kind of context specific semantics that is both
> unintuitive and undescribed.
cp have a nice man page, you should read it.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Lennart Borgman]

On Fri, Jan 28, 2011 at 6:13 PM, Thierry Volpiatto
<thierry.volpiatto@gmail.com> wrote:
>>
>> Isn't the semantics of "cp" broken + undescribed?
> No

Yes ;-)

>> I had a directory x1, but no x2. Doing
>>
>>   cp -r x1 x2
>>
>> works as I expect it to, i.e. x1 and x2 are identical.
>>
>> However after a second
>>
>>   cp -r x1 x2
>>
>> there is suddenly a directory x1 inside x2.
> That's what is expected, imagine with what you expect, x2 is your home
> directory or /etc, and you copy x1 to it by error...

So? The urge to copy?

>> I really dislike this kind of context specific semantics that is both
>> unintuitive and undescribed.
> cp have a nice man page, you should read it.

I do not have it on w32. Just the output from cp --help and it does
not describe this behaviour.

Re: bug in copy-directory

[Thierry Volpiatto]

Lennart Borgman <lennart.borgman@gmail.com> writes:

> On Fri, Jan 28, 2011 at 6:13 PM, Thierry Volpiatto
> <thierry.volpiatto@gmail.com> wrote:
>>>
>>> Isn't the semantics of "cp" broken + undescribed?
>> No
>
> Yes ;-)
>
>>> I had a directory x1, but no x2. Doing
>>>
>>>   cp -r x1 x2
>>>
>>> works as I expect it to, i.e. x1 and x2 are identical.
>>>
>>> However after a second
>>>
>>>   cp -r x1 x2
>>>
>>> there is suddenly a directory x1 inside x2.
>> That's what is expected, imagine with what you expect, x2 is your home
>> directory or /etc, and you copy x1 to it by error...
>
> So? The urge to copy?
So x1 will overwrite your important directory and you will lost all.
So i think cp do the right thing, since a long time as said by Andreas.

>>> I really dislike this kind of context specific semantics that is both
>>> unintuitive and undescribed.
>> cp have a nice man page, you should read it.
>
> I do not have it on w32. Just the output from cp --help and it does
> not describe this behaviour.
You can install VirtualBox or similar and install a GNU/Linux system on
it ...

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Lennart Borgman]

On Fri, Jan 28, 2011 at 7:11 PM, Thierry Volpiatto
<thierry.volpiatto@gmail.com> wrote:
> Lennart Borgman <lennart.borgman@gmail.com> writes:
>
>> On Fri, Jan 28, 2011 at 6:13 PM, Thierry Volpiatto
>> <thierry.volpiatto@gmail.com> wrote:
>>>>
>>>> Isn't the semantics of "cp" broken + undescribed?
>>> No
>>
>> Yes ;-)
>>
>>>> I had a directory x1, but no x2. Doing
>>>>
>>>>   cp -r x1 x2
>>>>
>>>> works as I expect it to, i.e. x1 and x2 are identical.
>>>>
>>>> However after a second
>>>>
>>>>   cp -r x1 x2
>>>>
>>>> there is suddenly a directory x1 inside x2.
>>> That's what is expected, imagine with what you expect, x2 is your home
>>> directory or /etc, and you copy x1 to it by error...
>>
>> So? The urge to copy?
> So x1 will overwrite your important directory and you will lost all.
> So i think cp do the right thing, since a long time as said by Andreas.

Sure it does the same thing as before and POSIX did not have much of a
mandate to change things.

But there are other semantics in use for this problem. If you look at
the semantics used by w32 "GUI shell" (Explorer) it handles the
situation that x2 exist by creating a new directory with a new name.
(Emacs uses that kind of semantic for other things.)

I would guess that such "GUI shells" on *nix have similar semantic. Or
what do they do?

Re: bug in copy-directory

[Jan Djärv]

Lennart Borgman skrev 2011-01-28 19.28:

> But there are other semantics in use for this problem. If you look at
> the semantics used by w32 "GUI shell" (Explorer) it handles the
> situation that x2 exist by creating a new directory with a new name.
> (Emacs uses that kind of semantic for other things.)
>

Actually it does different things if you use C-c C-v or drag and drop.
For drag and drop it does not make a new directory.

> I would guess that such "GUI shells" on *nix have similar semantic. Or
> what do they do?

Nautilus, the Gnome "GUI shell" behaves the same as the W32 one.

That said, it would be a huge surprise to many if copy-directory didn't behave 
as cp.

	Jan D.

Re: bug in copy-directory

[Lennart Borgman]

On Fri, Jan 28, 2011 at 9:12 PM, Jan Djärv <jan.h.d@swipnet.se> wrote:
>
>
> Lennart Borgman skrev 2011-01-28 19.28:
>
>> But there are other semantics in use for this problem. If you look at
>> the semantics used by w32 "GUI shell" (Explorer) it handles the
>> situation that x2 exist by creating a new directory with a new name.
>> (Emacs uses that kind of semantic for other things.)
>>
>
> Actually it does different things if you use C-c C-v or drag and drop.
> For drag and drop it does not make a new directory.
>
>> I would guess that such "GUI shells" on *nix have similar semantic. Or
>> what do they do?
>
> Nautilus, the Gnome "GUI shell" behaves the same as the W32 one.
>
> That said, it would be a huge surprise to many if copy-directory didn't
> behave as cp.

Probably. But it is also a hug surprise to many that it does not
behave as most "GUI shell".

I think the latter group is growing. The first group might not be growing.

Re: bug in copy-directory

[Jan Djärv]

Lennart Borgman skrev 2011-01-28 21.48:
> On Fri, Jan 28, 2011 at 9:12 PM, Jan Djärv<jan.h.d@swipnet.se>  wrote:
>>
>> That said, it would be a huge surprise to many if copy-directory didn't
>> behave as cp.
>
> Probably. But it is also a hug surprise to many that it does not
> behave as most "GUI shell".
>
> I think the latter group is growing. The first group might not be growing.

Perhaps, but users of GUI shells tend to use them for copying files, not Emacs.

	Jan D.

Re: bug in copy-directory

[Lennart Borgman]

On Fri, Jan 28, 2011 at 11:44 PM, Jan Djärv <jan.h.d@swipnet.se> wrote:
>
>
> Lennart Borgman skrev 2011-01-28 21.48:
>>
>> On Fri, Jan 28, 2011 at 9:12 PM, Jan Djärv<jan.h.d@swipnet.se>  wrote:
>>>
>>> That said, it would be a huge surprise to many if copy-directory didn't
>>> behave as cp.
>>
>> Probably. But it is also a hug surprise to many that it does not
>> behave as most "GUI shell".
>>
>> I think the latter group is growing. The first group might not be growing.
>
> Perhaps, but users of GUI shells tend to use them for copying files, not
> Emacs.

Yes, maybe. I tend to use GUI shells for copying directory trees for
nearly exactly the problems we have been discussing here. (Lack of
threading is another problem.)

Re: bug in copy-directory

[Eli Zaretskii]

> From: Lennart Borgman <lennart.borgman@gmail.com>
> Date: Fri, 28 Jan 2011 18:17:33 +0100
> Cc: Chong Yidong <cyd@stupidchicken.com>,
> 	Michael Albinus <michael.albinus@gmx.de>,
> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> > cp have a nice man page, you should read it.
> 
> I do not have it on w32.

Sure you do, see man/cat1/cp.1.txt under the directory where you
installed the GnuWin32 port of Coreutils.  If you still don't find it,
download and install coreutils-X.Y.Z-doc.zip.

Re: bug in copy-directory

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lennart Borgman <lennart.borgman@gmail.com>
>> Date: Fri, 28 Jan 2011 18:17:33 +0100
>> Cc: Chong Yidong <cyd@stupidchicken.com>,
>> 	Michael Albinus <michael.albinus@gmx.de>,
>> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
>> 
>> > cp have a nice man page, you should read it.
>> 
>> I do not have it on w32.
>
> Sure you do, see man/cat1/cp.1.txt under the directory where you
> installed the GnuWin32 port of Coreutils.  If you still don't find it,
> download and install coreutils-X.Y.Z-doc.zip.
Anyway you can find it online...

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Stefan Monnier]

>>>> Actually on 23.2.92.1, copy-directory, called interactively or not copy
>>>> the files of directory A to existing directory B instead of copying
>>>> directory A inside directory B.(as a subdirectory of B).
>>> Hmm, this is a bit problematic.
>> [...]
>>> The trouble is that Lisp callers might depend on the old behavior.
>> Couldn't/shouldn't we distinguish between interactive and
>> non-interactive calls, then?  I.e. have the interactive spec turn the
>> B into (expand-file-name (file-name-nondirectory A)) when B is an
>> existing directory.

> Wouldn't that be confusing?  "Copy directory A to directory B" really
> ought to mean the same as "cp -r a b".

It depends if you expect it to behave like `cp' even for
non-interactive uses.

I don't think there's a traditional C-level equivalent to `cp', but at
least for `mv', the C-level API (aka `rename') does not behave like
`mv', but instead signals an error if the destination is
a pre-existing directory.

As Lennart points out, the semantics of `rename' are a bit less magical,
which tends to work well when you care about race-conditions and other
fun stuff.  OTOH out `copy-file' behaves like `cp', so I guess it's OK
for copy-directory to also always behave like `cp' even for
non-interactive uses.


        Stefan

Re: bug in copy-directory

[Chong Yidong]

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

> I don't think there's a traditional C-level equivalent to `cp', but at
> least for `mv', the C-level API (aka `rename') does not behave like
> `mv', but instead signals an error if the destination is
> a pre-existing directory.
>
> As Lennart points out, the semantics of `rename' are a bit less magical,
> which tends to work well when you care about race-conditions and other
> fun stuff.  OTOH out `copy-file' behaves like `cp', so I guess it's OK
> for copy-directory to also always behave like `cp' even for
> non-interactive uses.

I've commited the patch to the branch.  Dired seems to still work fine.
I haven't checked whether the Tramp handlers need fixing, though.

Re: bug in copy-directory

[Thierry Volpiatto]

Chong Yidong <cyd@stupidchicken.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> I don't think there's a traditional C-level equivalent to `cp', but at
>> least for `mv', the C-level API (aka `rename') does not behave like
>> `mv', but instead signals an error if the destination is
>> a pre-existing directory.
>>
>> As Lennart points out, the semantics of `rename' are a bit less magical,
>> which tends to work well when you care about race-conditions and other
>> fun stuff.  OTOH out `copy-file' behaves like `cp', so I guess it's OK
>> for copy-directory to also always behave like `cp' even for
>> non-interactive uses.
>
> I've commited the patch to the branch.  Dired seems to still work fine.
IIRC dired don't use copy-directory, but dired-create-files for copying,
renaming, linking.

> I haven't checked whether the Tramp handlers need fixing, though.
If copy-directory works, then the handler should work as it reuse
copy-directory.

Thanks for the fix, i will try it tomorrow (when it come in git).

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>> I've commited the patch to the branch.  Dired seems to still work fine.
> IIRC dired don't use copy-directory, but dired-create-files for copying,
> renaming, linking.

Nope. `dired-copy-file-recursive' uses `copy-directory'.

>> I haven't checked whether the Tramp handlers need fixing, though.
> If copy-directory works, then the handler should work as it reuse
> copy-directory.

Tramp has an own implementation of `copy-directory'.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>>> I've commited the patch to the branch.  Dired seems to still work fine.
>> IIRC dired don't use copy-directory, but dired-create-files for copying,
>> renaming, linking.
>
> Nope. `dired-copy-file-recursive' uses `copy-directory'.
Indeed yes, sorry.

>>> I haven't checked whether the Tramp handlers need fixing, though.
>> If copy-directory works, then the handler should work as it reuse
>> copy-directory.
>
> Tramp has an own implementation of `copy-directory'.
Yes i saw that, but doesn't this implementation
(tramp-handle-copy-directory) reuse copy-directory?

> Best regards, Michael.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>> Tramp has an own implementation of `copy-directory'.
> Yes i saw that, but doesn't this implementation
> (tramp-handle-copy-directory) reuse copy-directory?

Not always. Methods, marked with `tramp-copy-recursive' in
`tramp-methods', use an alternative implementation.

Best regards, Michael.

Re: bug in copy-directory

[Michael Albinus]

Chong Yidong <cyd@stupidchicken.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> I don't think there's a traditional C-level equivalent to `cp', but at
>> least for `mv', the C-level API (aka `rename') does not behave like
>> `mv', but instead signals an error if the destination is
>> a pre-existing directory.
>>
>> As Lennart points out, the semantics of `rename' are a bit less magical,
>> which tends to work well when you care about race-conditions and other
>> fun stuff.  OTOH out `copy-file' behaves like `cp', so I guess it's OK
>> for copy-directory to also always behave like `cp' even for
>> non-interactive uses.
>
> I've commited the patch to the branch.  Dired seems to still work fine.

Sorry for coming in late.

A year ago, there was the related Bug#5343. I've fixed that.

Your patch breaks recursive copy now. Extend the use case from that bug
report:

- Create directory /tmp/test
- Create directory /tmp/test/test
- Create file /tmp/test/a
- Create file /tmp/test/test/b

- Apply (copy-directory "/tmp/test" "~/")
  Everything is fine

- Apply again (copy-directory "/tmp/test" "~/")
  The target directory structure is broken.

> I haven't checked whether the Tramp handlers need fixing, though.

When possible, Tramp will use native means for recursive copy. Try
(copy-directory "/tmp/test" "/rsync::~/")

I would like to keep this behaviour.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Chong Yidong <cyd@stupidchicken.com> writes:
>
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>> I don't think there's a traditional C-level equivalent to `cp', but at
>>> least for `mv', the C-level API (aka `rename') does not behave like
>>> `mv', but instead signals an error if the destination is
>>> a pre-existing directory.
>>>
>>> As Lennart points out, the semantics of `rename' are a bit less magical,
>>> which tends to work well when you care about race-conditions and other
>>> fun stuff.  OTOH out `copy-file' behaves like `cp', so I guess it's OK
>>> for copy-directory to also always behave like `cp' even for
>>> non-interactive uses.
>>
>> I've commited the patch to the branch.  Dired seems to still work fine.
>
> Sorry for coming in late.
>
> A year ago, there was the related Bug#5343. I've fixed that.
>
> Your patch breaks recursive copy now. Extend the use case from that bug
> report:
>
> - Create directory /tmp/test
> - Create directory /tmp/test/test
> - Create file /tmp/test/a
> - Create file /tmp/test/test/b
>
> - Apply (copy-directory "/tmp/test" "~/")
>   Everything is fine
>
> - Apply again (copy-directory "/tmp/test" "~/")
>   The target directory structure is broken.

Following patch seems to work in these cases, could you check?

---
 lisp/files.el |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 4659742..eebdf7d 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,6 +4723,7 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
+
 (defun copy-directory (directory newname &optional keep-time parents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
 If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.
@@ -4754,24 +4755,17 @@ this happens by default."
 	(funcall handler 'copy-directory directory newname keep-time parents)
 
       ;; Compute target name.
-      (setq directory (directory-file-name (expand-file-name directory))
-	    newname   (directory-file-name (expand-file-name newname)))
-
-      (if (not (file-directory-p newname))
-	  ;; If NEWNAME is not an existing directory, create it; that
-	  ;; is where we will copy the files of DIRECTORY.
-	  (make-directory newname parents)
-	;; If NEWNAME is an existing directory, we will copy into
-	;; NEWNAME/[DIRECTORY-BASENAME].
-	(setq newname (expand-file-name
-		       (file-name-nondirectory
-			(directory-file-name directory))
-		       newname))
-	(if (and (file-exists-p newname)
-		 (not (file-directory-p newname)))
-	    (error "Cannot overwrite non-directory %s with a directory"
-		   newname))
-	(make-directory newname t))
+      ;; (setq directory (directory-file-name (expand-file-name directory))
+      ;;       newname   (directory-file-name (expand-file-name newname)))
+      ;; (if (not (file-directory-p newname)) (make-directory newname parents))
+      (setq directory (directory-file-name (expand-file-name directory)))
+      (setq newname   (directory-file-name (if (file-directory-p newname)
+                                               (expand-file-name
+                                                (file-relative-name directory
+                                                 (file-name-directory directory))
+                                                newname)
+                                               newname)))
+      (if (not (file-directory-p newname)) (make-directory newname parents))
 
       ;; Copy recursively.
       (mapc
@@ -4792,6 +4786,7 @@ this happens by default."
       (set-file-modes newname (file-modes directory))
       (if keep-time
 	  (set-file-times newname (nth 5 (file-attributes directory)))))))
+
 \f
 (put 'revert-buffer-function 'permanent-local t)
 (defvar revert-buffer-function nil


>> I haven't checked whether the Tramp handlers need fixing, though.
>
> When possible, Tramp will use native means for recursive copy. Try
> (copy-directory "/tmp/test" "/rsync::~/")
>
> I would like to keep this behaviour.
Patch above works also in this case (not tested with rsync method but
should be the same).

> Best regards, Michael.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Following patch seems to work in these cases, could you check?

Still the same problem.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Following patch seems to work in these cases, could you check?
>
> Still the same problem.
Ah! yes didn't notice it create a third /test nested in the second one.
 
-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>
>>> Following patch seems to work in these cases, could you check?
>>
>> Still the same problem.
> Ah! yes didn't notice it create a third /test nested in the second one.
Can you try this one now (interactively and not):
It should work.

---
 lisp/files.el |   87 ++++++++++++++++++++++++++++++---------------------------
 1 files changed, 46 insertions(+), 41 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 4659742..e465157 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,6 +4723,7 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
+
 (defun copy-directory (directory newname &optional keep-time parents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
 If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.
@@ -4748,50 +4749,54 @@ this happens by default."
 	   current-prefix-arg t)))
   ;; If default-directory is a remote directory, make sure we find its
   ;; copy-directory handler.
-  (let ((handler (or (find-file-name-handler directory 'copy-directory)
-		     (find-file-name-handler newname 'copy-directory))))
+  (let ((handler   (or (find-file-name-handler directory 'copy-directory)
+                       (find-file-name-handler newname 'copy-directory)))
+        (dir-files (directory-files directory 'full directory-files-no-dot-files-regexp)))
     (if handler
 	(funcall handler 'copy-directory directory newname keep-time parents)
 
-      ;; Compute target name.
-      (setq directory (directory-file-name (expand-file-name directory))
-	    newname   (directory-file-name (expand-file-name newname)))
-
-      (if (not (file-directory-p newname))
-	  ;; If NEWNAME is not an existing directory, create it; that
-	  ;; is where we will copy the files of DIRECTORY.
-	  (make-directory newname parents)
-	;; If NEWNAME is an existing directory, we will copy into
-	;; NEWNAME/[DIRECTORY-BASENAME].
-	(setq newname (expand-file-name
-		       (file-name-nondirectory
-			(directory-file-name directory))
-		       newname))
-	(if (and (file-exists-p newname)
-		 (not (file-directory-p newname)))
-	    (error "Cannot overwrite non-directory %s with a directory"
-		   newname))
-	(make-directory newname t))
-
-      ;; Copy recursively.
-      (mapc
-       (lambda (file)
-	 (let ((target (expand-file-name
-			(file-name-nondirectory file) newname))
-	       (attrs (file-attributes file)))
-	   (cond ((file-directory-p file)
-		  (copy-directory file target keep-time parents))
-		 ((stringp (car attrs)) ; Symbolic link
-		  (make-symbolic-link (car attrs) target t))
-		 (t
-		  (copy-file file target t keep-time)))))
-       ;; We do not want to copy "." and "..".
-       (directory-files	directory 'full directory-files-no-dot-files-regexp))
-
-      ;; Set directory attributes.
-      (set-file-modes newname (file-modes directory))
-      (if keep-time
-	  (set-file-times newname (nth 5 (file-attributes directory)))))))
+        ;; Compute target name.
+        ;; (setq directory (directory-file-name (expand-file-name directory))
+        ;;       newname   (directory-file-name (expand-file-name newname)))
+        ;; (if (not (file-directory-p newname)) (make-directory newname parents))
+        (setq directory (directory-file-name (expand-file-name directory)))
+        (setq newname   (directory-file-name (if (file-directory-p newname)
+                                                 (expand-file-name
+                                                  (file-relative-name directory
+                                                                      (file-name-directory directory))
+                                                  newname)
+                                                 newname)))
+        (if (file-directory-p newname)
+            (when (and (interactive-p)
+                       (y-or-n-p (format "Directory `%s' exists, overwrite? " newname)))
+              (delete-directory newname t)
+              (make-directory newname 'parents))
+            (make-directory newname 'parents))
+
+        (when (> (length dir-files) 0)
+          (let ((allow-recurse  (or (and (interactive-p)
+                                         (y-or-n-p (format "Recursive copies of `%s'? " directory)))
+                                    t))) ; always allow recursive copy in non--interactive calls.
+            (when allow-recurse
+              ;; Copy recursively.
+              (mapc
+               (lambda (file)
+                 (let ((target (expand-file-name
+                                (file-name-nondirectory file) newname))
+                       (attrs (file-attributes file)))
+                   (cond ((file-directory-p file)
+                          (copy-directory file target keep-time parents))
+                         ((stringp (car attrs)) ; Symbolic link
+                          (make-symbolic-link (car attrs) target t))
+                         (t
+                          (copy-file file target t keep-time)))))
+               dir-files))))
+
+        ;; Set directory attributes.
+        (set-file-modes newname (file-modes directory))
+        (if keep-time
+            (set-file-times newname (nth 5 (file-attributes directory)))))))
+
 \f
 (put 'revert-buffer-function 'permanent-local t)
 (defvar revert-buffer-function nil

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Can you try this one now (interactively and not):
> It should work.
I made some more fix, i will send when you have tried this one.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>> Ah! yes didn't notice it create a third /test nested in the second one.
> Can you try this one now (interactively and not):
> It should work.

I doesn't.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>>> Ah! yes didn't notice it create a third /test nested in the second one.
>> Can you try this one now (interactively and not):
>> It should work.
>
> I doesn't.
Sorry, this one work now interactively and not.

---
 lisp/files.el |   98 ++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 55 insertions(+), 43 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 4659742..464f00a 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,6 +4723,7 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
+
 (defun copy-directory (directory newname &optional keep-time parents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
 If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.
@@ -4748,50 +4749,61 @@ this happens by default."
 	   current-prefix-arg t)))
   ;; If default-directory is a remote directory, make sure we find its
   ;; copy-directory handler.
-  (let ((handler (or (find-file-name-handler directory 'copy-directory)
-		     (find-file-name-handler newname 'copy-directory))))
+  (let ((handler   (or (find-file-name-handler directory 'copy-directory)
+                       (find-file-name-handler newname 'copy-directory)))
+        (dir-files (directory-files directory 'full directory-files-no-dot-files-regexp)))
     (if handler
-	(funcall handler 'copy-directory directory newname keep-time parents)
-
-      ;; Compute target name.
-      (setq directory (directory-file-name (expand-file-name directory))
-	    newname   (directory-file-name (expand-file-name newname)))
-
-      (if (not (file-directory-p newname))
-	  ;; If NEWNAME is not an existing directory, create it; that
-	  ;; is where we will copy the files of DIRECTORY.
-	  (make-directory newname parents)
-	;; If NEWNAME is an existing directory, we will copy into
-	;; NEWNAME/[DIRECTORY-BASENAME].
-	(setq newname (expand-file-name
-		       (file-name-nondirectory
-			(directory-file-name directory))
-		       newname))
-	(if (and (file-exists-p newname)
-		 (not (file-directory-p newname)))
-	    (error "Cannot overwrite non-directory %s with a directory"
-		   newname))
-	(make-directory newname t))
-
-      ;; Copy recursively.
-      (mapc
-       (lambda (file)
-	 (let ((target (expand-file-name
-			(file-name-nondirectory file) newname))
-	       (attrs (file-attributes file)))
-	   (cond ((file-directory-p file)
-		  (copy-directory file target keep-time parents))
-		 ((stringp (car attrs)) ; Symbolic link
-		  (make-symbolic-link (car attrs) target t))
-		 (t
-		  (copy-file file target t keep-time)))))
-       ;; We do not want to copy "." and "..".
-       (directory-files	directory 'full directory-files-no-dot-files-regexp))
-
-      ;; Set directory attributes.
-      (set-file-modes newname (file-modes directory))
-      (if keep-time
-	  (set-file-times newname (nth 5 (file-attributes directory)))))))
+        (funcall handler 'copy-directory directory newname keep-time parents)
+        ;; Else Compute target name.
+        (setq directory (directory-file-name (expand-file-name directory)))
+        (setq newname   (directory-file-name
+                         (if (file-directory-p newname)
+                             (expand-file-name
+                              (file-relative-name
+                               directory (file-name-directory directory))
+                              newname)
+                             newname)))
+        (if (file-directory-p newname)
+            (if (interactive-p)
+                (if (y-or-n-p (format "Directory `%s' exists, overwrite? "
+                                      newname))
+                    (progn
+                      (delete-directory newname t)
+                      (make-directory newname 'parents))
+                    (error "Abort copying directory"))
+                (delete-directory newname t)
+                (make-directory newname 'parents))
+            (if (file-exists-p newname)
+                (error "Cannot overwrite non-directory %s with a directory"
+                       newname)
+                (make-directory newname 'parents)))
+
+        (when (> (length dir-files) 0)
+          (let ((allow-recurse (or (and (interactive-p)
+                                        (y-or-n-p
+                                         (format "Recursive copies of `%s'? "
+                                                 directory)))
+                                   t))) ; always allow recursive copy in non--interactive calls.
+            (when allow-recurse
+              ;; Copy recursively.
+              (mapc
+               (lambda (file)
+                 (let ((target (expand-file-name
+                                (file-name-nondirectory file) newname))
+                       (attrs (file-attributes file)))
+                   (cond ((file-directory-p file)
+                          (copy-directory file target keep-time parents))
+                         ((stringp (car attrs)) ; Symbolic link
+                          (make-symbolic-link (car attrs) target t))
+                         (t
+                          (copy-file file target t keep-time)))))
+               dir-files))))
+
+        ;; Set directory attributes.
+        (set-file-modes newname (file-modes directory))
+        (when keep-time
+          (set-file-times newname (nth 5 (file-attributes directory)))))))
+
 \f
 (put 'revert-buffer-function 'permanent-local t)
 (defvar revert-buffer-function nil


-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>
>>>> Ah! yes didn't notice it create a third /test nested in the second one.
>>> Can you try this one now (interactively and not):
>>> It should work.
>>
>> I doesn't.
> Sorry, this one work now interactively and not.
Just a little error remaining when answering no for recursive copy of
dir.
It is now fixed, will send in next patch.

> ---
>  lisp/files.el |   98 ++++++++++++++++++++++++++++++++-------------------------
>  1 files changed, 55 insertions(+), 43 deletions(-)
>
> diff --git a/lisp/files.el b/lisp/files.el
> index 4659742..464f00a 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -4723,6 +4723,7 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
>  		 directory 'full directory-files-no-dot-files-regexp)))
>        (delete-directory-internal directory)))))
>  
> +
>  (defun copy-directory (directory newname &optional keep-time parents)
>    "Copy DIRECTORY to NEWNAME.  Both args must be strings.
>  If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.
> @@ -4748,50 +4749,61 @@ this happens by default."
>  	   current-prefix-arg t)))
>    ;; If default-directory is a remote directory, make sure we find its
>    ;; copy-directory handler.
> -  (let ((handler (or (find-file-name-handler directory 'copy-directory)
> -		     (find-file-name-handler newname 'copy-directory))))
> +  (let ((handler   (or (find-file-name-handler directory 'copy-directory)
> +                       (find-file-name-handler newname 'copy-directory)))
> +        (dir-files (directory-files directory 'full directory-files-no-dot-files-regexp)))
>      (if handler
> -	(funcall handler 'copy-directory directory newname keep-time parents)
> -
> -      ;; Compute target name.
> -      (setq directory (directory-file-name (expand-file-name directory))
> -	    newname   (directory-file-name (expand-file-name newname)))
> -
> -      (if (not (file-directory-p newname))
> -	  ;; If NEWNAME is not an existing directory, create it; that
> -	  ;; is where we will copy the files of DIRECTORY.
> -	  (make-directory newname parents)
> -	;; If NEWNAME is an existing directory, we will copy into
> -	;; NEWNAME/[DIRECTORY-BASENAME].
> -	(setq newname (expand-file-name
> -		       (file-name-nondirectory
> -			(directory-file-name directory))
> -		       newname))
> -	(if (and (file-exists-p newname)
> -		 (not (file-directory-p newname)))
> -	    (error "Cannot overwrite non-directory %s with a directory"
> -		   newname))
> -	(make-directory newname t))
> -
> -      ;; Copy recursively.
> -      (mapc
> -       (lambda (file)
> -	 (let ((target (expand-file-name
> -			(file-name-nondirectory file) newname))
> -	       (attrs (file-attributes file)))
> -	   (cond ((file-directory-p file)
> -		  (copy-directory file target keep-time parents))
> -		 ((stringp (car attrs)) ; Symbolic link
> -		  (make-symbolic-link (car attrs) target t))
> -		 (t
> -		  (copy-file file target t keep-time)))))
> -       ;; We do not want to copy "." and "..".
> -       (directory-files	directory 'full directory-files-no-dot-files-regexp))
> -
> -      ;; Set directory attributes.
> -      (set-file-modes newname (file-modes directory))
> -      (if keep-time
> -	  (set-file-times newname (nth 5 (file-attributes directory)))))))
> +        (funcall handler 'copy-directory directory newname keep-time parents)
> +        ;; Else Compute target name.
> +        (setq directory (directory-file-name (expand-file-name directory)))
> +        (setq newname   (directory-file-name
> +                         (if (file-directory-p newname)
> +                             (expand-file-name
> +                              (file-relative-name
> +                               directory (file-name-directory directory))
> +                              newname)
> +                             newname)))
> +        (if (file-directory-p newname)
> +            (if (interactive-p)
> +                (if (y-or-n-p (format "Directory `%s' exists, overwrite? "
> +                                      newname))
> +                    (progn
> +                      (delete-directory newname t)
> +                      (make-directory newname 'parents))
> +                    (error "Abort copying directory"))
> +                (delete-directory newname t)
> +                (make-directory newname 'parents))
> +            (if (file-exists-p newname)
> +                (error "Cannot overwrite non-directory %s with a directory"
> +                       newname)
> +                (make-directory newname 'parents)))
> +
> +        (when (> (length dir-files) 0)
> +          (let ((allow-recurse (or (and (interactive-p)
> +                                        (y-or-n-p
> +                                         (format "Recursive copies of `%s'? "
> +                                                 directory)))
> +                                   t))) ; always allow recursive copy in non--interactive calls.
> +            (when allow-recurse
> +              ;; Copy recursively.
> +              (mapc
> +               (lambda (file)
> +                 (let ((target (expand-file-name
> +                                (file-name-nondirectory file) newname))
> +                       (attrs (file-attributes file)))
> +                   (cond ((file-directory-p file)
> +                          (copy-directory file target keep-time parents))
> +                         ((stringp (car attrs)) ; Symbolic link
> +                          (make-symbolic-link (car attrs) target t))
> +                         (t
> +                          (copy-file file target t keep-time)))))
> +               dir-files))))
> +
> +        ;; Set directory attributes.
> +        (set-file-modes newname (file-modes directory))
> +        (when keep-time
> +          (set-file-times newname (nth 5 (file-attributes directory)))))))
> +
>  \f
>  (put 'revert-buffer-function 'permanent-local t)
>  (defvar revert-buffer-function nil

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>
>>>> Ah! yes didn't notice it create a third /test nested in the second one.
>>> Can you try this one now (interactively and not):
>>> It should work.
>>
>> It doesn't.
> Sorry, this one work now interactively and not.

It still doesn't.

What do you think about testing yourself? I have given the recipe.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Michael Albinus <michael.albinus@gmx.de> writes:
>>
>>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>>
>>>>> Ah! yes didn't notice it create a third /test nested in the second one.
>>>> Can you try this one now (interactively and not):
>>>> It should work.
>>>
>>> It doesn't.
>> Sorry, this one work now interactively and not.
>
> It still doesn't.
It work here.

> What do you think about testing yourself? I have given the recipe.
Of course i did:

,----
| - Create directory /tmp/test/test
| - Create file /tmp/test/a
| - Create file /tmp/test/test/b
| 
| - Apply (copy-directory "/tmp/test" "~/")
|   Everything is fine
| 
| - Apply again (copy-directory "/tmp/test" "~/")
|   The target directory structure is broken.
`----


,----
| - _Create the structure in /tmp_
| 
| (make-directory "/tmp/test")
| nil
| 
| (make-directory "/tmp/test/test")
| nil
| 
| (with-current-buffer (find-file-noselect "/tmp/test/a")
|   (insert "test1")
|   (save-buffer))
| 
| (with-current-buffer (find-file-noselect "/tmp/test/test/b")
|   (insert "test2")
|   (save-buffer))
| 
| (directory-files "/tmp/test")
| ("." ".." "a" "test")
| 
| (directory-files "/tmp/test/test")
| ("." ".." "b")
| 
| - _Copy the structure in ~/_
| 
| (copy-directory "/tmp/test" "~/")
| nil
| 
| (directory-files "~/test")
| ("." ".." "a" "test")
| 
| (directory-files "~/test/test")
| ("." ".." "b")
| 
| - _Copy again the same structure in ~/ (Overwrite)_
| 
| (copy-directory "/tmp/test" "~/")
| nil
| 
| (directory-files "~/test")
| ("." ".." "a" "test")
| 
| (directory-files "~/test/test")
| ("." ".." "b")
`----

As you can see the structure is always the same.
Or did i miss something?

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>
>>> Michael Albinus <michael.albinus@gmx.de> writes:
>>>
>>>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>>>
>>>>>> Ah! yes didn't notice it create a third /test nested in the second one.
>>>>> Can you try this one now (interactively and not):
>>>>> It should work.
>>>>
>>>> It doesn't.
>>> Sorry, this one work now interactively and not.
>>
>> It still doesn't.
> It work here.
>
>> What do you think about testing yourself? I have given the recipe.
> Of course i did:
>
> ,----
> | - Create directory /tmp/test/test
> | - Create file /tmp/test/a
> | - Create file /tmp/test/test/b
> | 
> | - Apply (copy-directory "/tmp/test" "~/")
> |   Everything is fine
> | 
> | - Apply again (copy-directory "/tmp/test" "~/")
> |   The target directory structure is broken.
> `----
>
>
> ,----
> | - _Create the structure in /tmp_
> | 
> | (make-directory "/tmp/test")
> | nil
> | 
> | (make-directory "/tmp/test/test")
> | nil
> | 
> | (with-current-buffer (find-file-noselect "/tmp/test/a")
> |   (insert "test1")
> |   (save-buffer))
> | 
> | (with-current-buffer (find-file-noselect "/tmp/test/test/b")
> |   (insert "test2")
> |   (save-buffer))
> | 
> | (directory-files "/tmp/test")
> | ("." ".." "a" "test")
> | 
> | (directory-files "/tmp/test/test")
> | ("." ".." "b")
> | 
> | - _Copy the structure in ~/_
> | 
> | (copy-directory "/tmp/test" "~/")
> | nil
> | 
> | (directory-files "~/test")
> | ("." ".." "a" "test")
> | 
> | (directory-files "~/test/test")
> | ("." ".." "b")
> | 
> | - _Copy again the same structure in ~/ (Overwrite)_
> | 
> | (copy-directory "/tmp/test" "~/")
> | nil
> | 
> | (directory-files "~/test")
> | ("." ".." "a" "test")
> | 
> | (directory-files "~/test/test")
> | ("." ".." "b")
> `----
>
> As you can see the structure is always the same.
> Or did i miss something?

So here my last patch, well it's work fine here, i have tested with many
cases and the recipe above.

---
 lisp/files.el |   99 ++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 56 insertions(+), 43 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 4659742..a63a448 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,6 +4723,7 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
+
 (defun copy-directory (directory newname &optional keep-time parents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
 If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.
@@ -4748,50 +4749,62 @@ this happens by default."
 	   current-prefix-arg t)))
   ;; If default-directory is a remote directory, make sure we find its
   ;; copy-directory handler.
-  (let ((handler (or (find-file-name-handler directory 'copy-directory)
-		     (find-file-name-handler newname 'copy-directory))))
+  (let ((handler   (or (find-file-name-handler directory 'copy-directory)
+                       (find-file-name-handler newname 'copy-directory)))
+        (dir-files (directory-files directory 'full directory-files-no-dot-files-regexp)))
     (if handler
-	(funcall handler 'copy-directory directory newname keep-time parents)
-
-      ;; Compute target name.
-      (setq directory (directory-file-name (expand-file-name directory))
-	    newname   (directory-file-name (expand-file-name newname)))
-
-      (if (not (file-directory-p newname))
-	  ;; If NEWNAME is not an existing directory, create it; that
-	  ;; is where we will copy the files of DIRECTORY.
-	  (make-directory newname parents)
-	;; If NEWNAME is an existing directory, we will copy into
-	;; NEWNAME/[DIRECTORY-BASENAME].
-	(setq newname (expand-file-name
-		       (file-name-nondirectory
-			(directory-file-name directory))
-		       newname))
-	(if (and (file-exists-p newname)
-		 (not (file-directory-p newname)))
-	    (error "Cannot overwrite non-directory %s with a directory"
-		   newname))
-	(make-directory newname t))
-
-      ;; Copy recursively.
-      (mapc
-       (lambda (file)
-	 (let ((target (expand-file-name
-			(file-name-nondirectory file) newname))
-	       (attrs (file-attributes file)))
-	   (cond ((file-directory-p file)
-		  (copy-directory file target keep-time parents))
-		 ((stringp (car attrs)) ; Symbolic link
-		  (make-symbolic-link (car attrs) target t))
-		 (t
-		  (copy-file file target t keep-time)))))
-       ;; We do not want to copy "." and "..".
-       (directory-files	directory 'full directory-files-no-dot-files-regexp))
-
-      ;; Set directory attributes.
-      (set-file-modes newname (file-modes directory))
-      (if keep-time
-	  (set-file-times newname (nth 5 (file-attributes directory)))))))
+        (funcall handler 'copy-directory directory newname keep-time parents)
+        ;; Else Compute target name.
+        (setq directory (directory-file-name (expand-file-name directory)))
+        (setq newname   (directory-file-name
+                         (if (file-directory-p newname)
+                             (expand-file-name
+                              (file-relative-name
+                               directory (file-name-directory directory))
+                              newname)
+                             newname)))
+        (if (file-directory-p newname)
+            (if (interactive-p)
+                (if (y-or-n-p (format "Directory `%s' exists, overwrite? "
+                                      newname))
+                    (progn
+                      (delete-directory newname t)
+                      (make-directory newname 'parents))
+                    (error "Abort copying directory"))
+                (delete-directory newname t)
+                (make-directory newname 'parents))
+            (if (file-exists-p newname)
+                (error "Cannot overwrite non-directory %s with a directory"
+                       newname)
+                (make-directory newname 'parents)))
+
+        (when (> (length dir-files) 0)
+          (let ((allow-recurse (or (and (interactive-p)
+                                        (y-or-n-p
+                                         (format "Recursive copies of `%s'? "
+                                                 directory)))
+                                   ;; Always allow recursive copy in non--interactive calls.
+                                   (not (interactive-p)))))
+            (when allow-recurse
+              ;; Copy recursively.
+              (mapc
+               (lambda (file)
+                 (let ((target (expand-file-name
+                                (file-name-nondirectory file) newname))
+                       (attrs (file-attributes file)))
+                   (cond ((file-directory-p file)
+                          (copy-directory file target keep-time parents))
+                         ((stringp (car attrs)) ; Symbolic link
+                          (make-symbolic-link (car attrs) target t))
+                         (t
+                          (copy-file file target t keep-time)))))
+               dir-files))))
+
+        ;; Set directory attributes.
+        (set-file-modes newname (file-modes directory))
+        (when keep-time
+          (set-file-times newname (nth 5 (file-attributes directory)))))))
+
 \f
 (put 'revert-buffer-function 'permanent-local t)
 (defvar revert-buffer-function nil

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Chong Yidong]

Michael Albinus <michael.albinus@gmx.de> writes:

> Your patch breaks recursive copy now. Extend the use case from that bug
> report:
>
> - Create directory /tmp/test
> - Create directory /tmp/test/test
> - Create file /tmp/test/a
> - Create file /tmp/test/test/b
>
> - Apply (copy-directory "/tmp/test" "~/")
>   Everything is fine
>
> - Apply again (copy-directory "/tmp/test" "~/")
>   The target directory structure is broken.

I think I see the problem: the arguments to the recursive call to
copy-directory were not taking the "copy as a subdirectory" behavior
into account.  I've committed a fix.

Re: bug in copy-directory

[Michael Albinus]

Chong Yidong <cyd@stupidchicken.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Your patch breaks recursive copy now. Extend the use case from that bug
>> report:
>>
>> - Create directory /tmp/test
>> - Create directory /tmp/test/test
>> - Create file /tmp/test/a
>> - Create file /tmp/test/test/b
>>
>> - Apply (copy-directory "/tmp/test" "~/")
>>   Everything is fine
>>
>> - Apply again (copy-directory "/tmp/test" "~/")
>>   The target directory structure is broken.
>
> I think I see the problem: the arguments to the recursive call to
> copy-directory were not taking the "copy as a subdirectory" behavior
> into account.  I've committed a fix.

It works fine, thank you. But there seems to be a bug in dired; when I
copy "/tmp/test" to "~/" twice, I see the corrupted directory structure.

(This seems to be the misunderstanding between Thierry and me, 'cause I
have used dired for my tests).

I've tested also

(copy-directory "/ssh::/tmp/test" "/ssh::~/")

(copy-directory "/rsync::/tmp/test" "/rsync::~/")

In the ssh case, Tramp falls back to the default implementation. For
rsync, there is an own implementation. Both tests are successful; Tramp
seems to be OK.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Chong Yidong <cyd@stupidchicken.com> writes:
>
>> Michael Albinus <michael.albinus@gmx.de> writes:
>>
>>> Your patch breaks recursive copy now. Extend the use case from that bug
>>> report:
>>>
>>> - Create directory /tmp/test
>>> - Create directory /tmp/test/test
>>> - Create file /tmp/test/a
>>> - Create file /tmp/test/test/b
>>>
>>> - Apply (copy-directory "/tmp/test" "~/")
>>>   Everything is fine
>>>
>>> - Apply again (copy-directory "/tmp/test" "~/")
>>>   The target directory structure is broken.
>>
>> I think I see the problem: the arguments to the recursive call to
>> copy-directory were not taking the "copy as a subdirectory" behavior
>> into account.  I've committed a fix.
>
> It works fine, thank you. But there seems to be a bug in dired; when I
> copy "/tmp/test" to "~/" twice, I see the corrupted directory structure.
Yes i think i found the cause:
When dired give the destination name to copy-directory, the destination
name is already computed like:
     	(setq newname (expand-file-name
		       (file-name-nondirectory
			(directory-file-name directory))
		       newname))

and copy-directory recompute it in the same way.

So from dired when you do:
copy directory /tmp/test to ~/

copy-directory does:
(copy-directory "/tmp/test" "~/test")

and then compute ~/test to ~/test/test.

It should do (copy-directory "/tmp/test" "~/")

> (This seems to be the misunderstanding between Thierry and me, 'cause I
> have used dired for my tests).
>
> I've tested also
>
> (copy-directory "/ssh::/tmp/test" "/ssh::~/")
>
> (copy-directory "/rsync::/tmp/test" "/rsync::~/")
>
> In the ssh case, Tramp falls back to the default implementation. For
> rsync, there is an own implementation. Both tests are successful; Tramp
> seems to be OK.
Nice.

> Best regards, Michael.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Chong Yidong <cyd@stupidchicken.com> writes:
>
>> Michael Albinus <michael.albinus@gmx.de> writes:

> It works fine, thank you. But there seems to be a bug in dired; when I
> copy "/tmp/test" to "~/" twice, I see the corrupted directory structure.
Michael and Chong, could you try on your side, using the last version of
copy-directory, modify the call to dired-create-files in
dired-do-create-files something like this: (not tested in dired yet)

    (dired-create-files
     fn (symbol-name action) files
     ;; CANDIDATE is the destination.
     (if (file-directory-p candidate)
         ;; When CANDIDATE is a directory, build file-name in this directory.
         ;; Else we use CANDIDATE.
         #'(lambda (from)
             (let ((target (expand-file-name (file-name-nondirectory from) candidate)))
               (if (file-directory-p target) candidate target)))
         #'(lambda (from) candidate))
     marker)

NOTE: the message "Overwrite ..." will be wrong, ignore it for the
moment (use non--important dirs to be sure).


-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Chong Yidong <cyd@stupidchicken.com> writes:
>
>> Michael Albinus <michael.albinus@gmx.de> writes:
> It works fine, thank you. But there seems to be a bug in dired; when I
> copy "/tmp/test" to "~/" twice, I see the corrupted directory structure.

What we could do also is create a function copy-directory-contents, that
reuse the code of precedent version of copy-directory (but not
interactive this time) and call this function in
dired-copy-file-recursive instead of copy-directory.

In this case we could rewrite copy-directory to avoid duplicate code,
possibly: (no urge in this case as actual copy-directory works fine) 

1) Writing copy-directory-contents (or whatever name) to allow
creating the structure like actual copy-directory, or not like ancient
version leaving this job to dired-create-files.

2) Writing a copy-directory that reuse dired code (i.e dired-create-files).
In this case it would have all interactive messages, ask etc..

WDYT?

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> What we could do also is create a function copy-directory-contents, that
> reuse the code of precedent version of copy-directory (but not
> interactive this time) and call this function in
> dired-copy-file-recursive instead of copy-directory.
>
> In this case we could rewrite copy-directory to avoid duplicate code,
> possibly: (no urge in this case as actual copy-directory works fine) 
>
> 1) Writing copy-directory-contents (or whatever name) to allow
> creating the structure like actual copy-directory, or not like ancient
> version leaving this job to dired-create-files.
>
> 2) Writing a copy-directory that reuse dired code (i.e dired-create-files).
> In this case it would have all interactive messages, ask etc..
>
> WDYT?

I would prefer option 2). There is no need to have an extra dired
implementation, now we have copy-directory.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> What we could do also is create a function copy-directory-contents, that
>> reuse the code of precedent version of copy-directory (but not
>> interactive this time) and call this function in
>> dired-copy-file-recursive instead of copy-directory.
>>
>> In this case we could rewrite copy-directory to avoid duplicate code,
>> possibly: (no urge in this case as actual copy-directory works fine) 
>>
>> 1) Writing copy-directory-contents (or whatever name) to allow
>> creating the structure like actual copy-directory, or not like ancient
>> version leaving this job to dired-create-files.
>>
>> 2) Writing a copy-directory that reuse dired code (i.e dired-create-files).
>> In this case it would have all interactive messages, ask etc..
>>
>> WDYT?
>
> I would prefer option 2). There is no need to have an extra dired
> implementation, now we have copy-directory.

So i made first steps:

- Create new function copy-directory-contents based on old
copy-directory code.

- Use it in dired-copy-file-recursive instead of copy-directory.

Seems to work fine.

---
 lisp/dired-aux.el |    2 +-
 lisp/files.el     |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 28b285f..f7a356d 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -1242,7 +1242,7 @@ Special value `always' suppresses confirmation."
 	     (or (eq recursive 'always)
 		 (yes-or-no-p (format "Recursive copies of %s? " from))))
 	;; This is a directory.
-	(copy-directory from to dired-copy-preserve-time)
+	(copy-directory-contents from to dired-copy-preserve-time)
       ;; Not a directory.
       (or top (dired-handle-overwrite to))
       (condition-case err
diff --git a/lisp/files.el b/lisp/files.el
index d896020..526372e 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,6 +4723,53 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
+
+(defun copy-directory-contents (directory newname &optional keep-time parents)
+  "Copy DIRECTORY contents to NEWNAME.  Both args must be strings.
+If NEWNAME names an existing directory, copy DIRECTORY contents as subdirectory there.
+
+This function always sets the file modes of the output files to match
+the corresponding input file.
+
+The third arg KEEP-TIME non-nil means give the output files the same
+last-modified time as the old ones.  (This works on only some systems.)
+
+The last argument PARENTS says whether to
+create parent directories if they don't exist."
+
+  ;; If default-directory is a remote directory, make sure we find its
+  ;; copy-directory handler.
+  (let ((handler (or (find-file-name-handler directory 'copy-directory)
+		     (find-file-name-handler newname 'copy-directory))))
+    (if handler
+	(funcall handler 'copy-directory directory newname keep-time parents)
+
+      ;; Compute target name.
+      (setq directory (directory-file-name (expand-file-name directory))
+	    newname   (directory-file-name (expand-file-name newname)))
+      (when (not (file-directory-p newname))
+        (make-directory newname parents))
+
+      ;; Copy recursively.
+      (mapc
+       (lambda (file)
+	 (let ((target (expand-file-name
+			(file-name-nondirectory file) newname))
+	       (attrs  (file-attributes file)))
+	   (cond ((file-directory-p file)
+		  (copy-directory file newname keep-time parents))
+		 ((stringp (car attrs)) ; Symbolic link
+		  (make-symbolic-link (car attrs) target t))
+		 (t
+		  (copy-file file target t keep-time)))))
+       ;; We do not want to copy "." and "..".
+       (directory-files	directory 'full directory-files-no-dot-files-regexp))
+
+      ;; Set directory attributes.
+      (set-file-modes newname (file-modes directory))
+      (when keep-time
+        (set-file-times newname (nth 5 (file-attributes directory)))))))
+
 (defun copy-directory (directory newname &optional keep-time parents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
 If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.



-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>
>>> What we could do also is create a function copy-directory-contents, that
>>> reuse the code of precedent version of copy-directory (but not
>>> interactive this time) and call this function in
>>> dired-copy-file-recursive instead of copy-directory.
>>>
>>> In this case we could rewrite copy-directory to avoid duplicate code,
>>> possibly: (no urge in this case as actual copy-directory works fine) 
>>>
>>> 1) Writing copy-directory-contents (or whatever name) to allow
>>> creating the structure like actual copy-directory, or not like ancient
>>> version leaving this job to dired-create-files.
>>>
>>> 2) Writing a copy-directory that reuse dired code (i.e dired-create-files).
>>> In this case it would have all interactive messages, ask etc..
>>>
>>> WDYT?
>>
>> I would prefer option 2). There is no need to have an extra dired
>> implementation, now we have copy-directory.
>
> So i made first steps:
>
> - Create new function copy-directory-contents based on old
> copy-directory code.
>
> - Use it in dired-copy-file-recursive instead of copy-directory.
>
> Seems to work fine.
There was an error though.

Now the last fix. (sent to Chong)
I think that's correct:
Use a new function copy-directory1 that can be reused by copy-directory,
avoiding duplicate code.
This function use an extra arg `create-struct'.
When this arg is used copy-directory1 works like actual copy-directory.
Otherwise it does like before and doesn't break dired.
As i am using dired code in anything, i have inlined this changes in
anything-config.el waiting this patch is applied or maybe another fix if
you have better idea.
Thanks Michael and Chong.

Here the patch:

---
 lisp/dired-aux.el |    2 +-
 lisp/files.el     |   98 ++++++++++++++++++++++++++++-------------------------
 2 files changed, 53 insertions(+), 47 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 28b285f..b26ce0a 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -1242,7 +1242,7 @@ Special value `always' suppresses confirmation."
 	     (or (eq recursive 'always)
 		 (yes-or-no-p (format "Recursive copies of %s? " from))))
 	;; This is a directory.
-	(copy-directory from to dired-copy-preserve-time)
+	(copy-directory1 from to dired-copy-preserve-time)
       ;; Not a directory.
       (or top (dired-handle-overwrite to))
       (condition-case err
diff --git a/lisp/files.el b/lisp/files.el
index d896020..c75a330 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,7 +4723,56 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
-(defun copy-directory (directory newname &optional keep-time parents)
+(defun copy-directory1 (directory newname &optional keep-time parents create-struct)
+  ;; If default-directory is a remote directory, make sure we find its
+  ;; copy-directory handler.
+  (let ((handler (or (find-file-name-handler directory 'copy-directory)
+		     (find-file-name-handler newname 'copy-directory))))
+    (if handler
+	(funcall handler 'copy-directory directory newname keep-time parents)
+
+        ;; Compute target name.
+        (setq directory (directory-file-name (expand-file-name directory))
+              newname   (directory-file-name (expand-file-name newname)))
+
+        (if (not (file-directory-p newname))
+            ;; If NEWNAME is not an existing directory, create it; that
+            ;; is where we will copy the files of DIRECTORY.
+            (make-directory newname parents)
+            ;; If NEWNAME is an existing directory, we will copy into
+            ;; NEWNAME/[DIRECTORY-BASENAME].
+            (when create-struct
+              (setq newname (expand-file-name
+                             (file-name-nondirectory
+                              (directory-file-name directory))
+                             newname))
+              (and (file-exists-p newname)
+                   (not (file-directory-p newname))
+                   (error "Cannot overwrite non-directory %s with a directory"
+                          newname))
+              (make-directory newname t)))
+
+        ;; Copy recursively.
+        (dolist (file
+                  ;; We do not want to copy "." and "..".
+                  (directory-files directory 'full
+                                   directory-files-no-dot-files-regexp))
+          (let ((target (expand-file-name (file-name-nondirectory file) newname))
+                (attrs (file-attributes file)))
+            (cond ((and (file-directory-p file) create-struct)
+                   (copy-directory1 file newname keep-time parents create-struct))
+                  ((file-directory-p file)
+                   (copy-directory1 file target keep-time parents create-struct))
+                  ((stringp (car attrs)) ; Symbolic link
+                   (make-symbolic-link (car attrs) target t))
+                  (t (copy-file file target t keep-time)))))
+
+        ;; Set directory attributes.
+        (set-file-modes newname (file-modes directory))
+        (when keep-time
+          (set-file-times newname (nth 5 (file-attributes directory)))))))
+
+(defun copy-directory (directory newname &optional keep-time parents create-struct)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
 If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.
 
@@ -4745,51 +4794,8 @@ this happens by default."
 	   (read-file-name
 	    (format "Copy directory %s to: " dir)
 	    default-directory default-directory nil nil)
-	   current-prefix-arg t)))
-  ;; If default-directory is a remote directory, make sure we find its
-  ;; copy-directory handler.
-  (let ((handler (or (find-file-name-handler directory 'copy-directory)
-		     (find-file-name-handler newname 'copy-directory))))
-    (if handler
-	(funcall handler 'copy-directory directory newname keep-time parents)
-
-      ;; Compute target name.
-      (setq directory (directory-file-name (expand-file-name directory))
-	    newname   (directory-file-name (expand-file-name newname)))
-
-      (if (not (file-directory-p newname))
-	  ;; If NEWNAME is not an existing directory, create it; that
-	  ;; is where we will copy the files of DIRECTORY.
-	  (make-directory newname parents)
-	;; If NEWNAME is an existing directory, we will copy into
-	;; NEWNAME/[DIRECTORY-BASENAME].
-	(setq newname (expand-file-name
-		       (file-name-nondirectory
-			(directory-file-name directory))
-		       newname))
-	(and (file-exists-p newname)
-	     (not (file-directory-p newname))
-	     (error "Cannot overwrite non-directory %s with a directory"
-		    newname))
-	(make-directory newname t))
-
-      ;; Copy recursively.
-      (dolist (file
-	       ;; We do not want to copy "." and "..".
-	       (directory-files directory 'full
-				directory-files-no-dot-files-regexp))
-	(if (file-directory-p file)
-	    (copy-directory file newname keep-time parents)
-	  (let ((target (expand-file-name (file-name-nondirectory file) newname))
-		(attrs (file-attributes file)))
-	    (if (stringp (car attrs)) ; Symbolic link
-		(make-symbolic-link (car attrs) target t)
-	      (copy-file file target t keep-time)))))
-
-      ;; Set directory attributes.
-      (set-file-modes newname (file-modes directory))
-      (if keep-time
-	  (set-file-times newname (nth 5 (file-attributes directory)))))))
+	   current-prefix-arg t t)))
+  (copy-directory1 directory newname keep-time parents create-struct))
 \f
 (put 'revert-buffer-function 'permanent-local t)
 (defvar revert-buffer-function nil
-- 
1.7.1



-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Michael Albinus <michael.albinus@gmx.de> writes:
>>
>>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>>
>>>> What we could do also is create a function copy-directory-contents, that
>>>> reuse the code of precedent version of copy-directory (but not
>>>> interactive this time) and call this function in
>>>> dired-copy-file-recursive instead of copy-directory.
>>>>
>>>> In this case we could rewrite copy-directory to avoid duplicate code,
>>>> possibly: (no urge in this case as actual copy-directory works fine) 
>>>>
>>>> 1) Writing copy-directory-contents (or whatever name) to allow
>>>> creating the structure like actual copy-directory, or not like ancient
>>>> version leaving this job to dired-create-files.
>>>>
>>>> 2) Writing a copy-directory that reuse dired code (i.e dired-create-files).
>>>> In this case it would have all interactive messages, ask etc..
>>>>
>>>> WDYT?
>>>
>>> I would prefer option 2). There is no need to have an extra dired
>>> implementation, now we have copy-directory.
>>
>> So i made first steps:
>>
>> - Create new function copy-directory-contents based on old
>> copy-directory code.
>>
>> - Use it in dired-copy-file-recursive instead of copy-directory.
>>
>> Seems to work fine.
> There was an error though.
>
> Now the last fix. (sent to Chong)
> I think that's correct:
> Use a new function copy-directory1 that can be reused by copy-directory,
> avoiding duplicate code.

I haven't had time yet to review this patch (and the previous one) in
detail (pressure @work). On a first view, there is at least the problem
that `copy-directory1' calls the file name handler for `copy-directory'
- this is bad.

> This function use an extra arg `create-struct'.
> When this arg is used copy-directory1 works like actual copy-directory.
> Otherwise it does like before and doesn't break dired.

Why not adding this optional parameter to `copy-directory'? Then you
won't need `copy-directory1'.

> Thanks Michael and Chong.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>
>>> Michael Albinus <michael.albinus@gmx.de> writes:
>>>
>>>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>>>
>>>>> What we could do also is create a function copy-directory-contents, that
>>>>> reuse the code of precedent version of copy-directory (but not
>>>>> interactive this time) and call this function in
>>>>> dired-copy-file-recursive instead of copy-directory.
>>>>>
>>>>> In this case we could rewrite copy-directory to avoid duplicate code,
>>>>> possibly: (no urge in this case as actual copy-directory works fine) 
>>>>>
>>>>> 1) Writing copy-directory-contents (or whatever name) to allow
>>>>> creating the structure like actual copy-directory, or not like ancient
>>>>> version leaving this job to dired-create-files.
>>>>>
>>>>> 2) Writing a copy-directory that reuse dired code (i.e dired-create-files).
>>>>> In this case it would have all interactive messages, ask etc..
>>>>>
>>>>> WDYT?
>>>>
>>>> I would prefer option 2). There is no need to have an extra dired
>>>> implementation, now we have copy-directory.
>>>
>>> So i made first steps:
>>>
>>> - Create new function copy-directory-contents based on old
>>> copy-directory code.
>>>
>>> - Use it in dired-copy-file-recursive instead of copy-directory.
>>>
>>> Seems to work fine.
>> There was an error though.
>>
>> Now the last fix. (sent to Chong)
>> I think that's correct:
>> Use a new function copy-directory1 that can be reused by copy-directory,
>> avoiding duplicate code.
>
> I haven't had time yet to review this patch (and the previous one) in
> detail (pressure @work).
Forget the previous patch.
 
> On a first view, there is at least the problem
> that `copy-directory1' calls the file name handler for `copy-directory'
> - this is bad.

So if using just one function like you suggest after,
it should be ok to leave `copy-directory'?
NOTE:
To have the same behavior as copy-directory in non--interactive call,
you will have to use extra arg:

(copy-directory A B nil nil 'create-struct)

instead of

(copy-directory A B)

Don't know if it will have effect in tramp.
Maybe in ssh method?

>> This function use an extra arg `create-struct'.
>> When this arg is used copy-directory1 works like actual copy-directory.
>> Otherwise it does like before and doesn't break dired.
>
> Why not adding this optional parameter to `copy-directory'? Then you
> won't need `copy-directory1'.
Yes, why not.
In this case need only to put the interactive code in copy-directory1
without forgeting to put the optional argument create-struct at end.
(It have to be used in all interactive calls of copy-directory).

>> Thanks Michael and Chong.
>
> Best regards, Michael.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>> Why not adding this optional parameter to `copy-directory'? Then you
>> won't need `copy-directory1'.
> Yes, why not.
> In this case need only to put the interactive code in copy-directory1
> without forgeting to put the optional argument create-struct at end.
> (It have to be used in all interactive calls of copy-directory).

Something like this. Adaptations for Tramp and ange-ftp.el are needed;
I could do this afterwards.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>>> Why not adding this optional parameter to `copy-directory'? Then you
>>> won't need `copy-directory1'.
>> Yes, why not.
>> In this case need only to put the interactive code in copy-directory1
>> without forgeting to put the optional argument create-struct at end.
>> (It have to be used in all interactive calls of copy-directory).
>
> Something like this. Adaptations for Tramp and ange-ftp.el are needed;
> I could do this afterwards.
Ok thanks, i sent a patch to Chong.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Chong Yidong]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Ok thanks, i sent a patch to Chong.

I've committed your patch, with a number of changes (e.g. renaming the
new argument to `copy-as-subdir').  Please test it and see if everything
behaves as you expect.  Thanks.

Re: bug in copy-directory

[Thierry Volpiatto]

Chong Yidong <cyd@stupidchicken.com> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Ok thanks, i sent a patch to Chong.
>
> I've committed your patch, with a number of changes (e.g. renaming the
> new argument to `copy-as-subdir').  Please test it and see if everything
> behaves as you expect.  Thanks.
>
I can't get it until tonight (when the git repo is updated).
Did you fix the recursive call to copy-directory in the dolist block?
I forgot to replace 2 copy-directory1 with copy-directory.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Chong Yidong <cyd@stupidchicken.com> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Ok thanks, i sent a patch to Chong.
>
> I've committed your patch, with a number of changes (e.g. renaming the
> new argument to `copy-as-subdir').  Please test it and see if everything
> behaves as you expect.  Thanks.

Now it is even worse. Going to the initial test case:

- Create directory /tmp/test
- Create directory /tmp/test/test
- Create file /tmp/test/a
- Create file /tmp/test/test/b
- Apply (copy-directory "/tmp/test" "~/")

You get "~/a", "~/test" and "~/test/b".

Best regards, Michael.

Re: bug in copy-directory

[Chong Yidong]

Michael Albinus <michael.albinus@gmx.de> writes:

> Now it is even worse. Going to the initial test case:
>
> - Create directory /tmp/test
> - Create directory /tmp/test/test
> - Create file /tmp/test/a
> - Create file /tmp/test/test/b
> - Apply (copy-directory "/tmp/test" "~/")
>
> You get "~/a", "~/test" and "~/test/b".

I asumme that, by (copy-directory "/tmp/test" "~/"), you mean
M-: (copy-directory "/tmp/test" "~/") RET.

The behavior for that is the same as on 23.2.  If you want to copy into
a subdirectory in ~/, give it a non-nil copy-as-subdir arg.

Re: bug in copy-directory

[Michael Albinus]

Chong Yidong <cyd@stupidchicken.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Now it is even worse. Going to the initial test case:
>>
>> - Create directory /tmp/test
>> - Create directory /tmp/test/test
>> - Create file /tmp/test/a
>> - Create file /tmp/test/test/b
>> - Apply (copy-directory "/tmp/test" "~/")
>>
>> You get "~/a", "~/test" and "~/test/b".
>
> I asumme that, by (copy-directory "/tmp/test" "~/"), you mean
> M-: (copy-directory "/tmp/test" "~/") RET.

Yes.

> The behavior for that is the same as on 23.2.  If you want to copy into
> a subdirectory in ~/, give it a non-nil copy-as-subdir arg.

"/tmp/test" is a directory. "~/test" does not exist yet. Therefore, I
would expect that a directory "~/test" is created, which is the copy of
"/tmp/test". Like you do in a shell:

# ll -R /tmp/test
/tmp/test:
total 12
drwxr-xr-x  3 albinus albinus 4096 2011-02-07 17:24 .
drwxrwxrwt 14 root    root    4096 2011-02-07 17:24 ..
-rw-r--r--  1 albinus albinus    0 2011-02-07 17:24 a
drwxr-xr-x  2 albinus albinus 4096 2011-02-07 17:24 test

/tmp/test/test:
total 8
drwxr-xr-x 2 albinus albinus 4096 2011-02-07 17:24 .
drwxr-xr-x 3 albinus albinus 4096 2011-02-07 17:24 ..
-rw-r--r-- 1 albinus albinus    0 2011-02-07 17:24 b

# ll -R ~/test
ls: cannot access /home/albinus/test: No such file or directory
# cp -pr /tmp/test ~/
# ll -R ~/test
/home/albinus/test:
total 12
drwxr-xr-x  3 albinus albinus 4096 2011-02-07 17:24 .
drwxr-xr-x 66 albinus albinus 4096 2011-02-07 17:31 ..
-rw-r--r--  1 albinus albinus    0 2011-02-07 17:24 a
drwxr-xr-x  2 albinus albinus 4096 2011-02-07 17:24 test

/home/albinus/test/test:
total 8
drwxr-xr-x 2 albinus albinus 4096 2011-02-07 17:24 .
drwxr-xr-x 3 albinus albinus 4096 2011-02-07 17:24 ..
-rw-r--r-- 1 albinus albinus    0 2011-02-07 17:24 b

But if I apply in Emacs 23

M-: (copy-directory "/tmp/test" "~/") RET

I get

# ll -R ~/test
/home/albinus/test:
total 8
drwxr-xr-x  2 albinus albinus 4096 2011-02-07 17:42 .
drwxr-xr-x 66 albinus albinus 4096 2011-02-07 17:42 ..
-rw-r--r--  1 albinus albinus    0 2011-02-07 17:42 b

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> But if I apply in Emacs 23
>
> M-: (copy-directory "/tmp/test" "~/") RET
>
> I get
>
> # ll -R ~/test
> /home/albinus/test:
> total 8
> drwxr-xr-x  2 albinus albinus 4096 2011-02-07 17:42 .
> drwxr-xr-x 66 albinus albinus 4096 2011-02-07 17:42 ..
> -rw-r--r--  1 albinus albinus    0 2011-02-07 17:42 b

Michael, could you apply the patch i sent here to Stefan and do:

M-: (copy-directory "/tmp/test" "~/") RET

You will see it works fine.

M-x copy-directory works fine too

and copying from dired works fine also.

Otherwise in the actual state you must do for non--interactive calls:

(copy-directory "/tmp/test" "~/" nil nil t)

which i understand is unacceptable.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Michael, could you apply the patch i sent here to Stefan and do:
>
> M-: (copy-directory "/tmp/test" "~/") RET
>
> You will see it works fine.

Yes.

> M-x copy-directory works fine too
>
> and copying from dired works fine also.

Yes.

However, when copying in dired "/tmp/test" to "~/", the second attempt
results in a wrong directory structure.

For file name handlers your approach is a little bit, hmm, surprising.
You call the handler for `copy-directory' inside of `copy-directory1'.
This could work, or course, but it isn't a clean approach, and it will
add a maintenance burden when all of us have forgotten what we are doing
here.

At least *I* am pretty good in forgetting (my wife says :-)

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Michael, could you apply the patch i sent here to Stefan and do:
>>
>> M-: (copy-directory "/tmp/test" "~/") RET
>>
>> You will see it works fine.
>
> Yes.
>
>> M-x copy-directory works fine too
>>
>> and copying from dired works fine also.
>
> Yes.
>
> However, when copying in dired "/tmp/test" to "~/", the second attempt
> results in a wrong directory structure.
Can't reproduce that here, all work fine from dired in emacs -Q
You can use also the unit-test i sent you, it exit with no failures.
(modify it a bit as code have changed now)


> For file name handlers your approach is a little bit, hmm, surprising.
> You call the handler for `copy-directory' inside of `copy-directory1'.
> This could work, or course, but it isn't a clean approach, and it will
> add a maintenance burden when all of us have forgotten what we are doing
> here.
Yes but copy-directory call copy-directory-1, so what's the problem?
file name handlers have to be in copy-directory-1 because dired MUST use
copy-directory-1 and not copy-directory.

> At least *I* am pretty good in forgetting (my wife says :-)
So nothing to remember, just use copy-directory as before. ;-)

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:


>> However, when copying in dired "/tmp/test" to "~/", the second attempt
>> results in a wrong directory structure.
> Can't reproduce that here, all work fine from dired in emacs -Q
> You can use also the unit-test i sent you, it exit with no failures.
> (modify it a bit as code have changed now)

Sorry, my bad. I've started the wrong Emacs instance. It works fine also
for me, all scenarios.

>> For file name handlers your approach is a little bit, hmm, surprising.
>> You call the handler for `copy-directory' inside of `copy-directory1'.
>> This could work, or course, but it isn't a clean approach, and it will
>> add a maintenance burden when all of us have forgotten what we are doing
>> here.
> Yes but copy-directory call copy-directory-1, so what's the problem?
> file name handlers have to be in copy-directory-1 because dired MUST use
> copy-directory-1 and not copy-directory.

Technically, there might be no problem. But it is unclean to call a file
name handler in a function which is not intended for. And also the
parameter lists differ between copy-directory and copy-directory1.

It's just a feeling that this kind of hacks could cause trouble in the
future. However, if Chong/Stefan do not oppose, I'll shut up and make
the changes in Tramp and ange-ftp.el.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>
>>> However, when copying in dired "/tmp/test" to "~/", the second attempt
>>> results in a wrong directory structure.
>> Can't reproduce that here, all work fine from dired in emacs -Q
>> You can use also the unit-test i sent you, it exit with no failures.
>> (modify it a bit as code have changed now)
>
> Sorry, my bad. I've started the wrong Emacs instance. It works fine also
> for me, all scenarios.
>
>>> For file name handlers your approach is a little bit, hmm, surprising.
>>> You call the handler for `copy-directory' inside of `copy-directory1'.
>>> This could work, or course, but it isn't a clean approach, and it will
>>> add a maintenance burden when all of us have forgotten what we are doing
>>> here.
>> Yes but copy-directory call copy-directory-1, so what's the problem?
>> file name handlers have to be in copy-directory-1 because dired MUST use
>> copy-directory-1 and not copy-directory.
>
> Technically, there might be no problem. But it is unclean to call a file
> name handler in a function which is not intended for. And also the
> parameter lists differ between copy-directory and copy-directory1.
Not now, parameters are the same, last arg has been removed in both
functions.
now we have:

(defun copy-directory (directory newname &optional keep-time parents)
...)


(defun copy-directory-1 (directory newname &optional keep-time parents)
...)

> It's just a feeling that this kind of hacks could cause trouble in the
> future. However, if Chong/Stefan do not oppose, I'll shut up and make
> the changes in Tramp and ange-ftp.el.
So maybe you will have no changes to do.


-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>> Technically, there might be no problem. But it is unclean to call a file
>> name handler in a function which is not intended for. And also the
>> parameter lists differ between copy-directory and copy-directory1.
> Not now, parameters are the same, last arg has been removed in both
> functions.
> now we have:
>
> (defun copy-directory (directory newname &optional keep-time parents)
> ...)
>
>
> (defun copy-directory-1 (directory newname &optional keep-time parents)
> ...)
>
>> It's just a feeling that this kind of hacks could cause trouble in the
>> future. However, if Chong/Stefan do not oppose, I'll shut up and make
>> the changes in Tramp and ange-ftp.el.
> So maybe you will have no changes to do.

At least I need to check which behaviour is implemented,
`copy-directory' or `copy-directory-1'. It must be the latter one.

Best regards, Michael.

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>> Technically, there might be no problem. But it is unclean to call a file
>> name handler in a function which is not intended for. And also the
>> parameter lists differ between copy-directory and copy-directory1.
> Not now, parameters are the same, last arg has been removed in both
> functions.
> now we have:
>
> (defun copy-directory (directory newname &optional keep-time parents)
> ...)
>
>
> (defun copy-directory-1 (directory newname &optional keep-time parents)
> ...)
>
>> It's just a feeling that this kind of hacks could cause trouble in the
>> future. However, if Chong/Stefan do not oppose, I'll shut up and make
>> the changes in Tramp and ange-ftp.el.
> So maybe you will have no changes to do.

Using your latest patches, I've tried

M-: (copy-directory "/ssh::/tmp/test" "/ssh::~/")

This results in a wrong target directory structure. This is, because
Tramp has no own implementation of copy-directory for the ssh method, it
falls back to the default implementation. So we have the call tree:

copy-directory
 -> copy-directory-1
 -> tramp-handle-copy-directory
 -> copy-directory

You see, there are serious problems when a file name handler is called
from a function which shouldn't.

M-: (copy-directory-1 "/ssh::/tmp/test" "/ssh::~/")

works fine. Given this behaviour, the file name handler should be
provided for copy-directory-1, which is just a helper function ...

Maybe it has been discussed at the beginning already, but I don't catch
what is wrong with using the implementation of copy-directory-1 for
copy-directory.

Best regards, Michael.

Re: bug in copy-directory

[Chong Yidong]

Michael Albinus <michael.albinus@gmx.de> writes:

>> I asumme that, by (copy-directory "/tmp/test" "~/"), you mean
>> M-: (copy-directory "/tmp/test" "~/") RET.
>
> Yes.
>
>> The behavior for that is the same as on 23.2.  If you want to copy into
>> a subdirectory in ~/, give it a non-nil copy-as-subdir arg.
>
> "/tmp/test" is a directory. "~/test" does not exist yet. Therefore, I
> would expect that a directory "~/test" is created, which is the copy of
> "/tmp/test".

To get copy-directory to behave like cp when optional args aren't
specified, we have to change all the calls to copy-directory, in Dired
and in Tramp.  The entire point of adding the COPY-AS-SUBDIR argument
was to avoid having to do that.

Given that we are well into pretest for 23.3, here are two options:

1. Keep the code that's in the emacs-23 branch.  From now on,
   copy-directory needs a non-nil COPY-AS-SUBDIR arg to get cp-like
   copying, when called from Lisp.  In the meantime, users of
   M-x copy-directory will be surprised, but that's life.

2. Revert the code in the emacs-23 branch to what it was in 23.2.
   Change copy-directory on the trunk (probably by flipping the meaning
   of the optional arg, and changing Dired and Tramp accordingly.)

Any arguments for either choice?

Re: bug in copy-directory

[Thierry Volpiatto]

Chong Yidong <cyd@stupidchicken.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>>> I asumme that, by (copy-directory "/tmp/test" "~/"), you mean
>>> M-: (copy-directory "/tmp/test" "~/") RET.
>>
>> Yes.
>>
>>> The behavior for that is the same as on 23.2.  If you want to copy into
>>> a subdirectory in ~/, give it a non-nil copy-as-subdir arg.
>>
>> "/tmp/test" is a directory. "~/test" does not exist yet. Therefore, I
>> would expect that a directory "~/test" is created, which is the copy of
>> "/tmp/test".
>
> To get copy-directory to behave like cp when optional args aren't
> specified, we have to change all the calls to copy-directory, in Dired
> and in Tramp.  The entire point of adding the COPY-AS-SUBDIR argument
> was to avoid having to do that.
>
> Given that we are well into pretest for 23.3, here are two options:
>
> 1. Keep the code that's in the emacs-23 branch.  From now on,
>    copy-directory needs a non-nil COPY-AS-SUBDIR arg to get cp-like
>    copying, when called from Lisp.  In the meantime, users of
>    M-x copy-directory will be surprised, but that's life.
Not anymore needed.

> 2. Revert the code in the emacs-23 branch to what it was in 23.2.
>    Change copy-directory on the trunk (probably by flipping the meaning
>    of the optional arg, and changing Dired and Tramp accordingly.)

I think you must fix that for 23.3, copying is important.

> Any arguments for either choice?

Finally, COPY-AS-SUBDIR is not needed, to remove it we used a second
function called copy-directory-1 and call copy-directory-1 from dired.
Now with these change the state is:

--8<---------------cut here---------------start------------->8---
Executing expectations in /home/thierry/.emacs.d/save-scratch.el...
5 expectations, 1 failures, 0 errors
Expectations finished at Wed Feb  9 07:51:24 2011
==== Failures and Errors ====
12 :FAIL: Expected <("/ssh:thievol:/home/thierry/test/a" "/ssh:thievol:/home/thierry/test/test" "/ssh:thievol:/home/thierry/test/test/b")> but was <("/ssh:thievol:/home/thierry/test/a" "/ssh:thievol:/home/thierry/test/test" "/ssh:thievol:/home/thierry/test/test/a" "/ssh:thievol:/home/thierry/test/test/b" "/ssh:thievol:/home/thierry/test/test/test" "/ssh:thievol:/home/thierry/test/test/test/b")>

==== All Results ====
1  :+++++ New expectations +++++
2  :copy-directory and overwrite
3  :OK
4  :+++++ New expectations +++++
5  :copy-directory and overwrite with tramp ssh method
6  :OK
7  :+++++ New expectations +++++
8  :Simulation copy-directory and overwrite from dired
9  :OK
10 :+++++ New expectations +++++
11 :Simulation copy-directory and overwrite from dired with ssh tramp names
12 :FAIL: Expected <("/ssh:thievol:/home/thierry/test/a" "/ssh:thievol:/home/thierry/test/test" "/ssh:thievol:/home/thierry/test/test/b")> but was <("/ssh:thievol:/home/thierry/test/a" "/ssh:thievol:/home/thierry/test/test" "/ssh:thievol:/home/thierry/test/test/a" "/ssh:thievol:/home/thierry/test/test/b" "/ssh:thievol:/home/thierry/test/test/test" "/ssh:thievol:/home/thierry/test/test/test/b")>
13 :+++++ New expectations +++++
14 :Simulation copy-directory and overwrite from dired with scpc tramp names
15 :OK

5 expectations, 1 failures, 0 errors
Expectations finished at Wed Feb  9 07:51:24 2011
--8<---------------cut here---------------end--------------->8---

So it fail only in one place, copying from dired with ssh method.
Because it seem ssh is the only method that call copy-directory, 
so dired call copy-directory-1 but the handler reuse copy-directory.
Otherwise other methods seem to work fine (here default method scpc
tested).

NOTE: In this version, if i add the call to handler also in
copy-directory, copying from dired from ssh method succeed, but
non--interactive call to copy-directory with ssh method fail:

--8<---------------cut here---------------start------------->8---
Executing expectations in /home/thierry/.emacs.d/save-scratch.el...
5 expectations, 1 failures, 0 errors
Expectations finished at Wed Feb  9 08:09:14 2011
==== Failures and Errors ====
6  :FAIL: Expected <("/ssh:thievol:/home/thierry/test/a" "/ssh:thievol:/home/thierry/test/test" "/ssh:thievol:/home/thierry/test/test/b")> but was <("/ssh:thievol:/home/thierry/test/b")>

==== All Results ====
1  :+++++ New expectations +++++
2  :copy-directory and overwrite
3  :OK
4  :+++++ New expectations +++++
5  :copy-directory and overwrite with tramp ssh method
6  :FAIL: Expected <("/ssh:thievol:/home/thierry/test/a" "/ssh:thievol:/home/thierry/test/test" "/ssh:thievol:/home/thierry/test/test/b")> but was <("/ssh:thievol:/home/thierry/test/b")>
7  :+++++ New expectations +++++
8  :Simulation copy-directory and overwrite from dired
9  :OK
10 :+++++ New expectations +++++
11 :Simulation copy-directory and overwrite from dired with ssh tramp names
12 :OK
13 :+++++ New expectations +++++
14 :Simulation copy-directory and overwrite from dired with scpc tramp names
15 :OK

5 expectations, 1 failures, 0 errors
Expectations finished at Wed Feb  9 08:09:14 2011
--8<---------------cut here---------------end--------------->8---

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Chong Yidong <cyd@stupidchicken.com> writes:

>> Given that we are well into pretest for 23.3, here are two options:
>>
>> 1. Keep the code that's in the emacs-23 branch.  From now on,
>>    copy-directory needs a non-nil COPY-AS-SUBDIR arg to get cp-like
>>    copying, when called from Lisp.  In the meantime, users of
>>    M-x copy-directory will be surprised, but that's life.
> Not anymore needed.
>
>> 2. Revert the code in the emacs-23 branch to what it was in 23.2.
>>    Change copy-directory on the trunk (probably by flipping the meaning
>>    of the optional arg, and changing Dired and Tramp accordingly.)
>
> I think you must fix that for 23.3, copying is important.
>
>> Any arguments for either choice?
>
> Finally, COPY-AS-SUBDIR is not needed, to remove it we used a second
> function called copy-directory-1 and call copy-directory-1 from dired.
> Now with these change the state is:
>
> So it fail only in one place, copying from dired with ssh method.
> Because it seem ssh is the only method that call copy-directory, 
> so dired call copy-directory-1 but the handler reuse copy-directory.
> Otherwise other methods seem to work fine (here default method scpc
> tested).

It is not only one Tramp method which is broken. Tramp has an own
implementation for scp- and rsync-based methods. Everything else fails,
because Tramp falls back to the default copy-directory, including such
methods like ftp, ssh, su(do), smb. I believe, this is not acceptable.

I would vote for continuing with the last patch from Thierry, not
committed yet. Maybe it is possible to throw copy-directory-1 away, and
to adapt the call of copy-directory in Dired instead of.

If this is not possible, we shall fall back to the Emacs 23.2
implementation, which is less broken compared with the current status.

Best regards, Michael.

Re: bug in copy-directory

[Chong Yidong]

Michael Albinus <michael.albinus@gmx.de> writes:

> If this is not possible, we shall fall back to the Emacs 23.2
> implementation, which is less broken compared with the current status.

Explain again why the current status is more broken than Emacs 23.2.  As
far as I can tell, if the copy-as-subdir arg is nil, the Lisp behavior
is identical to 23.2.

> I would vote for continuing with the last patch from Thierry, not
> committed yet. Maybe it is possible to throw copy-directory-1 away,
> and to adapt the call of copy-directory in Dired instead of.

Adding a separate copy-directory-1 function does nothing different from
adding an optional argument to copy-directory that modifies its
behavior.  Not much point.

Re: bug in copy-directory

[Michael Albinus]

Chong Yidong <cyd@stupidchicken.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> If this is not possible, we shall fall back to the Emacs 23.2
>> implementation, which is less broken compared with the current status.
>
> Explain again why the current status is more broken than Emacs 23.2.  As
> far as I can tell, if the copy-as-subdir arg is nil, the Lisp behavior
> is identical to 23.2.

Sorry the confusion, I meant the patches provided by Thierry last days
(adding copy-directory-1).

>> I would vote for continuing with the last patch from Thierry, not
>> committed yet. Maybe it is possible to throw copy-directory-1 away,
>> and to adapt the call of copy-directory in Dired instead of.
>
> Adding a separate copy-directory-1 function does nothing different from
> adding an optional argument to copy-directory that modifies its
> behavior.  Not much point.

It does for Tramp, when (like Thierry did) the file name handler is
called inside copy-directory-1. *This* approach I was opposing.

OK, I'll test the current state (as in bzr) and Thierry's latest patch.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Chong Yidong <cyd@stupidchicken.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> If this is not possible, we shall fall back to the Emacs 23.2
>> implementation, which is less broken compared with the current status.
>
> Explain again why the current status is more broken than Emacs 23.2.  As
> far as I can tell, if the copy-as-subdir arg is nil, the Lisp behavior
> is identical to 23.2.
>
>> I would vote for continuing with the last patch from Thierry, not
>> committed yet. Maybe it is possible to throw copy-directory-1 away,
>> and to adapt the call of copy-directory in Dired instead of.
>
> Adding a separate copy-directory-1 function does nothing different from
> adding an optional argument to copy-directory that modifies its
> behavior.  Not much point.

I am actually trying something different, i sent patch to Michael and
Stephane.
I restart from the Chong version without extra arg.
This version works outside of dired interactvely and not.

Then i modify a little dired-create-files.
dired-copy-file-recursive have to call now copy-directory as
copy-directory-1 doesn't exists anymore.

It works here in dired, all scenarios of copy-directory and i tried with
sudo method from dired and it works also.

Could you test on your side?

---
 lisp/dired-aux.el |   91 ++---------------------------------------------------
 lisp/files.el     |   53 +++++++++++++------------------
 2 files changed, 25 insertions(+), 119 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 28b285f..5bb9d73 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -1345,6 +1345,7 @@ Special value `always' suppresses confirmation."
 	(when cons (setcar cons cur-dir))))))
 \f
 ;; The basic function for half a dozen variations on cp/mv/ln/ln -s.
+\f
 (defun dired-create-files (file-creator operation fn-list name-constructor
 					&optional marker-char)
 
@@ -1403,6 +1404,8 @@ ESC or `q' to not overwrite any of the remaining files,
                   (cond  ((integerp marker-char) marker-char)
                          (marker-char (dired-file-marker from)) ; slow
                          (t nil))))
+            (when (and (file-directory-p from) (eq file-creator 'dired-copy-file))
+              (setq to (file-name-directory to)))
             (condition-case err
                 (progn
                   (funcall file-creator from to dired-overwrite-confirmed)
@@ -1445,94 +1448,6 @@ ESC or `q' to not overwrite any of the remaining files,
       (message "%s: %s file%s"
 	       operation success-count (dired-plural-s success-count)))))
   (dired-move-to-filename))
-\f
-(defun dired-do-create-files (op-symbol file-creator operation arg
-					&optional marker-char op1
-					how-to)
-  "Create a new file for each marked file.
-Prompts user for target, which is a directory in which to create
-  the new files.  Target may also be a plain file if only one marked
-  file exists.  The way the default for the target directory is
-  computed depends on the value of `dired-dwim-target-directory'.
-OP-SYMBOL is the symbol for the operation.  Function `dired-mark-pop-up'
-  will determine whether pop-ups are appropriate for this OP-SYMBOL.
-FILE-CREATOR and OPERATION as in `dired-create-files'.
-ARG as in `dired-get-marked-files'.
-Optional arg MARKER-CHAR as in `dired-create-files'.
-Optional arg OP1 is an alternate form for OPERATION if there is
-  only one file.
-Optional arg HOW-TO determiness how to treat the target.
-  If HOW-TO is nil, use `file-directory-p' to determine if the
-   target is a directory.  If so, the marked file(s) are created
-   inside that directory.  Otherwise, the target is a plain file;
-   an error is raised unless there is exactly one marked file.
-  If HOW-TO is t, target is always treated as a plain file.
-  Otherwise, HOW-TO should be a function of one argument, TARGET.
-   If its return value is nil, TARGET is regarded as a plain file.
-   If it return value is a list, TARGET is a generalized
-    directory (e.g. some sort of archive).  The first element of
-    this list must be a function with at least four arguments:
-      operation - as OPERATION above.
-      rfn-list  - list of the relative names for the marked files.
-      fn-list   - list of the absolute names for the marked files.
-      target    - the name of the target itself.
-      The rest of into-dir are optional arguments.
-   For any other return value, TARGET is treated as a directory."
-  (or op1 (setq op1 operation))
-  (let* ((fn-list (dired-get-marked-files nil arg))
-	 (rfn-list (mapcar (function dired-make-relative) fn-list))
-	 (dired-one-file	; fluid variable inside dired-create-files
-	  (and (consp fn-list) (null (cdr fn-list)) (car fn-list)))
-	 (target-dir (dired-dwim-target-directory))
-	 (default (and dired-one-file
-		       (expand-file-name (file-name-nondirectory (car fn-list))
-					 target-dir)))
-	 (defaults (dired-dwim-target-defaults fn-list target-dir))
-	 (target (expand-file-name ; fluid variable inside dired-create-files
-		  (minibuffer-with-setup-hook
-		      (lambda ()
-			(set (make-local-variable 'minibuffer-default-add-function) nil)
-			(setq minibuffer-default defaults))
-		    (dired-mark-read-file-name
-		     (concat (if dired-one-file op1 operation) " %s to: ")
-		     target-dir op-symbol arg rfn-list default))))
-	 (into-dir (cond ((null how-to)
-			  ;; Allow DOS/Windows users to change the letter
-			  ;; case of a directory.  If we don't test these
-			  ;; conditions up front, file-directory-p below
-			  ;; will return t because the filesystem is
-			  ;; case-insensitive, and Emacs will try to move
-			  ;; foo -> foo/foo, which fails.
-			  (if (and (memq system-type '(ms-dos windows-nt cygwin))
-				   (eq op-symbol 'move)
-				   dired-one-file
-				   (string= (downcase
-					     (expand-file-name (car fn-list)))
-					    (downcase
-					     (expand-file-name target)))
-				   (not (string=
-					 (file-name-nondirectory (car fn-list))
-					 (file-name-nondirectory target))))
-			      nil
-			    (file-directory-p target)))
-			 ((eq how-to t) nil)
-			 (t (funcall how-to target)))))
-    (if (and (consp into-dir) (functionp (car into-dir)))
-	(apply (car into-dir) operation rfn-list fn-list target (cdr into-dir))
-      (if (not (or dired-one-file into-dir))
-	  (error "Marked %s: target must be a directory: %s" operation target))
-      ;; rename-file bombs when moving directories unless we do this:
-      (or into-dir (setq target (directory-file-name target)))
-      (dired-create-files
-       file-creator operation fn-list
-       (if into-dir			; target is a directory
-	   ;; This function uses fluid variable target when called
-	   ;; inside dired-create-files:
-	   (function
-	    (lambda (from)
-	      (expand-file-name (file-name-nondirectory from) target)))
-	 (function (lambda (from) target)))
-       marker-char))))
 
 ;; Read arguments for a marked-files command that wants a file name,
 ;; perhaps popping up the list of marked files.
diff --git a/lisp/files.el b/lisp/files.el
index 7ac88f8..d896020 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,23 +4723,21 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
-(defun copy-directory (directory newname &optional keep-time
-				 parents copy-as-subdir)
+(defun copy-directory (directory newname &optional keep-time parents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
+If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.
+
 This function always sets the file modes of the output files to match
 the corresponding input file.
 
 The third arg KEEP-TIME non-nil means give the output files the same
 last-modified time as the old ones.  (This works on only some systems.)
-A prefix arg makes KEEP-TIME non-nil.
 
-Optional arg PARENTS says whether to create parent directories if
-they don't exist.  When called interactively, PARENTS is t.
+A prefix arg makes KEEP-TIME non-nil.
 
-When NEWNAME is an existing directory, copy DIRECTORY into a
-subdirectory of NEWNAME if optional arg COPY-AS-SUBDIR is
-non-nil, otherwise copy the contents of DIRECTORY into NEWNAME.
-When called interactively, copy into a subdirectory by default."
+Noninteractively, the last argument PARENTS says whether to
+create parent directories if they don't exist.  Interactively,
+this happens by default."
   (interactive
    (let ((dir (read-directory-name
 	       "Copy directory: " default-directory default-directory t nil)))
@@ -4747,7 +4745,7 @@ When called interactively, copy into a subdirectory by default."
 	   (read-file-name
 	    (format "Copy directory %s to: " dir)
 	    default-directory default-directory nil nil)
-	   current-prefix-arg t t)))
+	   current-prefix-arg t)))
   ;; If default-directory is a remote directory, make sure we find its
   ;; copy-directory handler.
   (let ((handler (or (find-file-name-handler directory 'copy-directory)
@@ -4759,17 +4757,12 @@ When called interactively, copy into a subdirectory by default."
       (setq directory (directory-file-name (expand-file-name directory))
 	    newname   (directory-file-name (expand-file-name newname)))
 
-      (unless (file-directory-p directory)
-	(error "%s is not a directory" directory))
-
-      (cond
-       ((not (file-directory-p newname))
-	;; If NEWNAME is not an existing directory, create it;
-	;; that is where we will copy the files of DIRECTORY.
-	(make-directory newname parents))
-       (copy-as-subdir
-	;; If NEWNAME is an existing directory, and we are copying as
-	;; a subdirectory, the target is NEWNAME/[DIRECTORY-BASENAME].
+      (if (not (file-directory-p newname))
+	  ;; If NEWNAME is not an existing directory, create it; that
+	  ;; is where we will copy the files of DIRECTORY.
+	  (make-directory newname parents)
+	;; If NEWNAME is an existing directory, we will copy into
+	;; NEWNAME/[DIRECTORY-BASENAME].
 	(setq newname (expand-file-name
 		       (file-name-nondirectory
 			(directory-file-name directory))
@@ -4778,22 +4771,20 @@ When called interactively, copy into a subdirectory by default."
 	     (not (file-directory-p newname))
 	     (error "Cannot overwrite non-directory %s with a directory"
 		    newname))
-	(make-directory newname t)))
+	(make-directory newname t))
 
       ;; Copy recursively.
       (dolist (file
 	       ;; We do not want to copy "." and "..".
 	       (directory-files directory 'full
 				directory-files-no-dot-files-regexp))
-	(let ((target (expand-file-name
-		       (file-name-nondirectory file) newname))
-	      (attrs (file-attributes file)))
-	  (cond ((file-directory-p file)
-		 (copy-directory file target keep-time parents nil))
-		((stringp (car attrs)) ; Symbolic link
-		 (make-symbolic-link (car attrs) target t))
-		(t
-		 (copy-file file target t keep-time)))))
+	(if (file-directory-p file)
+	    (copy-directory file newname keep-time parents)
+	  (let ((target (expand-file-name (file-name-nondirectory file) newname))
+		(attrs (file-attributes file)))
+	    (if (stringp (car attrs)) ; Symbolic link
+		(make-symbolic-link (car attrs) target t)
+	      (copy-file file target t keep-time)))))
 
       ;; Set directory attributes.
       (set-file-modes newname (file-modes directory))



-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Chong Yidong]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>> Adding a separate copy-directory-1 function does nothing different from
>> adding an optional argument to copy-directory that modifies its
>> behavior.  Not much point.
>
> I am actually trying something different, i sent patch to Michael and
> Stephane.
> I restart from the Chong version without extra arg.
> This version works outside of dired interactvely and not.
>
> Then i modify a little dired-create-files.
> dired-copy-file-recursive have to call now copy-directory as
> copy-directory-1 doesn't exists anymore.
>
> It works here in dired, all scenarios of copy-directory and i tried with
> sudo method from dired and it works also.

Such an invasive patch is not suitable for the emacs-23 branch.  If we
do this, it should be for Emacs 24.

Re: bug in copy-directory

[Thierry Volpiatto]

Chong Yidong <cyd@stupidchicken.com> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>>> Adding a separate copy-directory-1 function does nothing different from
>>> adding an optional argument to copy-directory that modifies its
>>> behavior.  Not much point.
>>
>> I am actually trying something different, i sent patch to Michael and
>> Stephane.
>> I restart from the Chong version without extra arg.
>> This version works outside of dired interactvely and not.
>>
>> Then i modify a little dired-create-files.
>> dired-copy-file-recursive have to call now copy-directory as
>> copy-directory-1 doesn't exists anymore.
>>
>> It works here in dired, all scenarios of copy-directory and i tried with
>> sudo method from dired and it works also.
>
> Such an invasive patch is not suitable for the emacs-23 branch.  If we
> do this, it should be for Emacs 24.
What is invasive? It is no more than 2 lines in dired-create-files and
your initial working version of copy-directory.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

> Chong Yidong <cyd@stupidchicken.com> writes:
>
>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>
>>> Ok thanks, i sent a patch to Chong.
>>
>> I've committed your patch, with a number of changes (e.g. renaming the
>> new argument to `copy-as-subdir').  Please test it and see if everything
>> behaves as you expect.  Thanks.
>
> Now it is even worse. Going to the initial test case:
>
> - Create directory /tmp/test
> - Create directory /tmp/test/test
> - Create file /tmp/test/a
> - Create file /tmp/test/test/b
> - Apply (copy-directory "/tmp/test" "~/")
>
> You get "~/a", "~/test" and "~/test/b".
Now you have to do:
(copy-directory "/tmp/test" "~/" nil nil t)

And if you do M-x copy-directory, all work fine because last arg is
always used in interactive calls.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Chong Yidong <cyd@stupidchicken.com> writes:
>>
>>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>>
>>>> Ok thanks, i sent a patch to Chong.
>>>
>>> I've committed your patch, with a number of changes (e.g. renaming the
>>> new argument to `copy-as-subdir').  Please test it and see if everything
>>> behaves as you expect.  Thanks.
>>
>> Now it is even worse. Going to the initial test case:
>>
>> - Create directory /tmp/test
>> - Create directory /tmp/test/test
>> - Create file /tmp/test/a
>> - Create file /tmp/test/test/b
>> - Apply (copy-directory "/tmp/test" "~/")
>>
>> You get "~/a", "~/test" and "~/test/b".
> Now you have to do:
> (copy-directory "/tmp/test" "~/" nil nil t)
>
> And if you do M-x copy-directory, all work fine because last arg is
> always used in interactive calls.

If you want to be able to do:
(copy-directory "/tmp/test" "~/")
We will have to fallback to a function copy-directory1 used by dired and
a copy-directory function/command that ALWAYS use last arg of
copy-directory1.

Here are units-test for copy-directory:
(replace /home/thierry to /home/you)

;; Unit tests for copy-directory.
;; Download:
;; http://www.emacswiki.org/cgi-bin/wiki/download/el-expectations.el
;; http://www.emacswiki.org/cgi-bin/wiki/download/el-mock.el
(require 'cl)
(require 'el-expectations)
(require 'el-mock)
(expectations
  (desc "Copy directory and overwrite")
  (expect '("/home/thierry/test/a" "/home/thierry/test/test" "/home/thierry/test/test/b")
    (delete-directory "~/test" t)
    (make-directory "/tmp/test/test" t)
    (with-current-buffer (find-file-noselect "/tmp/test/a")
      (insert "test1")
      (save-buffer))
    (with-current-buffer (find-file-noselect "/tmp/test/test/b")
      (insert "test2")
      (save-buffer))
    (copy-directory "/tmp/test" "~/" nil nil t)
    (copy-directory "/tmp/test" "~/" nil nil t)
  (let (result)
    (labels ((ls-R (dir)
               (loop with ls = (directory-files dir t directory-files-no-dot-files-regexp)
                  for f in ls
                  if (file-directory-p f)
                  do (progn (push f result) (ls-R f))
                  else do (push f result))))
      (ls-R "~/test")
      (nreverse result)))))
(expectations
  (desc "Copy directory and overwrite from dired")
  (expect '("/home/thierry/test/a" "/home/thierry/test/test" "/home/thierry/test/test/b")
    (let ((A "/tmp/test")
          (B "/home/thierry/test"))
      (flet ((test-dired (flist dir)
               (dired-create-files 'dired-copy-file "copy" flist
                                   (if (file-directory-p dir)
                                       #'(lambda (from)
                                           (expand-file-name (anything-c-basename from) dir))
                                       #'(lambda (from) dir)))))

        (when (and (file-directory-p A)
                   (y-or-n-p (format "Delete directory %s? " A)))
          (delete-directory A t))
        (when (and (file-directory-p B)
                   (y-or-n-p (format "Delete directory %s? " B)))
          (delete-directory B t))
        (make-directory (concat (file-name-as-directory A) "test") t)
        (with-current-buffer (find-file-noselect "/tmp/test/a")
          (insert "test1")
          (save-buffer))
        (with-current-buffer (find-file-noselect "/tmp/test/test/b")
          (insert "test2")
          (save-buffer))
        ;; Create
        (test-dired (list A) "~/")
        ;; Overwrite
        (test-dired (list A) "~/")
        (let (result)
          (labels ((ls-R (dir)
                     (loop with ls = (directory-files dir t directory-files-no-dot-files-regexp)
                        for f in ls
                        if (file-directory-p f)
                        do (progn (push f result) (ls-R f))
                        else do (push f result))))
            (ls-R B)
            (nreverse result)))))))
 
-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Stefan Monnier]

> If you want to be able to do:
> (copy-directory "/tmp/test" "~/")
> We will have to fallback to a function copy-directory1 used by dired and
> a copy-directory function/command that ALWAYS use last arg of
> copy-directory1.

That sounds to me like the right thing to do (copy-directory already
has many arguments, and adding one which will (almost) always need to
be passed doesn't sound too attractive).

But you say it in a tone that seems to imply it's undesirable: could you
explain why you think it's undesirable?


        Stefan

Re: bug in copy-directory

[Thierry Volpiatto]

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

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

>> If you want to be able to do:
>> (copy-directory "/tmp/test" "~/")
>> We will have to fallback to a function copy-directory1 used by dired and
>> a copy-directory function/command that ALWAYS use last arg of
>> copy-directory1.
>
> That sounds to me like the right thing to do (copy-directory already
> has many arguments, and adding one which will (almost) always need to
> be passed doesn't sound too attractive).
>
> But you say it in a tone that seems to imply it's undesirable: could you
> explain why you think it's undesirable?
No, i agree to use copy-directory without adding extra arguments.
i.e (copy-directory "/tmp/test" "~/")

The problem we have actually is to make something that:

- work in interactive call to copy-directory (M-x copy-directory)
- work non--interactively ==> (copy-directory "/tmp/test" "~/")
- work from dired.

If you have only one function copy-directory that work fine in case 2,
it will fail in dired because dired already does a part of the job of
this function (maybe because long time ago dired-create-files has been
wrote on this bug), and if you make it work in dired it will fail in
directs calls to copy-directory.

So to achieve the 3 cases above i propose:

- Make a copy-directory1 function that work with dired (like ancient
  code).
- call copy-directory1 in dired-copy-file-recursive.
- Make a copy-directory function/command that reuse copy-directory1
  without extra args (like it was before, last arg is parents).

This actually works and is IMHO the best solution.(see patch)

Otherwise you have to call in dired-do-copy dired-create-files something
like this:

,----
| (dired-create-files
|  fn (symbol-name action) files
|  ;; CANDIDATE is the destination.
|  (if (file-directory-p candidate)
|      ;; When CANDIDATE is a directory, build file-name in this directory.
|      ;; Else we use CANDIDATE.
|      #'(lambda (from)
|          (let ((target (expand-file-name (file-name-nondirectory from) candidate)))
|            (if (file-directory-p target) candidate target)))
|      #'(lambda (from) candidate))
|  marker)
`----

but in this case the NAME-CONSTRUCTOR returns wrong value for message,
but not for executing code (it does the right thing)

So in this case a rewrite (partial) of dired-create-files is needed in
addition of modification of dired-do-copy call to dired-create-files.
(with possible breakage of symlinking etc...)

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

[-- Attachment #2: patch-copy-dir5 --]
[-- Type: application/octet-stream, Size: 4332 bytes --]

patch-copy-dir5

From: Thierry Volpiatto <thierry.volpiatto@gmail.com>


---
 lisp/dired-aux.el |    2 +-
 lisp/files.el     |   54 ++++++++++++++++++++++++++++-------------------------
 2 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 28b285f..b26ce0a 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -1242,7 +1242,7 @@ Special value `always' suppresses confirmation."
 	     (or (eq recursive 'always)
 		 (yes-or-no-p (format "Recursive copies of %s? " from))))
 	;; This is a directory.
-	(copy-directory from to dired-copy-preserve-time)
+	(copy-directory1 from to dired-copy-preserve-time)
       ;; Not a directory.
       (or top (dired-handle-overwrite to))
       (condition-case err
diff --git a/lisp/files.el b/lisp/files.el
index 7ac88f8..32ce50b 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,31 +4723,8 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
-(defun copy-directory (directory newname &optional keep-time
+(defun copy-directory1 (directory newname &optional keep-time
 				 parents copy-as-subdir)
-  "Copy DIRECTORY to NEWNAME.  Both args must be strings.
-This function always sets the file modes of the output files to match
-the corresponding input file.
-
-The third arg KEEP-TIME non-nil means give the output files the same
-last-modified time as the old ones.  (This works on only some systems.)
-A prefix arg makes KEEP-TIME non-nil.
-
-Optional arg PARENTS says whether to create parent directories if
-they don't exist.  When called interactively, PARENTS is t.
-
-When NEWNAME is an existing directory, copy DIRECTORY into a
-subdirectory of NEWNAME if optional arg COPY-AS-SUBDIR is
-non-nil, otherwise copy the contents of DIRECTORY into NEWNAME.
-When called interactively, copy into a subdirectory by default."
-  (interactive
-   (let ((dir (read-directory-name
-	       "Copy directory: " default-directory default-directory t nil)))
-     (list dir
-	   (read-file-name
-	    (format "Copy directory %s to: " dir)
-	    default-directory default-directory nil nil)
-	   current-prefix-arg t t)))
   ;; If default-directory is a remote directory, make sure we find its
   ;; copy-directory handler.
   (let ((handler (or (find-file-name-handler directory 'copy-directory)
@@ -4789,7 +4766,7 @@ When called interactively, copy into a subdirectory by default."
 		       (file-name-nondirectory file) newname))
 	      (attrs (file-attributes file)))
 	  (cond ((file-directory-p file)
-		 (copy-directory file target keep-time parents nil))
+		 (copy-directory1 file target keep-time parents nil))
 		((stringp (car attrs)) ; Symbolic link
 		 (make-symbolic-link (car attrs) target t))
 		(t
@@ -4799,6 +4776,33 @@ When called interactively, copy into a subdirectory by default."
       (set-file-modes newname (file-modes directory))
       (if keep-time
 	  (set-file-times newname (nth 5 (file-attributes directory)))))))
+
+(defun copy-directory (directory newname &optional keep-time parents)
+  "Copy DIRECTORY to NEWNAME.  Both args must be strings.
+This function always sets the file modes of the output files to match
+the corresponding input file.
+
+The third arg KEEP-TIME non-nil means give the output files the same
+last-modified time as the old ones.  (This works on only some systems.)
+A prefix arg makes KEEP-TIME non-nil.
+
+Optional arg PARENTS says whether to create parent directories if
+they don't exist.  When called interactively, PARENTS is t.
+
+When NEWNAME is an existing directory, copy DIRECTORY into a
+subdirectory of NEWNAME if optional arg COPY-AS-SUBDIR is
+non-nil, otherwise copy the contents of DIRECTORY into NEWNAME.
+When called interactively, copy into a subdirectory by default."
+  (interactive
+   (let ((dir (read-directory-name
+	       "Copy directory: " default-directory default-directory t nil)))
+     (list dir
+	   (read-file-name
+	    (format "Copy directory %s to: " dir)
+	    default-directory default-directory nil nil)
+	   current-prefix-arg t)))
+  (copy-directory1 directory newname keep-time parents 'copy-as-subdir))
+
 \f
 (put 'revert-buffer-function 'permanent-local t)
 (defvar revert-buffer-function nil

Re: bug in copy-directory

[Stefan Monnier]

> If you have only one function copy-directory that work fine in case 2,
> it will fail in dired because dired already does a part of the job of
> this function (maybe because long time ago dired-create-files has been
> wrote on this bug), and if you make it work in dired it will fail in
> directs calls to copy-directory.

Right, part of the problem seems to be that dired tried to work around
some lack of support in files.el.  Maybe we should revisit dired's code
to clean it up in this respect.

> So to achieve the 3 cases above i propose:

> - Make a copy-directory1 function that work with dired (like ancient
>   code).
> - call copy-directory1 in dired-copy-file-recursive.
> - Make a copy-directory function/command that reuse copy-directory1
>   without extra args (like it was before, last arg is parents).

> This actually works and is IMHO the best solution.(see patch)

Yes, it seems like the best course for now (tho we usually use foo-bar-1
rather than foo-bar1).   Your patch has copy-directory's docstring
mention the non-existent argument `copy-as-subdir', tho.

Also, shouldn't we do the find-file-name-handler dance in copy-directory
rather than in copy-directory1?

Finally, shouldn't the copy-as-subdir dance be done within
copy-directory (sole place where the arg is non-nil), which then lets us
get rid of the argument altogether?

I.e. something like the untested patch below?


        Stefan


=== modified file 'lisp/files.el'
--- lisp/files.el	2011-02-06 04:59:06 +0000
+++ lisp/files.el	2011-02-07 18:54:15 +0000
@@ -4723,8 +4723,7 @@
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
-(defun copy-directory (directory newname &optional keep-time
-				 parents copy-as-subdir)
+(defun copy-directory (directory newname &optional keep-time parents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
 This function always sets the file modes of the output files to match
 the corresponding input file.
@@ -4737,9 +4736,7 @@
 they don't exist.  When called interactively, PARENTS is t.
 
 When NEWNAME is an existing directory, copy DIRECTORY into a
-subdirectory of NEWNAME if optional arg COPY-AS-SUBDIR is
-non-nil, otherwise copy the contents of DIRECTORY into NEWNAME.
-When called interactively, copy into a subdirectory by default."
+subdirectory of NEWNAME."
   (interactive
    (let ((dir (read-directory-name
 	       "Copy directory: " default-directory default-directory t nil)))
@@ -4762,12 +4759,7 @@
       (unless (file-directory-p directory)
 	(error "%s is not a directory" directory))
 
-      (cond
-       ((not (file-directory-p newname))
-	;; If NEWNAME is not an existing directory, create it;
-	;; that is where we will copy the files of DIRECTORY.
-	(make-directory newname parents))
-       (copy-as-subdir
+      (when (file-directory-p newname)
 	;; If NEWNAME is an existing directory, and we are copying as
 	;; a subdirectory, the target is NEWNAME/[DIRECTORY-BASENAME].
 	(setq newname (expand-file-name
@@ -4777,8 +4769,15 @@
 	(and (file-exists-p newname)
 	     (not (file-directory-p newname))
 	     (error "Cannot overwrite non-directory %s with a directory"
-		    newname))
-	(make-directory newname t)))
+		    newname)))
+
+      (copy-directory-1 directory newname keep-time parents))))
+
+(defun copy-directory-1 (directory newname &optional keep-time parents)
+  (if (not (file-directory-p newname))
+      ;; If NEWNAME is not an existing directory, create it;
+      ;; that is where we will copy the files of DIRECTORY.
+      (make-directory newname parents))
 
       ;; Copy recursively.
       (dolist (file
@@ -4789,11 +4788,11 @@
 		       (file-name-nondirectory file) newname))
 	      (attrs (file-attributes file)))
 	  (cond ((file-directory-p file)
-		 (copy-directory file target keep-time parents nil))
+             (copy-directory-1 file target keep-time parents nil))
 		((stringp (car attrs)) ; Symbolic link
 		 (make-symbolic-link (car attrs) target t))
 		(t
-		 (copy-file file target t keep-time)))))
+             (copy-file file target t keep-time))))))
 
       ;; Set directory attributes.
       (set-file-modes newname (file-modes directory))

Re: bug in copy-directory

[Thierry Volpiatto]

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

>> If you have only one function copy-directory that work fine in case 2,
>> it will fail in dired because dired already does a part of the job of
>> this function (maybe because long time ago dired-create-files has been
>> wrote on this bug), and if you make it work in dired it will fail in
>> directs calls to copy-directory.
>
> Right, part of the problem seems to be that dired tried to work around
> some lack of support in files.el.  Maybe we should revisit dired's code
> to clean it up in this respect.
Yes it would be great, i don't think it's a lot of work, but need
attention to not break other actions (many) handled by dired-create-files.

May if the NAME-CONSTRUCTOR return a cons like:
(filename_for_message . real_filename)

we could test with consp and use the car for message and the cdr for
action otherwise only use the string filename.

That's just an idea untested.

> Yes, it seems like the best course for now (tho we usually use foo-bar-1
> rather than foo-bar1).   Your patch has copy-directory's docstring
> mention the non-existent argument `copy-as-subdir', tho.
Sorry, i didn't polish that, i just move code in a "foo-bar whatever
name" for the moment.

> Also, shouldn't we do the find-file-name-handler dance in copy-directory
> rather than in copy-directory1?
I am not sure of that, what happen when we copy from dired something like:
/foo/bar to /scpc:host:/foo

(Let keep copy-directory1 as name for the moment)

with the patch i sent you we have
dired-do-copy ==> dired-create-files ==> dired-copy-file ==>
dired-copy-file-recursive ==> copy-directory1

So what happen if copy-directory1 is not aware of handler to use?
Michael?

> Finally, shouldn't the copy-as-subdir dance be done within
> copy-directory (sole place where the arg is non-nil), which then lets us
> get rid of the argument altogether?
Yes, i think it's a good idea, it should work.

> I.e. something like the untested patch below?
>
>
>         Stefan
>
>
> === modified file 'lisp/files.el'
> --- lisp/files.el	2011-02-06 04:59:06 +0000
> +++ lisp/files.el	2011-02-07 18:54:15 +0000
> @@ -4723,8 +4723,7 @@
>  		 directory 'full directory-files-no-dot-files-regexp)))
>        (delete-directory-internal directory)))))
>  
> -(defun copy-directory (directory newname &optional keep-time
> -				 parents copy-as-subdir)
> +(defun copy-directory (directory newname &optional keep-time parents)
>    "Copy DIRECTORY to NEWNAME.  Both args must be strings.
>  This function always sets the file modes of the output files to match
>  the corresponding input file.
> @@ -4737,9 +4736,7 @@
>  they don't exist.  When called interactively, PARENTS is t.
>  
>  When NEWNAME is an existing directory, copy DIRECTORY into a
> -subdirectory of NEWNAME if optional arg COPY-AS-SUBDIR is
> -non-nil, otherwise copy the contents of DIRECTORY into NEWNAME.
> -When called interactively, copy into a subdirectory by default."
> +subdirectory of NEWNAME."
>    (interactive
>     (let ((dir (read-directory-name
>  	       "Copy directory: " default-directory default-directory t nil)))
> @@ -4762,12 +4759,7 @@
>        (unless (file-directory-p directory)
>  	(error "%s is not a directory" directory))
>  
> -      (cond
> -       ((not (file-directory-p newname))
> -	;; If NEWNAME is not an existing directory, create it;
> -	;; that is where we will copy the files of DIRECTORY.
> -	(make-directory newname parents))
> -       (copy-as-subdir
> +      (when (file-directory-p newname)
>  	;; If NEWNAME is an existing directory, and we are copying as
>  	;; a subdirectory, the target is NEWNAME/[DIRECTORY-BASENAME].
>  	(setq newname (expand-file-name
> @@ -4777,8 +4769,15 @@
>  	(and (file-exists-p newname)
>  	     (not (file-directory-p newname))
>  	     (error "Cannot overwrite non-directory %s with a directory"
> -		    newname))
> -	(make-directory newname t)))
> +		    newname)))
> +
> +      (copy-directory-1 directory newname keep-time parents))))
> +
> +(defun copy-directory-1 (directory newname &optional keep-time parents)
> +  (if (not (file-directory-p newname))
> +      ;; If NEWNAME is not an existing directory, create it;
> +      ;; that is where we will copy the files of DIRECTORY.
> +      (make-directory newname parents))
>  
>        ;; Copy recursively.
>        (dolist (file
> @@ -4789,11 +4788,11 @@
>  		       (file-name-nondirectory file) newname))
>  	      (attrs (file-attributes file)))
>  	  (cond ((file-directory-p file)
> -		 (copy-directory file target keep-time parents nil))
> +             (copy-directory-1 file target keep-time parents nil))
>  		((stringp (car attrs)) ; Symbolic link
>  		 (make-symbolic-link (car attrs) target t))
>  		(t
> -		 (copy-file file target t keep-time)))))
> +             (copy-file file target t keep-time))))))
>  
>        ;; Set directory attributes.
>        (set-file-modes newname (file-modes directory))
>

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Stefan Monnier]

> with the patch i sent you we have
> dired-do-copy ==> dired-create-files ==> dired-copy-file ==>
> dired-copy-file-recursive ==> copy-directory1

> So what happen if copy-directory1 is not aware of handler to use?
> Michael?

Indeed, then we'd need a copy-directory-2 for dired's use (which would
do the needed cleanup and the check for file-name-handlers).


        Stefan

Re: bug in copy-directory

[Chong Yidong]

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

> copy-directory already has many arguments, and adding one which will
> (almost) always need to be passed doesn't sound too attractive.

I'm not sure that's true.  Suppose you want to do something like this:

  Move the files /foo/file-1, /foo/file-2, etc., so they are found
  in /bar: /bar/file-1, /bzr/file-2, etc.

Currently, you can do this with (copy-directory "/foo" "/bar").  Even if
/bar exists, that is OK.

If you want to use copy-directory with cp-like semantics (via a
copy-as-subdir argument, or if we give copy-directory such behavior by
default), that imposes an extra step: you have to check if /bar already
exists.  If it exists, you have to do take some non-trivial steps to
either delete it or merge the contents of /foo into /bar.

In the shell, of course, you just do "cp -r /foo/* /bar".  That's
because cp acts on both files and directories; copy-directory only
copies directories.  This implies that giving copy-directory cp-like
semantics might be a mistake.

Re: bug in copy-directory

[Stefan Monnier]

> In the shell, of course, you just do "cp -r /foo/* /bar".  That's
> because cp acts on both files and directories; copy-directory only
> copies directories.  This implies that giving copy-directory cp-like
> semantics might be a mistake.

OK.


        Stefan

Re: bug in copy-directory

[Thierry Volpiatto]

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

>> In the shell, of course, you just do "cp -r /foo/* /bar".  That's
>> because cp acts on both files and directories; copy-directory only
>> copies directories.  This implies that giving copy-directory cp-like
>> semantics might be a mistake.
>
> OK.
If you want to keep the ability to do "cp -r /foo/* /bar" you can just
add a new arg copy-contents-only (or whatever name) to copy-directory:


--8<---------------cut here---------------start------------->8---
(defun copy-directory (directory newname &optional keep-time parents copy-contents-only)

[...]

            (unless copy-contents-only
              (setq newname (expand-file-name
                             (file-name-nondirectory
                              (directory-file-name directory))
                             newname))
              (and (file-exists-p newname)
                   (not (file-directory-p newname))
                   (error "Cannot overwrite non-directory %s with a directory"
                          newname))
              (make-directory newname t)))


[...]
--8<---------------cut here---------------end--------------->8---


and do:

(copy-directory "/foo" "/existing/directory/bar" nil nil t)

NOTE: 
- this feature would not work with tramp names without changes to
  tramp.
  However (copy-directory "/foo" "/existing/directory/bar")
  will work without changes.


-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> In the shell, of course, you just do "cp -r /foo/* /bar".  That's
>>> because cp acts on both files and directories; copy-directory only
>>> copies directories.  This implies that giving copy-directory cp-like
>>> semantics might be a mistake.
>>
>> OK.
> If you want to keep the ability to do "cp -r /foo/* /bar" you can just
> add a new arg copy-contents-only (or whatever name) to copy-directory:
>
> (defun copy-directory (directory newname &optional keep-time parents copy-contents-only)

I'm in favor of this proposal (compared with the recently introduced
`copy-as-subdir'). Thierry's proposal keeps the semantics of
`copy-directory', and the changed semantics are activated by the
optional argument.

> NOTE:
> - this feature would not work with tramp names without changes to
>   tramp.
>   However (copy-directory "/foo" "/existing/directory/bar")
>   will work without changes.

Changes in Tramp would be simple.

Best regards, Michael.

Re: bug in copy-directory

[Thierry Volpiatto]

Michael Albinus <michael.albinus@gmx.de> writes:

>> (defun copy-directory (directory newname &optional keep-time parents copy-contents-only)
>
> I'm in favor of this proposal (compared with the recently introduced
> `copy-as-subdir'). Thierry's proposal keeps the semantics of
> `copy-directory', and the changed semantics are activated by the
> optional argument.

So when everybody is ok, i can send patch with this modification.
Tell me also if copy-contents-only as arg name is ok for you (if you
have better idea i take).

> Changes in Tramp would be simple.
Cool ;-)

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> So when everybody is ok, i can send patch with this modification.

I would say Chong or Stefan shall decide.

Best regards, Michael.

Re: bug in copy-directory

[Chong Yidong]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>>> (defun copy-directory (directory newname &optional keep-time
>> parents copy-contents-only)
>>
>> I'm in favor of this proposal (compared with the recently introduced
>> `copy-as-subdir'). Thierry's proposal keeps the semantics of
>> `copy-directory', and the changed semantics are activated by the
>> optional argument.
>
> So when everybody is ok, i can send patch with this modification.
> Tell me also if copy-contents-only as arg name is ok for you (if you
> have better idea i take).

This is OK for the trunk.  If we go this route, I'd prefer to revert the
branch to the Emacs 23.2 behavior, since it requires non-local changes
and is backward incompatible; then we'll make the change for Emacs 24.

Go ahead and send the patch anytime it's ready.

Re: bug in copy-directory

[Thierry Volpiatto]

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

Chong Yidong <cyd@stupidchicken.com> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Michael Albinus <michael.albinus@gmx.de> writes:
>>
>>>> (defun copy-directory (directory newname &optional keep-time
>>> parents copy-contents-only)
>>>
>>> I'm in favor of this proposal (compared with the recently introduced
>>> `copy-as-subdir'). Thierry's proposal keeps the semantics of
>>> `copy-directory', and the changed semantics are activated by the
>>> optional argument.
>>
>> So when everybody is ok, i can send patch with this modification.
>> Tell me also if copy-contents-only as arg name is ok for you (if you
>> have better idea i take).
>
> This is OK for the trunk.  If we go this route, I'd prefer to revert the
> branch to the Emacs 23.2 behavior, since it requires non-local changes
> and is backward incompatible; then we'll make the change for Emacs 24.
???I would say revert to Emacs 23.2 bug...

> Go ahead and send the patch anytime it's ready.
Find it attached.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

[-- Attachment #2: copy-only-contents --]
[-- Type: application/octet-stream, Size: 4625 bytes --]

Bugfix copy-directory

From: Thierry Volpiatto <thierry.volpiatto@gmail.com>

lisp/files.el (copy-directory): new arg copy-only-contents Allow to copy contents of directory whitout subdir.

lisp/dired-aux.el (dired-create-files): Don't create subdirectory in dired-copy-file.
---
 lisp/files.el |   77 +++++++++++++++++++++++++++++----------------------------
 1 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index d896020..61131dc 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,7 +4723,7 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
-(defun copy-directory (directory newname &optional keep-time parents)
+(defun copy-directory (directory newname &optional keep-time parents contents-only)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
 If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.
 
@@ -4753,43 +4753,44 @@ this happens by default."
     (if handler
 	(funcall handler 'copy-directory directory newname keep-time parents)
 
-      ;; Compute target name.
-      (setq directory (directory-file-name (expand-file-name directory))
-	    newname   (directory-file-name (expand-file-name newname)))
-
-      (if (not (file-directory-p newname))
-	  ;; If NEWNAME is not an existing directory, create it; that
-	  ;; is where we will copy the files of DIRECTORY.
-	  (make-directory newname parents)
-	;; If NEWNAME is an existing directory, we will copy into
-	;; NEWNAME/[DIRECTORY-BASENAME].
-	(setq newname (expand-file-name
-		       (file-name-nondirectory
-			(directory-file-name directory))
-		       newname))
-	(and (file-exists-p newname)
-	     (not (file-directory-p newname))
-	     (error "Cannot overwrite non-directory %s with a directory"
-		    newname))
-	(make-directory newname t))
-
-      ;; Copy recursively.
-      (dolist (file
-	       ;; We do not want to copy "." and "..".
-	       (directory-files directory 'full
-				directory-files-no-dot-files-regexp))
-	(if (file-directory-p file)
-	    (copy-directory file newname keep-time parents)
-	  (let ((target (expand-file-name (file-name-nondirectory file) newname))
-		(attrs (file-attributes file)))
-	    (if (stringp (car attrs)) ; Symbolic link
-		(make-symbolic-link (car attrs) target t)
-	      (copy-file file target t keep-time)))))
-
-      ;; Set directory attributes.
-      (set-file-modes newname (file-modes directory))
-      (if keep-time
-	  (set-file-times newname (nth 5 (file-attributes directory)))))))
+        ;; Compute target name.
+        (setq directory (directory-file-name (expand-file-name directory))
+              newname   (directory-file-name (expand-file-name newname)))
+
+        (if (not (file-directory-p newname))
+            ;; If NEWNAME is not an existing directory, create it; that
+            ;; is where we will copy the files of DIRECTORY.
+            (make-directory newname parents)
+            ;; If NEWNAME is an existing directory, we will copy into
+            ;; NEWNAME/[DIRECTORY-BASENAME].
+            (unless contents-only
+              (setq newname (expand-file-name
+                             (file-name-nondirectory
+                              (directory-file-name directory))
+                             newname))
+              (and (file-exists-p newname)
+                   (not (file-directory-p newname))
+                   (error "Cannot overwrite non-directory %s with a directory"
+                          newname))
+              (make-directory newname t)))
+
+        ;; Copy recursively.
+        (dolist (file
+                  ;; We do not want to copy "." and "..".
+                  (directory-files directory 'full
+                                   directory-files-no-dot-files-regexp))
+          (if (file-directory-p file)
+              (copy-directory file newname keep-time parents)
+              (let ((target (expand-file-name (file-name-nondirectory file) newname))
+                    (attrs (file-attributes file)))
+                (if (stringp (car attrs)) ; Symbolic link
+                    (make-symbolic-link (car attrs) target t)
+                    (copy-file file target t keep-time)))))
+
+        ;; Set directory attributes.
+        (set-file-modes newname (file-modes directory))
+        (when keep-time
+          (set-file-times newname (nth 5 (file-attributes directory)))))))
 \f
 (put 'revert-buffer-function 'permanent-local t)
 (defvar revert-buffer-function nil

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Chong Yidong <cyd@stupidchicken.com> writes:
>
>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>
>>> Michael Albinus <michael.albinus@gmx.de> writes:
>>>
>>>>> (defun copy-directory (directory newname &optional keep-time
>>>> parents copy-contents-only)
>>>>
>>>> I'm in favor of this proposal (compared with the recently introduced
>>>> `copy-as-subdir'). Thierry's proposal keeps the semantics of
>>>> `copy-directory', and the changed semantics are activated by the
>>>> optional argument.
>>>
>>> So when everybody is ok, i can send patch with this modification.
>>> Tell me also if copy-contents-only as arg name is ok for you (if you
>>> have better idea i take).
>>
>> This is OK for the trunk.  If we go this route, I'd prefer to revert the
>> branch to the Emacs 23.2 behavior, since it requires non-local changes
>> and is backward incompatible; then we'll make the change for Emacs 24.
> ???I would say revert to Emacs 23.2 bug...

Thierry's patch, removing the recently introduced `copy-as-subdir',
could go into Emacs 23.3 IMHO. The extension `copy-contents-only' shall
be applied in the trunk.

Best regards, Michael.

Re: bug in copy-directory

[Chong Yidong]

Michael Albinus <michael.albinus@gmx.de> writes:

>>> This is OK for the trunk.  If we go this route, I'd prefer to revert the
>>> branch to the Emacs 23.2 behavior, since it requires non-local changes
>>> and is backward incompatible; then we'll make the change for Emacs 24.
>> ???I would say revert to Emacs 23.2 bug...
>
> Thierry's patch, removing the recently introduced `copy-as-subdir',
> could go into Emacs 23.3 IMHO. The extension `copy-contents-only' shall
> be applied in the trunk.

Yep.

Re: bug in copy-directory

[Chong Yidong]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>> Go ahead and send the patch anytime it's ready.
>
> Find it attached.

Thanks, I've applied it to the trunk.  A few comments:

1. I didn't apply the change to dired-aux.el.  It doesn't seem to do the
right thing.  With that change patch, if I try to use the `C' command in
Dired, I get an error.

   mkdir ~/a
   emacs ~/
   [move point to a]
   C
   ~/b RET  => error.

Could you check this part again?  What's its purpose?

2. You have non-standard indentation settings which make your patches
bigger than they should be, harder to read, and requires re-editing of
the results before they can be committed.  Stuff like this should not be
happening:

-      ;; Compute target name.
-      (setq directory (directory-file-name (expand-file-name directory))
-	    newname   (directory-file-name (expand-file-name newname)))
...
+        ;; Compute target name.
+        (setq directory (directory-file-name (expand-file-name directory))
+              newname   (directory-file-name (expand-file-name newname)))

3. Don't include frivolous whitespace changes in patches, they are
irrelevant and make it harder to follow.

 \f
 ;; The basic function for half a dozen variations on cp/mv/ln/ln -s.
+\f
 (defun dired-create-files (file-creator operation fn-list name-constructor
 					&optional marker-char)

Re: bug in copy-directory

[Thierry Volpiatto]

Chong Yidong <cyd@stupidchicken.com> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>>> Go ahead and send the patch anytime it's ready.
>>
>> Find it attached.
>
> Thanks, I've applied it to the trunk.  A few comments:
>
> 1. I didn't apply the change to dired-aux.el.  It doesn't seem to do the
> right thing.  With that change patch, if I try to use the `C' command in
> Dired, I get an error.
>
>    mkdir ~/a
>    emacs ~/
>    [move point to a]
>    C
>    ~/b RET  => error.
To be sure:
b is a non--existing directory, right?

> Could you check this part again?  What's its purpose?


> 2. You have non-standard indentation settings which make your patches
> bigger than they should be, harder to read, and requires re-editing of
> the results before they can be committed.  Stuff like this should not be
I use cl indent, just apply patch and reindent.

> 3. Don't include frivolous whitespace changes in patches, they are
> irrelevant and make it harder to follow.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Chong Yidong]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>> 1. I didn't apply the change to dired-aux.el.  It doesn't seem to do the
>> right thing.  With that change patch, if I try to use the `C' command in
>> Dired, I get an error.
>>
>>    mkdir ~/a
>>    emacs ~/
>>    [move point to a]
>>    C
>>    ~/b RET  => error.
>
> To be sure:
> b is a non--existing directory, right?

Yep.

>> 2. You have non-standard indentation settings which make your patches
>> bigger than they should be, harder to read, and requires re-editing of
>> the results before they can be committed.  Stuff like this should not be
>
> I use cl indent, just apply patch and reindent.

It's unreasonable to ask others to apply a patch, edit the results, and
rediff just to read your patches.  It defeats the purpose of diffs.

Re: bug in copy-directory

[Thierry Volpiatto]

Chong Yidong <cyd@stupidchicken.com> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>>> 1. I didn't apply the change to dired-aux.el.  It doesn't seem to do the
>>> right thing.  With that change patch, if I try to use the `C' command in
>>> Dired, I get an error.
>>>
>>>    mkdir ~/a
>>>    emacs ~/
>>>    [move point to a]
>>>    C
>>>    ~/b RET  => error.
>>
>> To be sure:
>> b is a non--existing directory, right?
>
> Yep.
Ok now it is fixed and applied on emacs, just a final note:
The changes i made to dired-create-files could go to dired-copy-file,
(not tested though) maybe they fit better there than in
dired-create-files.
It would be even simpler:

(when (and (file-directory-p from)
            (file-directory-p to)
            (eq file-creator 'dired-copy-file)) <<<< can be removed
   (setq to (file-name-directory to)))


-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

Re: bug in copy-directory

[Thierry Volpiatto]

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

Chong Yidong <cyd@stupidchicken.com> writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>
>> Michael Albinus <michael.albinus@gmx.de> writes:
>>
>>>> (defun copy-directory (directory newname &optional keep-time
>>> parents copy-contents-only)
>>>
>>> I'm in favor of this proposal (compared with the recently introduced
>>> `copy-as-subdir'). Thierry's proposal keeps the semantics of
>>> `copy-directory', and the changed semantics are activated by the
>>> optional argument.
>>
>> So when everybody is ok, i can send patch with this modification.
>> Tell me also if copy-contents-only as arg name is ok for you (if you
>> have better idea i take).
>
> This is OK for the trunk.  If we go this route, I'd prefer to revert the
> branch to the Emacs 23.2 behavior, since it requires non-local changes
> and is backward incompatible; then we'll make the change for Emacs 24.
>
> Go ahead and send the patch anytime it's ready.
Sorry, last patch apply on my precedent patch, this one should be correct.

-- 
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

[-- Attachment #2: dired-create-files --]
[-- Type: application/octet-stream, Size: 7603 bytes --]

Bugfix copy-directory,dired-create-files

From: Thierry Volpiatto <thierry.volpiatto@gmail.com>

lisp/files.el (copy-directory): new arg copy-only-contents Allow to copy contents of directory whitout subdir.

lisp/dired-aux.el (dired-create-files): Don't create subdirectory in dired-copy-file.
---
 lisp/dired-aux.el |    5 +++
 lisp/files.el     |  100 ++++++++++++++++++++++++-----------------------------
 2 files changed, 51 insertions(+), 54 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 28b285f..7c991b7 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -1345,6 +1345,7 @@ Special value `always' suppresses confirmation."
 	(when cons (setcar cons cur-dir))))))
 \f
 ;; The basic function for half a dozen variations on cp/mv/ln/ln -s.
+\f
 (defun dired-create-files (file-creator operation fn-list name-constructor
 					&optional marker-char)
 
@@ -1403,6 +1404,8 @@ ESC or `q' to not overwrite any of the remaining files,
                   (cond  ((integerp marker-char) marker-char)
                          (marker-char (dired-file-marker from)) ; slow
                          (t nil))))
+            (when (and (file-directory-p from) (eq file-creator 'dired-copy-file))
+              (setq to (file-name-directory to)))
             (condition-case err
                 (progn
                   (funcall file-creator from to dired-overwrite-confirmed)
@@ -1445,7 +1448,9 @@ ESC or `q' to not overwrite any of the remaining files,
       (message "%s: %s file%s"
 	       operation success-count (dired-plural-s success-count)))))
   (dired-move-to-filename))
+
 \f
+
 (defun dired-do-create-files (op-symbol file-creator operation arg
 					&optional marker-char op1
 					how-to)
diff --git a/lisp/files.el b/lisp/files.el
index 7ac88f8..61131dc 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4723,23 +4723,21 @@ If RECURSIVE is non-nil, all files in DIRECTORY are deleted as well."
 		 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
-(defun copy-directory (directory newname &optional keep-time
-				 parents copy-as-subdir)
+(defun copy-directory (directory newname &optional keep-time parents contents-only)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
+If NEWNAME names an existing directory, copy DIRECTORY as subdirectory there.
+
 This function always sets the file modes of the output files to match
 the corresponding input file.
 
 The third arg KEEP-TIME non-nil means give the output files the same
 last-modified time as the old ones.  (This works on only some systems.)
-A prefix arg makes KEEP-TIME non-nil.
 
-Optional arg PARENTS says whether to create parent directories if
-they don't exist.  When called interactively, PARENTS is t.
+A prefix arg makes KEEP-TIME non-nil.
 
-When NEWNAME is an existing directory, copy DIRECTORY into a
-subdirectory of NEWNAME if optional arg COPY-AS-SUBDIR is
-non-nil, otherwise copy the contents of DIRECTORY into NEWNAME.
-When called interactively, copy into a subdirectory by default."
+Noninteractively, the last argument PARENTS says whether to
+create parent directories if they don't exist.  Interactively,
+this happens by default."
   (interactive
    (let ((dir (read-directory-name
 	       "Copy directory: " default-directory default-directory t nil)))
@@ -4747,7 +4745,7 @@ When called interactively, copy into a subdirectory by default."
 	   (read-file-name
 	    (format "Copy directory %s to: " dir)
 	    default-directory default-directory nil nil)
-	   current-prefix-arg t t)))
+	   current-prefix-arg t)))
   ;; If default-directory is a remote directory, make sure we find its
   ;; copy-directory handler.
   (let ((handler (or (find-file-name-handler directory 'copy-directory)
@@ -4755,50 +4753,44 @@ When called interactively, copy into a subdirectory by default."
     (if handler
 	(funcall handler 'copy-directory directory newname keep-time parents)
 
-      ;; Compute target name.
-      (setq directory (directory-file-name (expand-file-name directory))
-	    newname   (directory-file-name (expand-file-name newname)))
-
-      (unless (file-directory-p directory)
-	(error "%s is not a directory" directory))
-
-      (cond
-       ((not (file-directory-p newname))
-	;; If NEWNAME is not an existing directory, create it;
-	;; that is where we will copy the files of DIRECTORY.
-	(make-directory newname parents))
-       (copy-as-subdir
-	;; If NEWNAME is an existing directory, and we are copying as
-	;; a subdirectory, the target is NEWNAME/[DIRECTORY-BASENAME].
-	(setq newname (expand-file-name
-		       (file-name-nondirectory
-			(directory-file-name directory))
-		       newname))
-	(and (file-exists-p newname)
-	     (not (file-directory-p newname))
-	     (error "Cannot overwrite non-directory %s with a directory"
-		    newname))
-	(make-directory newname t)))
-
-      ;; Copy recursively.
-      (dolist (file
-	       ;; We do not want to copy "." and "..".
-	       (directory-files directory 'full
-				directory-files-no-dot-files-regexp))
-	(let ((target (expand-file-name
-		       (file-name-nondirectory file) newname))
-	      (attrs (file-attributes file)))
-	  (cond ((file-directory-p file)
-		 (copy-directory file target keep-time parents nil))
-		((stringp (car attrs)) ; Symbolic link
-		 (make-symbolic-link (car attrs) target t))
-		(t
-		 (copy-file file target t keep-time)))))
-
-      ;; Set directory attributes.
-      (set-file-modes newname (file-modes directory))
-      (if keep-time
-	  (set-file-times newname (nth 5 (file-attributes directory)))))))
+        ;; Compute target name.
+        (setq directory (directory-file-name (expand-file-name directory))
+              newname   (directory-file-name (expand-file-name newname)))
+
+        (if (not (file-directory-p newname))
+            ;; If NEWNAME is not an existing directory, create it; that
+            ;; is where we will copy the files of DIRECTORY.
+            (make-directory newname parents)
+            ;; If NEWNAME is an existing directory, we will copy into
+            ;; NEWNAME/[DIRECTORY-BASENAME].
+            (unless contents-only
+              (setq newname (expand-file-name
+                             (file-name-nondirectory
+                              (directory-file-name directory))
+                             newname))
+              (and (file-exists-p newname)
+                   (not (file-directory-p newname))
+                   (error "Cannot overwrite non-directory %s with a directory"
+                          newname))
+              (make-directory newname t)))
+
+        ;; Copy recursively.
+        (dolist (file
+                  ;; We do not want to copy "." and "..".
+                  (directory-files directory 'full
+                                   directory-files-no-dot-files-regexp))
+          (if (file-directory-p file)
+              (copy-directory file newname keep-time parents)
+              (let ((target (expand-file-name (file-name-nondirectory file) newname))
+                    (attrs (file-attributes file)))
+                (if (stringp (car attrs)) ; Symbolic link
+                    (make-symbolic-link (car attrs) target t)
+                    (copy-file file target t keep-time)))))
+
+        ;; Set directory attributes.
+        (set-file-modes newname (file-modes directory))
+        (when keep-time
+          (set-file-times newname (nth 5 (file-attributes directory)))))))
 \f
 (put 'revert-buffer-function 'permanent-local t)
 (defvar revert-buffer-function nil

Re: bug in copy-directory

[Michael Albinus]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Chong Yidong <cyd@stupidchicken.com> writes:
>>
>>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>>
>>>> Ok thanks, i sent a patch to Chong.
>>>
>>> I've committed your patch, with a number of changes (e.g. renaming the
>>> new argument to `copy-as-subdir').  Please test it and see if everything
>>> behaves as you expect.  Thanks.
>>
>> Now it is even worse. Going to the initial test case:
>>
>> - Create directory /tmp/test
>> - Create directory /tmp/test/test
>> - Create file /tmp/test/a
>> - Create file /tmp/test/test/b
>> - Apply (copy-directory "/tmp/test" "~/")
>>
>> You get "~/a", "~/test" and "~/test/b".
> Now you have to do:
> (copy-directory "/tmp/test" "~/" nil nil t)

No, I haven't. KEEP-TIME, PARENTS and COPY-AS-SUBDIR are optional
arguments; if I use `copy-directory' w/o them the result shall be
acceptable.