Pádraig Brady <[email protected]> writes: >> Here is a proposed patch that allows the options to be used >> together. > > It's best not to set timestamps in two places. > Could we adjust copy_file to return OK,FAIL,SKIP enum > and then adjust the conditionals in install_file_in_file() ? > > Also we could add a line to the test to be more robust: > > echo a > a || framework_failure_ > echo a > b || framework_failure_ > touch -d 2026-01-01 a || framework_failure_ > test b -nt a || framework_failure_ # Handle systems with bad time
Good ideas. I've attatched a v2 patch that makes those changes. Will push tomorrow. Collin
>From e2a0b54a440c864b9aa7244d5c79c1e8cfb88d5f Mon Sep 17 00:00:00 2001 Message-ID: <e2a0b54a440c864b9aa7244d5c79c1e8cfb88d5f.1772599812.git.collin.fu...@gmail.com> From: Collin Funk <[email protected]> Date: Sun, 1 Mar 2026 15:31:28 -0800 Subject: [PATCH v2] install: allow the combination of --compare and --preserve-timestamps * NEWS: Mention the improvement. * src/install.c (enum copy_status): New type to let the caller know if the copy was performed or skipped. (copy_file): Return the new type instead of bool. Reduce variable scope. (install_file_in_file): Only strip the file if the copy was performed. Update the timestamps if the copy was skipped. (main): Don't error when --compare and --preserve-timestamps are combined. * tests/install/install-C.sh: Add some test cases. --- NEWS | 3 +++ src/install.c | 41 ++++++++++++++++++-------------------- tests/install/install-C.sh | 28 ++++++++++++++++++++++++-- 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/NEWS b/NEWS index 7eb70b6d1..500c50e6b 100644 --- a/NEWS +++ b/NEWS @@ -28,6 +28,9 @@ GNU coreutils NEWS -*- outline -*- 'groups' and 'id' will now exit sooner after a write error, which is significant when listing information for many users. + 'install' now allows the combination of the --compare and + --preserve-timestamps options. + 'nl' now supports multi-byte --section-delimiter characters. 'shuf -i' now operates up to two times faster on systems with unlocked stdio diff --git a/src/install.c b/src/install.c index 58ecfbc31..bccfe606f 100644 --- a/src/install.c +++ b/src/install.c @@ -406,17 +406,18 @@ process_dir (char *dir, struct savewd *wd, void *options) return ret; } +enum copy_status { COPY_FAILED, COPY_OK, COPY_SKIPPED }; + /* Copy file FROM onto file TO aka TO_DIRFD+TO_RELNAME, creating TO if - necessary. Return true if successful. */ + necessary. Return COPY_OK if successful or COPY_SKIPPED if the copy + was skipped. */ -static bool +static enum copy_status copy_file (char const *from, char const *to, int to_dirfd, char const *to_relname, const struct cp_options *x) { - bool copy_into_self; - if (copy_only_if_needed && !need_copy (from, to, to_dirfd, to_relname, x)) - return true; + return COPY_SKIPPED; /* Allow installing from non-regular files like /dev/null. Charles Karney reported that some Sun version of install allows that @@ -424,7 +425,9 @@ copy_file (char const *from, char const *to, However, since !x->recursive, the call to "copy" will fail if FROM is a directory. */ - return copy (from, to, to_dirfd, to_relname, 0, x, ©_into_self, NULL); + bool copy_into_self; + return (copy (from, to, to_dirfd, to_relname, 0, x, ©_into_self, NULL) + ? COPY_OK : COPY_FAILED); } /* Set the attributes of file or directory NAME aka DIRFD+RELNAME. @@ -721,16 +724,17 @@ install_file_in_file (char const *from, char const *to, error (0, errno, _("cannot stat %s"), quoteaf (from)); return false; } - if (! copy_file (from, to, to_dirfd, to_relname, x)) + enum copy_status copy_status = copy_file (from, to, to_dirfd, to_relname, x); + if (copy_status == COPY_FAILED) return false; - if (strip_files) - if (! strip (to)) - { - if (unlinkat (to_dirfd, to_relname, 0) != 0) /* Cleanup. */ - error (EXIT_FAILURE, errno, _("cannot unlink %s"), quoteaf (to)); - return false; - } - if (x->preserve_timestamps && (strip_files || ! S_ISREG (from_sb.st_mode)) + if (copy_status == COPY_OK && strip_files && ! strip (to)) + { + if (unlinkat (to_dirfd, to_relname, 0) != 0) /* Cleanup. */ + error (EXIT_FAILURE, errno, _("cannot unlink %s"), quoteaf (to)); + return false; + } + if (x->preserve_timestamps && (copy_status == COPY_SKIPPED || strip_files + || ! S_ISREG (from_sb.st_mode)) && ! change_timestamps (&from_sb, to, to_dirfd, to_relname)) return false; return change_attributes (to, to_dirfd, to_relname); @@ -1051,13 +1055,6 @@ main (int argc, char **argv) error (0, 0, _("WARNING: ignoring --strip-program option as -s option was " "not specified")); - if (copy_only_if_needed && x.preserve_timestamps) - { - error (0, 0, _("options --compare (-C) and --preserve-timestamps are " - "mutually exclusive")); - usage (EXIT_FAILURE); - } - if (copy_only_if_needed && strip_files) { error (0, 0, _("options --compare (-C) and --strip are mutually " diff --git a/tests/install/install-C.sh b/tests/install/install-C.sh index 7f188c06e..447247c8f 100755 --- a/tests/install/install-C.sh +++ b/tests/install/install-C.sh @@ -115,8 +115,32 @@ compare out out_installed_second || fail=1 ginstall -Cv -m$mode2 a b > out || fail=1 compare out out_empty || fail=1 -# options -C and --preserve-timestamps are mutually exclusive -returns_ 1 ginstall -C --preserve-timestamps a b || fail=1 +# Check -C without --preserve-timestamps with files having the same contents. +echo a > a || framework_failure_ +echo a > b || framework_failure_ +touch -d 2026-01-01 a || framework_failure_ +test b -nt a || framework_failure_ # Handle systems with bad time. +ginstall -C a b || fail=1 +test b -nt a || fail=1 + +# Likewise, but with --preserve-timestamps. +ginstall -C --preserve-timestamps a b || fail=1 +case $(stat --format=%y b) in + 2026-01-01*) ;; + *) fail=1 ;; +esac + +# Check -C without --preserve-timestamps with files having differing contents. +echo b > b || framework_failure_ +ginstall -C a b || fail=1 +test b -nt a || fail=1 + +# Check -C with --preserve-timestamps with files having differing contents. +ginstall -C --preserve-timestamps a b || fail=1 +case $(stat --format=%y b) in + 2026-01-01*) ;; + *) fail=1 ;; +esac # options -C and --strip are mutually exclusive returns_ 1 ginstall -C --strip --strip-program=echo a b || fail=1 -- 2.53.0
