[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarch
From: |
Eli Zaretskii |
Subject: |
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@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.
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, (continued)
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/21
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Stefan Monnier, 2012/02/21
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/22
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Stefan Monnier, 2012/02/22
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/23
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/23
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Stefan Monnier, 2012/02/23
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/23
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy,
Eli Zaretskii <=
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Michael Albinus, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Michael Albinus, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Thierry Volpiatto, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Michael Albinus, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Eli Zaretskii, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Michael Albinus, 2012/02/24
- bug#10489: 24.0.92; dired-do-copy may create infinite directory hierarchy, Eli Zaretskii, 2012/02/24