bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy

[Thierry Volpiatto]

Hi Eli and thanks for your feedback.

Eli Zaretskii <eliz@gnu.org> writes:

> Some feedback below.  Apologies if I say something silly because I
> didn't track this discussion from the beginning.
>
>> +  (when (file-subdir-of-p to from)
>> +    (error "Can't copy directory `%s' on itself" from))
>
> A better error message would be
>
>   (error "Cannot copy `%s' into its subdirectory `%s'" from to)
Agree.

> IOW, don't assume that the fact of TO being a subdirectory of FROM is
> immediately evident to the user, just by looking at one of them.
>
>> +            ;; In this case the 'name-constructor' have set the destination
>> +            ;; 'to' to "~/test/foo" because the old
>> +            ;; emacs23 behavior of `copy-directory'
>> +            ;; was no not create the subdir and copy instead the contents 
>> only.
>                       ^^^^^^^^^^^^^
> Something's wrong here.  Did you mean "to not create"?  If so, "not to
> create the subdirectory and instead copy the contents" is clearer.
>
>> +            ;; With it's new behavior
>                        ^^^^
> "its".  "It's" is a short for "it is", which is not what you want here.
Yes thanks, I always do this mistake.

>>                                        (similar to cp shell command) we don't
>                                                   ^^^^^^^^^^^^^^^^^^^
> "to the `cp' shell command"
>
>> +            ;; need such a construction, so modify the destination 'to' to
>                   ^^^^^^^^^^^^^^^^^^^^^^^^
> What "construction"?  This word doesn't belong here.
>
>> +            ;; "~/test/" instead of "~/test/foo/".
>> +            ;; If from and to are the same directory do the same,
Ok for all corrections in comments.

> Suggest to use FROM and TO, in caps, to distinguish arguments from the
> rest of the comment text.
>
>> +                   (error "Can't copy directory `%s' on itself" from)))
>
> See above for a better error message text.
Same agree on this.

>> +(defun files-equal-p (file1 file2)
>> +  "Return non-nil if FILE1 and FILE2 name the same file."
>> +  (and (equal (file-remote-p file1) (file-remote-p file2))
>> +       (equal (file-attributes (file-truename (expand-file-name file1)))
>> +              (file-attributes (file-truename (expand-file-name file2))))))
>
> I don't understand why you use expand-file-name here: file-truename
> does it for you anyway.
Yes, agree, vestiges of precedents changes.

>> +(defun file-subdir-of-p (file1 file2)
>> +  "Check if FILE1 is a subdirectory of FILE2 on current filesystem.
>> +If directory FILE1 is the same than directory FILE2, return non--nil."
>
> Suggest to modify the doc string as follows:
>
>   "Return non-nil if FILE1 is a subdirectory of FILE2.
> Note that a directory is treated by this function as a subdirectory of 
> itself."
Ok

> Btw, I would call the arguments DIR1 and DIR2, otherwise the above
> sounds awkward ("FILE1 is a subdirectory ...") and even begs a
> question about what happens if FILE1 is not a directory, but a file
> living inside the directory FILE2.
Agree.

>> +  (when (and (not (or (file-remote-p file1)
>> +                      (file-remote-p file2)))
>> +             (not (string= file1 "/"))
>
> Unixism alert!  What about the equivalent "X:/" on Windows?
Think this is no more needed.

> Also, what should the following return?
>
>    (file-subdir-of-p "/" "/")
>
> According to the doc string, it should return non-nil, but the above
> string= condition would seem to cause it return nil, right?
Right.

>> +    (or (string= file2 "/")
>
> Same here, on both accounts.  Why do you single-case "/", anyway?
> It's as good a directory as any.
Should be removed yes.

>> +        (loop with f1 = (expand-file-name (file-truename file1))
>> +              with f2 = (expand-file-name (file-truename file2))
>
> file-truename already expands its argument, so why would you need to
> run it through expand-file-name again?
Yes same as above.

>> +              for p = (string-match "^/" f1)
>
> Unixism alert again!
No, it is not, this allow the function to work both on window and nix.
See (if p (concat "/" i) (concat i "/"))
Will add comment there.

>> +              (equal (file-attributes (file-truename root))
>> +                     (file-attributes f2))))))
>
> Why don't you use files-equal-p here?
I think I can, don't remember though, I have to check.

>> +  (when (file-subdir-of-p newname directory)
>> +    (error "Can't copy directory `%s' on itself" directory))
>
> See above.
Same agree.

> Finally, it looks like this function only works when its two arguments
> already exist; when they don't, it returns nil.  If this is the
> intent, it should be reflected in the doc string.
It is not the intent, but that is a good question, what if I copy e.g:

~/tmp/Test to ~/tmp/Test/Test1

where Test1 is a non--existing subdir of Test.

Will check this, thanks.

Though that the change that would follow later after 24.1 should fix
this, falling back with the same behavior as 'cp'.

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