GNU bug report logs - #8370
RFC: cp --no-preserve=contents

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 8370 in the body.
You can then email your comments to 8370 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: bug-coreutils <bug-coreutils <at> gnu.org>
Subject: RFC: cp --no-preserve=contents
Date: Mon, 28 Mar 2011 14:55:32 -0600

[Message part 1 (text/plain, inline)]

cp --attributes-only is great for preserving all metadata attributes
without corrupting contents, but what if I want to preserve only some of
the metadata (for example, copying SELinux context but _not_ timestamps
or content)?  It seems like --attributes-only would be a great synonym
for '--preserve=all  --no-preserve=contents', and that by adding the
'contents' category to --preserve (and defaulting it to on unless turned
off explicitly), that you expose finer-grained tuning to what metadata
gets copied.

-- 
Eric Blake   eblake <at> redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #8 received at 8370 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Eric Blake <eblake <at> redhat.com>
Cc: 8370 <at> debbugs.gnu.org
Subject: Re: bug#8370: RFC: cp --no-preserve=contents
Date: Mon, 28 Mar 2011 22:56:53 +0100

On 28/03/11 21:55, Eric Blake wrote:
> cp --attributes-only is great for preserving all metadata attributes
> without corrupting contents, but what if I want to preserve only some of
> the metadata (for example, copying SELinux context but _not_ timestamps
> or content)?  It seems like --attributes-only would be a great synonym
> for '--preserve=all  --no-preserve=contents', and that by adding the
> 'contents' category to --preserve (and defaulting it to on unless turned
> off explicitly), that you expose finer-grained tuning to what metadata
> gets copied.

--attr and --preserve can be combined already. Is that enough?
Here are the info docs:

`--attributes-only'
     Preserve the specified attributes of the original files in the
     copy, but do not copy any data.  See the `--preserve' option for
     controlling which attributes to copy.

cheers,
Pádraig.

Message #11 received at 8370 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Eric Blake <eblake <at> redhat.com>
Cc: 8370 <at> debbugs.gnu.org
Subject: Re: bug#8370: RFC: cp --no-preserve=contents
Date: Thu, 12 Apr 2012 01:41:46 +0100

On 03/28/2011 10:56 PM, Pádraig Brady wrote:
> On 28/03/11 21:55, Eric Blake wrote:
>> cp --attributes-only is great for preserving all metadata attributes
>> without corrupting contents, but what if I want to preserve only some of
>> the metadata (for example, copying SELinux context but _not_ timestamps
>> or content)?  It seems like --attributes-only would be a great synonym
>> for '--preserve=all  --no-preserve=contents', and that by adding the
>> 'contents' category to --preserve (and defaulting it to on unless turned
>> off explicitly), that you expose finer-grained tuning to what metadata
>> gets copied.
> 
> --attr and --preserve can be combined already. Is that enough?
> Here are the info docs:
> 
> `--attributes-only'
>      Preserve the specified attributes of the original files in the
>      copy, but do not copy any data.  See the `--preserve' option for
>      controlling which attributes to copy.

Cross referencing a related bug report:
https://bugzilla.redhat.com/show_bug.cgi?id=811746#c1

cheers,
Pádraig.

Message #14 received at 8370 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Eric Blake <eblake <at> redhat.com>
Cc: 8370 <at> debbugs.gnu.org
Subject: Re: bug#8370: RFC: cp --no-preserve=contents
Date: Thu, 12 Apr 2012 13:56:57 +0100

[Message part 1 (text/plain, inline)]

On 04/12/2012 01:41 AM, Pádraig Brady wrote:
> On 03/28/2011 10:56 PM, Pádraig Brady wrote:
>> On 28/03/11 21:55, Eric Blake wrote:
>>> cp --attributes-only is great for preserving all metadata attributes
>>> without corrupting contents, but what if I want to preserve only some of
>>> the metadata (for example, copying SELinux context but _not_ timestamps
>>> or content)?  It seems like --attributes-only would be a great synonym
>>> for '--preserve=all  --no-preserve=contents', and that by adding the
>>> 'contents' category to --preserve (and defaulting it to on unless turned
>>> off explicitly), that you expose finer-grained tuning to what metadata
>>> gets copied.
>>
>> --attr and --preserve can be combined already. Is that enough?
>> Here are the info docs:
>>
>> `--attributes-only'
>>      Preserve the specified attributes of the original files in the
>>      copy, but do not copy any data.  See the `--preserve' option for
>>      controlling which attributes to copy.
> 
> Cross referencing a related bug report:
> https://bugzilla.redhat.com/show_bug.cgi?id=811746#c1

So thinking a bit more about this,
and given the confusion expressed in the above bug report,
perhaps it's best to change --attributes-only to
_not_ truncate existing files?

I think scripts relying on the truncation behavior
of this relative new feature would be very rare,
and the non truncating behavior is more generally useful.

Patch to implement this change is attached.

cheers,
Pádraig.

[cp-attr-existing.diff (text/plain, attachment)]

Message #17 received at 8370 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 8370 <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#8370: RFC: cp --no-preserve=contents
Date: Thu, 12 Apr 2012 16:04:36 +0200

Pádraig Brady wrote:
...
> So thinking a bit more about this,
> and given the confusion expressed in the above bug report,
> perhaps it's best to change --attributes-only to
> _not_ truncate existing files?
>
> I think scripts relying on the truncation behavior
> of this relative new feature would be very rare,
> and the non truncating behavior is more generally useful.
>
> Patch to implement this change is attached.

Thanks for doing this.
I prefer the new non-truncating behavior, too.

> Subject: [PATCH] cp: change --attributes-only to not truncate existing files
>
> * src/copy.c (copy_reg): Don't truncate an existing file,
> to support copying attributes between existing files.
> The original use case only considered creating new files,
> and it would be a very unusual use case to be relying
> on the truncating behavior.
> * doc/coreutils.texi (cp invocation): Mention the non
> truncating behavior.
> * tests/cp/attr-existing: A new test to ensure O_TRUNC skipped.
> * tests/Makefile.am: Reference the new test.
> * NEWS: Mention the change in behavior.
> ---
>  NEWS                   |    5 +++++
>  doc/coreutils.texi     |    5 +++--
>  src/copy.c             |    5 ++++-
>  tests/Makefile.am      |    1 +
>  tests/cp/attr-existing |   29 +++++++++++++++++++++++++++++
>  5 files changed, 42 insertions(+), 3 deletions(-)
>  create mode 100755 tests/cp/attr-existing
>
> diff --git a/NEWS b/NEWS
> index c8d2bbc..92a2ec4 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,6 +2,11 @@ GNU coreutils NEWS                                    -*- outline -*-
>
>  * Noteworthy changes in release ?.? (????-??-??) [?]
>
> +** Changes in behavior
> +
> +  cp --attributes-only no longer truncates the data in existing files,
> +  allowing for more general copying of attributes from one file to another.

Perhaps "... no longer truncates any existing destination file, ..." ?


> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 510abb9..9f1d7b9 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -7650,8 +7650,9 @@ Equivalent to @option{-dR --preserve=all} with the reduced diagnostics.
>  @itemx --attributes-only
>  @opindex --attributes-only
>  Preserve the specified attributes of the original files in the copy,
> -but do not copy any data.  See the @option{--preserve} option for
> -controlling which attributes to copy.
> +but do not copy any data.  Data in existing destination files is not
> +truncated.

Similarly here.
IMHO, it's the file that is being truncated, not its data.
Maybe say something like "the data in a destination file is not modified"?

> See the @option{--preserve} option for controlling which
> +attributes to copy.
>
>  @item -b
>  @itemx @w{@kbd{--backup}[=@var{method}]}
> diff --git a/src/copy.c b/src/copy.c
> index f63a726..03d68db 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -826,7 +826,10 @@ copy_reg (char const *src_name, char const *dst_name,
>       by the specs for both cp and mv.  */
>    if (! *new_dst)
>      {
> -      dest_desc = open (dst_name, O_WRONLY | O_TRUNC | O_BINARY);
> +      /* We don't truncate with --attributes-only to support
> +         copying attributes to an existing file.  */
> +      dest_desc = open (dst_name, O_WRONLY | O_BINARY
> +                        | (x->data_copy_required ? O_TRUNC : 0));

What do you think about separating the flag definition
and open separate?

         /* We don't truncate with --attributes-only to support
            copying attributes to an existing file.  */
         int flags = (O_WRONLY | O_BINARY
                      | (x->data_copy_required ? O_TRUNC : 0));
         dest_desc = open (dst_name, flags);


> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 011051a..4d73a92 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -320,6 +320,7 @@ TESTS =						\
>    chown/separator				\
>    cp/abuse					\
>    cp/acl					\
> +  cp/attr-existing				\
>    cp/backup-1					\
>    cp/backup-dir					\
>    cp/backup-is-src				\
> diff --git a/tests/cp/attr-existing b/tests/cp/attr-existing
> new file mode 100755
> index 0000000..9cf0ffc
> --- /dev/null
> +++ b/tests/cp/attr-existing
> @@ -0,0 +1,29 @@
> +#!/bin/sh
> +# Make sure cp --attributes-only doesn't truncate existing data
> +
> +# Copyright 2012 Free Software Foundation, Inc.
> +
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +. "${srcdir=.}/init.sh"; path_prepend_ ../src
> +print_ver_ cp
> +
> +printf '1' > file1
> +printf '2' > file2
> +printf '2' > file2.exp
> +
> +cp --attributes-only file1 file2 || fail=1
> +cmp file2 file2.exp || fail=1
> +
> +Exit $fail

Message #22 received at 8370-done <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 8370-done <at> debbugs.gnu.org, Jim Meyering <jim <at> meyering.net>
Subject: Re: bug#8370: RFC: cp --no-preserve=contents
Date: Thu, 12 Apr 2012 08:39:31 -0600

[Message part 1 (text/plain, inline)]

On 04/12/2012 08:20 AM, Pádraig Brady wrote:
> On 04/12/2012 03:04 PM, Jim Meyering wrote:
>> Pádraig Brady wrote:
>> ...
>>> So thinking a bit more about this,
>>> and given the confusion expressed in the above bug report,
>>> perhaps it's best to change --attributes-only to
>>> _not_ truncate existing files?
>>>
>>> I think scripts relying on the truncation behavior
>>> of this relatively new feature would be very rare,
>>> and the non truncating behavior is more generally useful.
>>>
>>> Patch to implement this change is attached.
>>
>> Thanks for doing this.
>> I prefer the new non-truncating behavior, too.
> 
> Cool.
> 
> I'll push with your clean-ups later on.
> Eric is it OK to close this bug,
> or are there cases not handled yet?
> (I didn't fully understand your suggestion TBH).

Yes, I closed the bug now.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #25 received at 8370 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 8370 <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#8370: RFC: cp --no-preserve=contents
Date: Thu, 12 Apr 2012 08:02:13 -0700

I like this idea too.  Some comments:

>  Preserve the specified attributes of the original files in the copy,
> -but do not copy any data.  See the @option{--preserve} option for
> -controlling which attributes to copy.
> +but do not copy any data.  Data in existing destination files is not
> +truncated.  See the @option{--preserve} option for controlling which
> +attributes to copy.

The word "data" is plural.  But there is no need to talk about
truncation: the point is that the file's contents aren't changed at
all.  Something like this, maybe?

  Copy only the specified attributes of the source file to the destination.
  If the destination already exists, do not alter its contents.
  See the @option{--preserve} option for controlling which attributes to copy.


> -      dest_desc = open (dst_name, O_WRONLY | O_TRUNC | O_BINARY);
> +      /* We don't truncate with --attributes-only to support
> +         copying attributes to an existing file.  */
> +      dest_desc = open (dst_name, O_WRONLY | O_BINARY
> +                        | (x->data_copy_required ? O_TRUNC : 0));

The indentation of the flags expression isn't right, and the
comment is unnecessary clutter (the code is clear).  Perhaps something
like this instead?

     int open_flags =
       O_WRONLY | O_BINARY | (x->data_copy_required ? O_TRUNC : 0);
     dest_desc = open (dst_name, open_flags);

Message #28 received at 8370 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 8370 <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#8370: RFC: cp --no-preserve=contents
Date: Thu, 12 Apr 2012 15:20:30 +0100

On 04/12/2012 03:04 PM, Jim Meyering wrote:
> Pádraig Brady wrote:
> ...
>> So thinking a bit more about this,
>> and given the confusion expressed in the above bug report,
>> perhaps it's best to change --attributes-only to
>> _not_ truncate existing files?
>>
>> I think scripts relying on the truncation behavior
>> of this relatively new feature would be very rare,
>> and the non truncating behavior is more generally useful.
>>
>> Patch to implement this change is attached.
> 
> Thanks for doing this.
> I prefer the new non-truncating behavior, too.

Cool.

I'll push with your clean-ups later on.
Eric is it OK to close this bug,
or are there cases not handled yet?
(I didn't fully understand your suggestion TBH).

thanks for the review,
Pádraig.

Message #31 received at 8370 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 8370 <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#8370: RFC: cp --no-preserve=contents
Date: Thu, 12 Apr 2012 16:13:43 +0100

On 04/12/2012 04:02 PM, Paul Eggert wrote:
> I like this idea too.  Some comments:
> 
>>  Preserve the specified attributes of the original files in the copy,
>> -but do not copy any data.  See the @option{--preserve} option for
>> -controlling which attributes to copy.
>> +but do not copy any data.  Data in existing destination files is not
>> +truncated.  See the @option{--preserve} option for controlling which
>> +attributes to copy.
> 
> The word "data" is plural.  But there is no need to talk about
> truncation: the point is that the file's contents aren't changed at
> all.  Something like this, maybe?
> 
>   Copy only the specified attributes of the source file to the destination.
>   If the destination already exists, do not alter its contents.
>   See the @option{--preserve} option for controlling which attributes to copy.
> 
> 
>> -      dest_desc = open (dst_name, O_WRONLY | O_TRUNC | O_BINARY);
>> +      /* We don't truncate with --attributes-only to support
>> +         copying attributes to an existing file.  */
>> +      dest_desc = open (dst_name, O_WRONLY | O_BINARY
>> +                        | (x->data_copy_required ? O_TRUNC : 0));
> 
> The indentation of the flags expression isn't right, and the
> comment is unnecessary clutter (the code is clear).  Perhaps something
> like this instead?
> 
>      int open_flags =
>        O_WRONLY | O_BINARY | (x->data_copy_required ? O_TRUNC : 0);
>      dest_desc = open (dst_name, open_flags);
> 

Cool.

thanks for the review,
Pádraig.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.