Fix overzealous setting of mtime & tweak time comparisons

- Stop setting the mtime on a file we didn't transfer (or didn't verify the checksum) when the time diff is within the modify window. - Stop computing a time difference (-1|0|1) when all we care about is time equality.
RsyncProject · Jun 13, 2020 · d326961 · d326961
1 parent 7dec402
commit d326961
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 49 deletions.
diff --git a/backup.c b/backup.c
@@ -336,7 +336,7 @@ int make_backup(const char *fname, BOOL prefer_rename)
 
 	save_preserve_xattrs = preserve_xattrs;
 	preserve_xattrs = 0;
-	set_file_attrs(buf, file, NULL, fname, ATTRS_SET_NANO);
+	set_file_attrs(buf, file, NULL, fname, ATTRS_ACCURATE_TIME);
 	preserve_xattrs = save_preserve_xattrs;
 
 	unmake_file(file);

diff --git a/generator.c b/generator.c
@@ -59,6 +59,7 @@ extern int human_readable;
 extern int ignore_existing;
 extern int ignore_non_existing;
 extern int want_xattr_optim;
+extern int modify_window;
 extern int inplace;
 extern int append_mode;
 extern int make_backups;
@@ -100,7 +101,7 @@ extern struct file_list *cur_flist, *first_flist, *dir_flist;
 extern filter_rule_list filter_list, daemon_filter_list;
 
 int maybe_ATTRS_REPORT = 0;
-int maybe_ATTRS_SET_NANO = 0;
+int maybe_ATTRS_ACCURATE_TIME = 0;
 
 static dev_t dev_zero;
 static int deldelay_size = 0, deldelay_cnt = 0;
@@ -391,12 +392,12 @@ static void do_delete_pass(void)
 		rprintf(FINFO, "                    \r");
 }
 
-static inline int time_diff(STRUCT_STAT *stp, struct file_struct *file)
+static inline int time_differs(STRUCT_STAT *stp, struct file_struct *file)
 {
 #ifdef ST_MTIME_NSEC
-	return cmp_time(stp->st_mtime, stp->ST_MTIME_NSEC, file->modtime, F_MOD_NSEC_or_0(file));
+	return !same_time(stp->st_mtime, stp->ST_MTIME_NSEC, file->modtime, F_MOD_NSEC_or_0(file));
 #else
-	return cmp_time(stp->st_mtime, 0L, file->modtime, 0L);
+	return !same_time(stp->st_mtime, 0, file->modtime, 0);
 #endif
 }
 
@@ -454,7 +455,7 @@ int unchanged_attrs(const char *fname, struct file_struct *file, stat_x *sxp)
 {
 	if (S_ISLNK(file->mode)) {
 #ifdef CAN_SET_SYMLINK_TIMES
-		if (preserve_times & PRESERVE_LINK_TIMES && time_diff(&sxp->st, file))
+		if (preserve_times & PRESERVE_LINK_TIMES && time_differs(&sxp->st, file))
 			return 0;
 #endif
 #ifdef CAN_CHMOD_SYMLINK
@@ -474,7 +475,7 @@ int unchanged_attrs(const char *fname, struct file_struct *file, stat_x *sxp)
 			return 0;
 #endif
 	} else {
-		if (preserve_times && time_diff(&sxp->st, file))
+		if (preserve_times && time_differs(&sxp->st, file))
 			return 0;
 		if (perms_differ(file, sxp))
 			return 0;
@@ -509,12 +510,12 @@ void itemize(const char *fnamecmp, struct file_struct *file, int ndx, int statre
 			if (iflags & ITEM_LOCAL_CHANGE)
 				iflags |= symlink_timeset_failed_flags;
 		} else if (keep_time
-		 ? time_diff(&sxp->st, file)
+		 ? time_differs(&sxp->st, file)
 		 : iflags & (ITEM_TRANSFER|ITEM_LOCAL_CHANGE) && !(iflags & ITEM_MATCHED)
 		  && (!(iflags & ITEM_XNAME_FOLLOWS) || *xname))
 			iflags |= ITEM_REPORT_TIME;
 		if (atimes_ndx && !S_ISDIR(file->mode) && !S_ISLNK(file->mode)
-		 && cmp_time(F_ATIME(file), 0, sxp->st.st_atime, 0) != 0)
+		 && !same_time(F_ATIME(file), 0, sxp->st.st_atime, 0))
 			iflags |= ITEM_REPORT_ATIME;
 #if !defined HAVE_LCHMOD && !defined HAVE_SETATTRLIST
 		if (S_ISLNK(file->mode)) {
@@ -604,7 +605,7 @@ int unchanged_file(char *fn, struct file_struct *file, STRUCT_STAT *st)
 	if (ignore_times)
 		return 0;
 
-	return time_diff(st, file) == 0;
+	return !time_differs(st, file);
 }
 
 
@@ -781,7 +782,7 @@ static struct file_struct *find_fuzzy(struct file_struct *file, struct file_list
 			if (!S_ISREG(fp->mode) || !F_LENGTH(fp) || fp->flags & FLAG_FILE_SENT)
 				continue;
 
-			if (F_LENGTH(fp) == F_LENGTH(file) && cmp_time(fp->modtime, 0L, file->modtime, 0L) == 0) {
+			if (F_LENGTH(fp) == F_LENGTH(file) && same_time(fp->modtime, 0, file->modtime, 0)) {
 				if (DEBUG_GTE(FUZZY, 2))
 					rprintf(FINFO, "fuzzy size/modtime match for %s\n", f_name(fp, NULL));
 				*fnamecmp_type_ptr = FNAMECMP_FUZZY + i;
@@ -1228,7 +1229,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx,
 		return;
 	}
 
-	maybe_ATTRS_SET_NANO = always_checksum ? ATTRS_SET_NANO : 0;
+	maybe_ATTRS_ACCURATE_TIME = always_checksum ? ATTRS_ACCURATE_TIME : 0;
 
 	if (skip_dir) {
 		if (is_below(file, skip_dir)) {
@@ -1685,7 +1686,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx,
 		goto cleanup;
 	}
 
-	if (update_only > 0 && statret == 0 && time_diff(&sx.st, file) > 0) {
+	if (update_only > 0 && statret == 0 && file->modtime - sx.st.st_mtime <= modify_window) {
 		if (INFO_GTE(SKIP, 1))
 			rprintf(FINFO, "%s is newer\n", fname);
 #ifdef SUPPORT_HARD_LINKS
@@ -1785,7 +1786,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx,
 			do_unlink(partialptr);
 			handle_partial_dir(partialptr, PDIR_DELETE);
 		}
-		set_file_attrs(fname, file, &sx, NULL, maybe_ATTRS_REPORT | maybe_ATTRS_SET_NANO);
+		set_file_attrs(fname, file, &sx, NULL, maybe_ATTRS_REPORT | maybe_ATTRS_ACCURATE_TIME);
 		if (itemizing)
 			itemize(fnamecmp, file, ndx, statret, &sx, 0, 0, NULL);
 #ifdef SUPPORT_HARD_LINKS
@@ -2088,7 +2089,7 @@ static void touch_up_dirs(struct file_list *flist, int ndx)
 			do_chmod(fname, file->mode);
 		if (need_retouch_dir_times) {
 			STRUCT_STAT st;
-			if (link_stat(fname, &st, 0) == 0 && time_diff(&st, file)) {
+			if (link_stat(fname, &st, 0) == 0 && time_differs(&st, file)) {
 				st.st_mtime = file->modtime;
 #ifdef ST_MTIME_NSEC
 				st.ST_MTIME_NSEC = F_MOD_NSEC_or_0(file);

diff --git a/rsync.c b/rsync.c
@@ -468,6 +468,21 @@ mode_t dest_mode(mode_t flist_mode, mode_t stat_mode, int dflt_perms,
 	return new_mode;
 }
 
+static int same_mtime(struct file_struct *file, STRUCT_STAT *st, int extra_accuracy)
+{
+#ifdef ST_MTIME_NSEC
+	uint32 f1_nsec = F_MOD_NSEC_or_0(file);
+	uint32 f2_nsec = (uint32)st->ST_MTIME_NSEC;
+#else
+	uint32 f1_nsec = 0, f2_nsec = 0;
+#endif
+
+	if (extra_accuracy) /* ignore modify_window when setting the time after a transfer or checksum check */
+		return file->modtime == st->st_mtime && f1_nsec == f2_nsec;
+
+	return same_time(file->modtime, f1_nsec, st->st_mtime , f2_nsec);
+}
+
 int set_file_attrs(const char *fname, struct file_struct *file, stat_x *sxp,
 		   const char *fnamecmp, int flags)
 {
@@ -570,12 +585,7 @@ int set_file_attrs(const char *fname, struct file_struct *file, stat_x *sxp,
 		memcpy(&sx2.st, &sxp->st, sizeof (sx2.st));
 	if (!atimes_ndx || S_ISDIR(sxp->st.st_mode))
 		flags |= ATTRS_SKIP_ATIME;
-	if (!(flags & ATTRS_SKIP_MTIME)
-	 && (sxp->st.st_mtime != file->modtime
-#ifdef ST_MTIME_NSEC
-	  || (flags & ATTRS_SET_NANO && NSEC_BUMP(file) && (uint32)sxp->st.ST_MTIME_NSEC != F_MOD_NSEC(file))
-#endif
-	  )) {
+	if (!(flags & ATTRS_SKIP_MTIME) && !same_mtime(file, &sxp->st, flags & ATTRS_ACCURATE_TIME)) {
 		sx2.st.st_mtime = file->modtime;
 #ifdef ST_MTIME_NSEC
 		sx2.st.ST_MTIME_NSEC = F_MOD_NSEC_or_0(file);
@@ -584,7 +594,7 @@ int set_file_attrs(const char *fname, struct file_struct *file, stat_x *sxp,
 	}
 	if (!(flags & ATTRS_SKIP_ATIME)) {
 		time_t file_atime = F_ATIME(file);
-		if (cmp_time(sxp->st.st_atime, 0, file_atime, 0) != 0) {
+		if (flags & ATTRS_ACCURATE_TIME || !same_time(sxp->st.st_atime, 0, file_atime, 0)) {
 			sx2.st.st_atime = file_atime;
 #ifdef ST_ATIME_NSEC
 			sx2.st.ST_ATIME_NSEC = 0;
@@ -709,7 +719,7 @@ int finish_transfer(const char *fname, const char *fnametmp,
 
 	/* Change permissions before putting the file into place. */
 	set_file_attrs(fnametmp, file, NULL, fnamecmp,
-		       ok_to_set_time ? ATTRS_SET_NANO : ATTRS_SKIP_MTIME | ATTRS_SKIP_ATIME);
+		       ok_to_set_time ? ATTRS_ACCURATE_TIME : ATTRS_SKIP_MTIME | ATTRS_SKIP_ATIME);
 
 	/* move tmp file over real file */
 	if (DEBUG_GTE(RECV, 1))
@@ -734,7 +744,7 @@ int finish_transfer(const char *fname, const char *fnametmp,
 
   do_set_file_attrs:
 	set_file_attrs(fnametmp, file, NULL, fnamecmp,
-		       ok_to_set_time ? ATTRS_SET_NANO : ATTRS_SKIP_MTIME | ATTRS_SKIP_ATIME);
+		       ok_to_set_time ? ATTRS_ACCURATE_TIME : ATTRS_SKIP_MTIME | ATTRS_SKIP_ATIME);
 
 	if (temp_copy_name) {
 		if (do_rename(fnametmp, fname) < 0) {

diff --git a/rsync.h b/rsync.h
@@ -175,7 +175,7 @@
 
 #define ATTRS_REPORT		(1<<0)
 #define ATTRS_SKIP_MTIME	(1<<1)
-#define ATTRS_SET_NANO		(1<<2)
+#define ATTRS_ACCURATE_TIME	(1<<2)
 #define ATTRS_SKIP_ATIME	(1<<3)
 
 #define MSG_FLUSH	2

diff --git a/testsuite/exclude.test b/testsuite/exclude.test
@@ -1,6 +1,6 @@
 #! /bin/sh
 
-# Copyright (C) 2003, 2004, 2005 by Wayne Davison <wayned@samba.org>
+# Copyright (C) 2003-2020 Wayne Davison
 
 # This program is distributable under the terms of the GNU GPL (see
 # COPYING).
@@ -12,6 +12,9 @@
 
 . "$suitedir/rsync.fns"
 
+chkfile="$scratchdir/rsync.chk"
+outfile="$scratchdir/rsync.out"
+
 CVSIGNORE='*.junk'
 export CVSIGNORE
 
@@ -113,6 +116,11 @@ rm -rf "$todir"
 # Add a directory symlink.
 ln -s too "$fromdir/bar/down/to/foo/sym"
 
+# Start to prep an --update test dir
+mkdir "$scratchdir/up1" "$scratchdir/up2"
+touch "$scratchdir/up1/older" "$scratchdir/up2/newer"
+touch "$scratchdir/up1/extra-src" "$scratchdir/up2/extra-dest"
+
 # Create chkdir with what we expect to be excluded.
 checkit "$RSYNC -avv '$fromdir/' '$chkdir/'" "$fromdir" "$chkdir"
 sleep 1 # Ensures that the rm commands will tweak the directory times.
@@ -124,6 +132,9 @@ rm "$chkdir"/foo/file[235-9]
 rm "$chkdir"/bar/down/to/foo/to "$chkdir"/bar/down/to/foo/file[235-9]
 rm "$chkdir"/mid/for/foo/extra
 
+# Finish prep for the --update test (run last)
+touch "$scratchdir/up1/newer" "$scratchdir/up2/older"
+
 # Un-tweak the directory times in our first (weak) exclude test (though
 # it's a good test of the --existing option).
 $RSYNC -av --existing --include='*/' --exclude='*' "$fromdir/" "$chkdir/"
@@ -215,5 +226,14 @@ $RSYNC -av $relative_opts --existing --filter='-! */' "$fromdir/foo" "$chkdir/"
 checkit "$RSYNC -avv $relative_opts --exclude='$fromdir/foo/down' \
     '$fromdir/foo' '$todir'" "$chkdir$fromdir/foo" "$todir$fromdir/foo"
 
+# Now we'll test the --update option.
+$RSYNC -aiO --update touch "$scratchdir/up1/" "$scratchdir/up2/" \
+    | tee "$outfile"
+cat <<EOT >"$chkfile"
+>f$all_plus extra-src
+>f..t.$dots newer
+EOT
+diff $diffopt "$chkfile" "$outfile" || test_fail "--update test failed"
+
 # The script would have aborted on error, so getting here means we've won.
 exit 0
diff --git a/util.c b/util.c
@@ -1354,30 +1354,17 @@ char *timestring(time_t t)
 
 /* Determine if two time_t values are equivalent (either exact, or in
  * the modification timestamp window established by --modify-window).
- *
- * @retval 0 if the times should be treated as the same
- *
- * @retval +1 if the first is later
- *
- * @retval -1 if the 2nd is later
- **/
-int cmp_time(time_t f1_sec, unsigned long f1_nsec, time_t f2_sec, unsigned long f2_nsec)
+ * Returns 1 if the times the "same", or 0 if they are different. */
+int same_time(time_t f1_sec, unsigned long f1_nsec, time_t f2_sec, unsigned long f2_nsec)
 {
-	if (f2_sec > f1_sec) {
-		/* The final comparison makes sure that modify_window doesn't overflow a
-		 * time_t, which would mean that f2_sec must be in the equality window. */
-		if (modify_window <= 0 || (f2_sec > f1_sec + modify_window && f1_sec + modify_window > f1_sec))
-			return -1;
-	} else if (f1_sec > f2_sec) {
-		if (modify_window <= 0 || (f1_sec > f2_sec + modify_window && f2_sec + modify_window > f2_sec))
-			return 1;
-	} else if (modify_window < 0) {
-		if (f2_nsec > f1_nsec)
-			return -1;
-		else if (f1_nsec > f2_nsec)
-			return 1;
-	}
-	return 0;
+	if (modify_window == 0)
+		return f1_sec == f2_sec;
+	if (modify_window < 0)
+		return f1_sec == f2_sec && f1_nsec == f2_nsec;
+	/* The nano seconds doesn't figure into these checks -- time windows don't care about that. */
+	if (f2_sec > f1_sec)
+		return f2_sec - f1_sec <= modify_window;
+	return f1_sec - f2_sec <= modify_window;
 }
 
 #ifdef __INSURE__XX