Hello Paul,
On Mon, Jul 29, 2019 at 06:50:46PM -0500, Paul Eggert wrote:
> On 7/29/19 1:28 AM, Assaf Gordon wrote:
> > + if (rename_errno == ENOTEMPTY || rename_errno == EEXIST)
> > +{
> > + error (0, 0, _("cannot move %s to %s: Target directory not
> > empty"),
> > + quoteaf_n (0, src_name), quoteaf_n (1, dst_name));
>
> Although this is an improvement, it is not general enough, as other errno
> values are relevant only for the destination. Better would be to have a
> special case for errno values that matter only for the destination, and use
> the existing code for errno values where we don't know whether the problem
> is the source or the destination. Something like the attached, say.
> +case EDQUOT: case EEXIST: case EISDIR: case ENOSPC: case
> ENOTEMPTY:
> + error (0, rename_errno, "%s", quotearg_colon (dst_name));
> + break;
> +
Thanks for the review.
At the risk of bikeshedding, I'd like to argue for the prior method.
While it is not general enough, I think it provides a clearer error message.
For example, with the more general implementation the errors would be:
$ mv A B
mv: B/A: Directory not empty
$ mv A B
mv: B/A: No space left on device
$ mv A B
mv: B/A: Quota exceeded
In the first case,
I think this error is potentially more confusing than
before: while it doesn't mention the source directory, it also doesn't
say "cannot move" - so it is only implied it is an error (an
inexperienced user might dismiss this as a warning).
Also, it could be that there will be a source directory named very similarly
to the destination directory, and from a quick glace it would not be easy to
understand what happened.
An explicit error explicitly saying "cannot move", and mention the source and
destination, and also "blames" the target directory seems the most
user-friendly and least ambiguous.
---
For the second and third cases,
"No space" and "Quota exceeded" seem to me to always relate to the
destination, and I don't think users get confused about those
(other opinions of course welcomed).
---
Your patch also added "EISDIR", for which rename(2) says:
"newpath is an existing directory, but oldpath is not a directory."
But I don't think this error can happen with gnu mv.
If we try to move a file onto a directory, we get:
$ mkdir C C/D ; touch D
$ mv D C
mv: cannot overwrite directory 'C/D' with non-directory
And this case is specifically handled in copy.c line 2131, before
calling rename(2) (and also this is an example of a custom error
message instead of using stock libc messages).
---
Happy to hear your opinion,
- assaf
