GNU bug report logs - #10489
24.0.92; dired-do-copy may create infinite directory hierarchy

Previous Next

Full log

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thierry Volpiatto <thierry.volpiatto <at> gmail.com>
Cc: 10489 <at> debbugs.gnu.org
Subject: Re: bug#10489: 24.0.92;
	dired-do-copy may create infinite directory hierarchy
Date: Fri, 24 Feb 2012 11:19:04 +0200

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

Previous Next

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