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

[Eli Zaretskii <eliz@gnu.org>]

> From: Thierry Volpiatto <thierry.volpiatto@gmail.com>
> Date: Fri, 24 Feb 2012 08:16:08 +0100
> 
> Just realize that this match was quite old.
> I have merged this patch with last revision of today.
> So ignore precedent and review this one.
> I it's ok I will apply it on trunk.

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)

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.

>                                        (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,

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.

> +(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.

> +(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."

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.

> +  (when (and (not (or (file-remote-p file1)
> +                      (file-remote-p file2)))
> +             (not (string= file1 "/"))

Unixism alert!  What about the equivalent "X:/" on Windows?

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?

> +    (or (string= file2 "/")

Same here, on both accounts.  Why do you single-case "/", anyway?
It's as good a directory as any.

> +        (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?

> +              for p = (string-match "^/" f1)

Unixism alert again!

> +              (equal (file-attributes (file-truename root))
> +                     (file-attributes f2))))))

Why don't you use files-equal-p here?

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

See above.

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.

Thanks.