Re: [coreutils] cp/reflink-perm fails on btrfs (and probably on ocfs2, too)

[Jim Meyering]

Pádraig Brady wrote:
> On 26/10/10 14:54, Jim Meyering wrote:
>> Pádraig Brady wrote:
>>> On 26/10/10 12:48, Pádraig Brady wrote:
>>>> So in summary error if any of --link, --symbolic-link,
>>>> --reflink or --attributes-only are combined.
>>>
>>> I.E. leave the docs alone and do:
>>
>> Thanks.  That sounds good.
>> Do you feel like writing the NEWS entry, too?
>
> I'll apply the attached this evening some time.
>
> cheers,
> Pádraig.
>
>>From 9540b44e5dbae1bc8125bd1aeadbb40d8944fe3c Mon Sep 17 00:00:00 2001
> From: =?utf-8?q?P=C3=A1draig=20Brady?= <address@hidden>
> Date: Tue, 26 Oct 2010 15:25:28 +0100
> Subject: [PATCH] cp: disallow combinations of --*link options and 
> --attributes-only
>
> * src/cp.c (main): Disallow combining --reflink with --link or
> --symbolic-link as they override --reflink.  Also disallow --attr
> in combination as it's only pertinent to the --reflink=auto case,
> and even then there is no reason the user would need to specify
> both of those options.
> * tests/cp/reflink-perm: Verify the combinations are not allowed.
> * NEWS: Mention the change in behavior.
> ---
>  NEWS                  |    6 ++++++
>  src/cp.c              |    9 +++++++--
>  tests/cp/reflink-perm |    5 +++--
>  3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 7dbbf1f..3c2134e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,12 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>    tail -F once again notices changes in a currently unavailable
>    remote directory [bug introduced in coreutils-7.5]
>
> +** Changes in behavior
> +
> +  cp now disallows any combination of --reflink, --symbolic-link, --link
> +  and --attributes-only.  Previously --attributes-only was overridden by
> +  --reflink which in turn was overridden by the other linking modes.
> +
>
>  * Noteworthy changes in release 8.6 (2010-10-15) [stable]
>
> diff --git a/src/cp.c b/src/cp.c
> index 5b14f3a..07774fe 100644
> --- a/src/cp.c
> +++ b/src/cp.c
> @@ -1097,9 +1097,14 @@ main (int argc, char **argv)
>          }
>      }
>
> -  if (x.hard_link && x.symbolic_link)
> +  if (1 < ((x.reflink_mode != REFLINK_NEVER) + x.hard_link + x.symbolic_link
> +      + !x.data_copy_required))

Thanks.  That looks fine modulo an indentation nit:

     if (1 < ((x.reflink_mode != REFLINK_NEVER) + x.hard_link + x.symbolic_link
              + !x.data_copy_required))

or

     if (1 < ((x.reflink_mode != REFLINK_NEVER) + x.hard_link
              + x.symbolic_link + !x.data_copy_required))

or even like this: (eliminating the "((" make it a little more readable)

     if (1 < (x.hard_link + x.symbolic_link
              + (x.reflink_mode != REFLINK_NEVER)
              + !x.data_copy_required))

>      {
> -      error (0, 0, _("cannot make both hard and symbolic links"));
> +      error (0, 0, "%s",
> +             (x.data_copy_required
> +              ? _("cannot combine linking modes")
> +              : _("cannot combine linking modes with --attributes-only")));
> +
>        usage (EXIT_FAILURE);
>      }
>
> diff --git a/tests/cp/reflink-perm b/tests/cp/reflink-perm
> index 77f119f..7f48a24 100755
> --- a/tests/cp/reflink-perm
> +++ b/tests/cp/reflink-perm
> @@ -39,8 +39,9 @@ test "$mode" = "-rwxrwxrwx" || fail=1
>
>  test copy -nt file && fail=1
>
> +# reflink is incompatible with other linking modes and --attributes-only
>  echo > file2 # file with data
> -cp --reflink=auto --preserve --attributes-only file2 empty_copy || fail=1
> -test -s empty_copy && fail=1
> +cp --reflink=auto --attributes-only file2 empty_copy && fail=1
> +cp --reflink=auto --symbolic-link file2 empty_copy && fail=1
>
>  Exit $fail