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

[Bernhard Voelker]

> On October 29, 2013 at 7:27 PM Pádraig Brady <address@hidden> wrote:
> On 10/20/2013 02:11 AM, Bernhard Voelker wrote:
> >> I would have expected both 'cp-Hl' and 'cp-Ll' to be hardlinked to
> >> 'file', not to 'link'.
> >
> > I'd also say that the above is an error ... or at least contradicts
> > to what is documented in the code (copy.c, line 2377):
> >
> >    Yet cp, invoked with '--link --no-dereference',
> >    should not follow the link.
>
> Yes I would think that `ln -L` and `cp --link -L` should
> be equivalent in this regard.

s/ln -L/ln -n/,  I guess.
 
> > 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);
>
> Actually since linkat() has defined semantics,
> _and thanks to gnulib, guaranteed semantics_
> we should just leverage the gnulib magic here,
> and use linkat with flags.

Thanks for the clarification. I changed it back to Gian's
version.

> > 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*
>
> Nice basis for a test.

Yes, but it's not enough: I exercised also some combinations with
-lrP -lrH -lrL, and the whole stuff again for a symlink pointing
to a directory.

It got a bit late yesterday, and I couldn't get the things sorted
to present here. All Ican say now is that there are still corner
cases where cp now behaves differently (e.g. "cp -l link2dir dst").
I hope I can get back here with the results this evening.

> >   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
>
> The above look correct.

Great, thanks.

> For comparison on FreeBSD 9.1
>
> 1755429 -rw-r--r--  1 pbrady  pbrady  - 0 Oct 29 12:16 cp
> 1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 cp-l
> 1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 cp-lH
> 1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 cp-lL
> 1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 cp-lP
> 1755430 -rw-r--r--  1 pbrady  pbrady  - 0 Oct 29 12:16 cp-r
> 1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 file
> 1755428 lrwxr-xr-x  1 pbrady  pbrady  - 4 Oct 29 12:16 link -> file

Interesting.
 
> > BTW: tests/cp/same-file.sh now fails with the patch.
>
> Yes better to consolidate the functionality,
> before doing the tests to avoid awkward test churn.

yes, regarding from what I got with the tests mentioned above,
I'm still not convinced we're at that point.

> >>From fca06077e11d6338b35029bc61fbee5dbfb5ab66 Mon Sep 17 00:00:00 2001
> > From: Gian Piero Carrubba <address@hidden>
> > Date: Sun, 20 Oct 2013 03:04:41 +0200
> > Subject: [PATCH] cp: fix --link --dereference [DRAFT]
> >
> > * src/copy.c (create_hard_link): Add a bool 'dereference' parameter,
> > and canonicalize the given src_name when dereference is true.
> > (copy_internal): Add a value for the above new parameter in all
> > three calls to create_hard_link: true if the src_name should be
> > dereferenced before creating the hard link, false otherwise.
> > * src/cp.c (main): after parsing the options, if x.dereference is
> > still DEFEF_UNDEFINED and the --links option was specified, initialize
>
> s/--links/--link/

Fixed, thanks.
 
> > diff --git a/src/copy.c b/src/copy.c
> > index f66ab46..fa4d734 100644
> > --- a/src/copy.c
> > +++ b/src/copy.c
[...]
> > @@ -1748,7 +1757,10 @@ copy_internal (char const *src_name, char const
> > *dst_name,
> >                        /* Note we currently replace DST_NAME
> >unconditionally,
> >                           even if it was a newer separate file.  */
> >                        if (! create_hard_link (earlier_file, dst_name, true,
> > -                                              x->verbose))
> > +                                              x->verbose,
> > +                                              x->dereference ==
> > DEREF_ALWAYS
> > +                                              || (x->dereference ==
> > DEREF_COMMAND_LINE_ARGUMENTS
> > +                                                  && command_line_arg)))
> >                          {
> >                            goto un_backup;
> >                          }
> > @@ -2078,7 +2090,10 @@ copy_internal (char const *src_name, char const
> > *dst_name,
> >          }
> >        else
> >          {
> > -          if (! create_hard_link (earlier_file, dst_name, true,
> > x->verbose))
> > +          if (! create_hard_link (earlier_file, dst_name, true, x->verbose,
> > +                                  x->dereference == DEREF_ALWAYS
> > +                                  || (x->dereference ==
> > DEREF_COMMAND_LINE_ARGUMENTS
> > +                                      && command_line_arg)))
> >              goto un_backup;
> >
> >            return true;
> > @@ -2389,7 +2404,10 @@ copy_internal (char const *src_name, char const
> > *dst_name,
> >             && !(LINK_FOLLOWS_SYMLINKS && S_ISLNK (src_mode)
> >                  && x->dereference == DEREF_NEVER))
> >      {
> > -      if (! create_hard_link (src_name, dst_name, false, false))
> > +      if (! create_hard_link (src_name, dst_name, false, false,
> > +                              x->dereference == DEREF_ALWAYS
> > +                              || (x->dereference ==
> > DEREF_COMMAND_LINE_ARGUMENTS
> > +                                  && command_line_arg)))
>
> Lots and copy and paste there.
> Maybe use a helper like:
>
> static inline bool _GL_ATTRIBUTE_PURE
> should_dereference (const struct cp_options *x, bool cli_arg)
> {
>   return x->dereference == DEREF_ALWAYS
>          || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
>              && cli_arg);
> }

Good idea!
 
Thanks again for looking into this.  It's really quite complex.
I'll come up with the test (old vs. new) cases soon.

Have a nice day,
Berny