From: Paul Eggert <eggert@cs.ucla.edu>
To: "Bernhard Voelker" <mail@bernhard-voelker.de>,
"Pádraig Brady" <P@draigBrady.com>
Cc: 69532@debbugs.gnu.org, util-linux <util-linux@vger.kernel.org>,
Dominique Martinet <asmadeus@codewreck.org>,
Petr Malat <oss@malat.biz>, Karel Zak <kzak@redhat.com>,
Rob Landley <rob@landley.net>
Subject: Re: bug#69532: mv's new -x option should be made orthogonal to -t/-T/default
Date: Fri, 22 Mar 2024 18:44:39 -0700 [thread overview]
Message-ID: <47c15dcf-3054-4a0a-9ca9-d9f7601db3ca@cs.ucla.edu> (raw)
In-Reply-To: <d6530404-4fc7-40d0-be99-cb37426be32d@bernhard-voelker.de>
[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]
On 3/21/24 14:45, Bernhard Voelker wrote:
> On 3/21/24 00:56, Paul Eggert wrote:
>> On 3/20/24 15:53, Bernhard Voelker wrote:
>> Yes, that's the expected behavior for this contrived case. Just as one
>> would get odd behavior if one did the same thing without --exchange.
>
> There's another which is not consistent with/without --exchange:
>
> $ src/mv -v a a
> src/mv: 'a' and 'a' are the same file
>
> $ src/mv -v --exchange a a
> renamed 'a' -> 'a'
>
> RENAME_EXCHANGE is allowed (but useless?) for 1 file.
Yes, thanks, --exchange should act more like non --exchange there.
> BTW: shouldn't the -v diagnostic better say "exchanged 'a' <-> 'a'"
> because that's what happened?
Good suggestion.
> It seems that -i is skipped:
>
> $ src/mv -iv --exchange a b
> renamed 'a' -> 'b'
Yes, I suppose -i should be treated similarly too.
I installed the attached patches to do the above. (Basically, the
problem was that my earlier patches were too ambitious; these patches
scale things back to avoid some optimizations so that mv --exchange is
more like ordinary mv.)
The first patch simplifies the code (and fixes a diagnostic to be more
useful) without otherwise changing behavior; it's more of a refactoring.
The second patch does the real work.
Thanks again.
[-- Attachment #2: 0001-cp-ln-mv-improve-dir-vs-nondir-diagnostics.patch --]
[-- Type: text/x-patch, Size: 5009 bytes --]
From ff42adb55e99226f55a7b141316ee2a7b84a4857 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 22 Mar 2024 12:02:41 -0700
Subject: [PATCH 1/2] cp,ln,mv: improve dir vs nondir diagnostics
* src/copy.c (copy_internal): Simplify logic for copying
from directory to non-directory or vice versa, and always
diagnose with both source and destination file names.
---
src/copy.c | 88 ++++++++++++++++--------------------------------------
1 file changed, 26 insertions(+), 62 deletions(-)
diff --git a/src/copy.c b/src/copy.c
index e7bf6022f..8b4a29692 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2451,72 +2451,36 @@ skip:
if (return_now)
return return_val;
- if (!S_ISDIR (dst_sb.st_mode))
+ /* Copying a directory onto a non-directory, or vice versa,
+ is ok only with --backup. */
+ if (!S_ISDIR (src_mode) != !S_ISDIR (dst_sb.st_mode)
+ && x->backup_type == no_backups)
{
- if (S_ISDIR (src_mode))
- {
- if (x->move_mode && x->backup_type != no_backups)
- {
- /* Moving a directory onto an existing
- non-directory is ok only with --backup. */
- }
- else
- {
- error (0, 0,
- _("cannot overwrite non-directory %s with directory %s"),
- quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
- return false;
- }
- }
-
- /* Don't let the user destroy their data, even if they try hard:
- This mv command must fail (likewise for cp):
- rm -rf a b c; mkdir a b c; touch a/f b/f; mv a/f b/f c
- Otherwise, the contents of b/f would be lost.
- In the case of 'cp', b/f would be lost if the user simulated
- a move using cp and rm.
- Note that it works fine if you use --backup=numbered. */
- if (command_line_arg
- && x->backup_type != numbered_backups
- && seen_file (x->dest_info, dst_relname, &dst_sb))
- {
- error (0, 0,
- _("will not overwrite just-created %s with %s"),
- quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
- return false;
- }
- }
-
- if (!S_ISDIR (src_mode))
- {
- if (S_ISDIR (dst_sb.st_mode))
- {
- if (x->move_mode && x->backup_type != no_backups)
- {
- /* Moving a non-directory onto an existing
- directory is ok only with --backup. */
- }
- else
- {
- error (0, 0,
- _("cannot overwrite directory %s with non-directory"),
- quoteaf (dst_name));
- return false;
- }
- }
+ error (0, 0,
+ _(S_ISDIR (src_mode)
+ ? ("cannot overwrite non-directory %s "
+ "with directory %s")
+ : ("cannot overwrite directory %s "
+ "with non-directory %s")),
+ quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
+ return false;
}
- if (x->move_mode)
+ /* Don't let the user destroy their data, even if they try hard:
+ This mv command must fail (likewise for cp):
+ rm -rf a b c; mkdir a b c; touch a/f b/f; mv a/f b/f c
+ Otherwise, the contents of b/f would be lost.
+ In the case of 'cp', b/f would be lost if the user simulated
+ a move using cp and rm.
+ Note that it works fine if you use --backup=numbered. */
+ if (!S_ISDIR (dst_sb.st_mode) && command_line_arg
+ && x->backup_type != numbered_backups
+ && seen_file (x->dest_info, dst_relname, &dst_sb))
{
- /* Don't allow user to move a directory onto a non-directory. */
- if (S_ISDIR (src_sb.st_mode) && !S_ISDIR (dst_sb.st_mode)
- && x->backup_type == no_backups)
- {
- error (0, 0,
- _("cannot move directory onto non-directory: %s -> %s"),
- quotef_n (0, src_name), quotef_n (0, dst_name));
- return false;
- }
+ error (0, 0,
+ _("will not overwrite just-created %s with %s"),
+ quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
+ return false;
}
char const *srcbase;
--
2.44.0
[-- Attachment #3: 0002-mv-treat-exchange-more-like-non-exchange.patch --]
[-- Type: text/x-patch, Size: 6162 bytes --]
From 5a1d00e4504204dc4c84eb641abb961e8074a218 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 22 Mar 2024 18:38:08 -0700
Subject: [PATCH 2/2] mv: treat --exchange more like non-exchange
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Also, improve quality of diagnostics.
Problems/suggestions by Bernhard Voelker in
<https://bugs.gnu.org/69532#82>.
* src/copy.c (emit_verbose): New arg FORMAT. All uses changed,
to improve quality of diagnostics when --exchange is used.
(copy_internal): Don’t try to optimize --exchange so much; this
simplifies the code and keeps it closer to the non --exchange case.
---
src/copy.c | 48 +++++++++++++++++++++++-------------------------
1 file changed, 23 insertions(+), 25 deletions(-)
diff --git a/src/copy.c b/src/copy.c
index 8b4a29692..817d5b13b 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2075,9 +2075,10 @@ abandon_move (const struct cp_options *x,
If BACKUP_DST_NAME is non-null, then also indicate that it is
the name of a backup file. */
static void
-emit_verbose (char const *src, char const *dst, char const *backup_dst_name)
+emit_verbose (char const *format, char const *src, char const *dst,
+ char const *backup_dst_name)
{
- printf ("%s -> %s", quoteaf_n (0, src), quoteaf_n (1, dst));
+ printf (format, quoteaf_n (0, src), quoteaf_n (1, dst));
if (backup_dst_name)
printf (_(" (backup: %s)"), quoteaf (backup_dst_name));
putchar ('\n');
@@ -2219,15 +2220,13 @@ copy_internal (char const *src_name, char const *dst_name,
*copy_into_self = false;
int rename_errno = x->rename_errno;
- if (x->move_mode)
+ if (x->move_mode && !x->exchange)
{
if (rename_errno < 0)
rename_errno = (renameatu (AT_FDCWD, src_name, dst_dirfd, drelname,
- (x->exchange
- ? RENAME_EXCHANGE : RENAME_NOREPLACE))
+ RENAME_NOREPLACE)
? errno : 0);
- *rename_succeeded = rename_errno == 0;
- nonexistent_dst = *rename_succeeded && !x->exchange;
+ nonexistent_dst = *rename_succeeded = rename_errno == 0;
}
if (rename_errno == 0
@@ -2248,7 +2247,7 @@ copy_internal (char const *src_name, char const *dst_name,
src_mode = src_sb.st_mode;
- if (S_ISDIR (src_mode) && !x->recursive && !x->exchange)
+ if (S_ISDIR (src_mode) && !x->recursive)
{
error (0, 0, ! x->install_mode /* cp */
? _("-r not specified; omitting directory %s")
@@ -2291,7 +2290,7 @@ copy_internal (char const *src_name, char const *dst_name,
treated the same as nonexistent files. */
bool new_dst = 0 < nonexistent_dst;
- if (! new_dst && ! x->exchange)
+ if (! new_dst)
{
/* Normally, fill in DST_SB or set NEW_DST so that later code
can use DST_SB if NEW_DST is false. However, don't bother
@@ -2452,9 +2451,9 @@ skip:
return return_val;
/* Copying a directory onto a non-directory, or vice versa,
- is ok only with --backup. */
+ is ok only with --backup or --exchange. */
if (!S_ISDIR (src_mode) != !S_ISDIR (dst_sb.st_mode)
- && x->backup_type == no_backups)
+ && x->backup_type == no_backups && !x->exchange)
{
error (0, 0,
_(S_ISDIR (src_mode)
@@ -2472,9 +2471,9 @@ skip:
Otherwise, the contents of b/f would be lost.
In the case of 'cp', b/f would be lost if the user simulated
a move using cp and rm.
- Note that it works fine if you use --backup=numbered. */
+ Nothing is lost if you use --backup=numbered or --exchange. */
if (!S_ISDIR (dst_sb.st_mode) && command_line_arg
- && x->backup_type != numbered_backups
+ && x->backup_type != numbered_backups && !x->exchange
&& seen_file (x->dest_info, dst_relname, &dst_sb))
{
error (0, 0,
@@ -2591,7 +2590,7 @@ skip:
sure we'll create a directory. Also don't announce yet when moving
so we can distinguish renames versus copies. */
if (x->verbose && !x->move_mode && !S_ISDIR (src_mode))
- emit_verbose (src_name, dst_name, dst_backup);
+ emit_verbose ("%s -> %s", src_name, dst_name, dst_backup);
/* Associate the destination file name with the source device and inode
so that if we encounter a matching dev/ino pair in the source tree
@@ -2718,17 +2717,19 @@ skip:
if (x->move_mode)
{
- if (rename_errno == EEXIST && !x->exchange)
- rename_errno = (renameat (AT_FDCWD, src_name, dst_dirfd, drelname) == 0
+ if (rename_errno == EEXIST)
+ rename_errno = ((renameatu (AT_FDCWD, src_name, dst_dirfd, drelname,
+ x->exchange ? RENAME_EXCHANGE : 0)
+ == 0)
? 0 : errno);
if (rename_errno == 0)
{
if (x->verbose)
- {
- printf (_("renamed "));
- emit_verbose (src_name, dst_name, dst_backup);
- }
+ emit_verbose (_(x->exchange
+ ? "exchanged %s <-> %s"
+ : "renamed %s -> %s"),
+ src_name, dst_name, dst_backup);
if (x->set_security_context)
{
@@ -2853,10 +2854,7 @@ skip:
}
if (x->verbose && !S_ISDIR (src_mode))
- {
- printf (_("copied "));
- emit_verbose (src_name, dst_name, dst_backup);
- }
+ emit_verbose (_("copied %s -> %s"), src_name, dst_name, dst_backup);
new_dst = true;
}
@@ -2956,7 +2954,7 @@ skip:
if (x->move_mode)
printf (_("created directory %s\n"), quoteaf (dst_name));
else
- emit_verbose (src_name, dst_name, nullptr);
+ emit_verbose ("%s -> %s", src_name, dst_name, nullptr);
}
}
else
--
2.44.0
next prev parent reply other threads:[~2024-03-23 1:44 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <10c814a7-cb68-4fb4-ad8d-f88f286fb0b1@cs.ucla.edu>
[not found] ` <58281f96-f9c6-4567-e3c9-c6a6cfa6ce27@draigBrady.com>
[not found] ` <ZeZqzB4-OzHYfFeQ@codewreck.org>
[not found] ` <5914e8b2-48ac-456b-9753-6a7bae7a9bbb@cs.ucla.edu>
2024-03-05 14:16 ` bug#69532: mv's new -x option should be made orthogonal to -t/-T/default Pádraig Brady
2024-03-05 20:41 ` Karel Zak
2024-03-05 22:13 ` Masatake YAMATO
2024-03-17 6:10 ` Paul Eggert
2024-03-17 11:32 ` Pádraig Brady
2024-03-17 11:40 ` Pádraig Brady
2024-03-20 22:10 ` Paul Eggert
2024-03-20 19:43 ` Bernhard Voelker
2024-03-20 20:56 ` Paul Eggert
2024-03-20 22:53 ` Bernhard Voelker
2024-03-20 23:56 ` Paul Eggert
2024-03-21 21:45 ` Bernhard Voelker
2024-03-23 1:44 ` Paul Eggert [this message]
2024-03-23 10:24 ` Bernhard Voelker
2024-03-22 10:22 ` Karel Zak
2024-03-23 10:24 ` Bernhard Voelker
2024-03-21 0:03 ` Rob Landley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=47c15dcf-3054-4a0a-9ca9-d9f7601db3ca@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=69532@debbugs.gnu.org \
--cc=P@draigBrady.com \
--cc=asmadeus@codewreck.org \
--cc=kzak@redhat.com \
--cc=mail@bernhard-voelker.de \
--cc=oss@malat.biz \
--cc=rob@landley.net \
--cc=util-linux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.