On Thu, Aug 01, 2019 at 03:58:51PM -0700, Paul Eggert wrote: > Thanks, that's better, but we're still missing some opportunities for > improvement. > > > mv: cannot move 'A' to 'B/A': Target directory not empty > > This should be "Destination" not "Target". [...] > You meant "mv" not "rm". [...] > > +static char* > Space before "*". [...] > > +strerror_target (int e) > Change name to "strerror_dest" [...] > This function should return NULL instead of aborting when the errno value is > inapplicable. That way, its callers need not hardcode which errno values it > handles.
Thanks for the review and suggestions - attached an updated patch. > Come to think of it, the same improvement should be made to ln, cp, install > and shred. Basically, to any program that uses 'rename' or 'link' or similar > syscalls, and which reports an error if the syscall fails. OK, I will work on that next. -assaf
>From 8dc6158a6fde668e55312b5fb69384f438b7e55a Mon Sep 17 00:00:00 2001 From: Assaf Gordon <assafgor...@gmail.com> Date: Mon, 29 Jul 2019 00:23:20 -0600 Subject: [PATCH] mv: improve error messages when destination directory is at fault Suggested by Alex Mantel <alexmante...@gmail.com> in https://bugs.gnu.org/36831 . $ mkdir A B B/A $ touch A/bar B/A/foo Before: $ mv A B mv: cannot move 'A' to 'B/A': Directory not empty After: $ mv A B mv: cannot move 'A' to 'B/A': Destination directory not empty The following errors are handled: EDQUOT, EEXIST, ENOTEMPTY, EISDIR, ENOSPC, ETXTBSY. * src/copy.c (copy_internal): Print custom messages for errors that explicitly fault the destination directory. (strerror_dest): New function, return custom, translatable error messages for errors relating to 'destination' component. * tests/mv/dir2dir.sh: Adjust expected error message. * NEWS: Mention change. --- NEWS | 6 +++++ src/copy.c | 53 ++++++++++++++++++++++++++++++++++++++++++--- tests/mv/dir2dir.sh | 8 ++++--- 3 files changed, 61 insertions(+), 6 deletions(-) diff --git a/NEWS b/NEWS index fd0543351..3d80665ae 100644 --- a/NEWS +++ b/NEWS @@ -44,6 +44,12 @@ GNU coreutils NEWS -*- outline -*- stat(1) also supports a new --cached= option to control cache coherency of file system attributes, useful on network file systems. +** Improvements + + mv now prints clearer error messages when a failure relates to the + destination directory (e.g., "Destination directory is not empty" instead + of "Directory not empty"). + * Noteworthy changes in release 8.31 (2019-03-10) [stable] diff --git a/src/copy.c b/src/copy.c index 65cf65895..602c8307b 100644 --- a/src/copy.c +++ b/src/copy.c @@ -1867,6 +1867,44 @@ source_is_dst_backup (char const *srcbase, struct stat const *src_st, return dst_back_status == 0 && SAME_INODE (*src_st, dst_back_sb); } +/* Return custom error messages replacing the default libc's + messages. These messages explicity fault the destination component + in the error. + + Return NULL if E (errno value) is not handled (and by implication + should use the system's default text for the error message). */ +static char * +strerror_dest (int e) +{ + /* TRANSLATORS: These strings should mimick libc's standard + error messages (from strerror(3)), but explicitly mention + the fault is with the destination directory. */ + switch (errno) + { + case EDQUOT: + return _("Disk quota exceeded on destination device"); + case EEXIST: + case ENOTEMPTY: + return _("Destination directory not empty"); + case EISDIR: + return _("Tried to overwrite a directory with a file"); + case ENOSPC: + return _("No space left on destination device"); + case ETXTBSY: + /* NOTE: The error is "Text file busy" - but "text" in that context + refers to "text segment" of an executable file (as opposed to + "data segment" and "BSS segment"). + + This error message is meant for users, and 'text file' can be easily + confused with an actual text file (i.e., one containing only ASCII + characters. Thus, say 'executable' instead of 'text'.*/ + return _("Destination executable file is busy"); + default: + return NULL; + } +} + + /* Copy the file SRC_NAME to the file DST_NAME. The files may be of any type. NEW_DST should be true if the file DST_NAME cannot exist because its parent directory was just created; NEW_DST should @@ -2477,9 +2515,18 @@ copy_internal (char const *src_name, char const *dst_name, If the permissions on the directory containing the source or destination file are made too restrictive, the rename will fail. Etc. */ - error (0, rename_errno, - _("cannot move %s to %s"), - quoteaf_n (0, src_name), quoteaf_n (1, dst_name)); + + const char *custom_err_msg = strerror_dest (rename_errno); + if (custom_err_msg) + error (0, 0, + _("cannot move %s to %s: %s"), + quoteaf_n (0, src_name), quoteaf_n (1, dst_name), + custom_err_msg) ; + else + error (0, rename_errno, + _("cannot move %s to %s"), + quoteaf_n (0, src_name), quoteaf_n (1, dst_name)); + forget_created (src_sb.st_ino, src_sb.st_dev); return false; } diff --git a/tests/mv/dir2dir.sh b/tests/mv/dir2dir.sh index 6455386b9..869f624a3 100755 --- a/tests/mv/dir2dir.sh +++ b/tests/mv/dir2dir.sh @@ -30,11 +30,13 @@ touch a/t/f || framework_failure_ mv b/t a 2> out && fail=1 # Accept any of these: EEXIST, ENOTEMPTY, EBUSY. -sed 's/: File exists/: Directory not empty/'<out>o1;mv o1 out -sed 's/: Device or resource busy/: Directory not empty/'<out>o1;mv o1 out +sed 's/: File exists/: Destination directory not empty/'<out>o1 +mv o1 out +sed 's/: Device or resource busy/: Destination directory not empty/'<out>o1 +mv o1 out cat <<\EOF > exp || framework_failure_ -mv: cannot move 'b/t' to 'a/t': Directory not empty +mv: cannot move 'b/t' to 'a/t': Destination directory not empty EOF compare exp out || fail=1 -- 2.20.1
