diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 96ddedd..f7f0876 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -147,11 +147,11 @@ Acceptance criteria: ### Make dry-run genuinely non-mutating -- [ ] Separate operation planning from execution. -- [ ] Ensure dry-run creates no files, hardlinks, locks, metadata updates, or +- [x] Separate operation planning from execution. +- [x] Ensure dry-run creates no files, hardlinks, locks, metadata updates, or temporary snapshots. -- [ ] Print proposed copy, exclusion, and deletion operations. -- [ ] Expose validated retention policy through CLI options or configuration. +- [x] Print proposed copy, exclusion, and deletion operations. +- [x] Expose validated retention policy through CLI options or configuration. ### Strengthen tests and CI diff --git a/README.md b/README.md index e1d18af..28c215d 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,17 @@ Optional Arguments: -v, --verbose Enable debug logging --external-rsync Use external rsync instead of Python implementation --external-hardlink Use cp/gcp command for hardlinking + --keep-all DAYS Keep every backup up to this many days in the past + (default: 7) + --keep-daily DAYS Keep one backup per day up to this many days in + the past (default: 30) + --keep-weekly WEEKS Keep one backup per week up to this many weeks in + the past (default: 52) + --keep-monthly MONTHS + Keep one backup per month up to this many months + in the past (default: 12) + --keep-yearly YEARS Keep one backup per year up to this many years in + the past (default: 5) ``` ### Examples @@ -159,7 +170,7 @@ cura-te-ipsum creates complete directory snapshots, but files that haven't chang ### Retention Policy -Default retention (configurable in code): +Default retention (configurable via the `--keep-*` CLI options above): - **7 days**: Keep all backups - **30 days**: Keep one backup per day @@ -232,13 +243,14 @@ backups/ backend's hardlinker cannot carry forward into the next backup. Pick one of `--external-rsync`/`--external-hardlink` or the Python defaults and use it consistently for a given backup destination. -- **`--dry-run` behaves differently per backend, though neither leaves - a lasting change.** With `--external-rsync`, rsync itself avoids - writing to the destination (it still reads and stats files to - compute what would change). With the Python backend, files are still - fully copied into a throwaway staging directory, which is then - deleted - correct, but slower and more I/O-heavy than the external - backend's dry run. +- **`--dry-run` never writes anything, with either backend.** Neither + backend creates a staging directory, hardlink, lock, or any other + file or metadata update - each source is compared directly against + the latest completed snapshot (never a copy of it), and the proposed + create/rewrite/delete/exclude operations are logged. `--external-rsync` + runs real rsync with its own `--dry-run` flag (it still reads and + stats files to compute what would change); the Python backend performs + the same read-only comparison itself. ## Development diff --git a/curateipsum/backup.py b/curateipsum/backup.py index 383b981..a484d07 100644 --- a/curateipsum/backup.py +++ b/curateipsum/backup.py @@ -637,14 +637,20 @@ def cleanup_old_backups(backups_dir: str, ) backups_dir_abs = _canonical(backups_dir) - repo_id = _ensure_repo_id(backups_dir) + if dry_run: + # _ensure_repo_id() would create the repo-id file on first use - + # a mutation dry-run must never perform - and it isn't needed + # for anything below, since nothing is actually deleted here. + for backup in all_backups: + if backup not in keep: + _lg.info("Would remove old backup %s", backup.name) + return + + repo_id = _ensure_repo_id(backups_dir) for backup in all_backups: if backup in keep: continue - if dry_run: - _lg.info("Would remove old backup %s", backup.name) - continue _quarantine_and_delete(backup, backups_dir_abs, repo_id) @@ -750,6 +756,50 @@ def process_backed_entry(backup_dir: str, dst_dir=os.path.join(backup_dir, DELTA_DIR)) +def _plan_backup(sources, backups_dir: str, + latest_backup: Optional[os.DirEntry], + rsync_func) -> None: + """ + Print the operations a real run would perform, without creating, + deleting, or modifying anything: no staging directory, no lock, + no hardlinks, no metadata updates. Each source is diffed directly + against its counterpart in the latest completed snapshot (or + reported as entirely new, if there is no latest snapshot, or this + source wasn't part of it) - never against a throwaway copy. + """ + # A placeholder path that is guaranteed not to exist, used as the + # comparison target for a source with nothing to diff against yet. + # rsync_func(..., dry_run=True) treats a missing dst as "everything + # in src is new" and never creates it. + no_prior_copy = os.path.join( + backups_dir, "%sdry-run-%s" % (STAGING_PREFIX, uuid.uuid4().hex) + ) + + for src in sources: + src_abs = os.path.abspath(src) + src_name = os.path.basename(src_abs) + if latest_backup is not None: + dst_abs = os.path.join(latest_backup.path, src_name) + else: + dst_abs = os.path.join(no_prior_copy, src_name) + + _lg.info("Dry-run: comparing %s against %s", src_abs, dst_abs) + for entry_relpath, action, msg in rsync_func( + src_abs, dst_abs, dry_run=True + ): + full_relpath = os.path.join(src_name, entry_relpath) + if action == fs.Actions.ERROR: + _lg.error("Would fail to back up %s: %s", + full_relpath, msg) + continue + if action == fs.Actions.EXCLUDE: + _lg.info("Would exclude %s (%s)", full_relpath, msg) + continue + if action == fs.Actions.NOTHING: + continue + _lg.info("Would %s: %s", action.name.lower(), full_relpath) + + def initiate_backup(sources, backups_dir: str, dry_run: bool = False, @@ -765,6 +815,10 @@ def initiate_backup(sources, never be mistaken for a successful one. On success, the staging directory is fsync'd and atomically renamed into its final snapshot name. + + With dry_run=True, this only plans and prints proposed operations - + see _plan_backup(). No staging directory, hardlink, or file is ever + created, and no existing file is touched. :param sources: list of directories to backup (relative paths ok) :param backups_dir: directory where all backups are stored :param dry_run: if True, no actual changes will be made @@ -774,6 +828,11 @@ def initiate_backup(sources, snapshot was created (dry-run or no changes since last backup). :raises BackupFailedError: if the backup could not be completed. """ + if dry_run: + _plan_backup(sources, backups_dir, _get_latest_backup(backups_dir), + fs.rsync_ext if external_rsync else fs.rsync) + return None + cleanup_stale_staging_dirs(backups_dir) snapshot_name = _reserve_snapshot_name(backups_dir) @@ -840,9 +899,7 @@ def initiate_backup(sources, dst_abs = os.path.join(staging.path, src_name) _lg.info("Backing up directory %s to staging for %s", src_abs, snapshot_name) - for entry_relpath, action, msg in rsync_func( - src_abs, dst_abs, dry_run=dry_run - ): + for entry_relpath, action, msg in rsync_func(src_abs, dst_abs): if action == fs.Actions.ERROR: raise BackupFailedError( "Failed to copy %s: %s" % (entry_relpath, msg) @@ -889,11 +946,6 @@ def initiate_backup(sources, shutil.rmtree(staging.path, ignore_errors=True) raise - # do not create backup on dry-run - if dry_run: - _lg.info("Dry-run, removing staging directory: %s", snapshot_name) - shutil.rmtree(staging.path, ignore_errors=True) - return None # do not create backup if no change from previous one - but a change # in *which* entries are excluded is itself a change worth keeping, # or the new exclusions would never make it into a written manifest. diff --git a/curateipsum/cli.py b/curateipsum/cli.py index 6d6182e..08aea3f 100755 --- a/curateipsum/cli.py +++ b/curateipsum/cli.py @@ -29,6 +29,20 @@ def _normalize_argv(argv): return ["backup", *argv] +def _nonneg_int(value: str) -> int: + """ + argparse type for --keep-* options: a negative threshold shifts + retention comparisons into the future, which can make cleanup treat + every existing backup as eligible for deletion. + """ + parsed = int(value) + if parsed < 0: + raise argparse.ArgumentTypeError( + "must be zero or a positive integer, got %r" % value + ) + return parsed + + def _build_parser(): parser = argparse.ArgumentParser( prog="cura-te-ipsum", @@ -72,6 +86,40 @@ def _build_parser(): default=False, help="Use cp command for creating hardlink " "copies") + backup_parser.add_argument("--keep-all", + type=_nonneg_int, + default=7, + metavar="DAYS", + help="keep every backup up to this many days " + "in the past (default: %(default)s)") + backup_parser.add_argument("--keep-daily", + type=_nonneg_int, + default=30, + metavar="DAYS", + help="keep one backup per day up to this " + "many days in the past " + "(default: %(default)s)") + backup_parser.add_argument("--keep-weekly", + type=_nonneg_int, + default=52, + metavar="WEEKS", + help="keep one backup per week up to this " + "many weeks in the past " + "(default: %(default)s)") + backup_parser.add_argument("--keep-monthly", + type=_nonneg_int, + default=12, + metavar="MONTHS", + help="keep one backup per month up to this " + "many months in the past " + "(default: %(default)s)") + backup_parser.add_argument("--keep-yearly", + type=_nonneg_int, + default=5, + metavar="YEARS", + help="keep one backup per year up to this " + "many years in the past " + "(default: %(default)s)") backup_parser.add_argument("sources", nargs="+", metavar="SOURCE", @@ -143,7 +191,11 @@ def _run_backup(args) -> int: start_time = time.time() - if not backup.set_backups_lock(backups_dir_abs, args.force): + # A dry-run never writes anything, including lock metadata, so it + # doesn't need to acquire the lock at all - only a real run has to + # serialize against other backups/restores. + if not args.dry_run and not backup.set_backups_lock( + backups_dir_abs, args.force): return 1 exit_code = 0 @@ -160,13 +212,21 @@ def _run_backup(args) -> int: external_rsync=args.external_rsync, external_hardlink=args.external_hardlink, ) - backup.cleanup_old_backups(backups_dir=backups_dir_abs, - dry_run=args.dry_run) + backup.cleanup_old_backups( + backups_dir=backups_dir_abs, + dry_run=args.dry_run, + keep_all=args.keep_all, + keep_daily=args.keep_daily, + keep_weekly=args.keep_weekly, + keep_monthly=args.keep_monthly, + keep_yearly=args.keep_yearly, + ) except backup.BackupFailedError as err: _lg.error("Backup failed: %s", err) exit_code = 1 finally: - backup.release_backups_lock(backups_dir_abs) + if not args.dry_run: + backup.release_backups_lock(backups_dir_abs) end_time = time.time() spent_time = end_time - start_time @@ -189,7 +249,11 @@ def _run_restore(args) -> int: # while it's still in progress. set_backups_lock() already logs why # (e.g. "Previous backup is still in progress") - add restore-specific # context here so a restore failure doesn't read like a backup one. - if not backup.set_backups_lock(backups_dir_abs, args.force): + # A dry-run never writes to dest_dir, so it doesn't need this + # protection - nothing it reads can be invalidated by a concurrent + # writer in a way that produces anything worse than a stale preview. + if not args.dry_run and not backup.set_backups_lock( + backups_dir_abs, args.force): _lg.error("Could not acquire the backups lock, restore aborted " "(use --force to wait for it instead)") return 1 @@ -243,7 +307,8 @@ def _run_restore(args) -> int: else: _lg.info("Verified: restored content matches the manifest") finally: - backup.release_backups_lock(backups_dir_abs) + if not args.dry_run: + backup.release_backups_lock(backups_dir_abs) return exit_code diff --git a/curateipsum/fs.py b/curateipsum/fs.py index 8784ceb..e3eac0d 100644 --- a/curateipsum/fs.py +++ b/curateipsum/fs.py @@ -439,6 +439,18 @@ def update_direntry(src_entry: os.DirEntry, return copy_direntry(src_entry, dst_entry.path) +def _maybe_update(src_entry: os.DirEntry, dst_entry: os.DirEntry, + dry_run: bool) -> Optional[str]: + """ + update_direntry(), unless dry_run - in which case dst_entry is left + untouched and no digest is available (nothing was actually copied to + hash). + """ + if dry_run: + return None + return update_direntry(src_entry, dst_entry) + + def rsync(src_dir, dst_dir, dry_run=False) -> Iterable[Tuple[str, Actions, str]]: @@ -447,6 +459,14 @@ def rsync(src_dir, Yield (path, action, error message) tuples. Entries in dst_dir will be removed if not present in src_dir. Analog of 'rsync --delete -irltpog'. + + With dry_run=True, dst_dir is never created, written to, or mutated + in any way (no copy, delete, chmod, chown, or utime call runs) - the + same comparison logic that decides each action still runs, so the + yielded (path, action, message) tuples describe exactly what a real + run would do, but purely by reading src_dir and dst_dir. This lets a + dry run diff straight against the previous, immutable snapshot + instead of a throwaway hardlinked copy. """ _lg.debug("Rsync: %s -> %s", src_dir, dst_dir) @@ -457,13 +477,18 @@ def rsync(src_dir, raise BackupCreationError( "Error during reading source directory: %s" % src_root_abs ) - if os.path.exists(dst_root_abs): - if not os.path.isdir(dst_root_abs): - raise BackupCreationError( - "Destination path is not a directory: %s" % dst_root_abs - ) - else: - os.mkdir(dst_root_abs) + dst_exists = os.path.exists(dst_root_abs) + if dst_exists and not os.path.isdir(dst_root_abs): + raise BackupCreationError( + "Destination path is not a directory: %s" % dst_root_abs + ) + if not dst_exists: + if dry_run: + # Nothing to compare against and nothing to create - every + # source entry below is naturally reported as a CREATE. + dst_root_abs = None + else: + os.mkdir(dst_root_abs) # Create source map {rel_path: dir_entry} src_files_map = { @@ -472,7 +497,8 @@ def rsync(src_dir, } # process dst tree - for dst_entry in scantree(dst_root_abs, dir_first=False): + dst_iter = scantree(dst_root_abs, dir_first=False) if dst_root_abs else () + for dst_entry in dst_iter: rel_path = dst_entry.path[len(dst_root_abs) + 1:] src_entry = src_files_map.get(rel_path) @@ -480,6 +506,9 @@ def rsync(src_dir, # remove dst entries not existing in source if src_entry is None: _lg.debug("Rsync, deleting: %s", rel_path) + if dry_run: + yield rel_path, Actions.DELETE, "" + continue try: rm_direntry(dst_entry) yield rel_path, Actions.DELETE, "" @@ -513,7 +542,7 @@ def rsync(src_dir, " (src is a file, dst is not a file): %s", rel_path) try: - digest = update_direntry(src_entry, dst_entry) + digest = _maybe_update(src_entry, dst_entry, dry_run) yield rel_path, Actions.REWRITE, digest or "" except (OSError, BackupCreationError) as exc: yield rel_path, Actions.ERROR, str(exc) @@ -525,7 +554,7 @@ def rsync(src_dir, " (src is a dir, dst is not a dir): %s", rel_path) try: - update_direntry(src_entry, dst_entry) + _maybe_update(src_entry, dst_entry, dry_run) yield rel_path, Actions.REWRITE, "" except (OSError, BackupCreationError) as exc: yield rel_path, Actions.ERROR, str(exc) @@ -537,7 +566,7 @@ def rsync(src_dir, " (src is a symlink, dst is not a symlink): %s", rel_path) try: - update_direntry(src_entry, dst_entry) + _maybe_update(src_entry, dst_entry, dry_run) yield rel_path, Actions.REWRITE, "" except (OSError, BackupCreationError) as exc: yield rel_path, Actions.ERROR, str(exc) @@ -547,7 +576,7 @@ def rsync(src_dir, if src_entry.inode() == dst_entry.inode(): _lg.debug("Rsync, rewriting (shared inode with src): %s", rel_path) try: - digest = update_direntry(src_entry, dst_entry) + digest = _maybe_update(src_entry, dst_entry, dry_run) yield rel_path, Actions.REWRITE, digest or "" except (OSError, BackupCreationError) as exc: yield rel_path, Actions.ERROR, str(exc) @@ -565,7 +594,7 @@ def rsync(src_dir, _lg.debug("Rsync, rewriting (different %s): %s", reason, rel_path) try: - digest = update_direntry(src_entry, dst_entry) + digest = _maybe_update(src_entry, dst_entry, dry_run) yield rel_path, Actions.REWRITE, digest or "" except (OSError, BackupCreationError) as exc: yield rel_path, Actions.ERROR, str(exc) @@ -577,7 +606,7 @@ def rsync(src_dir, _lg.debug("Rsync, rewriting (different symlink target): %s", rel_path) try: - update_direntry(src_entry, dst_entry) + _maybe_update(src_entry, dst_entry, dry_run) yield rel_path, Actions.REWRITE, "" except (OSError, BackupCreationError) as exc: yield rel_path, Actions.ERROR, str(exc) @@ -598,25 +627,30 @@ def rsync(src_dir, # before mutating permissions/ownership in place, or the change # would silently apply to every older snapshot sharing the inode. if src_entry.is_file(follow_symlinks=False): - if src_stat.st_mode != dst_stat.st_mode or ( + if not dry_run and (src_stat.st_mode != dst_stat.st_mode or ( src_stat.st_uid != dst_stat.st_uid - or src_stat.st_gid != dst_stat.st_gid): + or src_stat.st_gid != dst_stat.st_gid)): _break_shared_hardlink(dst_entry.path) # update permissions and ownership if src_stat.st_mode != dst_stat.st_mode: _lg.debug("Rsync, updating permissions: %s", rel_path) - os.chmod(dst_entry.path, src_stat.st_mode) + if not dry_run: + os.chmod(dst_entry.path, src_stat.st_mode) yield rel_path, Actions.UPDATE_PERM, "" if (src_stat.st_uid != dst_stat.st_uid or src_stat.st_gid != dst_stat.st_gid): _lg.debug("Rsync, updating owners: %s", rel_path) - try: - os.chown(dst_entry.path, src_stat.st_uid, src_stat.st_gid) + if dry_run: yield rel_path, Actions.UPDATE_OWNER, "" - except PermissionError: - _lg.debug("Cannot change ownership (not root): %s", rel_path) + else: + try: + os.chown(dst_entry.path, src_stat.st_uid, src_stat.st_gid) + yield rel_path, Actions.UPDATE_OWNER, "" + except PermissionError: + _lg.debug("Cannot change ownership (not root): %s", + rel_path) # process remained source entries (new files/dirs/symlinks) for rel_path, src_entry in src_files_map.items(): @@ -627,6 +661,10 @@ def rsync(src_dir, yield rel_path, Actions.EXCLUDE, src_kind continue + if dry_run: + yield rel_path, Actions.CREATE, "" + continue + dst_path = os.path.join(dst_root_abs, rel_path) _lg.debug("Rsync, creating: %s", rel_path) try: @@ -635,6 +673,11 @@ def rsync(src_dir, except (OSError, BackupCreationError) as exc: yield rel_path, Actions.ERROR, str(exc) + if dry_run: + # Nothing below this point does anything but restore directory + # mtimes disturbed by the mutations above - none of which ran. + return + # restore dir mtimes in dst, updated by updating files for src_entry in scantree(src_root_abs, dir_first=True, one_filesystem=True): diff --git a/tests/test_cli.py b/tests/test_cli.py index f13637b..52033bf 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -117,6 +117,32 @@ class TestArgumentParsing: assert m_cleanup.call_args.kwargs['dry_run'] is True assert m_init.call_args.kwargs['dry_run'] is True + def test_dry_run_does_not_acquire_lock(self, tmp_path): + """ + A dry-run must not take the backups lock at all - set_backups_lock() + writes PID/timestamp metadata to the lock file, which is itself a + mutation a dry-run must never make. + """ + backups_dir = tmp_path / "backups" + backups_dir.mkdir() + source = tmp_path / "src" + source.mkdir() + + with mock.patch('sys.argv', ['cura-te-ipsum', '-b', + str(backups_dir), str(source), + '--dry-run']): + with mock.patch('curateipsum.backup.set_backups_lock') \ + as m_lock: + with mock.patch('curateipsum.backup.cleanup_old_backups'): + with mock.patch('curateipsum.backup.initiate_backup'): + with mock.patch( + 'curateipsum.backup.release_backups_lock') \ + as m_release: + cli.main() + + m_lock.assert_not_called() + m_release.assert_not_called() + def test_force_flag_passed_to_lock(self, tmp_path): """Should pass force=True to set_backups_lock when --force is set.""" backups_dir = tmp_path / "backups" @@ -655,6 +681,31 @@ class TestRestoreSubcommand: assert result == 0 assert not dest.exists() + def test_restore_dry_run_does_not_acquire_lock(self, tmp_path): + """ + Mirrors TestMain.test_dry_run_does_not_acquire_lock for restore: + restore --dry-run must not touch the lock either. + """ + backups_dir = tmp_path / "backups" + backups_dir.mkdir() + source = tmp_path / "src" + source.mkdir() + self._make_backup(backups_dir, source) + + dest = tmp_path / "dest" + with mock.patch('sys.argv', ['cura-te-ipsum', 'restore', '-b', + str(backups_dir), '-n', str(dest)]): + with mock.patch('curateipsum.backup.set_backups_lock') \ + as m_lock: + with mock.patch( + 'curateipsum.backup.release_backups_lock') \ + as m_release: + result = cli.main() + + assert result == 0 + m_lock.assert_not_called() + m_release.assert_not_called() + def test_restore_verify_passes_after_clean_restore(self, tmp_path): backups_dir = tmp_path / "backups" backups_dir.mkdir() diff --git a/tests/test_integration.py b/tests/test_integration.py index fd5ce65..416caa1 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -193,6 +193,127 @@ def test_dry_run_creates_no_backup(integration_dirs): assert len(backups) == 0 +def test_dry_run_leaves_backups_dir_byte_for_byte_unchanged(integration_dirs): + """ + A dry-run must never create a staging directory, hardlink, lock + metadata, or any other trace on disk - not even a dotfile. Compare + every entry's full stat() (not just name/existence) before and + after, so an in-place mutation of an existing entry (e.g. the + previous snapshot getting chmod'd) can't slip past a listing-only + check. + """ + backups_dir, source_dir = integration_dirs + + (source_dir / "file.txt").write_text("original") + bk.initiate_backup( + sources=[str(source_dir)], backups_dir=str(backups_dir), + dry_run=False + ) + + def _snapshot_all_stats(root): + st = os.lstat(root) + result = {root: (st.st_mode, st.st_mtime_ns, st.st_ino, st.st_size)} + for dirpath, dirnames, filenames in os.walk(root): + for name in dirnames + filenames: + path = os.path.join(dirpath, name) + st = os.lstat(path) + result[path] = (st.st_mode, st.st_mtime_ns, st.st_ino, + st.st_size) + return result + + before = _snapshot_all_stats(str(backups_dir)) + + # Change the source so the dry-run has something to report + # (create, rewrite, and permission-change candidates all included). + (source_dir / "file.txt").write_text("changed") + (source_dir / "new.txt").write_text("new") + + bk.initiate_backup( + sources=[str(source_dir)], backups_dir=str(backups_dir), + dry_run=True + ) + bk.cleanup_old_backups(backups_dir=str(backups_dir), dry_run=True) + + after = _snapshot_all_stats(str(backups_dir)) + assert before == after + + +def test_dry_run_cleanup_does_not_create_repo_id_file(integration_dirs): + """ + cleanup_old_backups(dry_run=True) must not create the repo-id file + as a side effect of computing what it would delete - that file's + on-disk creation is itself a mutation a dry-run must never make. + """ + backups_dir, source_dir = integration_dirs + + (source_dir / "file.txt").write_text("content") + bk.initiate_backup( + sources=[str(source_dir)], backups_dir=str(backups_dir), + dry_run=False + ) + + # A real backup already creates the repo-id file (the manifest needs + # it); remove it to simulate a legacy repo predating that file, so a + # dry-run cleanup recreating it would actually be observable. + repo_id_path = os.path.join(str(backups_dir), bk.REPO_ID_FILE) + assert os.path.exists(repo_id_path) + os.remove(repo_id_path) + + bk.cleanup_old_backups(backups_dir=str(backups_dir), dry_run=True, + keep_all=0) + + assert not os.path.exists(repo_id_path) + + +def test_dry_run_reports_changes_against_previous_snapshot(integration_dirs): + """ + A dry-run's reported operations must reflect a real diff against the + latest snapshot, not just "everything is new" - proving the new + direct-comparison path (no throwaway staging copy) actually diffs + against the right target. + """ + backups_dir, source_dir = integration_dirs + + (source_dir / "unchanged.txt").write_text("same") + (source_dir / "will_change.txt").write_text("before") + bk.initiate_backup( + sources=[str(source_dir)], backups_dir=str(backups_dir), + dry_run=False + ) + + (source_dir / "will_change.txt").write_text("after") + (source_dir / "brand_new.txt").write_text("new") + + seen = [] + latest_backup = bk._get_latest_backup(str(backups_dir)) + bk._plan_backup([str(source_dir)], str(backups_dir), latest_backup, + fs_rsync_recording(seen)) + + # fs_rsync_recording() wraps fs.rsync() directly, so recorded paths + # are relative to the source root itself (the source-name prefix + # _plan_backup() adds for logging isn't part of what rsync() yields). + kinds = {(relpath, action) for relpath, action in seen} + assert ("will_change.txt", "REWRITE") in kinds + assert ("brand_new.txt", "CREATE") in kinds + assert not any(relpath == "unchanged.txt" for relpath, _ in seen) + + +def fs_rsync_recording(seen): + """ + Wrap the real fs.rsync() so a test can inspect exactly which + (relpath, action) pairs a dry-run would report, without depending + on log output. + """ + from curateipsum import fs + + def _wrapped(src, dst, dry_run=False): + for relpath, action, msg in fs.rsync(src, dst, dry_run=dry_run): + seen.append((relpath, action.name)) + yield relpath, action, msg + + return _wrapped + + def test_no_backup_if_no_changes(integration_dirs): """Test that no backup is created if nothing changed""" backups_dir, source_dir = integration_dirs diff --git a/tests/test_rsync.py b/tests/test_rsync.py index e309b0e..23ef813 100644 --- a/tests/test_rsync.py +++ b/tests/test_rsync.py @@ -358,6 +358,89 @@ class TestRsyncBasic: assert dst_stat.st_mode == src_stat.st_mode +class TestRsyncDryRun: + """ + fs.rsync(dry_run=True) must yield the same actions a real run would, + while never creating, deleting, or modifying anything - including + dst_dir itself when it doesn't already exist. + """ + + @pytest.fixture + def rsync_dirs(self, tmp_path): + src_dir = os.path.join(str(tmp_path), "source") + dst_dir = os.path.join(str(tmp_path), "dest") + os.mkdir(src_dir) + return src_dir, dst_dir + + def test_dry_run_does_not_create_missing_destination(self, rsync_dirs): + src_dir, dst_dir = rsync_dirs + with open(os.path.join(src_dir, "file.txt"), "w") as f: + f.write("content") + + actions = list(fs.rsync(src_dir, dst_dir, dry_run=True)) + + assert not os.path.exists(dst_dir) + assert [a[1] for a in actions] == [fs.Actions.CREATE] + + def test_dry_run_does_not_copy_new_files(self, rsync_dirs): + src_dir, dst_dir = rsync_dirs + os.mkdir(dst_dir) + with open(os.path.join(src_dir, "file.txt"), "w") as f: + f.write("content") + + actions = list(fs.rsync(src_dir, dst_dir, dry_run=True)) + + assert not os.path.exists(os.path.join(dst_dir, "file.txt")) + assert [a[1] for a in actions] == [fs.Actions.CREATE] + + def test_dry_run_does_not_delete_stale_files(self, rsync_dirs): + src_dir, dst_dir = rsync_dirs + os.mkdir(dst_dir) + stale = os.path.join(dst_dir, "stale.txt") + with open(stale, "w") as f: + f.write("stale") + + actions = list(fs.rsync(src_dir, dst_dir, dry_run=True)) + + assert os.path.exists(stale) + assert [a[1] for a in actions] == [fs.Actions.DELETE] + + def test_dry_run_does_not_rewrite_changed_files(self, rsync_dirs): + src_dir, dst_dir = rsync_dirs + os.mkdir(dst_dir) + src_file = os.path.join(src_dir, "file.txt") + dst_file = os.path.join(dst_dir, "file.txt") + with open(src_file, "w") as f: + f.write("updated content") + with open(dst_file, "w") as f: + f.write("modified") + + actions = list(fs.rsync(src_dir, dst_dir, dry_run=True)) + + with open(dst_file, "r") as f: + assert f.read() == "modified" + assert [a[1] for a in actions] == [fs.Actions.REWRITE] + + def test_dry_run_does_not_change_permissions(self, rsync_dirs): + src_dir, dst_dir = rsync_dirs + os.mkdir(dst_dir) + src_file = os.path.join(src_dir, "script.sh") + dst_file = os.path.join(dst_dir, "script.sh") + with open(src_file, "w") as f: + f.write("content") + with open(dst_file, "w") as f: + f.write("content") + os.utime(dst_file, ns=(os.stat(src_file).st_atime_ns, + os.stat(src_file).st_mtime_ns)) + os.chmod(src_file, 0o755) + os.chmod(dst_file, 0o644) + + actions = list(fs.rsync(src_dir, dst_dir, dry_run=True)) + + assert os.stat(dst_file).st_mode & 0o777 == 0o644 + assert [a[1] for a in actions] == [fs.Actions.UPDATE_PERM] + + class TestParseRsyncOutput: """Test _parse_rsync_output() parsing of rsync --itemize-changes."""