Re: [PATCH] install: add -C option to install file only when necessary

[Jim Meyering]

Kamil Dudka <address@hidden> wrote:
> it was discussed here 5 years ago (and considered good idea) to add -C option
> to install:
> http://lists.gnu.org/archive/html/bug-coreutils/2003-11/msg00017.html
>
> With this option install checks an existing destination file and if it is not
> different (by content, owner, group and mode) from source, the file is not
> installed. Preserving destination's original mtime can significantly decrease
> time of building when a system library is reinstalled but the header files
> are not changed at all.
>
> This option was already introduced in RHEL-4, but it was never accepted by
> upstream. There were some issues which should be solved by this patch.

Hi Kamil,

Does any other install implementation provide -C?
The precedent of RHEL4 is probably enough to justify burning the
short -C option, but finding at least one more would be better.

No long option?
How about using --compare?

Thanks for all the tests.

Please adjust according to the comments below,
and it should be good to go.

> From 9d4ae524b93e3bb2f8cb2c99e22f3f192e8dfae8 Mon Sep 17 00:00:00 2001
> From: Kamil Dudka <address@hidden>
> Date: Fri, 16 Jan 2009 11:58:26 +0100
> Subject: [PATCH] install: add -C option to install file only when necessary
>
> * src/install.c (have_same_content): New function to compare files
> content.
> (need_copy): New function to check if copy is necessary.
> (main): Handle new option -C.
> (copy_file): Skip file copying if not necessary.
> (usage): Show new option -C in --help.
> * tests/install/install-C: Basic tests for install -C.
> * tests/install/install-C-root: Tests requiring root privileges.
> * tests/install/install-C-selinux: Tests requiring SELinux.
> * tests/Makefile.am: Add new tests for install -C.
> * doc/coreutils.texi: Document new install option -C.
> * NEWS: Mention the change.
> ---
>  NEWS                            |    3 +
>  doc/coreutils.texi              |    5 ++
>  src/install.c                   |  112 
> ++++++++++++++++++++++++++++++++++++++-
>  tests/Makefile.am               |    3 +
>  tests/install/install-C         |   75 ++++++++++++++++++++++++++
>  tests/install/install-C-root    |   64 ++++++++++++++++++++++
>  tests/install/install-C-selinux |   56 +++++++++++++++++++
>  7 files changed, 317 insertions(+), 1 deletions(-)
>  create mode 100755 tests/install/install-C
>  create mode 100755 tests/install/install-C-root
>  create mode 100755 tests/install/install-C-selinux
>
> diff --git a/NEWS b/NEWS
> index f1b383e..05d6644 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,9 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>    dd accepts iflag=cio and oflag=cio to open the file in CIO (concurrent I/O)
>    mode where this feature is available.
>
> +  install -C installs file, unless target already exists and is the same 
> file,
> +  in which case the modification time is not changed
> +
>    ls --color now highlights hard linked files, too
>
>    stat -f recognizes the Lustre file system type
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index d8df107..b1c22e0 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -2123,6 +2123,11 @@ The program accepts the following options.  Also see 
> @ref{Common options}.
>
>  @table @samp
>
> address@hidden -C
> address@hidden -C
> +Install file, unless target already exists and is the same file, in which
> +case the modification time is not changed.
> +
>  @item -c
>  @itemx --crown-margin
>  @opindex -c
> diff --git a/src/install.c b/src/install.c
> index 9dda05a..03c849a 100644
> --- a/src/install.c
> +++ b/src/install.c
> @@ -31,6 +31,7 @@
>  #include "cp-hash.h"
>  #include "copy.h"
>  #include "filenamecat.h"
> +#include "full-read.h"
>  #include "mkancesdirs.h"
>  #include "mkdir-p.h"
>  #include "modechange.h"
> @@ -125,6 +126,9 @@ static mode_t dir_mode = DEFAULT_MODE;
>     or S_ISGID bits.  */
>  static mode_t dir_mode_bits = CHMOD_MODE_BITS;
>
> +/* Compare files before installing (-C) */
> +static bool copy_only_if_needed;
> +
>  /* If true, strip executable files after copying them. */
>  static bool strip_files;
>
> @@ -167,6 +171,90 @@ static struct option const long_options[] =
>    {NULL, 0, NULL, 0}
>  };
>
> +/* Compare content of opened files using file descriptors A_FD and B_FD. 
> Return
> +   true if files are equal. */
> +static bool
> +have_same_content (int a_fd, int b_fd)
> +{
> +#define CMP_BLOCK_SIZE 65536
> +  char a_buff[CMP_BLOCK_SIZE];
> +  char b_buff[CMP_BLOCK_SIZE];
> +
> +  size_t size;
> +  while (0 < (size = full_read (a_fd, a_buff, CMP_BLOCK_SIZE))) {
> +    if (size != full_read (b_fd, b_buff, CMP_BLOCK_SIZE))
> +      return false;
> +
> +    if (memcmp (a_buff, b_buff, size) != 0)
> +      return false;
> +  }
> +
> +  return size == 0;
> +#undef CMP_BLOCK_SIZE
> +}
> +
> +/* Return true if copy of file FILE to destination TO is necessary.  */
> +static bool
> +need_copy (const char *file, const char *to, const struct cp_options *x)

Please rename:
s/file/src_name/
s/to/dest_name/

> +{
> +  struct stat file_s, to_s;

please rename: src_sb, dest_sb

> +  int file_fd, to_fd;

rename: src_fd, dest_fd

> +  bool match;
> +  security_context_t file_scontext = NULL;
> +  security_context_t to_scontext = NULL;

Move the above two decls "down" into scope where used.

> +  /* compare files using stat */
> +  if (stat (file, &file_s) != 0)
> +    return true;
> +
> +  if (stat (to, &to_s) != 0)
> +    return true;

Have you considered requiring that both files be `regular', too?
(i.e., you'd do use lstat for both above)

> +  if (file_s.st_size != to_s.st_size
> +      || (to_s.st_mode & CHMOD_MODE_BITS) != mode
> +      || (owner_id != (uid_t) -1 && to_s.st_uid != owner_id)
> +      || (group_id != (gid_t) -1 && to_s.st_gid != group_id))
> +    return true;
> +
> +  /* compare SELinux context if preserving */
> +  if (selinux_enabled && x->preserve_security_context)
> +    {
> +      if (getfilecon (file, &file_scontext) == -1)
> +     return true;
> +
> +      if (getfilecon (to, &to_scontext) == -1)
> +     {
> +       freecon (file_scontext);
> +       return true;
> +     }
> +
> +      match = strcmp (file_scontext, to_scontext) == 0;
> +
> +      freecon (file_scontext);
> +      freecon (to_scontext);
> +      if (!match)
> +     return true;
> +    }
> +
> +  /* compare files content */
> +  file_fd = open (file, O_RDONLY);
> +  if (file_fd < 0)
> +    return true;
> +
> +  to_fd = open (to, O_RDONLY);
> +  if (to_fd < 0)
> +    {
> +      close (file_fd);
> +      return true;
> +    }
> +
> +  match = have_same_content (file_fd, to_fd);
> +
> +  close (file_fd);
> +  close (to_fd);
> +  return !match;
> +}