bug#15173: [cp] --link overrides dereference settings

[Gian Piero Carrubba]

* [Sun, Oct 20, 2013 at 03:11:04AM +0200] Bernhard Voelker:
> Hello Gian,

Hi Bernhard,

thank you for your answer and the fixed patch.

[...]
> Instead of passing 'flags' to linkat (of which I don't know if it works
> on all platforms), I'd rather dereference the symlink manually:
>
>  char *src_name_new = canonicalize_filename_mode (src_name, CAN_EXISTING);

Good idea. It sounds sensible and should avoid portability issues (I 
have to admit I didn't investigate further about those).

> > AFAICS, at least in one case the patch changes the behaviour of cp:
> >
> > $ echo testfile > file; ln -s file link;
> > cp -l link cp-l.old; ../src/cp -l link cp-l.new;
> > ls -li
> >
> > total 8
> > 8912975 -rw-r--r-- 2 gpiero gpiero 9 Aug 23 22:58 cp-l.new
> > 8913010 lrwxrwxrwx 2 gpiero gpiero 4 Aug 23 22:58 cp-l.old -> file
> > 8912975 -rw-r--r-- 2 gpiero gpiero 9 Aug 23 22:58 file
> > 8913010 lrwxrwxrwx 2 gpiero gpiero 4 Aug 23 22:58 link -> file
>
> Not ideal ...

Surely not, but I think the 'new' behaviour is the "right" one.

Being my turn for quoting the SUS :)

    If source_file is a file of type symbolic link:

    If the -R option was not specified, cp shall take actions based on 
    the type and contents of the file referenced by the symbolic link, 
    and not by the symbolic link itself

This is also consistent with:

$ echo testfile > file; ln -s file link; cp link cp ; ls -li
total 8
5678474 -rw-r--r-- 1 gpiero gpiero 9 Oct 23 22:45 cp
5678472 -rw-r--r-- 1 gpiero gpiero 9 Oct 23 22:45 file
5678473 lrwxrwxrwx 1 gpiero gpiero 4 Oct 23 22:45 link -> file

where 'cp' is a copy of 'file', not of 'link'.

> My guess is that this because POSIX [1] allows either behavior:
>
>  If source_file is a file of type symbolic link:
>  [...]
>  If the -R option was specified:
>  If none of the options -H, -L, nor -P were specified, it is
>  unspecified which of -H, -L, or -P will be used as a default.
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html
>
> This means that coreutils has chosen in between what makes most sense
> and compatibility to other implementations.
>
> > Not sure why this snippet is there[0], but it seems to me that it leads
> > to inconsistent behaviour:
> >
> > $ echo testfile > file; ln -s file link;
> > cp link cp; cp -r link cp-r;
> > ls -li
> >
> > total 8
> > 3358743 -rw-r--r-- 1 gpiero gpiero 9 Aug 23 23:06 cp
> > 3358744 lrwxrwxrwx 1 gpiero gpiero 4 Aug 23 23:06 cp-r -> file
> > 3358741 -rw-r--r-- 1 gpiero gpiero 9 Aug 23 23:06 file
> > 3358742 lrwxrwxrwx 1 gpiero gpiero 4 Aug 23 23:06 link -> file
> >
> > I think it is counterintuitive that '--recursive' had effects on the
> > referentiation of the link.[1]
>
> I think that is correct as POSIX requires this (see also in [1] above).

Not sure. Probably I failed to clearly explain what I think the problem 
is, but I miss to see how POSIX mandates it (from the quote you posted, 
being '-r' the only option, the behaviour is unspecified, so I don't 
think we could contradict POSIX modifying how it behaves).

What I mean is that currently, if the source is a symlink:
- `cp` copies the file referenced by the symlink
- `cp -r` copies the symlink itself

This is inconsistent, as '--recursive' should not have effect on the 
dereferencing setting, being them two orthogonal features. It seems like 
a side-(not-expected/desired)-effect. If I wanted to copy the symlink, I 
should have used `cp -P`.

What I propose is:
- if the symlink points to a regular file, `cp -r` should copy the file 
  referenced by the symlink (acts like a plain `cp`, that in turn acts 
  the same as `cp -H` in this case)
- if the symlink points to a directory, ditto but taking care of not 
  dereferencing the symlinks inside the dir (acts like if '-H' was used, 
  and again like a plain `cp`, with the only obvious difference that a 
  plain `cp` would fail in this case)

I don't think this would be "prohibited" by POSIX (and as a bonus it's a 
singe line patch :) ).

Anyway, all this probably belongs to another bug report (and though I 
think it's the "right thing", I'm also afraid of the possible impact of 
such a change).

> Instead, I'd change the above initialization of x.dereference in the
> case the --link option is specified.
>
> I have wrapped it for you into a Git patch (below), yet without a test
> case.
>
> This is the result:
>
>  $ : > file; ln -s file link
>  $ for opt in "" -l -lH -lL -lP -r ; do cp $opt link cp$opt ; done
>  $ ls -ldogi link file cp*
>  15335993 -rw-r--r-- 1 0 Oct 20 03:09 cp
>  15335992 lrwxrwxrwx 3 4 Oct 20 03:08 cp-l -> file
>  15335991 -rw-r--r-- 3 0 Oct 20 03:08 cp-lH
>  15335991 -rw-r--r-- 3 0 Oct 20 03:08 cp-lL
>  15335992 lrwxrwxrwx 3 4 Oct 20 03:08 cp-lP -> file
>  15335994 lrwxrwxrwx 1 4 Oct 20 03:09 cp-r -> file
>  15335991 -rw-r--r-- 3 0 Oct 20 03:08 file
>  15335992 lrwxrwxrwx 3 4 Oct 20 03:08 link -> file

As said above, I think `cp -l` should dereference the symlink, so I 
would omit the change in the initialization of x.dereference.

> BTW: tests/cp/same-file.sh now fails with the patch.

I hope I can double check and submit a patch for that test during the 
next days. As for the test case, what do you suggest ? The test you 
posted above (for opt in "" -l -lH -lL -lP -r ...) would be sufficient ?

Ciao,
Gian Piero.