Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-02-24 Thread Pádraig Brady 

On 01/02/2024 00:36, Paul Eggert wrote:

On 1/31/24 06:06, Pádraig Brady wrote:

To my mind the most protective option takes precedence.


That's not how POSIX works with mv -i and mv -f. The last flag wins. I
assume this is so that people can have aliases or shell scripts that
make -i the default, but you can override by specifying -f on the
command line. E.g., in mymv:

 #!/bin/sh
 mv -i "$@"

then "mymv -f a b" works as expected.

Wouldn't a similar argument apply to cp's --update options?

Or perhaps we should play it safe, and reject any combination of
--update etc. options that are incompatible. We can always change our
mind later and say that later options override earlier ones, or do
something else that's less conservative.


OK I err'd on the side of changing as little as possible wrt precedence.
-n still has precedence over any -u,--update option.
That's simplest to understand (while not changing existing precedence),
and shouldn't cause any practical issues.

I plan to push the 2 attached patches for this tomorrow.

thanks,
PádraigFrom 51cf6f3ff272467bc9cde75c48d0250446be9b9c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Sat, 24 Feb 2024 19:51:56 +
Subject: [PATCH 1/2] cp,mv: reinstate that -n exits with success if files
 skipped

* src/cp.c (main): Adjust so that -n will exit success if skipped files.
* src/mv.c (main): Likewise.
* doc/coreutils.texi (cp invocation): Adjust the description of -n.
* src/system.h (emit_update_parameters_note): Adjust --update=none
comparison.
* tests/cp/cp-i.sh: Adjust -n exit status checks.
* tests/mv/mv-n.sh: Likewise.
* NEWS: Mention the change in behavior.
Fixes https://bugs.gnu.org/62572
---
 NEWS   |  4 
 doc/coreutils.texi | 17 +
 src/cp.c   | 14 --
 src/mv.c   | 10 ++
 src/system.h   |  4 ++--
 tests/cp/cp-i.sh   | 11 +--
 tests/mv/mv-n.sh   | 11 +--
 7 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/NEWS b/NEWS
index 36b7fd1fe..a52b4cf66 100644
--- a/NEWS
+++ b/NEWS
@@ -43,6 +43,10 @@ GNU coreutils NEWS-*- outline -*-
   basenc --base16 -d now supports lower case hexadecimal characters.
   Previously an error was given for lower case hex digits.
 
+  cp --no-clobber, and mv -n no longer exit with failure status if
+  existing files are encountered in the destination.  Instead they revert
+  to the behavior from before v9.2, silently skipping existing files.
+
   ls --dired now implies long format output without hyperlinks enabled,
   and will take precedence over previously specified formats or hyperlink mode.
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index c42126955..911e15b46 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9059,12 +9059,13 @@ a regular file in the destination tree.
 @itemx --no-clobber
 @opindex -n
 @opindex --no-clobber
-Do not overwrite an existing file; silently fail instead.
-This option overrides a previous
-@option{-i} option.  This option is mutually exclusive with @option{-b} or
-@option{--backup} option.
-See also the @option{--update=none} option which will
-skip existing files but not fail.
+Do not overwrite an existing file; silently skip instead.
+This option overrides a previous @option{-i} option.
+This option is mutually exclusive with @option{-b} or @option{--backup} option.
+This option is deprecated due to having a different exit status from
+other platforms.  See also the @option{--update} option which will
+give more control over how to deal with existing files in the destination,
+and over the exit status in particular.
 
 @item -P
 @itemx --no-dereference
@@ -9335,8 +9336,8 @@ This is the default operation when an @option{--update} option is not specified,
 and results in all existing files in the destination being replaced.
 
 @item none
-This is similar to the @option{--no-clobber} option, in that no files in the
-destination are replaced, but also skipping a file does not induce a failure.
+This is like the deprecated @option{--no-clobber} option, where no files in the
+destination are replaced, and also skipping a file does not induce a failure.
 
 @item older
 This is the default operation when @option{--update} is specified, and results
diff --git a/src/cp.c b/src/cp.c
index 0355ed97f..36ae4fb66 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -195,8 +195,8 @@ Copy SOURCE to DEST, or multiple SOURCE(s) to DIRECTORY.\n\
   -L, --dereferencealways follow symbolic links in SOURCE\n\
 "), stdout);
   fputs (_("\
-  -n, --no-clobber ensure no existing files overwritten, and fail\n\
- silently instead. See also --update\n\
+  -n, --no-clobber silently skip existing files.\n\
+ See also --update\n\
 "), stdout);
   fputs (_("\
   -P, --no-dereference never follow symbolic links in SOURCE\n\
@@ -984,6 +984,7

Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-01-31 Thread Paul Eggert 

On 1/31/24 06:06, Pádraig Brady wrote:

To my mind the most protective option takes precedence.


That's not how POSIX works with mv -i and mv -f. The last flag wins. I 
assume this is so that people can have aliases or shell scripts that 
make -i the default, but you can override by specifying -f on the 
command line. E.g., in mymv:


   #!/bin/sh
   mv -i "$@"

then "mymv -f a b" works as expected.

Wouldn't a similar argument apply to cp's --update options?

Or perhaps we should play it safe, and reject any combination of 
--update etc. options that are incompatible. We can always change our 
mind later and say that later options override earlier ones, or do 
something else that's less conservative.

Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-01-31 Thread Pádraig Brady 

On 30/01/2024 18:31, Paul Eggert wrote:

On 2024-01-30 03:18, Pádraig Brady wrote:

So we now have the proposed change as:

    - revert -n to old silent success behavior
    - document -n as deprecated
    - Leave --update=none as is (will be synonymous with -n)
    - Provide --update=none-fail to diagnose and exit failure


Thanks, that's a better proposal, but I still see several opportunities
for confusion.

If I understand things correctly, cp --update=none is not synonymous
with the proposed (i.e., old-behavior) cp -n, because -n overrides
previous -i options but --update=none does not. Also, -n overrides
either previous or following --update=UPDATE options, but --update=none
overrides only previous --update=UPDATE options. (For what it's worth,
FreeBSD -n overrides


Good point.
Well -n is a protection mechanism really, so should override.
Since --update now incorporates a protection mode, it should also override I 
think.


Some of this complication seems to be for consistency with how mv
behaves with -f, -i, -n, and --update, and similarly with how rm behaves
with -f, -i, -I, and --interactive. To be honest I don't quite
understand the reason for all this complexity, which suggests it should
be documented somewhere (the manual?) if it isn't already.


To my mind the most protective option takes precedence.
So from least to most that would be -f -n --update=none --update=none-fail -i
So the main consideration there is that -n should not override 
--update=none-fail


This raises more questions:

* If we deprecate cp -n, what about mv -n? FreeBSD mv -n behaves like
Coreutils mv -n: it silently does nothing and exits successfully. So
there's no compatibility argument for changing mv -n's behavior.
However, whatever --update option we add to cp (to output a diagnostic
and exit with failure) should surely also be added to mv, to aid
consistency.


Yes I agree.


* Should cp --update=none be changed so that it really behaves like the
old cp -n, in that it overrides other options in ways that differ from
how the other --update=UPDATE options behave? I'm leaning toward "no" as
this adds complexity that I don't see the use for.

* If we don't change cp --update=none's overriding behavior, is it still
OK to tell users to substitute --update=none for -n even though the two
options are not exactly equivalent? I'm leaning towards "yes" but would
like other opinions.


Yes I think we should still give that advice to deprecate -n,
if we ensure that -n does not override --update=none=fail.

thanks,
Pádraig

Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-01-30 Thread Paul Eggert 

On 2024-01-30 03:18, Pádraig Brady wrote:

So we now have the proposed change as:

   - revert -n to old silent success behavior
   - document -n as deprecated
   - Leave --update=none as is (will be synonymous with -n)
   - Provide --update=none-fail to diagnose and exit failure


Thanks, that's a better proposal, but I still see several opportunities 
for confusion.


If I understand things correctly, cp --update=none is not synonymous 
with the proposed (i.e., old-behavior) cp -n, because -n overrides 
previous -i options but --update=none does not. Also, -n overrides 
either previous or following --update=UPDATE options, but --update=none 
overrides only previous --update=UPDATE options. (For what it's worth, 
FreeBSD -n overrides


Some of this complication seems to be for consistency with how mv 
behaves with -f, -i, -n, and --update, and similarly with how rm behaves 
with -f, -i, -I, and --interactive. To be honest I don't quite 
understand the reason for all this complexity, which suggests it should 
be documented somewhere (the manual?) if it isn't already.


This raises more questions:

* If we deprecate cp -n, what about mv -n? FreeBSD mv -n behaves like 
Coreutils mv -n: it silently does nothing and exits successfully. So 
there's no compatibility argument for changing mv -n's behavior. 
However, whatever --update option we add to cp (to output a diagnostic 
and exit with failure) should surely also be added to mv, to aid 
consistency.


* Should cp --update=none be changed so that it really behaves like the 
old cp -n, in that it overrides other options in ways that differ from 
how the other --update=UPDATE options behave? I'm leaning toward "no" as 
this adds complexity that I don't see the use for.


* If we don't change cp --update=none's overriding behavior, is it still 
OK to tell users to substitute --update=none for -n even though the two 
options are not exactly equivalent? I'm leaning towards "yes" but would 
like other opinions.

Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-01-30 Thread Pádraig Brady 

On 29/01/2024 21:44, Paul Eggert wrote:

On 1/29/24 08:11, Pádraig Brady wrote:


Right, that's why I'm still leaning towards my proposal in the last mail.


Well, I won't insist on doing nothing; however, the proposal needs
ironing out and now's a good time to do it before installing changes.



    - revert to previous exit success -n behavior
    - document -n as deprecated
    - provide --update=noclobber to give exit failure functionality


So --update=noclobber would differ in meaning from the deprecated-in-9.5
--no-clobber, but would agree in meaning with 9.4 --no-clobber? That
sounds pretty confusing for future users. (And a nit: why should one
spelling have a hyphen but the other doesn't?)


That's a fair point; just avoid "no-clobber" entirely.
We could choose a new name so. How about --update=none-fail


      - BTW, it probably makes sense to print a diagnostic for each
skipped file here
    as it's exceptional behavior, for which we're exiting with
failure for.


Coreutils 9.4 cp -n already does that, no? So I'm not sure what's being
proposed here.

$ touch a b
$ cp -n a b; echo $?
cp: not replacing 'b'
1


Sorry I got confused between your suggestion of added diagnostics,
and FreeBSD's silent behavior here. Looks like we just keep the
existing behavior of diagnosing 'not replacing' iff exiting with failure.


    - the existing --update=none provides the exit success functionality


It seems to me that this proposal conflates two questions:

* What rules should cp use to decide whether to update a destination?

* When cp decides not to update a destination, what should it do? Exit
with nonzero status? Output a diagnostic? Both? Neither?

Aren't these independent axes? If so, shouldn't they have independent
options? For example, since we have --update=older, shouldn't there be a
way to say "I want to copy A to B only if B is older than A, and I want
the exit status to be zero only if A was copied to B"?


Well they're not entirely independent.
It's more appropriate to output a diagnostic if exiting failure,
and POSIX also advises along the same lines.
We also have --verbose to control diagnostics somewhat
for the non failure case.

So we now have the proposed change as:

  - revert -n to old silent success behavior
  - document -n as deprecated
  - Leave --update=none as is (will be synonymous with -n)
  - Provide --update=none-fail to diagnose and exit failure

thanks,
Pádraig.

Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-01-29 Thread Paul Eggert 

On 1/29/24 08:11, Pádraig Brady wrote:


Right, that's why I'm still leaning towards my proposal in the last mail.


Well, I won't insist on doing nothing; however, the proposal needs 
ironing out and now's a good time to do it before installing changes.




   - revert to previous exit success -n behavior
   - document -n as deprecated
   - provide --update=noclobber to give exit failure functionality


So --update=noclobber would differ in meaning from the deprecated-in-9.5 
--no-clobber, but would agree in meaning with 9.4 --no-clobber? That 
sounds pretty confusing for future users. (And a nit: why should one 
spelling have a hyphen but the other doesn't?)



     - BTW, it probably makes sense to print a diagnostic for each 
skipped file here
   as it's exceptional behavior, for which we're exiting with 
failure for.


Coreutils 9.4 cp -n already does that, no? So I'm not sure what's being 
proposed here.


  $ touch a b
  $ cp -n a b; echo $?
  cp: not replacing 'b'
  1



   - the existing --update=none provides the exit success functionality


It seems to me that this proposal conflates two questions:

* What rules should cp use to decide whether to update a destination?

* When cp decides not to update a destination, what should it do? Exit 
with nonzero status? Output a diagnostic? Both? Neither?


Aren't these independent axes? If so, shouldn't they have independent 
options? For example, since we have --update=older, shouldn't there be a 
way to say "I want to copy A to B only if B is older than A, and I want 
the exit status to be zero only if A was copied to B"?

Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-01-29 Thread Michael Stone 

On Mon, Jan 29, 2024 at 04:11:05PM +, Pádraig Brady wrote:

You've introduced a silent incompatibility and I'm trying to find some
way to make that clear. If upstream would provide a better solution I
would certainly use it. I have despaired of there being such since your
attitude thus far seems to be entirely dismissive of compatibility
concerns.


That's a bit unfair.  The current upstream -n behavior is with a view
to being _more_ compat across all systems.
Now I agree this may not be worth it in this case,
but it is a laudable goal.


You are saying that again without explicitly acknowledging that "being 
_more_ compat" in this case means "becoming _incompat_ with the vast 
majority of installed systems". IMO it could be reasonably phrased as 
"being more compatible across all systems in the long term when all 
existing legacy systems are gone", but the key here is that I read 
"_more_ compat across all systems" as dismissing the coreutils installed 
base as part of "all systems". I understand that may not be/have been 
the intent, but I also can't help feeling the way that I do when the 
benefits of compatability with freebsd are repeatedly emphasized while 
the costs of incompatibility with the coreutils installed base are 
dismissed with something along the lines of "we'll see what breaks". (If 
the costs of incompatibility are really that low in this case, why would 
compatability be a worthwhile goal in this case?)


I do wish that more users had noticed the change earlier and that we're 
fairly deep into a mess, but it's not always easy to see the impact of 
what seems like a relatively minor patch. I do appreciate that the new 
version printed some diagnostics when the change was triggered, as that 
certainly helped call attention to scripts which were impacted.



With the above in place for the next coreutils release,
then debian could remove its noisy patch.


I would certainly align with that, and the sooner the better to decrease 
the chances that different distributions handle this in different ways 
or we get to the point of having to release in an interim state. If you 
commit a final version I'll apply that patch if the next release isn't 
imminent.

Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-01-29 Thread Pádraig Brady 

On 29/01/2024 14:01, Michael Stone wrote:

On Sun, Jan 28, 2024 at 11:14:14PM -0800, Paul Eggert wrote:

I'm not sure reverting would be best. It would introduce more
confusion, and would make coreutils incompatible with FreeBSD again.


Reverting makes more sense than the current situation. I do not
understand why you seem to value FreeBSD compatibility more than
compatibility with the vast majority of installed coreutils/linux
systems.


Yes, it's not a good place to be. Surely current coreutils is better
than what Debian is doing.


You've introduced a silent incompatibility and I'm trying to find some
way to make that clear. If upstream would provide a better solution I
would certainly use it. I have despaired of there being such since your
attitude thus far seems to be entirely dismissive of compatibility
concerns.


That's a bit unfair.  The current upstream -n behavior is with a view
to being _more_ compat across all systems.
Now I agree this may not be worth it in this case,
but it is a laudable goal.


Another possibility is to add a warning that is emitted only at the
end of 'cp'. The warning would occur only if the exit code differs
because of this cp -n business.


You'd only emit a notification of a change in behavior if some
(potentially uncommon/rarely encountered) situation arises which would
actually trigger breakage? So people can't prepare ahead of time and
change their script to handle the necessary change in logic, they can
only maybe figure out why something broke at 2am when the uncommon event
occurred?


Yes I agree with this point, mostly.
Outputting a diagnostic would help users diagnose what's going on,
and help users to fix forward and avoid their problematic -n usage.
But yes, the crux of the issue with fixing issues as they occur,
is it depends on the state of the destination and so can become an issue
at any time.  Now I previously did an audit with github and debian code search
and noticed very few problematic uses of cp -n, but that does miss
the mountain of private code.


At the end of the day, -n is basically a useless option with unknowable
semantics which should be avoided by everyone. In the past it was an
option which wasn't portable between coreutils/linux and freebsd systems,
and I guess you've "fixed" that (by making it an option everyone should
avoid entirely), but let's be honest about how common that concern was.


Right, that's why I'm still leaning towards my proposal in the last mail.
  - revert to previous exit success -n behavior
  - document -n as deprecated
  - provide --update=noclobber to give exit failure functionality
- BTW, it probably makes sense to print a diagnostic for each skipped file 
here
  as it's exceptional behavior, for which we're exiting with failure for.
  - the existing --update=none provides the exit success functionality

With the above in place for the next coreutils release,
then debian could remove its noisy patch.

thanks,
Pádraig

Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-01-29 Thread Michael Stone 

On Sun, Jan 28, 2024 at 11:14:14PM -0800, Paul Eggert wrote:
I'm not sure reverting would be best. It would introduce more 
confusion, and would make coreutils incompatible with FreeBSD again.


Reverting makes more sense than the current situation. I do not 
understand why you seem to value FreeBSD compatibility more than 
compatibility with the vast majority of installed coreutils/linux

systems.

Yes, it's not a good place to be. Surely current coreutils is better 
than what Debian is doing.


You've introduced a silent incompatibility and I'm trying to find some 
way to make that clear. If upstream would provide a better solution I 
would certainly use it. I have despaired of there being such since your 
attitude thus far seems to be entirely dismissive of compatibility 
concerns.


Another possibility is to add a warning that is emitted only at the 
end of 'cp'. The warning would occur only if the exit code differs 
because of this cp -n business.


You'd only emit a notification of a change in behavior if some 
(potentially uncommon/rarely encountered) situation arises which would 
actually trigger breakage? So people can't prepare ahead of time and 
change their script to handle the necessary change in logic, they can 
only maybe figure out why something broke at 2am when the uncommon event 
occurred?


At the end of the day, -n is basically a useless option with unknowable 
semantics which should be avoided by everyone. In the past it was an 
option which wasn't portable between coreutils/linux and freebsd systems, 
and I guess you've "fixed" that (by making it an option everyone should 
avoid entirely), but let's be honest about how common that concern was.

Bug#1058752: bug#62572: Bug#1058752: bug#62572: cp --no-clobber behavior has changed

2024-01-28 Thread Paul Eggert 

On 2024-01-28 05:22, Pádraig Brady wrote:

At this stage it seems best for us go back to the original Linux 
behiavor (use case 3),
and to silently deprecate -n in docs to document the portability issues 
with it.


I'm not sure reverting would be best. It would introduce more confusion, 
and would make coreutils incompatible with FreeBSD again.


The recent Debian change indicates that their intent is to move to the 
FreeBSD behavior too. This would improve cross-platform portability and 
I don't think we should discourage that.




  $ cp -n /bin/true tmp
  cp: warning: behavior of -n is non-portable and may change in future; use 
--update=none instead

This is problematic as:

  - It's noisy


Yes that's a problem, and I doubt whether we should mimic Debian.


  - There is no way to get the behavior of indicating failure if existing files 
present


Yes, it's not a good place to be. Surely current coreutils is better 
than what Debian is doing.



  - The --update=none advice is only portable to newer coreutils


True, but that's not a deal-killer. No advice that we give can be 100% 
portable to all platforms.



We should also provide --update=noclobber for use case 1.
Having the control on the --update option, allows use to more clearly 
deprecate -n.


Adding an --update=noclobber sounds like a good thing to do.

Another possibility is to add a warning that is emitted only at the end 
of 'cp'. The warning would occur only if the exit code differs because 
of this cp -n business. We could stretch things a bit and have a 
configure-time option --enable-compat-warnings that builders like Debian 
could use if they want such warnings.