bug#5436: 23.1.91; Deleting directories causes unusable file layout in freedesktop trashcan

[Eli Zaretskii]

> Date: Sun, 24 Jan 2010 04:14:10 +0000
> From: David De La Harpe Golden <david@harpegolden.net>
> Cc: Tassilo Horn <tassilo@member.fsf.org>, 5436@debbugs.gnu.org
> 
> Please find attached small* patch that should address this (and Bug#3353).

Thanks.

> -        ;; We do not want to delete "." and "..".

This comment was not added to the new code.  I think it's useful, even
though the name directory-files-no-dot-files-regexp hints about the
intent.

> +      ;; move-file-to-trash moves directories, empty or
> +      ;; otherwise, into the trash as a unit. So if the directory is
> +      ;; non-empty, only call move-file-to-trash here if recursive is 
> non-nil,
> +      ;; so that having delete-by-moving-trash non-nil doesn't cause 
> "greedier"
> +      ;; (pseudo-)deletion behaviour.

Looks like this comment was not filled (via M-q).  Also, please always
leave two blanks after the period that ends a sentence.

> +      (if delete-by-moving-to-trash
> +          (if (and (not recursive)
> +                   (directory-files
> +                    directory 'full directory-files-no-dot-files-regexp))
> +              (error "Directory is not empty, not moving to trash.")
> +            (move-file-to-trash directory))

Why error out here?  Isn't it better to display a message and continue
(with other potential moves)?  I'm frequently annoyed by a tool's
tendency to give up early leaving me in the middle of a partly done
job.

> +          ;; do recursion if necessary if we're not moving to trash

Full sentences beginning with a capital letter, please.

> +deleting files outright. If FILENAME is a directory, it
                          ^^
One more space.

> +/* Lisp function for recursively copying directories. */
                                                        ^
Two spaces here.

> +    if (!NILP (Ffile_directory_p (file)))
> +      {
> +        newname = Fexpand_file_name (Ffile_name_nondirectory 
> (Fdirectory_file_name (file)), newname);
> +      }
> +    else
> +      {
> +        newname = Fexpand_file_name (Ffile_name_nondirectory (file), 
> newname);
> +      }

No need for braces if there's only one line in the clause.

Thanks again for working on this.