From fd32cb8ff99438077622fb2f88f02632ce99f837 Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 20:28:06 -0700 Subject: [PATCH 01/10] Find the repository from any folder inside it Commands now open the nearest enclosing repository with Repo.find, so hm works from subfolders of a worktree, and explain when no repository exists instead of failing on a missing .hm. Repo(path) still opens exactly path. Download paths are taken relative to the current folder and must stay inside the worktree, outside .hm. Repo.add(".") rebuilds the catalog from the current folder only, so from a subfolder it would drop every row outside it. The CLI therefore runs hm add . from the worktree root, staging the whole repository as the workflow specifies, and treats any other path to the root, such as .. from a subfolder or the absolute path, the same way instead of as a new pattern. Co-Authored-By: Claude Opus 5.5 --- README.rst | 11 ++-- mod/hallmark/cli.py | 73 +++++++++++++++++++++-- mod/hallmark/repo/__init__.py | 52 ++++++++++++++++- test/test_cli.py | 107 ++++++++++++++++++++++++++++++++-- test/test_repo_open.py | 54 +++++++++++++++++ 5 files changed, 279 insertions(+), 18 deletions(-) create mode 100644 test/test_repo_open.py diff --git a/README.rst b/README.rst index 04b526b..a1f63a4 100644 --- a/README.rst +++ b/README.rst @@ -85,11 +85,12 @@ Create a repository, stage a remote catalog, then commit it:: hm download --all --dry-run hm download --all -``hm`` is the CLI command; ``hallmark`` remains an alias. Initialization creates the empty -local repository. Remote ``add`` discovers files without downloading dataset -contents or committing the catalog. CyVerse, ordinary browsable HTTPS -directories, SSH and SFTP use the same workflow. Omit the filter to catalog -all discoverable files beneath the supplied URL. +``hm`` is the CLI command; ``hallmark`` remains an alias. Commands work from any +folder inside a repository; ``hm`` finds ``.hm`` in parent folders. +Initialization creates the empty local repository. Remote ``add`` discovers +files without downloading dataset contents or committing the catalog. CyVerse, +ordinary browsable HTTPS directories, SSH and SFTP use the same workflow. Omit +the filter to catalog all discoverable files beneath the supplied URL. Every nonempty CLI transfer asks for confirmation. For separate Python downloads, callers can inspect diff --git a/mod/hallmark/cli.py b/mod/hallmark/cli.py index ca1996b..82d38ed 100644 --- a/mod/hallmark/cli.py +++ b/mod/hallmark/cli.py @@ -14,6 +14,7 @@ """Hallmark CLI entrypoint and command wiring.""" +import os from contextlib import contextmanager from pathlib import Path @@ -25,7 +26,8 @@ from . import Repo from .remote.download import DownloadError from .remote.discovery import path_matches -from .error import CheckoutError, CloneError +from .error import CheckoutError, CloneError, DothmError +from .utils import use_working_directory # use a context manager to translate application errors into clean Click errors @@ -138,9 +140,58 @@ def hallmark(ctx): if ctx.invoked_subcommand in [None, "init", "clone"]: # return early without attempting to open a repository return - # attempt to open the hallmark repository in the current directory + # open the repository containing the current folder, searching its parents with _translate_cli_errors(GitError, prefix="Failed to open hallmark repository"): - ctx.obj = Repo(".") + with _translate_cli_errors(DothmError): + ctx.obj = Repo.find(Path.cwd()) + + +def _repo_relative_paths(repo, paths): + """ + Convert paths given relative to the current folder into repository paths. + + Bare repositories have no worktree, so their paths are already relative to + the catalog root and are returned unchanged. + + Args: + repo: The hallmark repository object. + paths (sequence[str]): Paths as typed by the user. + + Returns: + tuple[str, ...]: POSIX paths relative to the worktree; "." names the + whole worktree. + + Raises: + ClickException: If a path is outside the worktree or inside ``.hm``. + """ + if repo.worktree is None: + return tuple(paths) + worktree = Path(repo.worktree) + converted = [] + for value in paths: + # Normalize lexically: catalog paths are literal and need not exist yet. + absolute = Path(os.path.normpath(Path.cwd() / value)) + try: + relative = absolute.relative_to(worktree) + except ValueError: + raise ClickException( + f'"{value}" is outside the repository worktree "{worktree}"') from None + if relative.parts and relative.parts[0].lower() == ".hm": + raise ClickException(f'"{value}" is inside .hm, not a data path') + converted.append(relative.as_posix()) + return tuple(converted) + + +def _names_worktree_root(repo, value): + """Return whether an add input is "." or another path to the worktree root.""" + if repo.worktree is None: + return False + if value == ".": + return True + try: + return (Path.cwd() / value).resolve() == Path(repo.worktree).resolve() + except (OSError, RuntimeError, ValueError): + return False def _load_backend_options(path): @@ -258,7 +309,9 @@ def add(repo, encoding, inputs, auth, backend, backend_options, filters, remote_ `hm add [--regex] FORMAT` uses the branch format string workflow. `hm add "."` rebuilds the manifest from current files that match - the branch `fmt` in `config.yml`. + the branch `fmt` in `config.yml`, across the whole repository from any + folder inside it. Any other path to the worktree root, such as `..` from + a subfolder, does the same. Explicit path inputs such as shell-expanded `*` are not supported yet with the parameter-based manifest format. """ @@ -276,6 +329,11 @@ def add(repo, encoding, inputs, auth, backend, backend_options, filters, remote_ remote = "://" in inputs[0] if remote or any(value is not None for value in remote_options.values()): pf = repo.add(inputs[0], encoding, progress=True, **remote_options) + elif _names_worktree_root(repo, inputs[0]): + # Like ".", a path naming the worktree root stages the whole + # repository from any folder inside it. + with use_working_directory(repo.worktree): + pf = repo.add(".", encoding) else: pf = repo.add(inputs[0], encoding) # oterhwise, use the add_paths method for multiple inputs @@ -423,8 +481,10 @@ def download(repo, files, tsv_names, download_all, filters, fmt, remote_name, """ Download selected files from a configured data remote. - Preview the selection with --dry-run. Every nonempty download displays - its plan and asks for confirmation before transferring files. + FILES are relative to the current folder, which can be any folder inside + the repository. Preview the selection with --dry-run. Every nonempty + download displays its plan and asks for confirmation before transferring + files. """ if download_all and (files or tsv_names): raise ClickException("--all cannot be combined with file paths or --tsv") @@ -432,6 +492,7 @@ def download(repo, files, tsv_names, download_all, filters, fmt, remote_name, raise ClickException("Provide file paths, --tsv, --all, --filter, or --fmt") if repo.worktree is None and output is None: raise ClickException("--output is required when downloading from a bare repo") + files = _repo_relative_paths(repo, files) with _translate_cli_errors(DownloadError, ValueError): plan = repo.plan_download( output, file_paths=files, tsv_names=tsv_names, all_files=download_all, diff --git a/mod/hallmark/repo/__init__.py b/mod/hallmark/repo/__init__.py index b2fdacf..a13a48f 100644 --- a/mod/hallmark/repo/__init__.py +++ b/mod/hallmark/repo/__init__.py @@ -31,7 +31,7 @@ from .manifest import build_file_table, file_versions_by_path, iter_manifest_entries from .history import ( load_head_state) -from ..error import DestinationExistsError +from ..error import DestinationExistsError, DothmError from ..utils import ( FILE_IO_CHUNK_SIZE, use_working_directory, @@ -80,6 +80,56 @@ def resolve_repo_paths(path: Union[Path, str]) -> Tuple[Path, Optional[Path]]: return path, None return path / ".hm", path + @staticmethod + def find_root(start: Union[Path, str] = ".") -> Optional[Path]: + """ + Locate the repository containing a folder without opening it. + + Args: + start (Path | str): Folder to search from. Parent folders are + searched in turn. + + Returns: + Path | None: The worktree, or the bare ``.hm`` repository, of the + nearest enclosing repository; None if there is none. + """ + start = Path(start).expanduser().resolve() + for folder in (start, *start.parents): + # A bare repository is itself the ".hm" folder, as in Repo(path). + if folder.name == ".hm" or ( + folder.suffix == ".hm" and (folder / ".git").exists()): + return folder + # Stop at a damaged ".hm" too, so opening it can explain the damage. + if (folder / ".hm").exists() or (folder / ".hm").is_symlink(): + return folder + return None + + @classmethod + def find(cls, start: Union[Path, str] = ".") -> "Repo": + """ + Open the repository containing a folder, searching parent folders. + + ``Repo(path)`` opens exactly ``path``; ``find`` lets commands run from + any folder inside a worktree. + + Args: + start (Path | str): Folder to search from. Defaults to the current + folder. + + Returns: + Repo: The nearest enclosing repository. + + Raises: + DothmError: If neither the folder nor any parent contains ``.hm``. + """ + root = cls.find_root(start) + if root is None: + raise DothmError( + f'Not a Hallmark repository (or any parent folder): ' + f'"{Path(start).expanduser().resolve()}"; ' + "run hm init or hm clone") + return cls(root) + def __init__(self, path: Union[Path, str]) -> None: ''' Open an existing hallmark repository. diff --git a/test/test_cli.py b/test/test_cli.py index df11e45..afb96e6 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -42,7 +42,7 @@ def _install_repo(monkeypatch, worktree=Path("worktree")): """ - Install a fake hallmark repository for testing, monkeypatching the Repo class + Install a fake hallmark repository for testing, monkeypatching Repo.find to return a SimpleNamespace with the given worktree. Args: monkeypatch: pytest fixture for monkeypatching functions and attributes. @@ -55,7 +55,7 @@ def _install_repo(monkeypatch, worktree=Path("worktree")): worktree=worktree, plan_download=lambda output=None, **kwargs: _download_plan( 0, output or worktree or "downloads")) - monkeypatch.setattr(cli_module, "Repo", lambda path: repo) + monkeypatch.setattr(cli_module, "Repo", SimpleNamespace(find=lambda start: repo)) return repo @@ -102,10 +102,10 @@ def test_group_reports_repository_open_error(monkeypatch): Raises: GitError: Simulated error to test error handling in the CLI. """ - def fail_repo(path): + def fail_repo(start): """ Raise a GitError to simulate a failure to open a hallmark repository.""" raise GitError("not a hallmark repository") - monkeypatch.setattr(cli_module, "Repo", fail_repo) + monkeypatch.setattr(cli_module, "Repo", SimpleNamespace(find=fail_repo)) result = CliRunner().invoke(hallmark, ["info"]) assert result.exit_code != 0, f"Expected non-zero exit code, got {result.exit_code}" @@ -783,8 +783,10 @@ def test_download_cli_reports_empty_selection(monkeypatch): assert "Download these files?" not in result.output -def test_download_cli_passes_selection_and_options_to_downloader(monkeypatch): - repo = _install_repo(monkeypatch) +def test_download_cli_passes_selection_and_options_to_downloader( + monkeypatch, tmp_path): + monkeypatch.chdir(tmp_path) + repo = _install_repo(monkeypatch, worktree=tmp_path) plan = _download_plan(1) captured = {} @@ -987,3 +989,96 @@ def metadata(context, path): assert Repo(destination).state.data["path"].tolist() == ["a.fits"] assert (destination / "a.fits").exists() == (answer == "y\n") assert (result.exit_code == 0) == (answer == "y\n") + + +### repository discovery tests ### + +def test_cli_commands_find_the_repository_from_a_subfolder(monkeypatch, tmp_path): + repo = _local_cli_catalog(tmp_path / "project") + nested = repo.worktree / "sub" / "dir" + nested.mkdir(parents=True) + monkeypatch.chdir(nested) + runner = CliRunner() + + result = runner.invoke(hallmark, ["info"]) + assert result.exit_code == 0, result.output + assert f'dot-hallmark repo: "{repo.dothm.path}"' in result.output + assert f'hallmark worktree: "{repo.worktree}"' in result.output + + result = runner.invoke(hallmark, ["status"]) + assert result.exit_code == 0, result.output + assert "On branch main" in result.output + + result = runner.invoke(hallmark, ["download", "--all", "--dry-run"]) + assert result.exit_code == 0, result.output + assert f"Destination: {repo.worktree}" in result.output + assert "tiny.fits" in result.output + assert not (nested / ".hm").exists() + + +def test_cli_reports_missing_repository(monkeypatch, tmp_path): + monkeypatch.chdir(tmp_path) + result = CliRunner().invoke(hallmark, ["status"]) + assert result.exit_code == 1 + assert "Not a Hallmark repository (or any parent folder)" in result.output + assert not (tmp_path / ".hm").exists() + + +def test_cli_download_paths_are_relative_to_the_current_folder(monkeypatch, tmp_path): + repo = _local_cli_catalog(tmp_path / "project") + (repo.worktree / "sub").mkdir() + monkeypatch.chdir(repo.worktree / "sub") + result = CliRunner().invoke(hallmark, ["download", "../tiny.fits", "--dry-run"]) + assert result.exit_code == 0, result.output + assert "1 file(s)" in result.output + assert " tiny.fits" in result.output + + +@pytest.mark.parametrize("path, message", [ + ("../../outside.fits", "outside the repository"), + ("../.hm/config.yml", "inside .hm"), +]) +def test_cli_download_rejects_paths_outside_the_worktree( + monkeypatch, tmp_path, path, message): + repo = _local_cli_catalog(tmp_path / "project") + (repo.worktree / "sub").mkdir() + monkeypatch.chdir(repo.worktree / "sub") + + def reject_plan(*args, **kwargs): + raise AssertionError("invalid paths must be rejected before planning") + + monkeypatch.setattr(Repo, "plan_download", reject_plan) + result = CliRunner().invoke(hallmark, ["download", path, "--dry-run"]) + assert result.exit_code != 0 + assert message in result.output + + +@pytest.mark.parametrize("folder, argument", [ + ("A", "."), ("A", ".."), ("A", "../"), (".", "./"), ("A", ""), + ("A/deeper", "../.."), +]) +def test_cli_add_dot_from_a_subfolder_stages_the_whole_repository( + monkeypatch, tmp_path, folder, argument): + repo = Repo.init(tmp_path / "project") + for group in ("A", "B"): + (repo.worktree / group).mkdir() + for run in (1, 2): + (repo.worktree / group / f"run{run}.txt").write_text(f"{group}{run}") + repo.add("{group}/run{run:d}.txt") + repo.commit("Record both folders") + changed = repo.worktree / "A" / "run1.txt" + changed.write_text("A1 fixed") + (repo.worktree / folder).mkdir(parents=True, exist_ok=True) + monkeypatch.chdir(repo.worktree / folder) + if argument == "": + argument = str(repo.worktree) + result = CliRunner().invoke(hallmark, ["add", argument]) + assert result.exit_code == 0, result.output + reopened = Repo(repo.worktree) + assert reopened.state.config["data"][0]["fmt"] == "{group}/run{run:d}.txt" + data = reopened.state.data + rows = {(row["group"], row["run"]): row["sha1"] + for row in data.to_dict(orient="records")} + assert sorted(rows) == [("A", "1"), ("A", "2"), ("B", "1"), ("B", "2")] + assert rows[("A", "1")] == Repo.checksum(changed) + assert rows[("B", "2")] == Repo.checksum(repo.worktree / "B" / "run2.txt") diff --git a/test/test_repo_open.py b/test/test_repo_open.py new file mode 100644 index 0000000..3487bba --- /dev/null +++ b/test/test_repo_open.py @@ -0,0 +1,54 @@ +"""Test how existing repositories are located and opened.""" + +from pathlib import Path + +import pytest +from git.exc import GitError + +from hallmark import Repo +from hallmark.error import DothmError + + +def test_find_walks_parent_folders(tmp_path): + repo = Repo.init(tmp_path / "project") + nested = repo.worktree / "sub" / "dir" + nested.mkdir(parents=True) + for start in (repo.worktree, repo.worktree / "sub", nested): + found = Repo.find(start) + assert found.worktree == repo.worktree + assert found.dothm.path == repo.dothm.path + + +def test_find_prefers_the_nearest_repository(tmp_path): + outer = Repo.init(tmp_path / "outer") + inner = Repo.init(outer.worktree / "inner") + (inner.worktree / "data").mkdir() + assert Repo.find(inner.worktree / "data").worktree == inner.worktree + assert Repo.find(outer.worktree).worktree == outer.worktree + + +def test_find_opens_bare_repositories(tmp_path): + bare = Repo.init(tmp_path / "catalog.hm") + found = Repo.find(bare.dothm.path) + assert found.worktree is None + assert found.dothm.path == bare.dothm.path + + +def test_find_explains_missing_repository(tmp_path): + folder = tmp_path / "plain" / "folder" + folder.mkdir(parents=True) + with pytest.raises(DothmError) as error: + Repo.find(folder) + message = str(error.value) + assert "Not a Hallmark repository (or any parent folder)" in message + assert "hm init" in message and "hm clone" in message + assert not (tmp_path / "plain" / ".hm").exists() + + +def test_repo_path_stays_exact(tmp_path): + repo = Repo.init(tmp_path / "project") + (repo.worktree / "sub").mkdir() + with pytest.raises(GitError): + Repo(repo.worktree / "sub") + assert not (repo.worktree / "sub" / ".hm").exists() + assert Path(Repo(repo.worktree).worktree) == repo.worktree From 81e0a22ade7fcff5a935e82cec940b56eb0bb911 Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 20:31:30 -0700 Subject: [PATCH 02/10] Explain damaged repositories and stop without writing Opening a repository whose .hm is not a Git worktree, lacks config.yml, meta.yml or data.tsv, or holds a file that cannot be parsed now raises a DothmError naming the problem instead of a traceback, and leaves every file untouched. Opening no longer creates the objects link of a linked worktree; add_worktree creates it when the worktree is made. Co-Authored-By: Claude Opus 5.5 --- README.rst | 3 +- mod/hallmark/repo/__init__.py | 31 ++++++++----- mod/hallmark/repo/branches.py | 3 ++ mod/hallmark/repo/dothm.py | 35 +++++++++++++-- test/test_repo_open.py | 83 +++++++++++++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 14 deletions(-) diff --git a/README.rst b/README.rst index a1f63a4..74cf1ab 100644 --- a/README.rst +++ b/README.rst @@ -86,7 +86,8 @@ Create a repository, stage a remote catalog, then commit it:: hm download --all ``hm`` is the CLI command; ``hallmark`` remains an alias. Commands work from any -folder inside a repository; ``hm`` finds ``.hm`` in parent folders. +folder inside a repository; ``hm`` finds ``.hm`` in parent folders. If ``.hm`` +is damaged or missing files, commands explain the problem and change nothing. Initialization creates the empty local repository. Remote ``add`` discovers files without downloading dataset contents or committing the catalog. CyVerse, ordinary browsable HTTPS directories, SSH and SFTP use the same workflow. Omit diff --git a/mod/hallmark/repo/__init__.py b/mod/hallmark/repo/__init__.py index a13a48f..4cf7d9f 100644 --- a/mod/hallmark/repo/__init__.py +++ b/mod/hallmark/repo/__init__.py @@ -19,7 +19,7 @@ from dataclasses import dataclass from pathlib import Path from typing import Dict, List, Optional, Tuple, Union -from git.exc import GitCommandError +from git.exc import GitCommandError, InvalidGitRepositoryError, NoSuchPathError from .branches import checkout, add_worktree from ..remote.add import add_remote, is_remote_catalog @@ -140,18 +140,29 @@ def __init__(self, path: Union[Path, str]) -> None: none. ''' dothm_path, worktree_path = self.resolve_repo_paths(path) - self.dothm = Dothm(dothm_path) + # Opening only reads: a damaged repository is explained, not repaired. + try: + self.dothm = Dothm(dothm_path) + except InvalidGitRepositoryError as exc: + raise DothmError( + f'Repository at "{dothm_path}" is damaged: it is not a Git ' + "worktree; nothing was changed") from exc + except NoSuchPathError as exc: + # A missing .hm keeps the GitPython error; a dangling link is damage. + if not Path(dothm_path).is_symlink(): + raise + raise DothmError( + f'Repository at "{dothm_path}" is damaged: it links to a folder ' + "that does not exist; nothing was changed") from exc self.worktree = worktree_path and Worktree(worktree_path) - self.state = self.dothm.load_state() + try: + self.state = self.dothm.load_state() + except DothmError as exc: + raise DothmError(f"{exc}; nothing was changed") from exc self.download_result = None - common = Path(self.dothm.common_dir).resolve().parent - self.objects = Objects(common) - dothm_objects = Path(dothm_path) / "objects" - main_objects = common / "objects" - if dothm_objects.resolve() != main_objects.resolve() \ - and not dothm_objects.exists(): - dothm_objects.symlink_to(main_objects) + # Linked worktrees share the object store of the main ".hm". + self.objects = Objects(Path(self.dothm.common_dir).resolve().parent) def _resolve_worktree_path(self, value, *, label: str = "tracked path") -> Path: """ diff --git a/mod/hallmark/repo/branches.py b/mod/hallmark/repo/branches.py index b94c728..b88abe4 100644 --- a/mod/hallmark/repo/branches.py +++ b/mod/hallmark/repo/branches.py @@ -366,6 +366,9 @@ def add_worktree(repo, target_branch): # iterate over the target state data and restore files from the object store try: + # Point the new ".hm" at the shared object store for tools that read + # objects through a repository path, such as a clone of this worktree. + (target_dothm / "objects").symlink_to(repo.objects.root) for _, row in target_state.data.iterrows(): rel_path = row_to_path(row, target_fmt) # resolve relative path to an absolute path in the target worktree diff --git a/mod/hallmark/repo/dothm.py b/mod/hallmark/repo/dothm.py index 9243189..387c820 100644 --- a/mod/hallmark/repo/dothm.py +++ b/mod/hallmark/repo/dothm.py @@ -29,6 +29,9 @@ replace_file_on_success, load_yaml_file, validate_path_name) from .state import State +# Files that make up the state database of every ``.hm`` repository. +STATE_FILES = ("config.yml", "meta.yml", "data.tsv") + class _HallmarkYamlDumper(yaml.Dumper): """ @@ -196,10 +199,36 @@ def link_worktree(self, path: Union[Path, str], branch: Optional[str] = None): return Dothm(path) def load_state(self) -> State: + """ + Read the state database files. + + Returns: + State: The repository configuration, metadata and catalog. + + Raises: + DothmError: If a state file is missing or cannot be read. The + files are only read, never repaired. + """ + missing = [name for name in STATE_FILES if not (self.path / name).is_file()] + if missing: + raise DothmError( + f'Repository at "{self.path}" is damaged: missing ' + + ", ".join(missing)) + + def read(name, reader): + try: + return reader(Path(name).stem) + except (OSError, ValueError, yaml.YAMLError) as exc: + # ValueError covers pandas parser and Unicode decoding errors. + detail = " ".join(str(exc).split()) + raise DothmError( + f'Repository at "{self.path}" is damaged: cannot read ' + f"{name} ({detail})") from exc + return State( - config = self.read_yaml("config"), - meta = self.read_yaml("meta"), - data = self.read_tsv("data")) + config = read("config.yml", self.read_yaml), + meta = read("meta.yml", self.read_yaml), + data = read("data.tsv", self.read_tsv)) def save_state(self, state: State) -> None: self.write_yaml(state.config, "config") diff --git a/test/test_repo_open.py b/test/test_repo_open.py index 3487bba..c2f045a 100644 --- a/test/test_repo_open.py +++ b/test/test_repo_open.py @@ -52,3 +52,86 @@ def test_repo_path_stays_exact(tmp_path): Repo(repo.worktree / "sub") assert not (repo.worktree / "sub" / ".hm").exists() assert Path(Repo(repo.worktree).worktree) == repo.worktree + + +def _snapshot(root): + """Return the bytes of every file below root, keyed by relative path.""" + return {path.relative_to(root).as_posix(): path.read_bytes() + for path in sorted(Path(root).rglob("*")) if path.is_file()} + + +def _committed_local_repo(path): + repo = Repo.init(path) + (repo.worktree / "run1.txt").write_text("one") + repo.add("run{run:d}.txt") + repo.commit("Record run") + return repo + + +DAMAGE = { + "missing data.tsv": (lambda hm: (hm / "data.tsv").unlink(), "data.tsv"), + "missing config.yml": (lambda hm: (hm / "config.yml").unlink(), "config.yml"), + "missing meta.yml": (lambda hm: (hm / "meta.yml").unlink(), "meta.yml"), + "invalid config.yml": ( + lambda hm: (hm / "config.yml").write_text("data: [\n"), "config.yml"), + "meta.yml list": (lambda hm: (hm / "meta.yml").write_text("- a\n"), "meta.yml"), + "ragged data.tsv": ( + lambda hm: (hm / "data.tsv").write_text("sha1\trun\nx\t1\na\tb\tc\n"), + "data.tsv"), + "binary data.tsv": ( + lambda hm: (hm / "data.tsv").write_bytes(b"sha1\n\xff\xfe\n"), "data.tsv"), + "empty data.tsv": (lambda hm: (hm / "data.tsv").write_bytes(b""), "data.tsv"), + "not a Git worktree": ( + lambda hm: __import__("shutil").rmtree(hm / ".git"), "Git"), + "dangling .hm link": ( + lambda hm: (__import__("shutil").rmtree(hm), + hm.symlink_to(hm.parent / "moved-away")), + "links to a folder that does not exist"), +} + + +@pytest.mark.parametrize("damage", sorted(DAMAGE)) +def test_damaged_repository_is_explained_and_left_unchanged(tmp_path, damage): + repo = _committed_local_repo(tmp_path / "project") + apply_damage, detail = DAMAGE[damage] + apply_damage(repo.dothm.path) + before = _snapshot(repo.worktree) + with pytest.raises(DothmError) as error: + Repo(repo.worktree) + message = str(error.value) + assert "is damaged" in message + assert detail in message + assert "nothing was changed" in message + assert _snapshot(repo.worktree) == before + + +@pytest.mark.parametrize("damage", [ + "missing data.tsv", "invalid config.yml", "dangling .hm link"]) +def test_cli_status_stops_on_damaged_repository(tmp_path, monkeypatch, damage): + from click.testing import CliRunner + from hallmark.cli import hallmark + + repo = _committed_local_repo(tmp_path / "project") + DAMAGE[damage][0](repo.dothm.path) + (repo.worktree / "sub").mkdir() + monkeypatch.chdir(repo.worktree / "sub") + before = _snapshot(repo.worktree) + result = CliRunner().invoke(hallmark, ["status"]) + assert result.exit_code == 1 + assert "is damaged" in result.output + assert DAMAGE[damage][1] in result.output + assert "Traceback" not in result.output + assert _snapshot(repo.worktree) == before + + +def test_opening_a_linked_worktree_writes_nothing(tmp_path): + repo = _committed_local_repo(tmp_path / "project") + repo.add_worktree("experiment") + linked = tmp_path / "experiment" + objects = linked / ".hm" / "objects" + assert objects.is_symlink() + assert objects.resolve() == (repo.dothm.path / "objects").resolve() + objects.unlink() + opened = Repo(linked) + assert not objects.exists() and not objects.is_symlink() + assert opened.objects.contains(opened.state.data.iloc[0]["sha1"]) From e4e83768d5e0c9f92ea1ed06be473143f49ec5d9 Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 20:33:52 -0700 Subject: [PATCH 03/10] Keep credentials out of remote catalogue URLs Remote add and set-config --remote-url now refuse URLs carrying a password, an HTTP(S) username or token, a query string or a fragment before any network access, so no secret reaches config.yml. Clone refuses a source URL with a password or an HTTP(S) username before creating anything, since Git would save it in .hm/.git/config. The errors do not repeat the URL. SSH usernames remain allowed. The private data guide says where credentials belong instead. Co-Authored-By: Claude Opus 5.5 --- doc/private_data.rst | 11 ++++ mod/hallmark/remote/add.py | 3 +- mod/hallmark/remote/clone.py | 8 ++- mod/hallmark/repo/config.py | 2 + mod/hallmark/transport/base.py | 39 ++++++++++++ test/test_remote_add.py | 106 +++++++++++++++++++++++++++++++++ 6 files changed, 166 insertions(+), 3 deletions(-) diff --git a/doc/private_data.rst b/doc/private_data.rst index ecfdf6d..62b2c4e 100644 --- a/doc/private_data.rst +++ b/doc/private_data.rst @@ -104,6 +104,17 @@ nonsecret backend settings. These settings are saved with the data remote; credentials belong in local authentication configuration. See :doc:`backends` for the plugin interface. +Keep passwords and keys outside ``.hm``. ``add URL`` and +``set-config --remote-url`` reject URLs containing a password, an HTTP(S) +username or token, a query string or a fragment, before contacting the server +and without repeating the URL. Store SSH users and keys in ``~/.ssh/config`` or +a local profile in ``~/.config/hallmark/auth.yml`` (see section 4), and +HTTP(S) logins in ``~/.netrc``. An SSH username such as +``ssh://researcher@lab-data/srv/exports/lab/`` is not a secret and is allowed. +``clone`` likewise refuses a source URL with a password or an HTTP(S) username +or token, before creating anything, because Git would save it in +``.hm/.git/config``; use a Git credential helper or ``~/.netrc`` instead. + 3. Preview, approve and download -------------------------------- diff --git a/mod/hallmark/remote/add.py b/mod/hallmark/remote/add.py index 4df2a5a..5fa5f50 100644 --- a/mod/hallmark/remote/add.py +++ b/mod/hallmark/remote/add.py @@ -8,7 +8,7 @@ from ..utils import as_list_of_dicts from ..repo.config import normalize_remotes, validate_tsv_filename from ..transport import OperationContext, RemoteSpec -from ..transport.base import copy_backend_options +from ..transport.base import copy_backend_options, reject_url_secrets def is_remote_catalog(state): @@ -19,6 +19,7 @@ def split_remote_pattern(value, fmt=None): parts = urlsplit(value) if parts.scheme not in {"https", "http", "ssh", "sftp"}: raise ValueError("Remote add requires an HTTP(S), SSH or SFTP URL") + reject_url_secrets(value) path = parts.path if "{" in path: if fmt is not None: diff --git a/mod/hallmark/remote/clone.py b/mod/hallmark/remote/clone.py index 085dcd7..35bcabb 100644 --- a/mod/hallmark/remote/clone.py +++ b/mod/hallmark/remote/clone.py @@ -23,6 +23,7 @@ from ..transport import OperationContext, RemoteSpec from ..transport.base import RemoteObjectMissing, validate_remote_path from ..repo.worktree import Worktree +from ..transport.base import reject_url_credentials def _catalog_filenames(config): @@ -169,8 +170,9 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): """ Clone an existing Git catalog or published HTTP/SFTP catalog snapshot. - Existing destinations are rejected before source access. An incomplete - destination created by this call is removed on failure. Dataset discovery + Source URLs with credentials and existing destinations are rejected + before source access. An incomplete destination created by this call is + removed on failure. Dataset discovery belongs to ``Repo.add(URL)``. Args: @@ -191,6 +193,8 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): """ if source_type not in {"auto", "git", "catalog"}: raise ValueError("source_type must be auto, git, or catalog") + # Git records the source URL in .hm/.git/config, so it must hold no secret. + reject_url_credentials(str(url)) url = str(url) destination = Path(path).expanduser().absolute() if destination.exists() or destination.is_symlink(): diff --git a/mod/hallmark/repo/config.py b/mod/hallmark/repo/config.py index 31694bb..8338f7f 100644 --- a/mod/hallmark/repo/config.py +++ b/mod/hallmark/repo/config.py @@ -21,6 +21,7 @@ validate_backend_name, validate_profile_name, reject_control_characters, + reject_url_secrets, copy_backend_options, ) @@ -399,6 +400,7 @@ def set_config( if isinstance(remote_url, str): reject_control_characters(remote_url, "Remote URL") remote_url = require_nonempty_string(remote_url, label="remote_url") + reject_url_secrets(remote_url) if remote_auth not in (None, ""): validate_profile_name(remote_auth) diff --git a/mod/hallmark/transport/base.py b/mod/hallmark/transport/base.py index 58df83f..b88549c 100644 --- a/mod/hallmark/transport/base.py +++ b/mod/hallmark/transport/base.py @@ -95,6 +95,45 @@ def reject_control_characters(value: str, label: str) -> None: raise RemoteConfigurationError(f"{label} contains control characters") +def reject_url_credentials(url: str) -> None: + """ + Refuse a URL carrying a password, or an HTTP(S) username or token. + + SSH usernames are allowed. Error messages never repeat the URL. + + Raises: + RemoteConfigurationError: If the URL contains credentials. + """ + try: + parsed = urlsplit(url) + except ValueError: + raise RemoteConfigurationError("Invalid remote URL") from None + if parsed.password is not None or ( + parsed.username is not None + and parsed.scheme.lower() in {"http", "https"}): + raise RemoteConfigurationError( + "URLs must not contain credentials; configure them locally, for " + "example in ~/.netrc, ~/.ssh/config or a Git credential helper") + + +def reject_url_secrets(url: str) -> None: + """ + Refuse a data-remote URL that would store a secret in the catalog. + + Passwords, HTTP(S) usernames (often access tokens), query strings and + fragments are rejected; SSH usernames are allowed. Error messages never + repeat the URL. + + Raises: + RemoteConfigurationError: If the URL could contain a secret. + """ + reject_url_credentials(url) + if "?" in url or "#" in url: + raise RemoteConfigurationError( + "Remote URLs must not contain a query or fragment; " + "percent-encode ? and # in file names") + + def validate_remote_path(value) -> Path: """Validate a literal catalog path without decoding or trimming it.""" raw = str(value) diff --git a/test/test_remote_add.py b/test/test_remote_add.py index 3e26c3d..dfb1392 100644 --- a/test/test_remote_add.py +++ b/test/test_remote_add.py @@ -223,3 +223,109 @@ def test_removed_cli_interfaces_fail_clearly(monkeypatch, tmp_path, arguments): assert result.exit_code != 0 assert "No such" in result.output assert Repo(repo.worktree).state.data.empty + + +def _state_bytes(repo): + return {name: (repo.dothm.path / name).read_bytes() + for name in ("config.yml", "meta.yml", "data.tsv", ".git/index")} + + +@pytest.mark.parametrize("url, message", [ + ("https://user:secret@example.test/data/{a}.h5", "credentials"), + ("https://secret@example.test/data/{a}.h5", "credentials"), + ("http://secret@example.test/data/", "credentials"), + ("sftp://user:secret@example.test/data/", "credentials"), + ("https://example.test/data/{a}.h5?sig=secret", "query or fragment"), + ("https://example.test/data/?secret", "query or fragment"), + ("https://example.test/data/{a}.h5#secret", "query or fragment"), +]) +def test_remote_add_rejects_secrets_in_urls_before_network( + tmp_path, remote_listing, url, message): + repo = Repo.init(tmp_path / "data") + before = _state_bytes(repo) + with pytest.raises(ValueError, match=message) as error: + repo.add(url) + assert "secret" not in str(error.value) + assert remote_listing[1] == [] + assert _state_bytes(repo) == before + + +def test_remote_pattern_keeps_ssh_usernames(): + from hallmark.remote.add import split_remote_pattern + + assert split_remote_pattern("ssh://researcher@campus/srv/data/{run}.h5") == ( + "ssh://researcher@campus/srv/data/", "{run}.h5") + + +@pytest.mark.parametrize("url", [ + "https://user:secret@example.test/data/", "https://secret@example.test/data/", + "https://example.test/data/?token=secret", +]) +def test_set_config_rejects_secrets_in_remote_urls(tmp_path, url): + repo = Repo.init(tmp_path / "data") + before = _state_bytes(repo) + with pytest.raises(ValueError) as error: + repo.set_config(remote_url=url) + assert "secret" not in str(error.value) + assert _state_bytes(repo) == before + assert Repo(repo.worktree).state.config == repo.state.config + + +@pytest.mark.parametrize("arguments", [ + ["add", "https://user:hunter2@archive.test/ER2/{a}.h5"], + ["add", "https://hunter2@archive.test/ER2/"], + ["set-config", "--remote-url", "https://archive.test/ER2/?sig=hunter2"], +]) +def test_cli_rejects_url_secrets_without_echoing_them( + tmp_path, monkeypatch, remote_listing, arguments): + repo = Repo.init(tmp_path / "repo") + monkeypatch.chdir(repo.worktree) + before = _state_bytes(repo) + result = CliRunner().invoke(hallmark, arguments) + assert result.exit_code != 0 + assert "Error:" in result.output + assert "hunter2" not in result.output + assert remote_listing[1] == [] + assert _state_bytes(repo) == before + + +@pytest.mark.parametrize("url", [ + "http://user:SECRET@git.example.test/src.git", + "https://SECRET@git.example.test/src.git", + "ssh://git:SECRET@git.example.test/src.git", + "https://SECRET@catalogs.example.test/published/", +]) +def test_clone_rejects_credentials_before_creating_anything( + tmp_path, monkeypatch, url): + def reject(*args, **kwargs): + raise AssertionError("a source with credentials must not be contacted") + + monkeypatch.setattr("hallmark.remote.clone.Dothm.clone", reject) + monkeypatch.setattr(OperationContext, "read_text", reject) + target = tmp_path / "copy" + with pytest.raises(ValueError, match="credentials") as error: + Repo.clone(url, target, download=False) + assert "SECRET" not in str(error.value) + assert not target.exists() + result = CliRunner().invoke(hallmark, [ + "clone", url, str(target), "--no-download"]) + assert result.exit_code != 0 + assert "credentials" in result.output + assert "SECRET" not in result.output + assert not target.exists() + + +def test_clone_keeps_ssh_usernames(tmp_path, monkeypatch): + from hallmark.error import CloneError + + calls = [] + + def stop(url, *args, **kwargs): + calls.append(url) + raise CloneError("stop after the URL check") + + monkeypatch.setattr("hallmark.remote.clone.Dothm.clone", stop) + url = "ssh://git@git.example.test/team/src.git" + with pytest.raises(CloneError, match="stop after the URL check"): + Repo.clone(url, tmp_path / "copy", download=False) + assert calls == [url] From 119d25dc3253dc5bde77e64997d9415e2f1b8037 Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 20:37:35 -0700 Subject: [PATCH 04/10] Report remote scans that match nothing and keep the previous catalogue A remote add whose URL, filter and pattern match no files now fails with an error naming the selection and source instead of succeeding silently. Connection and access failures during the scan are reported as a failed scan. In both cases nothing is staged and the previous catalogue stays as it was. Co-Authored-By: Claude Opus 5.5 --- doc/private_data.rst | 5 +++++ mod/hallmark/cli.py | 3 ++- mod/hallmark/remote/add.py | 38 +++++++++++++++++++++++++++-------- test/test_backend_config.py | 2 +- test/test_catalog.py | 12 +++++++++-- test/test_remote_add.py | 40 ++++++++++++++++++++++++++++++++++++- 6 files changed, 87 insertions(+), 13 deletions(-) diff --git a/doc/private_data.rst b/doc/private_data.rst index 62b2c4e..32a0e9f 100644 --- a/doc/private_data.rst +++ b/doc/private_data.rst @@ -83,6 +83,11 @@ directories needed to find matching files. Discovery shows an indeterminate progress bar with counts of completed directories and discovered files. The percentage and completion time remain unknown until a total is available. +A scan stages its catalog only after it completes. If the connection or access +fails, or no file matches the URL, filter and filename format, ``add`` explains +the problem, exits with an error and keeps the previously staged catalog +unchanged; it never stages a partial result. + Both URL schemes use structured SFTP directory enumeration and file transfers. SFTP-only accounts work for discovery as well as downloading. No ``--allow-remote-commands`` or ``--remote-hash`` option is needed. Discovery diff --git a/mod/hallmark/cli.py b/mod/hallmark/cli.py index 82d38ed..d6946e0 100644 --- a/mod/hallmark/cli.py +++ b/mod/hallmark/cli.py @@ -305,7 +305,8 @@ def add(repo, encoding, inputs, auth, backend, backend_options, filters, remote_ """Add files to the hallmark index. A remote URL or URL pattern stages a catalog without downloading files - or committing. Use --fmt and --filter to select remote paths. + or committing. Use --fmt and --filter to select remote paths. A scan that + fails or matches no files keeps the previously staged catalog. `hm add [--regex] FORMAT` uses the branch format string workflow. `hm add "."` rebuilds the manifest from current files that match diff --git a/mod/hallmark/remote/add.py b/mod/hallmark/remote/add.py index 5fa5f50..f52a096 100644 --- a/mod/hallmark/remote/add.py +++ b/mod/hallmark/remote/add.py @@ -8,7 +8,9 @@ from ..utils import as_list_of_dicts from ..repo.config import normalize_remotes, validate_tsv_filename from ..transport import OperationContext, RemoteSpec -from ..transport.base import copy_backend_options, reject_url_secrets +from ..transport.base import ( + DownloadError, RemoteConfigurationError, copy_backend_options, + reject_url_secrets) def is_remote_catalog(state): @@ -32,6 +34,17 @@ def split_remote_pattern(value, fmt=None): return urlunsplit(parts._replace(path=path)), fmt +def _describe_selection(fmt, filter): + """Describe a remote selection for messages about the files it matched.""" + parts = [] + if fmt: + parts.append(f"pattern {fmt!r}") + if filter: + patterns = [filter] if isinstance(filter, str) else list(filter) + parts.append("filter " + ", ".join(repr(pattern) for pattern in patterns)) + return " and ".join(parts) or "any file" + + def add_remote(repo, value, *, fmt=None, filter=None, auth=None, backend=None, backend_options=None, progress=False): url, fmt = split_remote_pattern(value, fmt) @@ -62,10 +75,21 @@ def add_remote(repo, value, *, fmt=None, filter=None, auth=None, backend=None, or remotes[0].get("url") != source.url): raise ValueError( "Remote add currently supports one dataset root per repository") - with OperationContext(source) as context: - entries = discover_remote_files( - context, filter=filter, fmt=fmt, progress=progress - ) + try: + with OperationContext(source) as context: + entries = discover_remote_files( + context, filter=filter, fmt=fmt, progress=progress + ) + except RemoteConfigurationError: + raise + except DownloadError as exc: + # Nothing is saved until the scan completes, so the catalogue is intact. + raise DownloadError( + f"Remote scan failed (previous catalogue kept): {exc}") from exc + if not entries: + raise ValueError( + f"No remote files matched {_describe_selection(fmt, filter)} under " + f"{url}; previous catalogue kept") columns = ["path", "checksum_algorithm", "checksum", "size_bytes", "mtime"] rows = [] for entry in entries: @@ -76,9 +100,7 @@ def add_remote(repo, value, *, fmt=None, filter=None, auth=None, backend=None, row.update({key: value for key, value in matched.named.items() if key not in {*columns, "sha1"}}) rows.append(row) - frame = pd.DataFrame(rows) if rows else pd.DataFrame(columns=columns) - if frame.empty: - return frame + frame = pd.DataFrame(rows) state = deepcopy(repo.state) if not existing.empty: frame = pd.concat([existing, frame], ignore_index=True, sort=False) diff --git a/test/test_backend_config.py b/test/test_backend_config.py index f1c8029..5708189 100644 --- a/test/test_backend_config.py +++ b/test/test_backend_config.py @@ -54,7 +54,7 @@ def test_add_passes_backend_settings_to_discovery(monkeypatch, tmp_path): def discover_remote_files(context, **kwargs): captured.append(context.remote) - return [] + return [RemoteEntry("tile.fits")] # No backend installation or network is required to inspect this hand-off. class Context: diff --git a/test/test_catalog.py b/test/test_catalog.py index 61bd5e8..d5bd592 100644 --- a/test/test_catalog.py +++ b/test/test_catalog.py @@ -59,12 +59,20 @@ def test_remote_add_does_not_require_filename_inference(tmp_path, metadata_serve assert repo.plan_download().file_count == 1 -def test_empty_filtered_add_is_valid(tmp_path, metadata_server): +def test_empty_filtered_add_is_an_error_and_keeps_catalog(tmp_path, metadata_server): pages, _ = metadata_server root = "https://example.test/data/" pages[root] = '

Index of data

notes.txt' repo = Repo.init(tmp_path / "clone") - repo.add(root, remote_fmt="run{run:d}.h5") + before = {name: (repo.dothm.path / name).read_bytes() + for name in ("config.yml", "data.tsv", ".git/index")} + with pytest.raises(ValueError) as error: + repo.add(root, remote_fmt="run{run:d}.h5") + message = str(error.value) + assert "No remote files matched" in message + assert "run{run:d}.h5" in message and root in message + assert "previous catalogue kept" in message + assert before == {name: (repo.dothm.path / name).read_bytes() for name in before} assert repo.state.data.empty assert repo.plan_download().file_count == 0 diff --git a/test/test_remote_add.py b/test/test_remote_add.py index dfb1392..e9f4c98 100644 --- a/test/test_remote_add.py +++ b/test/test_remote_add.py @@ -59,8 +59,10 @@ def test_remote_add_merges_paths_and_leaves_unmatched_rows(tmp_path, remote_list repo.commit("Second selection") repo.add(root, filter="*002.fits") assert repo.dothm.index.diff("HEAD") == [] - repo.add(root, filter="absent*") + with pytest.raises(ValueError, match="No remote files matched"): + repo.add(root, filter="absent*") assert repo.dothm.index.diff("HEAD") == [] + assert repo.state.data.path.tolist() == ["M87_001.fits", "M87_002.fits"] def test_remote_catalog_clone_and_branches_keep_payloads_separate( @@ -329,3 +331,39 @@ def stop(url, *args, **kwargs): with pytest.raises(CloneError, match="stop after the URL check"): Repo.clone(url, tmp_path / "copy", download=False) assert calls == [url] + + +def test_cli_remote_add_matching_nothing_keeps_previous_catalog( + tmp_path, monkeypatch, remote_listing): + repo = Repo.init(tmp_path / "repo") + repo.add("https://example.test/data/", filter="*.fits") + monkeypatch.chdir(repo.worktree) + before = _state_bytes(repo) + result = CliRunner().invoke(hallmark, [ + "add", "https://example.test/data/", "--filter", "absent*"]) + assert result.exit_code == 1 + assert "No remote files matched" in result.output + assert "absent*" in result.output + assert "previous catalogue kept" in result.output + assert _state_bytes(repo) == before + + +def test_failed_remote_scan_is_explained_and_keeps_previous_catalog( + tmp_path, monkeypatch, remote_listing): + from hallmark.transport.base import DownloadError + + repo = Repo.init(tmp_path / "repo") + repo.add("https://example.test/data/", filter="*.fits") + before = _state_bytes(repo) + + def refuse(*args, **kwargs): + raise DownloadError("Cannot establish connection to https://example.test") + + monkeypatch.setattr("hallmark.remote.add.discover_remote_files", refuse) + with pytest.raises(DownloadError) as error: + repo.add("https://example.test/data/") + assert str(error.value) == ( + "Remote scan failed (previous catalogue kept): " + "Cannot establish connection to https://example.test") + assert _state_bytes(repo) == before + assert repo.state.data.path.tolist() == ["M87_001.fits", "M87_002.fits"] From e43f21611f11ea7d65a06f4a7ecd68783cea175d Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 20:42:39 -0700 Subject: [PATCH 05/10] Refuse remote patterns that would drop catalogued files A branch keeps one filename pattern. Remote add now refuses, before contacting the server, a new pattern that some catalogued file does not fit, listing a few of them; there is no force option. A broader pattern that fits every file replaces the old one and re-derives the fields of kept rows. Adding the bare URL again rescans with the saved pattern instead of dropping it. Co-Authored-By: Claude Opus 5.5 --- doc/private_data.rst | 8 +++-- mod/hallmark/remote/add.py | 41 ++++++++++++++++++---- mod/hallmark/repo/config.py | 48 ++++++++++++++++++++++++++ test/test_remote_add.py | 69 +++++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+), 9 deletions(-) diff --git a/doc/private_data.rst b/doc/private_data.rst index 32a0e9f..03f0cd9 100644 --- a/doc/private_data.rst +++ b/doc/private_data.rst @@ -66,8 +66,12 @@ Suppose the export contains ``runs/run_001.h5``, ``runs/run_002.h5`` and This creates ``./lab/.hm`` and catalogs only the matching run files. The URL is the exact discovery root. A filename format selects paths and extracts -parameters; it does not approve a download. A glob filter can select paths -without defining parameters: +parameters; it does not approve a download. Each branch has one filename +format: adding the bare URL again rescans with the saved format, and a new +format is refused, before the server is contacted, if any catalogued file does +not fit it. There is no option to force it; a broader format that fits every +catalogued file replaces the old one. A glob filter can select paths without +defining parameters: .. code-block:: bash diff --git a/mod/hallmark/remote/add.py b/mod/hallmark/remote/add.py index f52a096..19417a3 100644 --- a/mod/hallmark/remote/add.py +++ b/mod/hallmark/remote/add.py @@ -6,7 +6,9 @@ from .discovery import discover_remote_files, path_matches from ..utils import as_list_of_dicts -from ..repo.config import normalize_remotes, validate_tsv_filename +from ..repo.config import ( + check_pattern_keeps_catalogue, filename_fields, normalize_remotes, + single_data_format, validate_tsv_filename) from ..transport import OperationContext, RemoteSpec from ..transport.base import ( DownloadError, RemoteConfigurationError, copy_backend_options, @@ -34,6 +36,26 @@ def split_remote_pattern(value, fmt=None): return urlunsplit(parts._replace(path=path)), fmt +CATALOG_COLUMNS = ("path", "checksum_algorithm", "checksum", "size_bytes", "mtime") + + +def _pattern_fields(fmt, path): + """Return the filename fields that a matching pattern extracts from a path.""" + matched = parse.parse(fmt, path, case_sensitive=True) + return {key: value for key, value in matched.named.items() + if key not in {*CATALOG_COLUMNS, "sha1"}} + + +def _replace_pattern_fields(frame, fmt, old_fmt): + """Swap the old pattern's field columns for those of a new pattern.""" + fields = pd.DataFrame([_pattern_fields(fmt, path) for path in frame["path"]], + index=frame.index) + old_fields = set(filename_fields(old_fmt)) if old_fmt else set() + replaced = ((old_fields | set(fields.columns)) + & (set(frame.columns) - set(CATALOG_COLUMNS))) + return frame.drop(columns=sorted(replaced)).join(fields) + + def _describe_selection(fmt, filter): """Describe a remote selection for messages about the files it matched.""" parts = [] @@ -75,6 +97,12 @@ def add_remote(repo, value, *, fmt=None, filter=None, auth=None, backend=None, or remotes[0].get("url") != source.url): raise ValueError( "Remote add currently supports one dataset root per repository") + # A branch has one pattern: a bare URL rescans with the saved pattern, and + # a new pattern must fit every catalogued file before the server is read. + saved_fmt = single_data_format(repo.state.config) + if fmt is None: + fmt = saved_fmt + check_pattern_keeps_catalogue(repo.state, fmt) try: with OperationContext(source) as context: entries = discover_remote_files( @@ -90,19 +118,18 @@ def add_remote(repo, value, *, fmt=None, filter=None, auth=None, backend=None, raise ValueError( f"No remote files matched {_describe_selection(fmt, filter)} under " f"{url}; previous catalogue kept") - columns = ["path", "checksum_algorithm", "checksum", "size_bytes", "mtime"] rows = [] for entry in entries: - row = dict(zip(columns, (entry.path, entry.checksum_algorithm, - entry.checksum, entry.size, entry.mtime))) + row = dict(zip(CATALOG_COLUMNS, (entry.path, entry.checksum_algorithm, + entry.checksum, entry.size, entry.mtime))) if fmt: - matched = parse.parse(fmt, entry.path, case_sensitive=True) - row.update({key: value for key, value in matched.named.items() - if key not in {*columns, "sha1"}}) + row.update(_pattern_fields(fmt, entry.path)) rows.append(row) frame = pd.DataFrame(rows) state = deepcopy(repo.state) if not existing.empty: + if fmt != saved_fmt: + existing = _replace_pattern_fields(existing, fmt, saved_fmt) frame = pd.concat([existing, frame], ignore_index=True, sort=False) frame = frame.drop_duplicates(subset=["path"], keep="last") state.data = frame.fillna("").sort_values("path").reset_index(drop=True) diff --git a/mod/hallmark/repo/config.py b/mod/hallmark/repo/config.py index 8338f7f..fcf134e 100644 --- a/mod/hallmark/repo/config.py +++ b/mod/hallmark/repo/config.py @@ -12,6 +12,8 @@ from string import Formatter from typing import Dict, Optional +import parse + from ..utils import ( convert_format_value, require_nonempty_string, validate_path_name, validate_relative_path) @@ -486,6 +488,52 @@ def filename_fields(fmt: str) -> list[str]: return fields +def _catalogued_paths(state) -> list[str]: + """ + Used by check_pattern_keeps_catalogue. + Return the relative path of every file in a state's catalog. + """ + if "path" in state.data.columns: + return [str(path) for path in state.data["path"]] + fmt = single_data_format(state.config) + if fmt is None: + return [] + return [row_to_path(row, fmt).as_posix() + for row in state.data.to_dict(orient="records")] + + +def check_pattern_keeps_catalogue(state, new_fmt: Optional[str]) -> None: + """ + Refuse a filename pattern that files in the catalog do not fit. + + A branch has one pattern, so replacing it must not drop catalogued files. + Only local state is read. + + Args: + state (State): Staged repository state. + new_fmt (str, optional): Proposed filename pattern; None keeps none. + + Raises: + ValueError: If any catalogued path does not match ``new_fmt``. The + message lists a few of them; there is no force option. + """ + if new_fmt is None or state.data.empty: + return + parser = parse.compile(new_fmt, case_sensitive=True) + misfits = [path for path in _catalogued_paths(state) + if parser.parse(path) is None] + if not misfits: + return + shown = ", ".join(misfits[:3]) + if len(misfits) > 3: + shown += f" and {len(misfits) - 3} more" + raise ValueError( + f"Pattern {new_fmt!r} does not fit {len(misfits)} catalogued file(s) " + f"({shown}). A branch has one pattern and a new one cannot drop " + "catalogued files; use a pattern that fits them too (there is no " + "force option)") + + def validate_tsv_filename(value) -> str: """ Validate a TSV database name and add the .tsv suffix when necessary. diff --git a/test/test_remote_add.py b/test/test_remote_add.py index e9f4c98..27e5c97 100644 --- a/test/test_remote_add.py +++ b/test/test_remote_add.py @@ -367,3 +367,72 @@ def refuse(*args, **kwargs): "Cannot establish connection to https://example.test") assert _state_bytes(repo) == before assert repo.state.data.path.tolist() == ["M87_001.fits", "M87_002.fits"] + + +def test_new_pattern_that_drops_catalogued_files_is_refused_offline( + tmp_path, remote_listing): + repo = Repo.init(tmp_path / "data") + root = "https://example.test/data/" + repo.add(root + "{src}_{day:03d}.fits") + repo.commit("Record fits files") + remote_listing[1].clear() + before = _state_bytes(repo) + with pytest.raises(ValueError) as error: + repo.add(root + "run{run:d}.fits") + message = str(error.value) + assert "M87_001.fits" in message and "M87_002.fits" in message + assert "no force option" in message + assert remote_listing[1] == [] + assert _state_bytes(repo) == before + assert repo.state.config["data"] == [{"db": "data.tsv", + "fmt": "{src}_{day:03d}.fits"}] + + +def test_broader_pattern_is_accepted_and_kept_rows_keep_their_information( + tmp_path, remote_listing): + pages, _ = remote_listing + digest = "a" * 64 + pages[""] += 'SHA256SUMS' + pages["SHA256SUMS"] = f"{digest} M87_001.fits\n" + repo = Repo.init(tmp_path / "data") + root = "https://example.test/data/" + repo.add(root + "M87_{day:03d}.fits", filter="*001*") + repo.commit("Record the first file") + # The first file disappears from the server before the broader scan. + pages[""] = pages[""].replace( + 'M87_001.fits', "") + repo.add(root + "{src}_{day:03d}.fits") + assert repo.state.config["data"] == [{"db": "data.tsv", + "fmt": "{src}_{day:03d}.fits"}] + rows = repo.state.data.set_index("path") + assert rows.index.tolist() == ["M87_001.fits", "M87_002.fits"] + assert rows.loc["M87_001.fits", "checksum"] == digest + assert rows.loc["M87_001.fits", "checksum_algorithm"] == "sha256" + assert rows.loc["M87_001.fits", "src"] == "M87" + assert rows.loc["M87_002.fits", "src"] == "M87" + assert rows["day"].tolist() == ["1", "2"] + + +def test_bare_url_reuses_the_saved_pattern(tmp_path, remote_listing): + repo = Repo.init(tmp_path / "data") + root = "https://example.test/data/" + repo.add(root + "{src}_{day:03d}.fits") + repo.commit("Record fits files") + repo.add(root) + assert repo.state.config["data"] == [{"db": "data.tsv", + "fmt": "{src}_{day:03d}.fits"}] + assert repo.state.data.path.tolist() == ["M87_001.fits", "M87_002.fits"] + assert repo.dothm.index.diff("HEAD") == [] + + +def test_pattern_refusal_lists_only_a_few_files(): + import pandas as pd + from hallmark.repo.config import check_pattern_keeps_catalogue + from hallmark.repo.state import State + + state = State(data=pd.DataFrame({"path": [f"f{i}.txt" for i in range(5)]})) + with pytest.raises(ValueError, match=r"5 catalogued file\(s\) " + r"\(f0.txt, f1.txt, f2.txt and 2 more\)"): + check_pattern_keeps_catalogue(state, "{name}.fits") + check_pattern_keeps_catalogue(state, "{name}.txt") + check_pattern_keeps_catalogue(state, None) From 9fac9915850565ad7ebdc4a0f61b7e7096d38a74 Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 20:49:11 -0700 Subject: [PATCH 06/10] Select downloads by catalogued paths and folders, rejecting unknown ones offline A download path may now name a catalogued file or a folder, which selects every catalogued file below it. Paths that match no catalogued file are recorded in DownloadPlan.unknown_paths: --dry-run lists them, and the CLI and execute_download_plan refuse such a plan before contacting the server instead of fetching unlisted files without checksums. The CLI requires paths, --tsv or --all; --filter and --fmt only narrow that selection and show usage on their own. Co-Authored-By: Claude Opus 5.5 --- README.rst | 3 ++ demo/demo_cli.ipynb | 9 ++-- doc/api.rst | 13 +++--- doc/private_data.rst | 30 ++++++++----- doc/usecase.rst | 2 +- mod/hallmark/cli.py | 49 +++++++++++++++------ mod/hallmark/remote/download.py | 63 ++++++++++++++++++++++----- mod/hallmark/remote/plan.py | 15 +++++-- test/test_cli.py | 76 ++++++++++++++++++++++++++++++--- test/test_download_plan.py | 62 ++++++++++++++++++++++++++- test/test_downloader.py | 5 ++- 11 files changed, 269 insertions(+), 58 deletions(-) diff --git a/README.rst b/README.rst index 74cf1ab..56de15d 100644 --- a/README.rst +++ b/README.rst @@ -93,6 +93,9 @@ files without downloading dataset contents or committing the catalog. CyVerse, ordinary browsable HTTPS directories, SSH and SFTP use the same workflow. Omit the filter to catalog all discoverable files beneath the supplied URL. +``hm download PATH...`` selects catalogued files or folders instead of +``--all``; ``--filter`` and ``--fmt`` only narrow a selection, and paths that +are not in the catalog stop the download before the server is contacted. Every nonempty CLI transfer asks for confirmation. For separate Python downloads, callers can inspect ``repo.plan_download()`` and then execute ``repo.download(plan, approved=True)``. diff --git a/demo/demo_cli.ipynb b/demo/demo_cli.ipynb index 4c2107b..2d866e6 100644 --- a/demo/demo_cli.ipynb +++ b/demo/demo_cli.ipynb @@ -438,7 +438,7 @@ "source": [ "### 2.4 Download some files\n", "\n", - "`--filter` picks files by name. `*` matches any characters.\n", + "`--all` selects every file in the catalog, and `--filter` keeps only the names that match. `*` matches any characters.\n", "Hallmark shows the plan and asks `[y/N]`. Here we answer `y` with `echo y`.\n", "Each file is checked against its checksum." ] @@ -450,7 +450,7 @@ "metadata": {}, "outputs": [], "source": [ - "echo y | hm download --filter \"SgrA_*\" 2>/dev/null\n", + "echo y | hm download --all --filter \"SgrA_*\" 2>/dev/null\n", "ls SgrA_*.txt" ] }, @@ -541,7 +541,7 @@ "source": [ "echo n | hm clone eht-data/.hm later-copy\n", "cd later-copy\n", - "echo y | hm download --filter \"M87_*\" 2>/dev/null\n", + "echo y | hm download --all --filter \"M87_*\" 2>/dev/null\n", "cd .." ] }, @@ -588,7 +588,8 @@ "| `hm checkout NAME` | Switch to a branch, or create it |\n", "| `hm set-config --fmt \"PATTERN\"` | Change the file name pattern |\n", "| `hm set-config --encoding \"FIELD=REGEX\"` | Add an encoding rule |\n", - "| `hm download --all` / `--filter` / `PATHS` | Download files in the catalog (asks first) |\n", + "| `hm download --all` / `PATHS` | Download all files, or files and folders in the catalog (asks first) |\n", + "| `hm download ... --filter \"GLOB\"` | Download only the selected files that match (asks first) |\n", "| `hm download --dry-run` | Show what would be downloaded |\n", "| `hm clone SOURCE DIR` | Copy a repository and download its data (asks first) |\n", "| `hm clone SOURCE DIR --no-download` | Copy only the catalog and history |" diff --git a/doc/api.rst b/doc/api.rst index 689295e..4c28d52 100644 --- a/doc/api.rst +++ b/doc/api.rst @@ -109,11 +109,14 @@ Downloading remote data Preview the selected files using the local catalog, then confirm a download:: hm download --all --dry-run - hm download --filter 'runs/**' - -Explicit paths and ``--tsv data.tsv`` also select files. Every nonempty transfer -in the CLI requires interactive confirmation, including the default clone -transfer. ``clone --no-download`` skips data and the prompt. Clone filters and + hm download runs + +Choose catalogued paths or folders, relative to the current folder, or +``--all`` or ``--tsv data.tsv``; ``--filter`` and ``--fmt`` only narrow that +selection, as in ``hm download --all --filter 'runs/**'``. Paths that are not +in the catalog stop the download before the server is contacted. Every +nonempty transfer in the CLI requires interactive confirmation, including the +default clone transfer. ``clone --no-download`` skips data and the prompt. Clone filters and formats cannot be combined with this flag and leave the complete catalog unchanged. Declining clone's prompt keeps the catalog and exits successfully. A filter or filename format never authorizes a transfer. diff --git a/doc/private_data.rst b/doc/private_data.rst index 03f0cd9..9cd3370 100644 --- a/doc/private_data.rst +++ b/doc/private_data.rst @@ -137,7 +137,8 @@ From the directory containing ``lab``, enter that repository: hm download runs/run_001.h5 The dry run reads only the local catalog. It reports file count, known bytes, -the number of unknown file sizes, destination and source. The CLI reports +the number of unknown file sizes, destination and source, then lists the +selected files and any paths that are not in the catalog. The CLI reports duration as unknown. Python callers can supply a transfer rate to estimate duration when all file sizes are known. The dry run does not test credentials, host trust or remote-file existence. @@ -153,13 +154,20 @@ Choose a scope from inside ``lab``: .. code-block:: bash - hm download --filter 'runs/run_00[12].h5' --dry-run + hm download runs --dry-run + hm download --all --filter 'runs/run_00[12].h5' --dry-run hm download --all --output ./payload -Explicit paths use their recorded catalog checksums when available, just like -``--tsv`` and ``--all``. A path absent from the catalog can be requested, but -its size and checksum are unknown. Files without a usable publisher checksum -can be downloaded, but their contents cannot be checked against the catalog. +Choose files with catalogued paths or folders, ``--tsv`` or ``--all``. Paths +are relative to the current folder, which can be any folder inside the +repository. A folder selects every catalogued file below it, including +subfolders; ``.`` selects everything below the current folder. ``--filter`` +and ``--fmt`` only narrow that selection: on their own they show usage and +download nothing. A path that matches no catalogued file, such as a typo or a +folder without catalogued files, stops the download before the server is +contacted; ``--dry-run`` lists such paths. Selected files use their recorded +catalog checksums. Files without a usable publisher checksum can be +downloaded, but their contents cannot be checked against the catalog. ``--tsv`` can be repeated and combined with explicit paths; overlapping entries are deduplicated. ``--all`` cannot be combined with either selection mode. @@ -303,9 +311,11 @@ Without ``approved=True``, a nonempty transfer raises ``DownloadError`` before opening a connection. A plan freezes the selected files, source URL, profile reference, backend, backend options and destination. Later catalog or remote configuration changes do not redirect it. ``plan_download(filter="**/*.h5")`` -selects matching files from the catalog; -``plan_download(output_path="subset", tsv_names=["data.tsv"])`` selects a TSV -and destination. Planning makes no network requests. Known size totals and +selects matching files from the catalog; ``plan_download(file_paths=["runs"])`` +selects a folder; ``plan_download(output_path="subset", tsv_names=["data.tsv"])`` +selects a TSV and destination. Planning makes no network requests. Requested +paths that match no catalogued file are listed in ``plan.unknown_paths``; +downloading such a plan raises ``DownloadError`` before opening a connection. Known size totals and unknown-size counts are available as ``known_bytes`` and ``unknown_size_count``. ``estimated_seconds`` stays ``None`` unless all sizes are known and ``estimated_bytes_per_second`` was supplied when planning. @@ -366,7 +376,7 @@ access retains Requests' normal ``.netrc`` and environment behavior; Hallmark's named auth profiles apply to SSH/SFTP. From either initialized catalog, inspect and approve a selected subset using -``hm download --filter PATTERN --dry-run`` and then the same command +``hm download --all --filter PATTERN --dry-run`` and then the same command without ``--dry-run``. Python follows the same steps: .. code-block:: python diff --git a/doc/usecase.rst b/doc/usecase.rst index c6fd20e..53920e1 100644 --- a/doc/usecase.rst +++ b/doc/usecase.rst @@ -158,7 +158,7 @@ Downloads are written to temporary files and checked against any checksums supplied with the selection before atomically replacing their destinations. The ``--tsv`` and ``--all`` options include catalog checksums. Explicit -paths also use their recorded checksums when available. +paths and folders must be in the catalog and use their recorded checksums. If a transfer or checksum check fails, the existing destination is preserved and the temporary file is removed. Files that have already downloaded successfully remain available. diff --git a/mod/hallmark/cli.py b/mod/hallmark/cli.py index d6946e0..5e7780b 100644 --- a/mod/hallmark/cli.py +++ b/mod/hallmark/cli.py @@ -458,6 +458,17 @@ def checkout(repo, target_branch): click.echo(f'Switched to branch "{target_branch}".') +def _echo_paths(title, paths, limit=20): + """Print a titled list of paths, abbreviating long lists.""" + if not paths: + return + click.echo(title) + for path in paths[:limit]: + click.echo(f" {path}") + if len(paths) > limit: + click.echo(f" ... {len(paths) - limit} more") + + @hallmark.command(short_help="Download files from the configured data remote.") @click.argument("files", nargs=-1) @click.option("--tsv", "tsv_names", multiple=True, @@ -465,9 +476,9 @@ def checkout(repo, target_branch): @click.option("--all", "download_all", is_flag=True, help="Select all cataloged files.") @click.option("--filter", "filters", multiple=True, - help="Select paths matching a glob. ** matches recursively. " - "May be repeated.") -@click.option("--fmt", help="Select paths matching a filename format.") + help="Keep only selected paths matching a glob. ** matches " + "recursively. May be repeated.") +@click.option("--fmt", help="Keep only selected paths matching a filename format.") @click.option("--remote", "remote_name", help="Name of the configured data remote to use.") @click.option("--output", type=click.Path(file_okay=False), @@ -482,15 +493,21 @@ def download(repo, files, tsv_names, download_all, filters, fmt, remote_name, """ Download selected files from a configured data remote. - FILES are relative to the current folder, which can be any folder inside - the repository. Preview the selection with --dry-run. Every nonempty - download displays its plan and asks for confirmation before transferring - files. + Choose catalogued FILES or folders, or --all; a folder selects every + catalogued file below it. FILES are relative to the current folder, which + can be any folder inside the repository. --filter and --fmt only narrow + that selection. Paths that are not in the catalog stop the download + before the server is contacted. + + Preview the selection with --dry-run. Every nonempty download displays + its plan and asks for confirmation before transferring files. """ if download_all and (files or tsv_names): - raise ClickException("--all cannot be combined with file paths or --tsv") - if not files and not tsv_names and not download_all and not filters and not fmt: - raise ClickException("Provide file paths, --tsv, --all, --filter, or --fmt") + raise click.UsageError("Give paths or --all, not both") + if not files and not tsv_names and not download_all: + raise click.UsageError( + "Choose files with paths or --all; --filter and --fmt only narrow " + "that selection") if repo.worktree is None and output is None: raise ClickException("--output is required when downloading from a bare repo") files = _repo_relative_paths(repo, files) @@ -500,11 +517,15 @@ def download(repo, files, tsv_names, download_all, filters, fmt, remote_name, filter=filters or None, fmt=fmt, remote_name=remote_name) if dry_run: click.echo(plan.summary()) - for item in plan.items[:20]: - click.echo(f" {item.relative_path.as_posix()}") - if plan.file_count > 20: - click.echo(f" ... {plan.file_count - 20} more file(s)") + _echo_paths("Files to download:", [ + item.relative_path.as_posix() for item in plan.items]) + _echo_paths("Not in the catalog:", plan.unknown_paths) return + if plan.unknown_paths: + _echo_paths("Not in the catalog:", plan.unknown_paths) + raise ClickException( + f"{len(plan.unknown_paths)} path(s) not in the catalog; " + "nothing was downloaded") _run_download(repo, plan, max_workers=max_workers) diff --git a/mod/hallmark/remote/download.py b/mod/hallmark/remote/download.py index 888424b..aef5333 100644 --- a/mod/hallmark/remote/download.py +++ b/mod/hallmark/remote/download.py @@ -541,6 +541,31 @@ def _download_and_verify_file( raise DownloadError(f"Failed to write {destination.name}: {exc}") from None +def _normalize_requests(file_paths: Sequence[str]) -> tuple[str, ...]: + """ + Used by _select_download_items and plan_download. + Normalize requested catalog paths and folders, keeping their order. + + A trailing "/" is optional for folders, and "." requests the whole catalog. + + Returns: + tuple[str, ...]: Unique POSIX paths relative to the catalog root. + """ + requests = [] + for value in file_paths: + text = str(value) + request = "." if text.rstrip("/") == "." else validate_remote_path( + text).as_posix() + if request not in requests: + requests.append(request) + return tuple(requests) + + +def _is_requested(path: str, request: str) -> bool: + """Return whether a catalog path is a requested file or lies below a folder.""" + return request == "." or path == request or path.startswith(request + "/") + + def _select_download_items( repo, file_paths: Sequence[str] = (), @@ -553,7 +578,9 @@ def _select_download_items( Args: repo: The hallmark repository object. - file_paths: A sequence of specific file paths to download. + file_paths: Catalogued files or folders to download. A folder selects + every catalogued file below it; paths outside the catalog select + nothing. tsv_names: A sequence of TSV names to download files from. all_files: If True, include all files from the repository's configuration. @@ -568,7 +595,10 @@ def _select_download_items( # dictionary to store selected files with relative paths and optional checksums. selected: dict[str, tuple[Path, Optional[ChecksumSpec]]] = {} metadata = {} - explicit_paths = {validate_remote_path(path).as_posix() for path in file_paths} + explicit_paths = _normalize_requests(file_paths) + + def requested(path: str) -> bool: + return any(_is_requested(path, request) for request in explicit_paths) def add_file( value: Union[str, Path], @@ -609,12 +639,9 @@ def add_frame(frame: pd.DataFrame, fmt_entries: list[dict], row = dict(zip(columns, values)) # add the resolved remote path and its checksum to the selected files relative_path = validate_remote_path(_resolve_remote_path(row, fmt_entries)) - if not explicit_only or relative_path.as_posix() in explicit_paths: + if not explicit_only or requested(relative_path.as_posix()): add_file(relative_path, _checksum_from_row(row), row) - # Add explicitly requested file paths to the selected files. - for file_path in file_paths: - add_file(file_path) # Organize data configuration entries by their TSV names for easier access. entries_by_tsv: dict[str, list[dict]] = {} for entry in data_config: @@ -691,7 +718,7 @@ def add_frame(frame: pd.DataFrame, fmt_entries: list[dict], # if a file path is specified in the entry, add it to the selected files if file_path and ( all_files - or validate_remote_path(file_path).as_posix() in explicit_paths + or requested(validate_remote_path(file_path).as_posix()) ): add_file(file_path, _checksum_from_config(entry), entry) @@ -741,12 +768,16 @@ def plan_download( No network requests are made. With no explicit paths or TSVs, select the complete catalog before applying filters. Missing sizes remain unknown. + Requested paths that match no catalogued file are recorded in + ``unknown_paths``; such a plan cannot be executed. Args: repo: The hallmark repository object. output_path (Path | str, optional): Destination directory. Defaults to the worktree; required for a bare repository. - file_paths (sequence[str], optional): Remote-relative file paths. + file_paths (sequence[str], optional): Catalogued files or folders, + relative to the catalog root. A folder selects every catalogued + file below it, and "." selects the whole catalog. tsv_names (sequence[str], optional): Catalog TSVs to select. all_files (bool): Select all configured files. Cannot be combined with explicit paths or TSVs. Defaults to False. @@ -782,6 +813,11 @@ def plan_download( items = _select_download_items( repo, file_paths=file_paths, tsv_names=tsv_names, all_files=all_files or (not file_paths and not tsv_names)) + # A path is unknown when no catalogued file is, or lies below, that path. + selected_paths = [item.relative_path.as_posix() for item in items] + unknown_paths = tuple( + request for request in _normalize_requests(file_paths) + if not any(_is_requested(path, request) for path in selected_paths)) if filter is not None or fmt is not None: from .discovery import path_matches items = [item for item in items if path_matches( @@ -806,7 +842,8 @@ def plan_download( remote_auth=remote.get("auth"), remote_name=remote.get("name"), estimated_bytes_per_second=estimated_bytes_per_second, remote_backend=source.backend if source is not None else None, - backend_options=remote.get("backend_options")) + backend_options=remote.get("backend_options"), + unknown_paths=unknown_paths) def execute_download_plan( @@ -834,11 +871,15 @@ def execute_download_plan( Raises: TypeError: If ``plan`` is not a DownloadPlan. - DownloadError: If approval is missing, setup fails, or the - destination is invalid. + DownloadError: If the plan has unknown paths, approval is missing, + setup fails, or the destination is invalid. """ if not isinstance(plan, DownloadPlan): raise TypeError("plan must be a DownloadPlan") + if plan.unknown_paths: + raise DownloadError( + "Not in the catalog: " + ", ".join(plan.unknown_paths) + + "; nothing was downloaded") if plan.items and approved is not True: raise DownloadError("Dataset downloads require explicit approval") max_workers = _require_positive_integer(max_workers, label="max_workers") diff --git a/mod/hallmark/remote/plan.py b/mod/hallmark/remote/plan.py index fbf7854..34e51bb 100644 --- a/mod/hallmark/remote/plan.py +++ b/mod/hallmark/remote/plan.py @@ -68,6 +68,8 @@ class DownloadPlan: by the caller for duration estimates. remote_backend (str, optional): Registered backend selected for transfer. backend_options (mapping): Immutable backend configuration snapshot. + unknown_paths (tuple[str]): Requested paths that match no catalogued + file. A plan with unknown paths cannot be executed. """ items: tuple[DownloadItem, ...] @@ -78,10 +80,14 @@ class DownloadPlan: estimated_bytes_per_second: Optional[float] = None remote_backend: Optional[str] = None backend_options: Mapping = field(default_factory=dict, repr=False, hash=False) + unknown_paths: tuple[str, ...] = () def __post_init__(self) -> None: """Copy the selection and validate the source fields and rate.""" object.__setattr__(self, "items", tuple(self.items)) + object.__setattr__(self, "unknown_paths", tuple(self.unknown_paths)) + if any(not isinstance(path, str) for path in self.unknown_paths): + raise TypeError("unknown_paths must contain strings") object.__setattr__( self, "output_path", Path(self.output_path).expanduser().absolute()) if any(not isinstance(item, DownloadItem) for item in self.items): @@ -133,8 +139,9 @@ def summary(self) -> str: Describe the planned transfer, including unavailable estimates. Returns: - str: File count, size and duration estimates, source, and destination. - URL credentials, query parameters, and fragments are omitted. + str: File count, size and duration estimates, source, destination, + and the number of unknown paths. URL credentials, query + parameters, and fragments are omitted. """ size = f"{self.known_bytes:,} bytes" if self.unknown_size_count: @@ -147,6 +154,8 @@ def summary(self) -> str: source = urlunsplit(parsed._replace( netloc=parsed.netloc.rsplit("@", 1)[-1], query="", fragment="")) backend = f"\nBackend: {self.remote_backend}" if self.remote_backend else "" + unknown = (f"\n{len(self.unknown_paths)} path(s) not in the catalog" + if self.unknown_paths else "") return (f"{self.file_count} file(s); {size}; estimated duration: {duration}" f"\nSource: {source}{backend}" - f"\nDestination: {self.output_path}") + f"\nDestination: {self.output_path}{unknown}") diff --git a/test/test_cli.py b/test/test_cli.py index afb96e6..63bbd06 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -691,9 +691,11 @@ def test_clone_rejects_nonpositive_max_workers(max_workers): @pytest.mark.parametrize( "arguments, message", [ - (["download"], "Provide file paths, --tsv, --all, --filter, or --fmt"), - (["download", "file.dat", "--all"], "--all cannot be combined"), - (["download", "--tsv", "data", "--all"], "--all cannot be combined")]) + (["download"], "Choose files with paths or --all"), + (["download", "--filter", "*.dat"], "only narrow"), + (["download", "--fmt", "{name}.dat", "--dry-run"], "only narrow"), + (["download", "file.dat", "--all"], "Give paths or --all, not both"), + (["download", "--tsv", "data", "--all"], "Give paths or --all, not both")]) def test_download_cli_rejects_invalid_selection_combinations( monkeypatch, arguments, message): """ @@ -706,10 +708,15 @@ def test_download_cli_rejects_invalid_selection_combinations( arguments: The list of command line arguments to be tested. message: The expected error message to be found in the download command output. """ - _install_repo(monkeypatch) + repo = _install_repo(monkeypatch) + + def reject_plan(*args, **kwargs): + raise AssertionError("an invalid selection must not be planned") + + repo.plan_download = reject_plan result = CliRunner().invoke(hallmark, arguments) - assert result.exit_code != 0, f"Expected non-zero exit code for download with \ + assert result.exit_code == 2, f"Expected usage exit code for download with \ arguments {arguments}, got {result.exit_code}" assert message in result.output, \ f"Expected error message '{message}' in output, got: {result.output}" @@ -771,7 +778,7 @@ def reject_download(*args, **kwargs): assert "file-000.dat" in result.output assert "file-019.dat" in result.output assert "file-020.dat" not in result.output - assert "... 3 more file(s)" in result.output + assert "... 3 more" in result.output assert "Download these files?" not in result.output @@ -912,7 +919,7 @@ def capture_get(url, **kwargs): else: target = source.worktree monkeypatch.chdir(target) - arguments = ["download", "--filter", "*.fits"] + arguments = ["download", "--all", "--filter", "*.fits"] result = CliRunner().invoke(hallmark, arguments, input=answer) assert "1 file(s); 4 bytes" in result.output assert "Download these files? [y/N]" in result.output @@ -1082,3 +1089,58 @@ def test_cli_add_dot_from_a_subfolder_stages_the_whole_repository( assert sorted(rows) == [("A", "1"), ("A", "2"), ("B", "1"), ("B", "2")] assert rows[("A", "1")] == Repo.checksum(changed) assert rows[("B", "2")] == Repo.checksum(repo.worktree / "B" / "run2.txt") + + +def _nested_cli_catalog(path): + """Create a remote catalog whose files are spread over nested folders.""" + repo = Repo.init(path) + repo.state.config = { + "data": [{"db": "data.tsv"}], + "remote": {"name": "origin", "url": "https://example.test/data/"}} + repo.state.data = pd.DataFrame({"path": [ + "runs/a.fits", "runs/2024/b.fits", "notes/readme.txt"]}) + repo.dothm.save_state(repo.state) + repo.dothm.index.commit("Catalog nested remote files") + return repo + + +def test_cli_download_folder_from_inside_it(monkeypatch, tmp_path): + repo = _nested_cli_catalog(tmp_path / "project") + (repo.worktree / "runs").mkdir() + monkeypatch.chdir(repo.worktree / "runs") + result = CliRunner().invoke(hallmark, ["download", ".", "--dry-run"]) + assert result.exit_code == 0, result.output + assert "2 file(s)" in result.output + assert " runs/a.fits" in result.output + assert " runs/2024/b.fits" in result.output + assert "readme.txt" not in result.output + assert f"Destination: {repo.worktree}" in result.output + + +def test_cli_dry_run_reports_unknown_paths(monkeypatch, tmp_path): + repo = _nested_cli_catalog(tmp_path / "project") + monkeypatch.chdir(repo.worktree) + result = CliRunner().invoke(hallmark, [ + "download", "runs", "missing.fits", "empty/", "--dry-run"]) + assert result.exit_code == 0, result.output + assert "2 file(s)" in result.output + assert "2 path(s) not in the catalog" in result.output + assert "Not in the catalog:\n missing.fits\n empty\n" in result.output + + +def test_cli_unknown_paths_stop_before_prompt_or_server(monkeypatch, tmp_path): + repo = _nested_cli_catalog(tmp_path / "project") + monkeypatch.chdir(repo.worktree) + + def reject(*args, **kwargs): + raise AssertionError("unknown paths must not contact the server") + + monkeypatch.setattr(requests, "Session", reject) + monkeypatch.setattr("hallmark.remote.download.OperationContext", reject) + result = CliRunner().invoke(hallmark, ["download", "runs", "missing.fits"], + input="y\n") + assert result.exit_code == 1 + assert "Not in the catalog:\n missing.fits" in result.output + assert "1 path(s) not in the catalog; nothing was downloaded" in result.output + assert "Download these files?" not in result.output + assert not (repo.worktree / "runs").exists() diff --git a/test/test_download_plan.py b/test/test_download_plan.py index aef976c..7a84e02 100644 --- a/test/test_download_plan.py +++ b/test/test_download_plan.py @@ -55,12 +55,72 @@ def reject_network(*args, **kwargs): def test_explicit_paths_retain_catalog_checksums_and_metadata(catalog): - plan = plan_download(catalog, file_paths=["nested/a.fits", "unlisted.bin"]) + plan = plan_download(catalog, file_paths=["nested/a.fits", "b.txt"]) assert plan.file_count == 2 assert plan.items[0].checksum == ("sha256", sha256(b"abcdef").hexdigest()) assert plan.items[0].size_bytes == 6 assert plan.items[1].checksum is None assert plan.items[1].size_bytes is None + assert plan.unknown_paths == () + + +def _folder_catalog(tmp_path): + """Create a catalog with nested folders and a sibling sharing a prefix.""" + metadata = tmp_path / ".hm" + metadata.mkdir() + frame = pd.DataFrame({"path": [ + "runs/a.h5", "runs/2024/b.h5", "runs/2024/x/c.h5", "runsx/d.h5", "top.h5"]}) + frame.to_csv(metadata / "data.tsv", sep="\t", index=False) + return SimpleNamespace( + dothm=SimpleNamespace(path=metadata), worktree=tmp_path, + state=SimpleNamespace(data=frame, config={ + "data": [{"db": "data.tsv"}], + "remote": {"name": "origin", "url": "https://source.test/data/"}, + })) + + +@pytest.mark.parametrize("requested, expected", [ + (["runs"], ["runs/a.h5", "runs/2024/b.h5", "runs/2024/x/c.h5"]), + (["runs/2024/"], ["runs/2024/b.h5", "runs/2024/x/c.h5"]), + (["runs/a.h5", "top.h5"], ["runs/a.h5", "top.h5"]), + (["runs/2024", "runs/2024/x/c.h5"], ["runs/2024/b.h5", "runs/2024/x/c.h5"]), + (["."], ["runs/a.h5", "runs/2024/b.h5", "runs/2024/x/c.h5", "runsx/d.h5", + "top.h5"]), +]) +def test_folders_select_every_catalogued_file_below_them( + tmp_path, requested, expected): + plan = plan_download(_folder_catalog(tmp_path), file_paths=requested) + assert [item.relative_path.as_posix() for item in plan.items] == expected + assert plan.unknown_paths == () + + +def test_filters_narrow_a_folder_selection(tmp_path): + plan = plan_download(_folder_catalog(tmp_path), file_paths=["runs"], + filter="**/2024/*.h5") + assert [item.relative_path.as_posix() for item in plan.items] == [ + "runs/2024/b.h5"] + + +def test_unknown_paths_and_empty_folders_fail_before_contacting_server( + tmp_path, monkeypatch): + catalog = _folder_catalog(tmp_path) + plan = plan_download(catalog, file_paths=[ + "runs/a.h5", "missing.h5", "run", "empty/", "runs/a.h5/inner"]) + assert [item.relative_path.as_posix() for item in plan.items] == ["runs/a.h5"] + assert plan.unknown_paths == ("missing.h5", "run", "empty", "runs/a.h5/inner") + assert "4 path(s) not in the catalog" in plan.summary() + + def reject(*args, **kwargs): + raise AssertionError("unknown paths must fail before contacting a server") + + monkeypatch.setattr("hallmark.remote.download.OperationContext", reject) + monkeypatch.setattr("hallmark.remote.download.RemoteSpec", SimpleNamespace( + from_url=reject)) + for approved in (True, False): + with pytest.raises(DownloadError, match="missing.h5, run, empty") as error: + execute_download_plan(catalog, plan, approved=approved) + assert "nothing was downloaded" in str(error.value) + assert not (tmp_path / "runs").exists() def test_sizes_survive_nullable_numeric_tsv_serialization(catalog): diff --git a/test/test_downloader.py b/test/test_downloader.py index 8f7e49d..f57990d 100644 --- a/test/test_downloader.py +++ b/test/test_downloader.py @@ -419,7 +419,8 @@ def test_download_requires_remote_url(tmp_path): tmp_path: A pytest fixture providing a temporary directory. Raises: DownloadError: If the remote configuration is present but the URL is missing.""" - repo = _repo(tmp_path, {"remote": {"name": "origin"}}) + repo = _repo(tmp_path, {"remote": {"name": "origin"}}, + pd.DataFrame([{"path": "data.bin"}])) with pytest.raises(DownloadError, match="No remote URL"): plan_download(repo, tmp_path, file_paths=["data.bin"]) @@ -671,7 +672,7 @@ def test_download_rejects_selected_files_without_remote(tmp_path): Raises: DownloadError: If selected files are provided but no remote is configured. """ - repo = _repo(tmp_path) + repo = _repo(tmp_path, data=pd.DataFrame([{"path": "data.bin"}])) with pytest.raises(DownloadError, match="No remote URL is configured"): plan_download(repo, tmp_path, file_paths=["data.bin"]) From 6012e8cb12ba71a2ab8c59a2a2cca63ef4e9f02e Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 20:57:21 -0700 Subject: [PATCH 07/10] Skip verified files and refuse downloads that would replace different contents Planning now checks every selected file that already exists at its destination, first by its recorded size and then by its catalog checksum, hashing in a small parallel pool. Files with their catalog checksum are skipped, so repeating a download transfers only what is missing. When the catalog has no checksum, a file with the recorded size is skipped as present but unverified, so catalogs without checksum manifests can resume too. Checksums whose algorithm is "unknown" are identified by digest length. Files of another size or contents, files with neither a checksum nor a size to check, and anything that is not a regular file become DownloadPlan.conflicts, and such a plan downloads nothing; the CLI lists them with a hint to delete them first. A destination that appears while a transfer runs is no longer overwritten. Co-Authored-By: Claude Opus 5.5 --- doc/backends.rst | 4 +- doc/private_data.rst | 33 +++++-- doc/usecase.rst | 11 ++- mod/hallmark/__init__.py | 7 +- mod/hallmark/cli.py | 43 +++++++-- mod/hallmark/remote/download.py | 121 ++++++++++++++++++++++-- mod/hallmark/remote/plan.py | 59 ++++++++++-- test/test_cli.py | 131 ++++++++++++++++++++++++++ test/test_download_plan.py | 158 ++++++++++++++++++++++++++++++++ test/test_ssh_integration.py | 19 +++- 10 files changed, 542 insertions(+), 44 deletions(-) diff --git a/doc/backends.rst b/doc/backends.rst index c321cd9..c097f50 100644 --- a/doc/backends.rst +++ b/doc/backends.rst @@ -114,8 +114,8 @@ as missing objects. Discovery reads listings and published metadata only. It must not download payloads to infer checksums. The shared downloader owns approval, destination -path validation, checksum verification, atomic replacement and worker -concurrency. Backends map logical paths to server paths or API requests and +path validation, checksum verification, skipping or refusing existing files, +moving finished files into place and worker concurrency. Backends map logical paths to server paths or API requests and write only to the temporary destination they are given. An instance can serve concurrent ``fetch`` calls. Protect shared mutable diff --git a/doc/private_data.rst b/doc/private_data.rst index 9cd3370..03ac374 100644 --- a/doc/private_data.rst +++ b/doc/private_data.rst @@ -173,9 +173,23 @@ downloaded, but their contents cannot be checked against the catalog. are deduplicated. ``--all`` cannot be combined with either selection mode. Relative output paths are relative to the current directory. Without ``--output``, downloads go to the repository worktree; bare ``.hm`` repositories -require an explicit output directory. Repeating a download transfers the -selection again and replaces destinations only after successful transfer and -any recorded checksum verification. +require an explicit output directory. + +Existing files are never replaced. Before contacting the server, a download +checks each selected file that already exists at its destination, first by +its recorded size and then by its catalog checksum. A file with its catalog +checksum is reported as already downloaded and skipped, so repeating a +download, for example after a failed transfer, fetches only what is missing. +When the catalog has no checksum for a file, as for SSH exports without a +checksum manifest, a file with the recorded size is reported as present (size +matches, not verified) and also skipped. A file with a different size or +contents, a file the catalog records neither a checksum nor a size for, or a +folder in its place is a conflict: the download stops and transfers nothing. +Delete conflicting files first to replace them; there is no overwrite option. +``--dry-run`` lists files to download, skipped and unverified files, conflicts +and unknown paths. Checksums from manifests that do not name their algorithm, +such as ``checksums.txt``, are used when the digest length identifies MD5, +SHA-1, SHA-256 or SHA-512. After committing the catalog, use ``download`` to review and approve a transfer. Declining keeps the catalog available; an empty selection needs no approval: @@ -455,10 +469,12 @@ catalog hosting independent of data hosting. Download behavior and troubleshooting ------------------------------------- -Files are downloaded to temporary paths and published atomically after transfer -and any recorded checksum verification. A failed transfer preserves an existing -destination and removes its temporary file. Successful files remain available -when another file fails; a multi-file download is not a single transaction. +Unfinished downloads are kept in temporary ``.part`` files beside their +destinations and moved into place only after the transfer and any recorded +checksum verification succeed. A failed transfer removes its temporary file. +A destination that appears while the download runs is not replaced; that file +fails instead. Successful files remain available when another file fails; a +multi-file download is not a single transaction. The CLI exits nonzero on transfer failures. Cancellation stops the active operation and closes the SSH connections and processes it started. @@ -481,6 +497,9 @@ operation and closes the SSH connections and processes it started. * - Checksum mismatch - Check whether the export changed since catalog creation. Reconcile the catalog and source against a trusted version before retrying. + * - Existing files conflict with the catalog + - Compare the listed files with the catalog. Delete or move them, then + download again; Hallmark never overwrites existing files. * - Transfer timeout or session limit - Adjust the local per-file time budget or lower concurrency to match the server's capacity. diff --git a/doc/usecase.rst b/doc/usecase.rst index 53920e1..928397c 100644 --- a/doc/usecase.rst +++ b/doc/usecase.rst @@ -155,13 +155,16 @@ The ``--remote`` option selects one data server; |hallmark|_ does not automatically try another server if the download fails. Downloads are written to temporary files and checked against any -checksums supplied with the selection before atomically replacing -their destinations. +checksums supplied with the selection before they are moved into place. The ``--tsv`` and ``--all`` options include catalog checksums. Explicit paths and folders must be in the catalog and use their recorded checksums. -If a transfer or checksum check fails, the existing destination is -preserved and the temporary file is removed. +If a transfer or checksum check fails, the temporary file is removed. Files that have already downloaded successfully remain available. +Existing files are never replaced: files that already match their +catalog checksum, or their recorded size when the catalog has no checksum, +are skipped, and a file with different or uncheckable contents stops the +download before anything is transferred. Delete conflicting files first to +replace them. SSH access and file paths ~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/mod/hallmark/__init__.py b/mod/hallmark/__init__.py index 9ddf4d9..b20c557 100644 --- a/mod/hallmark/__init__.py +++ b/mod/hallmark/__init__.py @@ -17,13 +17,14 @@ from .repo import Repo from .paraframe import ParaFrame -from .remote.plan import DownloadItem, DownloadPlan +from .remote.plan import DownloadConflict, DownloadItem, DownloadPlan from .remote.backends import ( DataBackend, HttpBackend, SshBackend, CyVerseBackend, RemoteEntry, register_backend, ) __all__ = [ - "Repo", "ParaFrame", "DownloadItem", "DownloadPlan", "DataBackend", - "HttpBackend", "SshBackend", "CyVerseBackend", "RemoteEntry", "register_backend", + "Repo", "ParaFrame", "DownloadConflict", "DownloadItem", "DownloadPlan", + "DataBackend", "HttpBackend", "SshBackend", "CyVerseBackend", "RemoteEntry", + "register_backend", ] diff --git a/mod/hallmark/cli.py b/mod/hallmark/cli.py index 5e7780b..693c691 100644 --- a/mod/hallmark/cli.py +++ b/mod/hallmark/cli.py @@ -119,7 +119,9 @@ def _run_download(repo, plan, *, max_workers): """ click.echo(plan.summary()) if not plan.file_count: - click.echo("No files selected for download.") + click.echo("All selected files are already downloaded." + if plan.skipped or plan.unverified + else "No files selected for download.") return click.confirm("Download these files?", default=False, abort=True) results = repo.download(plan, approved=True, max_workers=max_workers, @@ -469,6 +471,24 @@ def _echo_paths(title, paths, limit=20): click.echo(f" ... {len(paths) - limit} more") +def _report_plan_problems(plan): + """ + List unknown paths and conflicting files that prevent a download. + + Returns: + list[str]: Short descriptions of the problems; empty if there are none. + """ + _echo_paths("Existing files that conflict with the catalog:", [ + f"{conflict.item.relative_path.as_posix()} ({conflict.reason})" + for conflict in plan.conflicts]) + if plan.conflicts: + click.echo("Delete conflicting files first to replace them.") + _echo_paths("Not in the catalog:", plan.unknown_paths) + return [f"{count} {text}" for count, text in ( + (len(plan.conflicts), "existing file(s) conflict with the catalog"), + (len(plan.unknown_paths), "path(s) not in the catalog")) if count] + + @hallmark.command(short_help="Download files from the configured data remote.") @click.argument("files", nargs=-1) @click.option("--tsv", "tsv_names", multiple=True, @@ -496,8 +516,11 @@ def download(repo, files, tsv_names, download_all, filters, fmt, remote_name, Choose catalogued FILES or folders, or --all; a folder selects every catalogued file below it. FILES are relative to the current folder, which can be any folder inside the repository. --filter and --fmt only narrow - that selection. Paths that are not in the catalog stop the download - before the server is contacted. + that selection. Files already downloaded with their catalog checksum, or + with the catalog size when it records no checksum, are skipped. Paths + that are not in the catalog, and existing files that differ from it or + cannot be checked, stop the download before the server is contacted; + delete conflicting files first to replace them. Preview the selection with --dry-run. Every nonempty download displays its plan and asks for confirmation before transferring files. @@ -519,13 +542,15 @@ def download(repo, files, tsv_names, download_all, filters, fmt, remote_name, click.echo(plan.summary()) _echo_paths("Files to download:", [ item.relative_path.as_posix() for item in plan.items]) - _echo_paths("Not in the catalog:", plan.unknown_paths) + _echo_paths("Already downloaded (skipped):", [ + item.relative_path.as_posix() for item in plan.skipped]) + _echo_paths("Present (size matches, not verified):", [ + item.relative_path.as_posix() for item in plan.unverified]) + _report_plan_problems(plan) return - if plan.unknown_paths: - _echo_paths("Not in the catalog:", plan.unknown_paths) - raise ClickException( - f"{len(plan.unknown_paths)} path(s) not in the catalog; " - "nothing was downloaded") + problems = _report_plan_problems(plan) + if problems: + raise ClickException("; ".join(problems) + "; nothing was downloaded") _run_download(repo, plan, max_workers=max_workers) diff --git a/mod/hallmark/remote/download.py b/mod/hallmark/remote/download.py index aef5333..632b571 100644 --- a/mod/hallmark/remote/download.py +++ b/mod/hallmark/remote/download.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os from concurrent.futures import FIRST_COMPLETED, ThreadPoolExecutor, wait from decimal import Decimal, InvalidOperation from pathlib import Path @@ -13,9 +14,10 @@ from ..transport import OperationContext, RemoteSpec from ..transport.base import DownloadError, validate_remote_path -from .plan import DownloadItem, DownloadPlan +from .plan import DownloadConflict, DownloadItem, DownloadPlan from ..utils import ( CHECKSUM_ALGORITHMS_BY_STRENGTH, + CHECKSUM_LENGTHS, SUPPORTED_CHECKSUM_ALGORITHMS, as_list_of_dicts, replace_file_on_success, @@ -34,6 +36,9 @@ ChecksumSpec = Union[str, tuple[str, str]] # The size of chunks to read from a file when downloading or computing checksums. DOWNLOAD_CHUNK_SIZE = 8192 +# Supported checksum algorithms by the length of their hexadecimal digests. +ALGORITHMS_BY_DIGEST_LENGTH = { + length: algorithm for algorithm, length in CHECKSUM_LENGTHS.items()} def _repository_config(repo) -> dict: """ @@ -152,7 +157,13 @@ def _parse_checksum(value, algorithm=None) -> Optional[ChecksumSpec]: algorithm_text = _clean_checksum(algorithm) # If the algorithm is None, return None to indicate no valid algorithm is available if algorithm_text is None: - return None + if str(algorithm).strip().lower() != "unknown": + return None + # Manifests such as checksums.txt do not name their algorithm; the + # length of a well-formed digest identifies the supported one it is. + algorithm_text = ALGORITHMS_BY_DIGEST_LENGTH.get(len(checksum)) + if algorithm_text is None or not is_valid_checksum(algorithm_text, checksum): + return None # Normalize the algorithm to lowercase and check if it is supported algorithm_text = algorithm_text.lower() if algorithm_text not in SUPPORTED_CHECKSUM_ALGORITHMS: @@ -504,7 +515,7 @@ def _download_and_verify_file( context, relative_path, destination, expected_checksum, chunk_size ): """ - Download and verify a file before replacing its destination. + Download and verify a file, then move it to its absent destination. Args: context (OperationContext): Transport and destination for this download. @@ -518,7 +529,8 @@ def _download_and_verify_file( int: Number of downloaded bytes. Raises: - DownloadError: If the transfer, checksum, or destination is invalid. + DownloadError: If the transfer, checksum, or destination is invalid, + or the destination exists. """ validated_checksum = _validate_expected_checksum(expected_checksum) try: @@ -535,6 +547,12 @@ def _download_and_verify_file( # Another process can still change the path after this check. resolve_path_in_root(context.output_root, relative_path, label="download destination") + # Planning skips or refuses existing files, so a file here appeared + # later; never replace it. The same race as above remains. + if destination.exists() or destination.is_symlink(): + raise DownloadError( + f"Refusing to replace {destination.name}: it appeared after " + "the download was planned; delete it first to replace it") context.check_cancelled() return size except (OSError, ValueError) as exc: @@ -738,6 +756,73 @@ def add_frame(frame: pd.DataFrame, fmt_entries: list[dict], for path, checksum in selected.values()] +def _existing_file_status(item: DownloadItem, path: Path): + """ + Used by _sort_existing_destinations. + Compare an existing destination with its catalog size and checksum. + + The size is compared first, so a file of the wrong size is never read. + + Returns: + tuple[str, str | None]: ``("verified", None)`` when the checksum + matches; ``("unverified", None)`` when the catalog has no usable + checksum but records the same size; otherwise ``("conflict", reason)``. + """ + if path.is_symlink() or not path.is_file(): + return "conflict", "not a regular file" + try: + size = path.stat().st_size + except OSError: + return "conflict", "cannot be read" + if item.size_bytes is not None and size != item.size_bytes: + return "conflict", (f"{size:,} bytes, but the catalog records " + f"{item.size_bytes:,}") + try: + expected = _validate_expected_checksum(item.checksum) + except DownloadError: + expected = None + if expected is None: + if item.size_bytes is None: + return "conflict", "no catalog checksum or size to verify it" + return "unverified", None + algorithm, digest = expected + try: + actual = calculate_file_checksum(path, algorithm=algorithm) + except OSError: + return "conflict", "cannot be read" + if actual.lower() != digest: + return "conflict", "different contents" + return "verified", None + + +def _sort_existing_destinations(items, destinations): + """ + Used by plan_download. + Separate files to transfer from those whose destinations already exist. + + Existing files are checked in parallel with their catalog size and + checksum algorithm, using a small pool because hashing reads whole files. + + Returns: + tuple: Items to transfer, items already downloaded and verified, + items present with the catalog size but no checksum, and conflicts. + """ + existing = [(item, path) for item, path in zip(items, destinations) + if path.exists() or path.is_symlink()] + if not existing: + return list(items), [], [], [] + with ThreadPoolExecutor(max_workers=min(8, os.cpu_count() or 1)) as executor: + statuses = list(executor.map( + lambda pair: _existing_file_status(*pair), existing)) + present = {item.relative_path for item, _ in existing} + by_status = {"verified": [], "unverified": [], "conflict": []} + for (item, _), (status, reason) in zip(existing, statuses): + by_status[status].append( + DownloadConflict(item, reason) if status == "conflict" else item) + return ([item for item in items if item.relative_path not in present], + by_status["verified"], by_status["unverified"], by_status["conflict"]) + + def _parse_file_size(value) -> Optional[int]: """Read a recorded size without treating missing or malformed sizes as zero.""" if value is None or isinstance(value, bool) or pd.isna(value): @@ -769,7 +854,12 @@ def plan_download( No network requests are made. With no explicit paths or TSVs, select the complete catalog before applying filters. Missing sizes remain unknown. Requested paths that match no catalogued file are recorded in - ``unknown_paths``; such a plan cannot be executed. + ``unknown_paths``. Existing destinations are compared with the catalog: + files with their catalog checksum are ``skipped``; files without a usable + catalog checksum but with the recorded size are ``unverified`` and also + not transferred; files that differ, cannot be checked at all or are not + regular files are ``conflicts``. A plan with unknown paths or conflicts + cannot be executed. Args: repo: The hallmark repository object. @@ -831,19 +921,23 @@ def plan_download( source = RemoteSpec.from_url( remote["url"], remote.get("auth"), backend=remote.get("backend"), backend_options=remote.get("backend_options")) + destinations = [] for item in items: try: - resolve_path_in_root(output_root, item.relative_path, - label="download destination") + destinations.append(resolve_path_in_root( + output_root, item.relative_path, label="download destination")) except ValueError as exc: raise DownloadError(str(exc)) from exc + items, skipped, unverified, conflicts = _sort_existing_destinations( + items, destinations) return DownloadPlan( tuple(items), remote.get("url"), output_root, remote_auth=remote.get("auth"), remote_name=remote.get("name"), estimated_bytes_per_second=estimated_bytes_per_second, remote_backend=source.backend if source is not None else None, backend_options=remote.get("backend_options"), - unknown_paths=unknown_paths) + unknown_paths=unknown_paths, skipped=tuple(skipped), + unverified=tuple(unverified), conflicts=tuple(conflicts)) def execute_download_plan( @@ -871,8 +965,8 @@ def execute_download_plan( Raises: TypeError: If ``plan`` is not a DownloadPlan. - DownloadError: If the plan has unknown paths, approval is missing, - setup fails, or the destination is invalid. + DownloadError: If the plan has unknown paths or conflicts, approval + is missing, setup fails, or the destination is invalid. """ if not isinstance(plan, DownloadPlan): raise TypeError("plan must be a DownloadPlan") @@ -880,6 +974,13 @@ def execute_download_plan( raise DownloadError( "Not in the catalog: " + ", ".join(plan.unknown_paths) + "; nothing was downloaded") + if plan.conflicts: + raise DownloadError( + "Existing files conflict with the catalog: " + ", ".join( + f"{conflict.item.relative_path.as_posix()} ({conflict.reason})" + for conflict in plan.conflicts) + + "; nothing was downloaded. " + "Delete conflicting files first to replace them") if plan.items and approved is not True: raise DownloadError("Dataset downloads require explicit approval") max_workers = _require_positive_integer(max_workers, label="max_workers") diff --git a/mod/hallmark/remote/plan.py b/mod/hallmark/remote/plan.py index 34e51bb..c762e64 100644 --- a/mod/hallmark/remote/plan.py +++ b/mod/hallmark/remote/plan.py @@ -50,6 +50,21 @@ def __post_init__(self) -> None: raise ValueError("mtime must be a string or None") +@dataclass(frozen=True) +class DownloadConflict: + """ + A selected file whose destination exists and cannot be kept as it is. + + Attributes: + item (DownloadItem): The catalogued file. + reason (str): Why the existing destination conflicts, for example + ``"different contents"``. + """ + + item: DownloadItem + reason: str + + @dataclass(frozen=True) class DownloadPlan: """ @@ -57,9 +72,12 @@ class DownloadPlan: Creating a plan does not approve a transfer. Sizes come from the catalog; a duration estimate requires every file size and a supplied transfer rate. + Existing destinations are never replaced: files already downloaded are + skipped, and any conflict stops the whole download. Attributes: - items (tuple[DownloadItem]): Selected files and their metadata. + items (tuple[DownloadItem]): Selected files to transfer, with their + metadata. remote_url (str, optional): Data source URL; may be None for an empty plan. output_path (Path): Absolute destination directory. remote_auth (str, optional): Local SSH profile name. @@ -70,6 +88,14 @@ class DownloadPlan: backend_options (mapping): Immutable backend configuration snapshot. unknown_paths (tuple[str]): Requested paths that match no catalogued file. A plan with unknown paths cannot be executed. + skipped (tuple[DownloadItem]): Selected files already present with + their catalog checksum; they are not transferred. + unverified (tuple[DownloadItem]): Selected files already present with + their catalog size but without a catalog checksum to verify them; + they are not transferred. + conflicts (tuple[DownloadConflict]): Selected files whose existing + destinations differ from the catalog or cannot be verified. A plan + with conflicts cannot be executed. """ items: tuple[DownloadItem, ...] @@ -81,13 +107,24 @@ class DownloadPlan: remote_backend: Optional[str] = None backend_options: Mapping = field(default_factory=dict, repr=False, hash=False) unknown_paths: tuple[str, ...] = () + skipped: tuple[DownloadItem, ...] = () + conflicts: tuple[DownloadConflict, ...] = () + unverified: tuple[DownloadItem, ...] = () def __post_init__(self) -> None: """Copy the selection and validate the source fields and rate.""" - object.__setattr__(self, "items", tuple(self.items)) - object.__setattr__(self, "unknown_paths", tuple(self.unknown_paths)) + for name in ("items", "unknown_paths", "skipped", "conflicts", + "unverified"): + object.__setattr__(self, name, tuple(getattr(self, name))) if any(not isinstance(path, str) for path in self.unknown_paths): raise TypeError("unknown_paths must contain strings") + for name in ("skipped", "unverified"): + if any(not isinstance(item, DownloadItem) + for item in getattr(self, name)): + raise TypeError(f"{name} must contain DownloadItem values") + if any(not isinstance(conflict, DownloadConflict) + for conflict in self.conflicts): + raise TypeError("conflicts must contain DownloadConflict values") object.__setattr__( self, "output_path", Path(self.output_path).expanduser().absolute()) if any(not isinstance(item, DownloadItem) for item in self.items): @@ -140,8 +177,9 @@ def summary(self) -> str: Returns: str: File count, size and duration estimates, source, destination, - and the number of unknown paths. URL credentials, query - parameters, and fragments are omitted. + and the numbers of skipped and unverified files, conflicts and + unknown paths. + URL credentials, query parameters, and fragments are omitted. """ size = f"{self.known_bytes:,} bytes" if self.unknown_size_count: @@ -154,8 +192,13 @@ def summary(self) -> str: source = urlunsplit(parsed._replace( netloc=parsed.netloc.rsplit("@", 1)[-1], query="", fragment="")) backend = f"\nBackend: {self.remote_backend}" if self.remote_backend else "" - unknown = (f"\n{len(self.unknown_paths)} path(s) not in the catalog" - if self.unknown_paths else "") + notes = "".join(f"\n{count} {text}" for count, text in ( + (len(self.skipped), "file(s) already downloaded; skipped"), + (len(self.unverified), + "file(s) present (size matches, not verified); skipped"), + (len(self.conflicts), "existing file(s) conflict with the catalog"), + (len(self.unknown_paths), "path(s) not in the catalog"), + ) if count) return (f"{self.file_count} file(s); {size}; estimated duration: {duration}" f"\nSource: {source}{backend}" - f"\nDestination: {self.output_path}{unknown}") + f"\nDestination: {self.output_path}{notes}") diff --git a/test/test_cli.py b/test/test_cli.py index 63bbd06..1e7a4d6 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -1144,3 +1144,134 @@ def reject(*args, **kwargs): assert "1 path(s) not in the catalog; nothing was downloaded" in result.output assert "Download these files?" not in result.output assert not (repo.worktree / "runs").exists() + + +def _checked_cli_catalog(path): + """Create a remote catalog whose files have published SHA-256 checksums.""" + from hashlib import sha256 + + repo = Repo.init(path) + repo.state.config = { + "data": [{"db": "data.tsv"}], + "remote": {"name": "origin", "url": "https://example.test/data/"}} + repo.state.data = pd.DataFrame([ + {"path": name, "checksum_algorithm": "sha256", + "checksum": sha256(content).hexdigest(), "size_bytes": len(content)} + for name, content in [("a.fits", b"aaaa"), ("b.fits", b"bbbb"), + ("c.fits", b"cccc")]]) + repo.dothm.save_state(repo.state) + repo.dothm.index.commit("Catalog checked remote files") + return repo + + +def test_cli_dry_run_reports_skipped_conflicting_and_unknown_files( + monkeypatch, tmp_path): + repo = _checked_cli_catalog(tmp_path / "project") + (repo.worktree / "a.fits").write_bytes(b"aaaa") + (repo.worktree / "b.fits").write_bytes(b"xxxx") + monkeypatch.chdir(repo.worktree) + result = CliRunner().invoke(hallmark, ["download", ".", "missing.fits", + "--dry-run"]) + assert result.exit_code == 0, result.output + assert "Files to download:\n c.fits\n" in result.output + assert "Already downloaded (skipped):\n a.fits\n" in result.output + assert "b.fits (different contents)" in result.output + assert "Delete conflicting files first to replace them" in result.output + assert "Not in the catalog:\n missing.fits\n" in result.output + assert (repo.worktree / "b.fits").read_bytes() == b"xxxx" + + +def test_cli_conflict_downloads_nothing_and_never_prompts(monkeypatch, tmp_path): + repo = _checked_cli_catalog(tmp_path / "project") + (repo.worktree / "b.fits").write_bytes(b"xxxx") + monkeypatch.chdir(repo.worktree) + + def reject(*args, **kwargs): + raise AssertionError("a conflict must not contact the server") + + monkeypatch.setattr(requests, "Session", reject) + result = CliRunner().invoke(hallmark, ["download", "--all"], input="y\n") + assert result.exit_code == 1 + assert "b.fits (different contents)" in result.output + assert "Delete conflicting files first to replace them" in result.output + assert "nothing was downloaded" in result.output + assert "Download these files?" not in result.output + assert not (repo.worktree / "a.fits").exists() + assert (repo.worktree / "b.fits").read_bytes() == b"xxxx" + + +def test_cli_second_download_skips_verified_files(monkeypatch, tmp_path): + from mock_server import MockServer + + repo = _checked_cli_catalog(tmp_path / "project") + server = MockServer("https://example.test/data/") + for name in ("a.fits", "b.fits", "c.fits"): + server.add_file(name, name[0].encode() * 4) + requested = [] + original_get = server.get + + def get(url, **kwargs): + requested.append(url) + return original_get(url, **kwargs) + + server.get = get + monkeypatch.setattr(requests, "Session", lambda: server) + monkeypatch.chdir(repo.worktree) + runner = CliRunner() + result = runner.invoke(hallmark, ["download", "a.fits"], input="y\n") + assert result.exit_code == 0, result.output + assert requested == ["https://example.test/data/a.fits"] + result = runner.invoke(hallmark, ["download", "--all"], input="y\n") + assert result.exit_code == 0, result.output + assert "1 file(s) already downloaded; skipped" in result.output + assert sorted(requested[1:]) == ["https://example.test/data/b.fits", + "https://example.test/data/c.fits"] + result = runner.invoke(hallmark, ["download", "--all"]) + assert result.exit_code == 0, result.output + assert "All selected files are already downloaded." in result.output + assert "Download these files?" not in result.output + assert len(requested) == 3 + + +def test_cli_retry_after_a_failed_clone_downloads_only_missing_files( + monkeypatch, tmp_path): + from mock_server import MockServer + + source = Repo.init(tmp_path / "source") + source.state.config = { + "data": [{"db": "data.tsv"}], + "remote": {"name": "origin", "url": "https://example.test/data/"}} + # Like an SSH export without a checksum manifest: only sizes are known. + source.state.data = pd.DataFrame([{"path": "a.fits", "size_bytes": 1}, + {"path": "b.fits", "size_bytes": 1}]) + source.dothm.save_state(source.state) + source.dothm.index.commit("Catalog files without checksums") + server = MockServer("https://example.test/data/") + server.add_file("a.fits", b"A") + server.add_file("b.fits", b"B") + requested, failing = [], {"b.fits"} + original_get = server.get + + def get(url, **kwargs): + requested.append(url) + if any(url.endswith(name) for name in failing): + raise requests.ConnectionError("transient network failure") + return original_get(url, **kwargs) + + server.get = get + monkeypatch.setattr(requests, "Session", lambda: server) + target = tmp_path / "copy" + result = CliRunner().invoke(hallmark, [ + "clone", str(source.dothm.path), str(target)], input="y\n") + assert result.exit_code != 0 + assert (target / "a.fits").read_bytes() == b"A" + failing.clear() + requested.clear() + monkeypatch.chdir(target) + result = CliRunner().invoke(hallmark, ["download", "--all", "--dry-run"]) + assert "Present (size matches, not verified):\n a.fits\n" in result.output + result = CliRunner().invoke(hallmark, ["download", "--all"], input="y\n") + assert result.exit_code == 0, result.output + assert "1 file(s) present (size matches, not verified); skipped" in result.output + assert requested == ["https://example.test/data/b.fits"] + assert (target / "b.fits").read_bytes() == b"B" diff --git a/test/test_download_plan.py b/test/test_download_plan.py index 7a84e02..3f204d9 100644 --- a/test/test_download_plan.py +++ b/test/test_download_plan.py @@ -333,3 +333,161 @@ def test_missing_plugin_can_be_planned_but_not_executed(catalog): with pytest.raises(DownloadError, match="missing-survey-plugin"): execute_download_plan(catalog, plan, approved=True) assert not (catalog.worktree / "nested/a.fits").exists() + + +def _reject_server(monkeypatch): + def reject(*args, **kwargs): + raise AssertionError("this download must not contact a server") + + monkeypatch.setattr("hallmark.remote.download.OperationContext", reject) + monkeypatch.setattr("requests.sessions.Session.request", reject) + + +def test_verified_existing_files_are_skipped_without_a_transfer( + catalog, monkeypatch): + (catalog.worktree / "nested").mkdir() + existing = catalog.worktree / "nested/a.fits" + existing.write_bytes(b"abcdef") + _reject_server(monkeypatch) + plan = plan_download(catalog, file_paths=["nested"]) + assert plan.items == () + assert [item.relative_path.as_posix() for item in plan.skipped] == [ + "nested/a.fits"] + assert plan.conflicts == () + assert "1 file(s) already downloaded; skipped" in plan.summary() + assert execute_download_plan(catalog, plan) == { + "succeeded": 0, "failed": 0, "total_bytes": 0, "errors": []} + assert existing.read_bytes() == b"abcdef" + + +@pytest.mark.parametrize("make_existing, reason", [ + (lambda path: path.write_bytes(b"ABCDEF"), "different contents"), + (lambda path: path.write_bytes(b"changed"), "7 bytes, but the catalog records 6"), + (lambda path: path.mkdir(), "not a regular file"), +]) +def test_conflicting_existing_file_stops_the_whole_download( + catalog, monkeypatch, make_existing, reason): + (catalog.worktree / "nested").mkdir() + existing = catalog.worktree / "nested/a.fits" + make_existing(existing) + plan = plan_download(catalog, file_paths=["nested/a.fits", "empty.fits"]) + assert [item.relative_path.as_posix() for item in plan.items] == ["empty.fits"] + assert [(conflict.item.relative_path.as_posix(), conflict.reason) + for conflict in plan.conflicts] == [("nested/a.fits", reason)] + assert "1 existing file(s) conflict with the catalog" in plan.summary() + _reject_server(monkeypatch) + with pytest.raises(DownloadError) as error: + execute_download_plan(catalog, plan, approved=True) + assert "nested/a.fits" in str(error.value) + assert "nothing was downloaded" in str(error.value) + assert "Delete conflicting files first to replace them" in str(error.value) + assert not (catalog.worktree / "empty.fits").exists() + + +def test_existing_file_without_catalog_checksum_is_a_conflict(catalog): + (catalog.worktree / "b.txt").write_bytes(b"cannot be verified") + plan = plan_download(catalog, file_paths=["b.txt"]) + assert plan.items == () + assert plan.conflicts[0].item.relative_path == Path("b.txt") + assert plan.conflicts[0].reason == "no catalog checksum or size to verify it" + + +def test_unverifiable_file_with_the_catalog_size_is_skipped(catalog, monkeypatch): + present = catalog.worktree / "empty.fits" + present.write_bytes(b"") + plan = plan_download(catalog, file_paths=["empty.fits", "b.txt"]) + assert [item.relative_path.as_posix() for item in plan.unverified] == [ + "empty.fits"] + assert [item.relative_path.as_posix() for item in plan.items] == ["b.txt"] + assert plan.skipped == () and plan.conflicts == () + assert "1 file(s) present (size matches, not verified); skipped" in plan.summary() + present.write_bytes(b"grown") + plan = plan_download(catalog, file_paths=["empty.fits"]) + assert [(conflict.item.relative_path.as_posix(), conflict.reason) + for conflict in plan.conflicts] == [ + ("empty.fits", "5 bytes, but the catalog records 0")] + + +def test_wrong_size_is_a_conflict_without_reading_the_file(catalog, monkeypatch): + (catalog.worktree / "nested").mkdir() + (catalog.worktree / "nested/a.fits").write_bytes(b"x" * 10) + + def reject(*args, **kwargs): + raise AssertionError("a file of the wrong size must not be hashed") + + monkeypatch.setattr("hallmark.remote.download.calculate_file_checksum", reject) + plan = plan_download(catalog, file_paths=["nested/a.fits"]) + assert plan.conflicts[0].reason == "10 bytes, but the catalog records 6" + + +@pytest.mark.parametrize("digest, content, expected", [ + (sha256(b"abcdef").hexdigest(), b"abcdef", "skipped"), + (sha256(b"abcdef").hexdigest(), b"ABCDEF", "different contents"), + ("a" * 36, b"abcdef", "unverified"), +]) +def test_unknown_checksum_algorithm_is_inferred_from_the_digest_length( + tmp_path, digest, content, expected): + catalog = _folder_catalog(tmp_path) + frame = pd.DataFrame([{"path": "run.h5", "checksum_algorithm": "unknown", + "checksum": digest, "size_bytes": "6"}]) + frame.to_csv(catalog.dothm.path / "data.tsv", sep="\t", index=False) + (tmp_path / "run.h5").write_bytes(content) + plan = plan_download(catalog, file_paths=["run.h5"]) + if expected == "skipped": + assert plan.skipped[0].checksum == ("sha256", digest) + elif expected == "unverified": + assert plan.unverified[0].checksum is None + else: + assert plan.conflicts[0].reason == expected + + +def test_existing_files_are_hashed_with_a_bounded_pool(catalog, monkeypatch): + from concurrent.futures import ThreadPoolExecutor + + workers = [] + + def pool(max_workers=None): + workers.append(max_workers) + return ThreadPoolExecutor(max_workers=max_workers) + + monkeypatch.setattr("hallmark.remote.download.ThreadPoolExecutor", pool) + monkeypatch.setattr("hallmark.remote.download.os.cpu_count", lambda: 64) + (catalog.worktree / "empty.fits").write_bytes(b"") + plan_download(catalog, file_paths=["empty.fits"]) + assert workers == [8] + + +def test_file_appearing_after_planning_is_never_replaced(catalog, monkeypatch): + plan = plan_download(catalog, file_paths=["nested/a.fits"]) + assert plan.file_count == 1 + late = catalog.worktree / "nested/a.fits" + + class Context: + def __init__(self, remote, output_root): + self.output_root = output_root + self.transport = self + self.on_bytes = None + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + def prepare(self): + pass + + def check_cancelled(self): + pass + + def fetch(self, relative_path, destination, **kwargs): + destination.write_bytes(b"abcdef") + # Another program creates the file while the transfer runs. + late.write_bytes(b"written meanwhile") + + monkeypatch.setattr("hallmark.remote.download.OperationContext", Context) + result = execute_download_plan(catalog, plan, approved=True) + assert result["succeeded"] == 0 and result["failed"] == 1 + assert "appeared after the download was planned" in result["errors"][0] + assert late.read_bytes() == b"written meanwhile" + assert not list(late.parent.glob("*.part")) diff --git a/test/test_ssh_integration.py b/test/test_ssh_integration.py index c5b272b..afa8e18 100644 --- a/test/test_ssh_integration.py +++ b/test/test_ssh_integration.py @@ -175,6 +175,8 @@ def test_explicit_endpoint_and_missing_file(ssh_server, tmp_path): output.write_bytes(b"keep") result = download_selection(repo, repo.worktree, [(Path('missing'), None)]) assert result["failed"] == 1 + # The transfer itself fails; the existing-file guard is never reached. + assert "appeared after" not in result["errors"][0] assert output.read_bytes() == b"keep" assert not list(Path(repo.worktree).glob("*.part")) @@ -403,6 +405,8 @@ def test_permission_denied_preserves_existing_file(ssh_server, tmp_path): try: result = download_selection(repo, repo.worktree, [(Path('private'), None)]) assert result["failed"] == 1 + # The transfer itself fails; the existing-file guard is never reached. + assert "appeared after" not in result["errors"][0] assert destination.read_bytes() == b"original" assert not list(Path(repo.worktree).glob("*.part")) finally: @@ -444,12 +448,14 @@ def slow_prepare(self): for transport in transports: transport.context.cancel() assert result["failed"] == 1 + # The transfer itself fails; the existing-file guard is never reached. + assert "appeared after" not in result["errors"][0] assert destination.read_bytes() == b"original" assert not list(Path(repo.worktree).glob("*.part")) assert not transports[0]._processes -def test_add_commit_download_clone_workflow(ssh_server, tmp_path): +def test_add_commit_download_clone_workflow(ssh_server, tmp_path, monkeypatch): root = ssh_server["root"] (root / "nested").mkdir() (root / "nested/item_1.dat").write_bytes(b"science") @@ -467,6 +473,17 @@ def test_add_commit_download_clone_workflow(ssh_server, tmp_path): assert (repo.worktree / "nested/item_1.dat").read_bytes() == b"science" assert not (repo.worktree / "bad_2.dat").exists() assert not list(repo.worktree.rglob("*.part")) + downloaded = (repo.worktree / "nested/item_1.dat").stat() + monkeypatch.chdir(repo.worktree) + result = CliRunner().invoke(hallmark, ["download", "--all"], input="y\n") + # The verified file is skipped; only the failed one is tried again. + assert "1 file(s) already downloaded; skipped" in result.output + assert "Failed to download 1 file(s)" in result.output + assert "bad_2.dat" in result.output + assert (repo.worktree / "nested/item_1.dat").stat().st_mtime_ns == \ + downloaded.st_mtime_ns + assert (repo.worktree / "nested/item_1.dat").stat().st_ino == downloaded.st_ino + monkeypatch.chdir(tmp_path) clone = Repo.clone(str(repo.dothm.path), tmp_path / "python-clone", download=False) assert clone.dothm.head.commit.hexsha == repo.dothm.head.commit.hexsha assert not (clone.worktree / "nested").exists() From 45d8bdbcd987e7c6267050a6e926497adafd0529 Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 21:01:52 -0700 Subject: [PATCH 08/10] Clone into a default folder and refuse unsafe destinations hm clone SOURCE without a directory, and Repo.clone without a path, now create a folder in the current folder named after the source, ignoring a .git suffix and naming a .hm folder after its worktree. An existing empty folder is accepted. If the clone fails, only what it created is removed: the .hm it added to an existing worktree folder, or the contents of an existing bare folder, never following a symbolic link. Destinations inside a Hallmark repository or overlapping a local source are refused before the source is read. Snapshot imports into a bare .hm destination no longer collide with the folder created for them. Co-Authored-By: Claude Opus 5.5 --- README.rst | 6 +- doc/api.rst | 6 +- doc/private_data.rst | 9 ++ mod/hallmark/cli.py | 10 +- mod/hallmark/remote/clone.py | 154 +++++++++++++++++++++++++--- mod/hallmark/repo/__init__.py | 18 +++- test/test_catalog.py | 27 +++++ test/test_clone_defaults.py | 183 ++++++++++++++++++++++++++++++++++ 8 files changed, 387 insertions(+), 26 deletions(-) diff --git a/README.rst b/README.rst index 56de15d..50e6f0c 100644 --- a/README.rst +++ b/README.rst @@ -104,8 +104,10 @@ Optional authentication profiles remain local and can be selected with The obsolete ``build`` and remote initialization interfaces have been removed. Use ``init``, ``add`` and ``commit`` for new remote catalogs. -Use ``hm clone CATALOG PATH`` for an existing Git-hosted ``.hm`` or a -published HTTP/SFTP catalog snapshot. Catalogs can live on GitHub or another +Use ``hm clone CATALOG [PATH]`` for an existing Git-hosted ``.hm`` or a +published HTTP/SFTP catalog snapshot. ``PATH`` defaults to a new folder named +after the source and must otherwise be new or empty, outside any Hallmark +repository. Catalogs can live on GitHub or another server while their data remotes point elsewhere. Git clones preserve the full catalog and its history. Clone displays a data download plan and asks for confirmation by default; declining keeps the catalog. Use ``hm clone CATALOG diff --git a/doc/api.rst b/doc/api.rst index 4c28d52..9ec0190 100644 --- a/doc/api.rst +++ b/doc/api.rst @@ -98,8 +98,10 @@ Stage and commit a remote catalog without downloading dataset files:: hm add 'ssh://lab-data/srv/data/run{run:d}.h5' hm commit -m 'Add remote data' -Use ``hm clone SOURCE PATH`` for an existing Hallmark Git repository -or published HTTP/SFTP snapshot. Git clones retain the full catalog and its +Use ``hm clone SOURCE [PATH]`` for an existing Hallmark Git repository +or published HTTP/SFTP snapshot. ``PATH`` defaults to a new folder named after +the source; it must be new or empty and outside any Hallmark repository, +including the source. Git clones retain the full catalog and its history; snapshots start new local history. Use ``--source-type git`` or ``--source-type catalog`` to override automatic detection. diff --git a/doc/private_data.rst b/doc/private_data.rst index 03ac374..cf5ba1a 100644 --- a/doc/private_data.rst +++ b/doc/private_data.rst @@ -210,6 +210,15 @@ HTTP/SFTP directory containing its metadata. For example, copy this catalog: hm clone ./.hm ../lab-copy +Without a destination, ``hm clone`` creates a folder in the current folder +named after the source: ``lab`` for ``./lab/.hm`` or +``https://github.com/example/lab.git``. A destination must be a new or empty +folder outside every Hallmark repository, including the source, because +repositories cannot be nested; these checks happen before the source is read. +A failed clone removes only what it created: the destination folder if it was +new, or the ``.hm`` it added to an existing empty folder. It never follows a +symbolic link while cleaning up. + Git endpoints preserve the complete catalog and history. Published HTTP/SFTP snapshots start new local history. A snapshot URL may name the metadata directory itself or its parent containing ``.hm``. Use ``--source-type git`` diff --git a/mod/hallmark/cli.py b/mod/hallmark/cli.py index 693c691..574dddb 100644 --- a/mod/hallmark/cli.py +++ b/mod/hallmark/cli.py @@ -25,6 +25,7 @@ from . import Repo from .remote.download import DownloadError +from .remote.clone import default_clone_destination from .remote.discovery import path_matches from .error import CheckoutError, CloneError, DothmError from .utils import use_working_directory @@ -556,7 +557,7 @@ def download(repo, files, tsv_names, download_all, filters, fmt, remote_name, @hallmark.command(short_help="Clone an existing Hallmark catalog.") @click.argument("url") -@click.argument("path") +@click.argument("path", required=False) @click.option("--auth", help="Optional local SSH authentication profile.") @click.option("--filter", "filters", multiple=True, help="Select paths matching a glob. ** matches recursively. " @@ -575,6 +576,8 @@ def clone(url, path, auth, filters, fmt, source_type, no_download, max_workers): """ Clone an existing Git catalog or published catalog snapshot at PATH. + PATH defaults to a new folder named after the source. It must be new or + an empty folder, outside any Hallmark repository and the source. By default, copy the catalog then ask before downloading dataset files. Declining keeps the catalog and exits successfully. --no-download skips data and the prompt; bare destinations require it. --filter and --fmt narrow the @@ -583,6 +586,11 @@ def clone(url, path, auth, filters, fmt, source_type, no_download, max_workers): """ if (filters or fmt is not None) and no_download: raise ClickException("--filter and --fmt cannot be used with --no-download") + if path is None: + try: + path = str(default_clone_destination(url)) + except CloneError as exc: + raise click.UsageError(str(exc)) from exc if not no_download and Repo.resolve_repo_paths(path)[1] is None: raise ClickException("Bare clones require --no-download") diff --git a/mod/hallmark/remote/clone.py b/mod/hallmark/remote/clone.py index 35bcabb..fdfbca6 100644 --- a/mod/hallmark/remote/clone.py +++ b/mod/hallmark/remote/clone.py @@ -6,7 +6,7 @@ from io import StringIO from pathlib import Path from shutil import rmtree -from urllib.parse import urlsplit +from urllib.parse import unquote, urlsplit import pandas as pd import yaml @@ -135,6 +135,121 @@ def _commit_catalog_metadata(repo, files, message): +def _scp_style(url): + """Return whether a Git source uses the SCP-like ``host:path`` form.""" + return ("://" not in url and not Path(url).expanduser().exists() + and bool(re.match(r"^(?:[^/@:]+@)?[^/:]+:.+", url))) + + +def default_clone_destination(source) -> Path: + """ + Name the folder that a clone creates when no destination is given. + + The name is the last segment of the source path or URL without a ``.git`` + suffix. A ``.hm`` folder is named after its worktree, and ``NAME.hm`` + after ``NAME``, so the default is never a bare repository. + + Args: + source (str): Git URL, local path, or published catalog URL. + + Returns: + Path: Relative folder name in the current folder. + + Raises: + CloneError: If the source has no usable name. + """ + text = str(source).strip() + if "://" in text: + path = unquote(urlsplit(text).path) + elif _scp_style(text): + path = text.split(":", 1)[1] + else: + path = text + segments = [part for part in path.split("/") if part not in ("", ".")] + while segments: + name = segments.pop() + if name == ".hm": + continue + for suffix in (".git", ".hm"): + if name.endswith(suffix): + name = name[:-len(suffix)] + if name and name != "..": + return Path(name) + break + raise CloneError("Cannot name a folder after this source; give a DIRECTORY") + + +def _local_source_root(url): + """Return the folder of a local source repository, or None for remote URLs.""" + if url.startswith("file://"): + local = Path(unquote(urlsplit(url).path)) + elif "://" in url: + return None + else: + local = Path(url).expanduser() + if not local.exists(): + return None + local = local.resolve() + return local.parent if local.name == ".hm" else local + + +def _check_destination(cls, url, destination, display_path): + """ + Refuse a destination that is not empty, is nested in a repository, or + overlaps a local source. Nothing is created or contacted. + + Returns: + bool: True when the destination is an existing empty folder. + """ + if destination.is_symlink() or (destination.exists() and ( + not destination.is_dir() or any(destination.iterdir()))): + raise DestinationExistsError( + f"fatal: destination path '{display_path}' already exists and is " + "not empty.") + target = destination.resolve() + source = _local_source_root(url) + if source is not None and (target == source or source in target.parents + or target in source.parents): + raise CloneError( + f"Destination '{display_path}' overlaps the source repository " + f"'{source}'; choose a separate folder") + enclosing = cls.find_root(target.parent) + if enclosing is not None: + raise CloneError( + f"Destination '{display_path}' is inside the Hallmark repository at " + f"'{enclosing}'; nested repositories are not supported, so choose " + "a folder outside it") + return destination.is_dir() + + +def _remove_partial_clone(cls, destination, *, existed): + """ + Remove what a failed clone created, never following a symbolic link. + + A destination the clone created is removed. In a worktree folder that + existed only its new ``.hm`` is removed; a bare folder that existed, and + held nothing before, is emptied again. + """ + if destination.is_symlink(): + return + if not existed: + rmtree(destination, ignore_errors=True) + return + if cls.resolve_repo_paths(destination)[1] is not None: + dothm = destination / ".hm" + if dothm.is_symlink(): + dothm.unlink() + else: + rmtree(dothm, ignore_errors=True) + return + destination.mkdir(exist_ok=True) + for child in destination.iterdir(): + if child.is_dir() and not child.is_symlink(): + rmtree(child, ignore_errors=True) + else: + child.unlink(missing_ok=True) + + def _is_git_source(url, source_type): """Determine whether the selected source should be cloned with Git.""" if source_type == "git": @@ -170,10 +285,12 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): """ Clone an existing Git catalog or published HTTP/SFTP catalog snapshot. - Source URLs with credentials and existing destinations are rejected - before source access. An incomplete destination created by this call is - removed on failure. Dataset discovery - belongs to ``Repo.add(URL)``. + Source URLs with credentials are rejected first. The destination must be + new or an empty folder, outside any Hallmark repository and the local + source; this is checked before source access. On failure, only what this + call created is removed: the destination it created, or the ``.hm`` it + added to an existing worktree folder. An existing bare folder is emptied + again. Dataset discovery belongs to ``Repo.add(URL)``. Args: cls: Repository class used to initialize or open the result. @@ -186,8 +303,10 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): Repo: Repository containing complete catalog metadata without payloads. Raises: - DestinationExistsError: If the destination already exists. - CloneError: If a requested catalog is missing or invalid. + DestinationExistsError: If the destination exists and is not an empty + folder. + CloneError: If the destination is inside a repository or overlaps a + local source, or a requested catalog is missing or invalid. ValueError: If source options are invalid. DownloadError: If remote metadata cannot be read or validated. """ @@ -197,17 +316,17 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): reject_url_credentials(str(url)) url = str(url) destination = Path(path).expanduser().absolute() - if destination.exists() or destination.is_symlink(): - raise DestinationExistsError( - f"fatal: destination path '{path}' already exists and is not empty.") + existed = _check_destination(cls, url, destination, path) is_git = _is_git_source(url, source_type) if is_git and auth is not None: raise ValueError("Git cloning uses Git/SSH authentication, not auth profiles") - destination.parent.mkdir(parents=True, exist_ok=True) - try: - destination.mkdir() - except FileExistsError as exc: - raise DestinationExistsError(f"Destination already exists: {path}") from exc + if not existed: + destination.parent.mkdir(parents=True, exist_ok=True) + try: + destination.mkdir() + except FileExistsError as exc: + raise DestinationExistsError( + f"Destination already exists: {path}") from exc try: if is_git: return _clone_git(cls, url, destination, path, auth) @@ -224,11 +343,14 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): raise CloneError("No published Hallmark catalog at this URL; " "use hm init PATH, then hm add URL " "for a raw dataset") + if cls.resolve_repo_paths(destination)[1] is None: + # Initialization creates a bare ".hm" folder itself. + destination.rmdir() repo = cls.init(destination) for name, contents in snapshot.items(): (repo.dothm.path / name).write_text(contents, encoding="utf-8") _commit_catalog_metadata(repo, snapshot, "Import published catalog snapshot") return repo except BaseException: - rmtree(destination, ignore_errors=True) + _remove_partial_clone(cls, destination, existed=existed) raise diff --git a/mod/hallmark/repo/__init__.py b/mod/hallmark/repo/__init__.py index 4cf7d9f..df1ee2e 100644 --- a/mod/hallmark/repo/__init__.py +++ b/mod/hallmark/repo/__init__.py @@ -287,7 +287,7 @@ def _download_cloned_files(self, *, approve, max_workers, progress, def clone( cls, url: str, - path: Union[Path, str], + path: Optional[Union[Path, str]] = None, *, auth: Optional[str] = None, filter=None, @@ -308,7 +308,10 @@ def clone( Args: url (str): Existing Git catalog or published snapshot location. - path (Path | str): New worktree or bare ``.hm`` repository path. + path (Path | str, optional): New or empty worktree folder, or bare + ``.hm`` repository path, outside any repository and the local + source. Defaults to a folder in the current folder named after + the source. auth (str, optional): Local profile for snapshot metadata access. Git sources use Git's authentication configuration instead. filter (str | list[str], optional): Download selection globs. @@ -331,16 +334,21 @@ def clone( when a download was attempted, including an empty selection. Raises: - DestinationExistsError: If the destination already exists. - CloneError: If a requested catalog is missing or invalid. + DestinationExistsError: If the destination exists and is not an + empty folder. + CloneError: If the destination is nested in a repository or + overlaps the source, or a requested catalog is missing or + invalid. ValueError: If source options are invalid or conflict. DownloadError: If metadata access or downloading fails, or a download is requested for a bare destination. """ - from ..remote.clone import clone_catalog + from ..remote.clone import clone_catalog, default_clone_destination from ..remote.discovery import path_matches from ..remote.download import DownloadError, _require_positive_integer + if path is None: + path = default_clone_destination(url) _require_positive_integer(max_workers, label="max_workers") if (filter is not None or fmt is not None) and not download: raise ValueError("clone filter and fmt require download=True; " diff --git a/test/test_catalog.py b/test/test_catalog.py index d5bd592..622c609 100644 --- a/test/test_catalog.py +++ b/test/test_catalog.py @@ -398,3 +398,30 @@ def get(url, **kwargs): assert all(url.startswith(catalog_url) for url in metadata_server[1]) assert (repo.worktree / "a.fits").read_bytes() == b"fits" assert repo.download_result["succeeded"] == 1 + + +@pytest.mark.parametrize("name", ["empty", "empty.hm"]) +def test_snapshot_clone_into_existing_empty_folder(tmp_path, metadata_server, name): + pages, _ = metadata_server + root = "https://example.test/published/" + pages[root + "config.yml"] = "data:\n- db: data.tsv\n" + pages[root + "meta.yml"] = "{}\n" + pages[root + "data.tsv"] = "path\na.h5\n" + target = tmp_path / name + target.mkdir() + repo = Repo.clone(root, target, download=False) + assert repo.state.data["path"].tolist() == ["a.h5"] + assert repo.dothm.path == (target if name.endswith(".hm") else target / ".hm") + + +def test_failed_snapshot_clone_keeps_existing_folder_empty(tmp_path, metadata_server): + pages, _ = metadata_server + root = "https://example.test/published/" + pages[root + "config.yml"] = "data:\n- db: data.tsv\n" + pages[root + "meta.yml"] = "{}\n" + pages[root + "data.tsv"] = "path\n../escape\n" + target = tmp_path / "empty" + target.mkdir() + with pytest.raises((CloneError, ValueError)): + Repo.clone(root, target, download=False) + assert target.is_dir() and list(target.iterdir()) == [] diff --git a/test/test_clone_defaults.py b/test/test_clone_defaults.py index 0cfbb5b..7ade070 100644 --- a/test/test_clone_defaults.py +++ b/test/test_clone_defaults.py @@ -1,15 +1,34 @@ from hashlib import sha1 +from pathlib import Path +import pandas as pd import pytest import requests from click.testing import CliRunner from hallmark import Repo from hallmark.cli import hallmark +from hallmark.error import CloneError, DestinationExistsError +from hallmark.remote.clone import default_clone_destination from hallmark.transport.base import DownloadError from mock_server import MockServer +def _remote_catalog_source(path, files=(("item1.txt", b"science"),)): + """Create a committed catalog of files that stay on a mock HTTP server.""" + repo = Repo.init(path) + repo.state.config = { + "data": [{"db": "data.tsv"}], + "remote": {"name": "origin", "url": "https://clone.test/data/"}} + repo.state.data = pd.DataFrame([ + {"path": name, "checksum_algorithm": "sha1", + "checksum": sha1(content).hexdigest(), "size_bytes": len(content)} + for name, content in files]) + repo.dothm.save_state(repo.state) + repo.dothm.index.commit("Catalogued remote data") + return repo + + @pytest.fixture def clone_source(tmp_path, monkeypatch): source = Repo.init(tmp_path / 'source') @@ -186,3 +205,167 @@ def test_clone_download_failure_keeps_catalog_and_valid_files( assert (target / 'item2.txt').read_bytes() == b'science' assert sorted(calls) == [ 'https://clone.test/data/item1.txt', 'https://clone.test/data/item2.txt'] + + +@pytest.mark.parametrize("source, expected", [ + ("https://github.com/team/eht-data.git", "eht-data"), + ("git@github.com:team/eht-data.git", "eht-data"), + ("ssh://git.example.test/srv/eht-data.git/", "eht-data"), + ("/data/eht-data/.hm", "eht-data"), + ("/data/eht-data/.hm/", "eht-data"), + ("/data/eht-data", "eht-data"), + ("../catalog.hm", "catalog"), + ("https://catalogs.example.org/lab/", "lab"), + ("https://catalogs.example.org/published/.hm/", "published"), + ("file:///data/eht%20data/.hm", "eht data"), +]) +def test_default_destination_is_named_after_the_source(source, expected): + assert default_clone_destination(source) == Path(expected) + + +@pytest.mark.parametrize("source", ["https://example.test/", "/", ".hm", ".."]) +def test_unnamed_source_requires_a_directory(source): + with pytest.raises(CloneError, match="DIRECTORY"): + default_clone_destination(source) + + +def test_cli_clone_defaults_to_a_folder_named_after_the_source( + tmp_path, monkeypatch): + source = _remote_catalog_source(tmp_path / "eht-data") + work = tmp_path / "work" + work.mkdir() + monkeypatch.chdir(work) + result = CliRunner().invoke(hallmark, [ + "clone", str(source.dothm.path), "--no-download"]) + assert result.exit_code == 0, result.output + assert 'Successfully cloned to "eht-data"' in result.output + assert Repo(work / "eht-data").dothm.head.commit.hexsha == \ + source.dothm.head.commit.hexsha + other = tmp_path / "other" + other.mkdir() + monkeypatch.chdir(other) + python = Repo.clone(str(source.worktree), download=False) + assert python.worktree == other / "eht-data" + + +@pytest.mark.parametrize("cli", [False, True]) +def test_clone_into_an_existing_empty_folder(tmp_path, cli): + source = _remote_catalog_source(tmp_path / "source") + target = tmp_path / "empty" + target.mkdir() + if cli: + result = CliRunner().invoke(hallmark, [ + "clone", str(source.dothm.path), str(target), "--no-download"]) + assert result.exit_code == 0, result.output + else: + Repo.clone(str(source.dothm.path), target, download=False) + assert Repo(target).dothm.head.commit.hexsha == source.dothm.head.commit.hexsha + + +def test_failed_clone_empties_an_existing_folder_but_keeps_it(tmp_path): + source = _remote_catalog_source(tmp_path / "source") + (source.dothm.path / "meta.yml").unlink() + source.dothm.index.remove(["meta.yml"]) + source.dothm.index.commit("Remove required metadata") + target = tmp_path / "empty" + target.mkdir() + with pytest.raises(CloneError, match="missing required file"): + Repo.clone(str(source.dothm.path), target, download=False) + assert target.is_dir() + assert list(target.iterdir()) == [] + + +def test_failed_clone_keeps_files_added_to_an_existing_folder( + tmp_path, monkeypatch): + from hallmark.remote.clone import Dothm + + source = _remote_catalog_source(tmp_path / "source") + target = tmp_path / "empty" + target.mkdir() + original = Dothm.clone + + def clone_then_fail(url, destination, **kwargs): + original(url, destination, **kwargs) + # Someone saves a file in the folder while the clone runs. + (target / "notes.txt").write_text("keep me") + raise CloneError("simulated failure") + + monkeypatch.setattr("hallmark.remote.clone.Dothm.clone", clone_then_fail) + with pytest.raises(CloneError, match="simulated failure"): + Repo.clone(str(source.dothm.path), target, download=False) + assert sorted(path.name for path in target.iterdir()) == ["notes.txt"] + assert (target / "notes.txt").read_text() == "keep me" + + +def test_failed_clone_empties_an_existing_bare_folder(tmp_path): + source = _remote_catalog_source(tmp_path / "source") + (source.dothm.path / "meta.yml").unlink() + source.dothm.index.remove(["meta.yml"]) + source.dothm.index.commit("Remove required metadata") + target = tmp_path / "copy.hm" + target.mkdir() + with pytest.raises(CloneError, match="missing required file"): + Repo.clone(str(source.dothm.path), target, download=False) + assert target.is_dir() and list(target.iterdir()) == [] + + +def test_failed_clone_never_follows_a_swapped_in_link(tmp_path, monkeypatch): + from hallmark.remote.clone import Dothm + + source = _remote_catalog_source(tmp_path / "source") + target = tmp_path / "copy.hm" + target.mkdir() + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + (elsewhere / "keep.txt").write_text("keep me") + original = Dothm.clone + + def clone_then_swap(url, destination, **kwargs): + original(url, destination, **kwargs) + target.rename(tmp_path / "moved.hm") + target.symlink_to(elsewhere, target_is_directory=True) + raise CloneError("simulated failure") + + monkeypatch.setattr("hallmark.remote.clone.Dothm.clone", clone_then_swap) + with pytest.raises(CloneError, match="simulated failure"): + Repo.clone(str(source.dothm.path), target, download=False) + assert target.is_symlink() + assert (elsewhere / "keep.txt").read_text() == "keep me" + + +def test_clone_refuses_a_destination_inside_a_repository(tmp_path, monkeypatch): + source = _remote_catalog_source(tmp_path / "source") + other = Repo.init(tmp_path / "other") + (other.worktree / "sub").mkdir() + monkeypatch.chdir(other.worktree / "sub") + for arguments in (["clone", str(source.dothm.path), "--no-download"], + ["clone", str(source.dothm.path), "nested/copy"]): + result = CliRunner().invoke(hallmark, arguments) + assert result.exit_code == 1 + assert "inside the Hallmark repository" in result.output + assert str(other.worktree) in result.output + assert sorted(path.name for path in (other.worktree / "sub").iterdir()) == [] + + +@pytest.mark.parametrize("relative", ["source/copy", "source/.hm/copy"]) +def test_clone_refuses_to_overlap_a_local_source(tmp_path, relative): + source = _remote_catalog_source(tmp_path / "source") + with pytest.raises(CloneError, match="overlaps the source|inside the Hallmark"): + Repo.clone(str(source.dothm.path), tmp_path / relative, download=False) + assert not (tmp_path / relative).exists() + + +def test_clone_refuses_a_nonempty_folder_or_symlink(tmp_path): + source = _remote_catalog_source(tmp_path / "source") + full = tmp_path / "full" + full.mkdir() + (full / "keep.txt").write_text("keep") + empty = tmp_path / "empty" + empty.mkdir() + alias = tmp_path / "alias" + alias.symlink_to(empty, target_is_directory=True) + for target in (full, alias): + with pytest.raises(DestinationExistsError): + Repo.clone(str(source.dothm.path), target, download=False) + assert (full / "keep.txt").read_text() == "keep" + assert list(empty.iterdir()) == [] From 94bf8f5a37815f6d48faeb3858d7a4952e603176 Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 21:07:12 -0700 Subject: [PATCH 09/10] Refuse to clone repositories that track local files Local-file repositories keep their contents in .hm/objects, which clone cannot copy yet. Clone now refuses a source with any branch whose committed catalog records local file versions, with or without --no-download, and creates nothing: local sources are inspected read-only before anything is written, while Git-host clones and published snapshots are checked once their metadata is read, and the partial copy is removed together with any parent folders the clone created. Source branch names are validated first, so a name such as --output=x is refused rather than read by Git as an option. Empty repositories and remote catalogs still clone. Tests that cloned local-file repositories now use remote catalogs, and the remote-branch checkout test copies its source with Git. Co-Authored-By: Claude Opus 5.5 --- README.rst | 4 +- doc/private_data.rst | 5 ++ doc/usecase.rst | 3 +- mod/hallmark/remote/clone.py | 129 +++++++++++++++++++++++++++--- mod/hallmark/repo/__init__.py | 20 +---- mod/hallmark/repo/branches.py | 35 +++++++- test/test_catalog.py | 11 +-- test/test_clone_defaults.py | 146 +++++++++++++++++++++++++++++++--- test/test_hallmark.py | 39 ++++----- 9 files changed, 324 insertions(+), 68 deletions(-) diff --git a/README.rst b/README.rst index 50e6f0c..922e009 100644 --- a/README.rst +++ b/README.rst @@ -130,7 +130,9 @@ Remote ``add`` supports one dataset root and one ``data.tsv`` table per repository. Keep remote catalogs separate from locally versioned files. Local ``add`` selects files by filename format; ``commit`` stores their contents in ``.hm/objects``. Clone copies catalog history and downloads from -its configured data remote; transfer of a local object store is not implemented. +its configured data remote. Repositories with a branch that tracks local files +cannot be cloned yet: clone reports this and creates nothing, even with +``--no-download``. Empty repositories and remote catalogs can be cloned. Development and support ----------------------- diff --git a/doc/private_data.rst b/doc/private_data.rst index cf5ba1a..8a0814b 100644 --- a/doc/private_data.rst +++ b/doc/private_data.rst @@ -218,6 +218,11 @@ repositories cannot be nested; these checks happen before the source is read. A failed clone removes only what it created: the destination folder if it was new, or the ``.hm`` it added to an existing empty folder. It never follows a symbolic link while cleaning up. +Only empty repositories and remote catalogs can be cloned: if any branch of the +source tracks local files, whose contents live in its ``.hm/objects``, clone +reports it and creates nothing, even with ``--no-download``. A local source is +checked before anything is copied; a Git host or snapshot is checked after its +metadata is read, and the partial copy is removed. Git endpoints preserve the complete catalog and history. Published HTTP/SFTP snapshots start new local history. A snapshot URL may name the metadata diff --git a/doc/usecase.rst b/doc/usecase.rst index 928397c..466b62b 100644 --- a/doc/usecase.rst +++ b/doc/usecase.rst @@ -297,7 +297,8 @@ an apparently complete catalog. The generated data remote ``origin`` records the source URL and optional profile name. Existing Git-hosted catalogs keep their complete catalog, history and -recorded data remotes when cloned. Clone offers data downloads by default and +recorded data remotes when cloned. Repositories that track local files cannot +be cloned yet. Clone offers data downloads by default and asks for confirmation. ``--no-download`` copies only the catalog. Clone filters narrow the download and cannot accompany ``--no-download``; the catalog and history remain unchanged. diff --git a/mod/hallmark/remote/clone.py b/mod/hallmark/remote/clone.py index fdfbca6..b22b3fd 100644 --- a/mod/hallmark/remote/clone.py +++ b/mod/hallmark/remote/clone.py @@ -10,6 +10,8 @@ import pandas as pd import yaml +from git import Repo as GitRepo +from git.exc import GitCommandError, GitError from ..repo.dothm import Dothm from ..error import CloneError, DestinationExistsError @@ -24,6 +26,7 @@ from ..transport.base import RemoteObjectMissing, validate_remote_path from ..repo.worktree import Worktree from ..transport.base import reject_url_credentials +from ..repo.branches import validate_branch_name def _catalog_filenames(config): @@ -222,18 +225,25 @@ def _check_destination(cls, url, destination, display_path): return destination.is_dir() -def _remove_partial_clone(cls, destination, *, existed): +def _remove_partial_clone(cls, destination, *, created): """ Remove what a failed clone created, never following a symbolic link. - A destination the clone created is removed. In a worktree folder that - existed only its new ``.hm`` is removed; a bare folder that existed, and - held nothing before, is emptied again. + The folders the clone created for its destination are removed. In a + worktree folder that existed only its new ``.hm`` is removed; a bare + folder that existed, and held nothing before, is emptied again. + + Args: + cls: Repository class, used to tell bare destinations apart. + destination (Path): Clone destination. + created (Path, optional): Highest folder created for the destination, + or None if the destination existed. """ - if destination.is_symlink(): + if created is not None: + if not created.is_symlink(): + rmtree(created, ignore_errors=True) return - if not existed: - rmtree(destination, ignore_errors=True) + if destination.is_symlink(): return if cls.resolve_repo_paths(destination)[1] is not None: dothm = destination / ".hm" @@ -250,6 +260,82 @@ def _remove_partial_clone(cls, destination, *, existed): child.unlink(missing_ok=True) +def _tracks_local_files(table_text): + """ + Return whether a ``data.tsv`` catalog versions local files. + + Local catalogs record a ``sha1`` per file and keep contents in + ``.hm/objects``; remote catalogs list ``path`` rows on a data server. + Unreadable tables are left to catalog validation. + """ + try: + frame = pd.read_csv(StringIO(table_text), sep="\t", dtype=str, + keep_default_na=False) + except (pd.errors.EmptyDataError, pd.errors.ParserError): + return False + return (not frame.empty and "sha1" in frame.columns + and "path" not in frame.columns) + + +def _refuse_local_file_branches(git, branches): + """ + Refuse a clone when a branch's committed catalog versions local files. + + Args: + git: GitPython command wrapper of the repository holding the branches. + branches (list[tuple[str, str]]): Branch names and their revisions. + + Raises: + CloneError: If any branch tracks local files. + """ + local = [] + for name, revision in branches: + try: + table = git.show("--end-of-options", f"{revision}:data.tsv") + except GitCommandError: + continue + if _tracks_local_files(table): + local.append(name) + if local: + names = ", ".join(f"'{name}'" for name in local) + raise CloneError( + f"Branch {names} tracks local files; cloning local-file repositories " + "is not supported yet. Nothing was created.") + + +def _refuse_invalid_branch_names(git, names): + """ + Refuse source branches whose names Git would misread, such as options. + + Raises: + CloneError: If a branch name is invalid. + """ + for name in names: + try: + validate_branch_name(git, name) + except ValueError: + raise CloneError( + f"Source branch {name!r} has an invalid name; cloning it is not " + "supported. Nothing was created.") from None + + +def _check_local_source(url): + """Inspect a local Git source read-only, before anything is written.""" + root = _local_source_root(url) + if root is None: + return + git_dir = root / ".hm" if (root / ".hm").is_dir() else root + try: + source = GitRepo(git_dir) + except GitError: + # Not a readable repository: cloning reports the problem itself. + return + with source: + names = [head.name for head in source.heads] + _refuse_invalid_branch_names(source.git, names) + _refuse_local_file_branches(source.git, [(name, name) for name in names]) + + def _is_git_source(url, source_type): """Determine whether the selected source should be cloned with Git.""" if source_type == "git": @@ -271,11 +357,17 @@ def _clone_git(cls, url, destination, display_path, auth): local = Path(url).expanduser() git_url = str(local / ".hm") if (local / ".hm").is_dir() else url try: - Dothm.clone(git_url, dothm_path, display_path=display_path) + dothm = Dothm.clone(git_url, dothm_path, display_path=display_path) except CloneError as exc: raise CloneError( f"{exc}\nFor a raw dataset, use hm init PATH, " "then hm add URL.") from exc + # Sources on a Git host can only be inspected once copied; the caller + # removes the copy if a check fails. + branches = [(ref.remote_head, ref.name) for remote in dothm.remotes + for ref in remote.refs if ref.remote_head != "HEAD"] + _refuse_invalid_branch_names(dothm.git, [name for name, _ in branches]) + _refuse_local_file_branches(dothm.git, branches) if worktree_path: Worktree.init(worktree_path) return cls(destination) @@ -285,8 +377,9 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): """ Clone an existing Git catalog or published HTTP/SFTP catalog snapshot. - Source URLs with credentials are rejected first. The destination must be - new or an empty folder, outside any Hallmark repository and the local + Source URLs with credentials are rejected first. Repositories with a + branch that tracks local files cannot be cloned yet. The destination must + be new or an empty folder, outside any Hallmark repository and the local source; this is checked before source access. On failure, only what this call created is removed: the destination it created, or the ``.hm`` it added to an existing worktree folder. An existing bare folder is emptied @@ -306,7 +399,8 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): DestinationExistsError: If the destination exists and is not an empty folder. CloneError: If the destination is inside a repository or overlaps a - local source, or a requested catalog is missing or invalid. + local source, a branch tracks local files, or a requested catalog + is missing or invalid. ValueError: If source options are invalid. DownloadError: If remote metadata cannot be read or validated. """ @@ -320,7 +414,14 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): is_git = _is_git_source(url, source_type) if is_git and auth is not None: raise ValueError("Git cloning uses Git/SSH authentication, not auth profiles") + if is_git: + _check_local_source(url) + created = None if not existed: + # Remember the highest missing folder so a failure removes them all. + created = destination + while not (created.parent.exists() or created.parent.is_symlink()): + created = created.parent destination.parent.mkdir(parents=True, exist_ok=True) try: destination.mkdir() @@ -343,6 +444,10 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): raise CloneError("No published Hallmark catalog at this URL; " "use hm init PATH, then hm add URL " "for a raw dataset") + if _tracks_local_files(snapshot["data.tsv"]): + raise CloneError( + "The published catalog tracks local files; cloning local-file " + "repositories is not supported yet. Nothing was created.") if cls.resolve_repo_paths(destination)[1] is None: # Initialization creates a bare ".hm" folder itself. destination.rmdir() @@ -352,5 +457,5 @@ def clone_catalog(cls, url, path, *, auth=None, source_type="auto"): _commit_catalog_metadata(repo, snapshot, "Import published catalog snapshot") return repo except BaseException: - _remove_partial_clone(cls, destination, existed=existed) + _remove_partial_clone(cls, destination, created=created) raise diff --git a/mod/hallmark/repo/__init__.py b/mod/hallmark/repo/__init__.py index df1ee2e..28c01ee 100644 --- a/mod/hallmark/repo/__init__.py +++ b/mod/hallmark/repo/__init__.py @@ -19,9 +19,9 @@ from dataclasses import dataclass from pathlib import Path from typing import Dict, List, Optional, Tuple, Union -from git.exc import GitCommandError, InvalidGitRepositoryError, NoSuchPathError +from git.exc import InvalidGitRepositoryError, NoSuchPathError -from .branches import checkout, add_worktree +from .branches import checkout, add_worktree, validate_branch_name from ..remote.add import add_remote, is_remote_catalog from .dothm import Dothm from .state import State @@ -196,21 +196,7 @@ def _validate_branch_name(self, value) -> str: Raises: ValueError: If the branch name is invalid. """ - # Normalize the branch name to ensure it is a non-empty string - branch_name = require_nonempty_string(value, label="branch name") - # if the branch name starts with a hyphen, raise a ValueError - if branch_name.startswith("-"): - raise ValueError(f"invalid branch name: {branch_name!r}") - - # try to validate the branch name using Git's check_ref_format command - try: - self.dothm.git.check_ref_format("--branch", branch_name) - # if Git raises a GitCommandError, re-raise it as a ValueError - except GitCommandError as exc: - raise ValueError(f"invalid branch name: {branch_name!r}") from exc - - # if all checks pass, return the normalized branch name - return branch_name + return validate_branch_name(self.dothm.git, value) def _calculate_file_checksums(self, pf: ParaFrame) -> None: """ diff --git a/mod/hallmark/repo/branches.py b/mod/hallmark/repo/branches.py index b88abe4..1d041ab 100644 --- a/mod/hallmark/repo/branches.py +++ b/mod/hallmark/repo/branches.py @@ -5,7 +5,7 @@ from git.exc import GitCommandError from ..error import CheckoutError, DestinationExistsError, DothmError -from ..utils import resolve_path_in_root +from ..utils import require_nonempty_string, resolve_path_in_root from ..remote.add import is_remote_catalog from .config import branch_filename_format, row_to_path, single_data_format from .manifest import iter_manifest_entries @@ -15,6 +15,39 @@ from .changes import ensure_clean_tracked_files, tracked_paths +def validate_branch_name(git, value) -> str: + """ + Validate and normalize a Git branch name. + + Names that Git would read as options, such as ``-q``, are refused. + + Args: + git: GitPython command wrapper used to run ``check-ref-format``. + value (str): The branch name to validate. + + Returns: + str: The normalized branch name. + + Raises: + ValueError: If the branch name is invalid. + """ + # Normalize the branch name to ensure it is a non-empty string + branch_name = require_nonempty_string(value, label="branch name") + # if the branch name starts with a hyphen, raise a ValueError + if branch_name.startswith("-"): + raise ValueError(f"invalid branch name: {branch_name!r}") + + # try to validate the branch name using Git's check_ref_format command + try: + git.check_ref_format("--branch", branch_name) + # if Git raises a GitCommandError, re-raise it as a ValueError + except GitCommandError as exc: + raise ValueError(f"invalid branch name: {branch_name!r}") from exc + + # if all checks pass, return the normalized branch name + return branch_name + + def checkout(repo, target_branch): # Validate and normalize the target branch name target_branch = repo._validate_branch_name(target_branch) diff --git a/test/test_catalog.py b/test/test_catalog.py index 622c609..b5ff875 100644 --- a/test/test_catalog.py +++ b/test/test_catalog.py @@ -109,14 +109,15 @@ def test_published_snapshot_preserves_data_remote(tmp_path, metadata_server, nes def test_filtered_git_clone_preserves_history_without_objects(tmp_path, monkeypatch): + import pandas as pd + source = Repo.init(tmp_path / "source") - (source.worktree / "run1.h5").write_text("one") - (source.worktree / "run2.h5").write_text("two") - source.add("run{run:d}.h5") - source.set_config(remote_url="https://example.test/data/") + source.state.config = {"data": [{"db": "data.tsv"}], + "remote": {"url": "https://example.test/data/"}} + source.state.data = pd.DataFrame({"path": ["run1.h5", "run2.h5"]}) + source.dothm.save_state(source.state) source.commit("Original scientific data") head = source.dothm.head.commit.hexsha - assert list(source.objects.root.rglob("*")) def fail(*args, **kwargs): raise AssertionError("Filtered metadata clone must not transfer data") diff --git a/test/test_clone_defaults.py b/test/test_clone_defaults.py index 7ade070..82af14e 100644 --- a/test/test_clone_defaults.py +++ b/test/test_clone_defaults.py @@ -14,9 +14,8 @@ from mock_server import MockServer -def _remote_catalog_source(path, files=(("item1.txt", b"science"),)): - """Create a committed catalog of files that stay on a mock HTTP server.""" - repo = Repo.init(path) +def _catalog_remote_files(repo, files, message="Catalogued remote data"): + """Commit a catalog of files that stay on the mock HTTP server.""" repo.state.config = { "data": [{"db": "data.tsv"}], "remote": {"name": "origin", "url": "https://clone.test/data/"}} @@ -25,18 +24,20 @@ def _remote_catalog_source(path, files=(("item1.txt", b"science"),)): "checksum": sha1(content).hexdigest(), "size_bytes": len(content)} for name, content in files]) repo.dothm.save_state(repo.state) - repo.dothm.index.commit("Catalogued remote data") + repo.dothm.index.commit(message) + + +def _remote_catalog_source(path, files=(("item1.txt", b"science"),)): + """Create a repository whose committed catalog lists remote files.""" + repo = Repo.init(path) + _catalog_remote_files(repo, files) return repo @pytest.fixture def clone_source(tmp_path, monkeypatch): - source = Repo.init(tmp_path / 'source') payload = b'science' - (source.worktree / 'item1.txt').write_bytes(payload) - source.add('item{item:d}.txt') - source.set_config(remote_url='https://clone.test/data/') - source.commit('Catalogued data') + source = _remote_catalog_source(tmp_path / 'source', [('item1.txt', payload)]) server = MockServer('https://clone.test/data/') server.add_file('item1.txt', payload) calls = [] @@ -59,7 +60,7 @@ def test_python_clone_downloads_by_default(clone_source, tmp_path): target = tmp_path / 'copy' repo = Repo.clone(str(source.dothm.path), target) assert (target / 'item1.txt').read_bytes() == b'science' - assert repo.state.data.iloc[0]['sha1'] == sha1(b'science').hexdigest() + assert repo.state.data.iloc[0]['checksum'] == sha1(b'science').hexdigest() assert repo.download_result['succeeded'] == 1 assert calls == ['https://clone.test/data/item1.txt'] @@ -180,9 +181,8 @@ def test_clone_filtered_to_zero_files_does_not_prompt(clone_source, tmp_path): def test_clone_download_failure_keeps_catalog_and_valid_files( clone_source, tmp_path, cli): source, calls, server = clone_source - (source.worktree / 'item2.txt').write_bytes(b'science') - source.add('item{item:d}.txt') - source.commit('Add another file') + _catalog_remote_files(source, [('item1.txt', b'science'), + ('item2.txt', b'science')], 'Add another file') server.add_file('item1.txt', b'invalid') server.add_file('item2.txt', b'science') target = tmp_path / 'copy' @@ -369,3 +369,123 @@ def test_clone_refuses_a_nonempty_folder_or_symlink(tmp_path): Repo.clone(str(source.dothm.path), target, download=False) assert (full / "keep.txt").read_text() == "keep" assert list(empty.iterdir()) == [] + + +def _local_file_source(path, *, on_branch=None): + """Create a repository whose catalog versions local files in .hm/objects.""" + source = Repo.init(path) + if on_branch is not None: + source.commit("Empty main", allow_empty=True) + source.dothm.git.checkout("-b", on_branch) + (source.worktree / "item1.txt").write_bytes(b"science") + source.add("item{item:d}.txt") + source.set_config(remote_url="https://clone.test/data/") + source.commit("Catalogued local data") + if on_branch is not None: + source.dothm.git.checkout("main") + return source + + +@pytest.mark.parametrize("arguments", [[], ["--no-download"]]) +@pytest.mark.parametrize("on_branch", [None, "experiment"]) +def test_cli_refuses_to_clone_local_file_repositories( + tmp_path, monkeypatch, arguments, on_branch): + source = _local_file_source(tmp_path / "source", on_branch=on_branch) + monkeypatch.chdir(tmp_path) + + def reject(*args, **kwargs): + raise AssertionError("a local-file source must be refused before copying") + + monkeypatch.setattr("hallmark.remote.clone.Dothm.clone", reject) + result = CliRunner().invoke(hallmark, [ + "clone", str(source.dothm.path), "copy", *arguments]) + assert result.exit_code == 1 + assert "tracks local files" in result.output + assert "not supported yet" in result.output + assert "Nothing was created." in result.output + assert not (tmp_path / "copy").exists() + + +@pytest.mark.parametrize("relative", ["copy", "new/deep/copy"]) +def test_local_file_check_runs_after_cloning_a_git_host( + tmp_path, monkeypatch, relative): + from hallmark.remote.clone import Dothm + + source = _local_file_source(tmp_path / "source", on_branch="experiment") + original = Dothm.clone + + def clone(url, destination, **kwargs): + return original(str(source.dothm.path), destination, **kwargs) + + monkeypatch.setattr("hallmark.remote.clone.Dothm.clone", clone) + target = tmp_path / relative + with pytest.raises(CloneError, match="'experiment' tracks local files"): + Repo.clone("https://git.example.test/team/data.git", target, download=False) + assert not (tmp_path / Path(relative).parts[0]).exists() + + +@pytest.mark.parametrize("relative", ["copy", "new/deep/copy"]) +def test_snapshot_of_local_files_is_refused(tmp_path, monkeypatch, relative): + from hallmark.transport import OperationContext + from hallmark.transport.base import RemoteObjectMissing + + root = "https://example.test/published/" + pages = {root + "config.yml": "data:\n- fmt: run{run:d}.txt\n", + root + "meta.yml": "{}\n", + root + "data.tsv": "sha1\trun\n" + "a" * 40 + "\t1\n"} + + def read_text(context, path): + url = context.remote.url.rstrip("/") + "/" + path + if url not in pages: + raise RemoteObjectMissing("No such metadata object") + return pages[url] + + monkeypatch.setattr(OperationContext, "read_text", read_text) + target = tmp_path / relative + with pytest.raises(CloneError, match="tracks local files"): + Repo.clone(root, target, download=False) + assert not (tmp_path / Path(relative).parts[0]).exists() + + +def test_empty_repository_with_remote_branch_can_be_cloned(tmp_path): + source = Repo.init(tmp_path / "source") + source.commit("Empty catalog", allow_empty=True) + source.dothm.git.checkout("-b", "remote-data") + _catalog_remote_files(source, [("item1.txt", b"science")]) + source.dothm.git.checkout("main") + repo = Repo.clone(str(source.dothm.path), tmp_path / "copy", download=False) + assert repo.state.data.empty + + +def test_local_folder_that_is_not_a_repository_reports_the_clone_failure(tmp_path): + plain = tmp_path / "plain" + plain.mkdir() + (plain / "notes.txt").write_text("not a repository") + with pytest.raises(CloneError, match="hm init PATH, then hm add URL"): + Repo.clone(str(plain), tmp_path / "copy", download=False) + assert not (tmp_path / "copy").exists() + + +@pytest.mark.parametrize("name", ["-q", "--output=stolen"]) +@pytest.mark.parametrize("host", [False, True]) +def test_clone_refuses_option_like_branch_names(tmp_path, monkeypatch, name, host): + from hallmark.remote.clone import Dothm + + source = _remote_catalog_source(tmp_path / "source") + source.dothm.git.update_ref(f"refs/heads/{name}", "HEAD") + url = str(source.dothm.path) + if host: + original = Dothm.clone + + def clone(git_url, destination, **kwargs): + return original(str(source.dothm.path), destination, **kwargs) + + monkeypatch.setattr("hallmark.remote.clone.Dothm.clone", clone) + url = "https://git.example.test/team/data.git" + work = tmp_path / "work" + work.mkdir() + monkeypatch.chdir(work) + result = CliRunner().invoke(hallmark, ["clone", url, "copy", "--no-download"]) + assert result.exit_code == 1 + assert f"Source branch {name!r} has an invalid name" in result.output + assert list(work.iterdir()) == [] diff --git a/test/test_hallmark.py b/test/test_hallmark.py index 7a5da12..8d29378 100644 --- a/test/test_hallmark.py +++ b/test/test_hallmark.py @@ -1280,13 +1280,22 @@ def record_restore(sha1, destination): ### Repo.clone() tests ### +def _remote_catalog(path, files): + """Commit a catalog of files kept on a data server, with SHA-1 checksums.""" + repo = Repo.init(path) + repo.state.config = {"data": [{"db": "data.tsv"}], + "remote": {"url": "https://example.com/data/"}} + repo.state.data = pd.DataFrame([ + {"path": name, "checksum_algorithm": "sha1", "checksum": checksum} + for name, checksum in files]) + repo.dothm.save_state(repo.state) + repo.commit("add source data") + return repo + + def test_repo_clone_downloads_after_plan_approval(monkeypatch, tmp_path): - source = Repo.init(tmp_path / "source") - _write_files(source.worktree, ["a0_i0.h5"]) - source.add("a{a}_i{i}.h5") - source.set_config(remote_url="https://example.com/data/") - expected_sha1 = Repo.checksum(source.worktree / "a0_i0.h5") - source.commit("add source data") + expected_sha1 = "a" * 40 + source = _remote_catalog(tmp_path / "source", [("a0_i0.h5", expected_sha1)]) captured = {} @@ -1306,7 +1315,7 @@ def fake_download_file(context, relative_path, destination, sha1, chunk_size): assert captured == { "url": "https://example.com/data/a0_i0.h5", - "sha1": expected_sha1, + "sha1": ("sha1", expected_sha1), } assert (clone.worktree / "a0_i0.h5").read_text(encoding="utf-8") == \ "downloaded\n" @@ -1315,11 +1324,7 @@ def fake_download_file(context, relative_path, destination, sha1, chunk_size): def test_repo_clone_can_skip_remote_data_download(monkeypatch, tmp_path): - source = Repo.init(tmp_path / "source") - _write_files(source.worktree, ["a0_i0.h5"]) - source.add("a{a}_i{i}.h5") - source.set_config(remote_url="https://example.com/data/") - source.commit("add source data") + source = _remote_catalog(tmp_path / "source", [("a0_i0.h5", "a" * 40)]) def fail_download(*args, **kwargs): raise AssertionError("download should not be attempted") @@ -2796,12 +2801,10 @@ def test_checkout_remote_branch(tmp_path): source.commit("Initial commit") - # Clone the source while it only has the main branch. - clone = Repo.clone( - str(source.dothm.path), - tmp_path / "clone", - download=False, - ) + # Clone the source while it only has the main branch. hm clone refuses + # repositories that track local files, so copy the history with Git. + GitRepo.clone_from(str(source.dothm.path), str(tmp_path / "clone" / ".hm")) + clone = Repo(tmp_path / "clone") # The cloned repository should have the tracked file in its worktree. (clone.worktree / "data.txt").write_text( From a10079d4462638d8b59aeb81395fe6eaf4a9d308 Mon Sep 17 00:00:00 2001 From: Dennis Date: Sun, 4 Oct 2026 21:14:18 -0700 Subject: [PATCH 10/10] Copy branch pointers and validate the catalogue when cloning A Git clone now creates a local branch for every branch of the source, not only the one it starts on, and still starts on the source's selected branch. Branch names are validated before any branch is created, and Git is told where its options end. Every branch's committed catalog is checked with the validator shared with published snapshots, which now also rejects malformed or unsupported checksums; an invalid branch stops the clone and removes the copy. When the download after cloning fails, the error reports the clone as incomplete, where its catalogue is, and the hm download command that retries the same selection. Co-Authored-By: Claude Opus 5.5 --- README.rst | 2 +- doc/api.rst | 4 +- doc/private_data.rst | 12 ++++- mod/hallmark/remote/clone.py | 99 ++++++++++++++++++++++++++++++----- mod/hallmark/repo/__init__.py | 37 +++++++++---- test/test_cli.py | 5 +- test/test_clone_defaults.py | 81 ++++++++++++++++++++++++++-- 7 files changed, 208 insertions(+), 32 deletions(-) diff --git a/README.rst b/README.rst index 922e009..a2465c1 100644 --- a/README.rst +++ b/README.rst @@ -109,7 +109,7 @@ published HTTP/SFTP catalog snapshot. ``PATH`` defaults to a new folder named after the source and must otherwise be new or empty, outside any Hallmark repository. Catalogs can live on GitHub or another server while their data remotes point elsewhere. Git clones preserve the full -catalog and its history. Clone displays a data download plan and asks for +catalog, its history and every branch, after checking each branch's catalog. Clone displays a data download plan and asks for confirmation by default; declining keeps the catalog. Use ``hm clone CATALOG PATH --no-download`` for catalog only. Filters narrow the download and cannot accompany ``--no-download``. Python ``Repo.clone`` downloads without prompting diff --git a/doc/api.rst b/doc/api.rst index 9ec0190..e5ee7bc 100644 --- a/doc/api.rst +++ b/doc/api.rst @@ -101,8 +101,8 @@ Stage and commit a remote catalog without downloading dataset files:: Use ``hm clone SOURCE [PATH]`` for an existing Hallmark Git repository or published HTTP/SFTP snapshot. ``PATH`` defaults to a new folder named after the source; it must be new or empty and outside any Hallmark repository, -including the source. Git clones retain the full catalog and its -history; snapshots start new local history. Use ``--source-type git`` or +including the source. Git clones retain the full catalog, its history and +every branch; snapshots start new local history. Use ``--source-type git`` or ``--source-type catalog`` to override automatic detection. Downloading remote data diff --git a/doc/private_data.rst b/doc/private_data.rst index 8a0814b..83ddab4 100644 --- a/doc/private_data.rst +++ b/doc/private_data.rst @@ -224,8 +224,12 @@ reports it and creates nothing, even with ``--no-download``. A local source is checked before anything is copied; a Git host or snapshot is checked after its metadata is read, and the partial copy is removed. -Git endpoints preserve the complete catalog and history. Published HTTP/SFTP -snapshots start new local history. A snapshot URL may name the metadata +Git endpoints preserve the complete catalog and history, with a local branch +for every branch of the source; the copy starts on the branch the source has +selected. Before finishing, clone checks every branch's committed catalog: +readable ``config.yml``, ``meta.yml`` and tables, safe relative paths and +well-formed checksums. An invalid catalog stops the clone and removes the +copy. Published HTTP/SFTP snapshots start new local history. A snapshot URL may name the metadata directory itself or its parent containing ``.hm``. Use ``--source-type git`` to require Git or ``--source-type catalog`` to require a snapshot. Automatic selection treats local paths, SCP-style addresses, Git/file/SSH URLs and URLs @@ -246,6 +250,10 @@ confirmation; Enter or ``n`` keeps the catalog and exits successfully: Use ``--no-download`` for catalog only, without a prompt. It cannot be combined with ``--filter`` or ``--fmt`` and is required for bare destinations. An empty selection prints ``No files selected for download.`` without prompting. +If the download cannot run or a file fails, clone reports the copy as +incomplete. It keeps the catalog and the files that downloaded, and names the +``hm download --all`` command, with the same filters, that retries; files +already downloaded are skipped. Git authentication and data authentication are independent; catalog-only cloning does not require credentials for its data. diff --git a/mod/hallmark/remote/clone.py b/mod/hallmark/remote/clone.py index b22b3fd..35d741b 100644 --- a/mod/hallmark/remote/clone.py +++ b/mod/hallmark/remote/clone.py @@ -15,7 +15,8 @@ from ..repo.dothm import Dothm from ..error import CloneError, DestinationExistsError -from ..utils import as_list_of_dicts +from ..utils import as_list_of_dicts, load_yaml +from .download import _checksum_from_row, _validate_expected_checksum from ..repo.config import ( filename_fields, normalize_remotes, @@ -23,9 +24,10 @@ row_to_path, ) from ..transport import OperationContext, RemoteSpec -from ..transport.base import RemoteObjectMissing, validate_remote_path +from ..transport.base import ( + DownloadError, RemoteObjectMissing, reject_url_credentials, + validate_remote_path) from ..repo.worktree import Worktree -from ..transport.base import reject_url_credentials from ..repo.branches import validate_branch_name @@ -67,14 +69,30 @@ def _resolve_catalog_path(row, formats): return paths[0] -def _validate_snapshot(files, config): - """Validate catalog tables and paths before writing the snapshot.""" +def _validate_catalog(files, config): + """ + Validate a catalog's tables before it is written or kept. + + Every row must resolve to one safe relative path, and its checksum, if + any, must use a supported algorithm and a well-formed digest. + + Args: + files (dict[str, str]): Text of ``data.tsv`` and the other TSVs + named by ``config``. + config (dict): Parsed ``config.yml``. + + Raises: + CloneError: If a table is missing, unreadable or invalid. + ValueError: If a path or table name is unsafe. + """ for name in _catalog_filenames(config): + if name not in files: + raise CloneError(f"Catalog table is missing: {name}") try: frame = pd.read_csv(StringIO(files[name]), sep="\t", dtype=str, keep_default_na=False) except (pd.errors.EmptyDataError, pd.errors.ParserError) as exc: - raise CloneError(f"Invalid published catalog table: {name}") from exc + raise CloneError(f"Invalid catalog table: {name}") from exc formats = _catalog_formats(config, name) has_fields = False for fmt in formats: @@ -83,9 +101,61 @@ def _validate_snapshot(files, config): has_fields = True break if not ({"path", "sha1"} & set(frame.columns) or has_fields): - raise CloneError(f"Unrecognized published catalog columns: {name}") + raise CloneError(f"Unrecognized catalog columns: {name}") for _, row in frame.iterrows(): - _resolve_catalog_path(row, formats) + path = _resolve_catalog_path(row, formats) + try: + _validate_expected_checksum(_checksum_from_row(row)) + except DownloadError as exc: + raise CloneError(f"{name}: {path}: {exc}") from exc + + +def _validate_branch(git, branch): + """ + Validate the committed catalog of one branch of a cloned repository. + + Raises: + CloneError: If a state file is missing or the catalog is invalid. + """ + def read(name): + try: + return git.show("--end-of-options", f"{branch}:{name}") + except GitCommandError: + raise CloneError( + f"Branch '{branch}' is missing {name}") from None + + try: + config = load_yaml(read("config.yml")) + texts = {"meta.yml": read("meta.yml"), "data.tsv": read("data.tsv")} + load_yaml(texts["meta.yml"]) + except (ValueError, yaml.YAMLError) as exc: + detail = " ".join(str(exc).split()) + raise CloneError( + f"Branch '{branch}' has an invalid config.yml or meta.yml: " + f"{detail}") from exc + try: + for name in _catalog_filenames(config): + texts.setdefault(name, read(name)) + _validate_catalog(texts, config) + except (CloneError, ValueError) as exc: + raise CloneError( + f"Branch '{branch}' has an invalid catalog: {exc}") from exc + + +def _copy_branch_pointers(dothm): + """ + Create a local branch for every branch copied from the source. + + Returns: + list[str]: Names of all local branches. + """ + local = {head.name for head in dothm.heads} + for remote in dothm.remotes: + for ref in remote.refs: + if ref.remote_head != "HEAD" and ref.remote_head not in local: + dothm.git.branch("--track", "--", ref.remote_head, ref.name) + local.add(ref.remote_head) + return sorted(local) def _read_catalog_snapshot(context): @@ -122,7 +192,7 @@ def _read_catalog_snapshot(context): for name in _catalog_filenames(config): if name not in files: files[name] = context.read_text(name) - _validate_snapshot(files, config) + _validate_catalog(files, config) return files @@ -364,10 +434,13 @@ def _clone_git(cls, url, destination, display_path, auth): "then hm add URL.") from exc # Sources on a Git host can only be inspected once copied; the caller # removes the copy if a check fails. - branches = [(ref.remote_head, ref.name) for remote in dothm.remotes - for ref in remote.refs if ref.remote_head != "HEAD"] - _refuse_invalid_branch_names(dothm.git, [name for name, _ in branches]) - _refuse_local_file_branches(dothm.git, branches) + _refuse_invalid_branch_names(dothm.git, [ + ref.remote_head for remote in dothm.remotes + for ref in remote.refs if ref.remote_head != "HEAD"]) + branches = _copy_branch_pointers(dothm) + _refuse_local_file_branches(dothm.git, [(name, name) for name in branches]) + for name in branches: + _validate_branch(dothm.git, name) if worktree_path: Worktree.init(worktree_path) return cls(destination) diff --git a/mod/hallmark/repo/__init__.py b/mod/hallmark/repo/__init__.py index 28c01ee..d38cefc 100644 --- a/mod/hallmark/repo/__init__.py +++ b/mod/hallmark/repo/__init__.py @@ -15,6 +15,7 @@ from __future__ import annotations +import shlex from concurrent.futures import ThreadPoolExecutor from dataclasses import dataclass from pathlib import Path @@ -253,21 +254,37 @@ def init(cls, path: Union[Path, str] = ".") -> "Repo": def _download_cloned_files(self, *, approve, max_workers, progress, filter=None, fmt=None): - """Run an approved transfer after catalog creation has completed.""" + """ + Run an approved transfer after catalog creation has completed. + + Raises: + DownloadError: If the download cannot run or a file fails. The + message reports the clone as incomplete and how to retry. + """ from ..remote.download import DownloadError - plan = self.plan_download(filter=filter, fmt=fmt) - if plan.file_count and approve is not None and not approve(plan): - return - self.download_result = self.download( - plan, approved=True, max_workers=max_workers, progress=progress) + patterns = [filter] if isinstance(filter, str) else list(filter or []) + command = ["hm download --all", + *(f"--filter {shlex.quote(pattern)}" for pattern in patterns)] + if fmt is not None: + command.append(f"--fmt {shlex.quote(fmt)}") + retry = f'run {" ".join(command)} in "{self.worktree}" to retry' + incomplete = f'Clone incomplete: catalogue at "{self.dothm.path}"' + try: + plan = self.plan_download(filter=filter, fmt=fmt) + if plan.file_count and approve is not None and not approve(plan): + return + self.download_result = self.download( + plan, approved=True, max_workers=max_workers, progress=progress) + except DownloadError as exc: + raise DownloadError( + f"{incomplete}; the download failed: {exc}; {retry}") from exc if self.download_result["failed"]: details = "\n".join(self.download_result["errors"][:5]) raise DownloadError( - f"Failed to download {self.download_result['failed']} " - f"file(s):\n{details}\n" - f'Catalog kept at "{self.dothm.path}". ' - "Successfully downloaded files were kept.") + f"{incomplete}; {self.download_result['failed']} file(s) " + f"failed:\n{details}\n" + f"Successfully downloaded files were kept; {retry}.") @classmethod def clone( diff --git a/test/test_cli.py b/test/test_cli.py index 1e7a4d6..c4e8116 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -642,8 +642,11 @@ def fail_download(self, plan, **kwargs): "clone", str(source.dothm.path), str(tmp_path / "target")], input="y\n") assert result.exit_code != 0 - assert "Error: Remote download failed" in result.output + assert "Error: Clone incomplete: catalogue at" in result.output + assert "Remote download failed" in result.output + assert "run hm download --all in" in result.output assert "Download these files? [y/N]" in result.output + assert (tmp_path / "target" / ".hm" / "data.tsv").is_file() def test_clone_cli_skips_download_when_no_remote_files(monkeypatch, tmp_path): diff --git a/test/test_clone_defaults.py b/test/test_clone_defaults.py index 82af14e..61d4455 100644 --- a/test/test_clone_defaults.py +++ b/test/test_clone_defaults.py @@ -195,11 +195,12 @@ def test_clone_download_failure_keeps_catalog_and_valid_files( with pytest.raises(DownloadError) as error: Repo.clone(str(source.dothm.path), target) message = str(error.value) - assert 'Failed to download 1 file(s)' in message + assert f'Clone incomplete: catalogue at "{target / ".hm"}"' in message + assert '1 file(s) failed' in message assert 'Checksum mismatch' in message assert 'item1.txt' in message - assert f'Catalog kept at "{target / ".hm"}"' in message - assert 'Successfully downloaded files were kept.' in message + assert 'Successfully downloaded files were kept' in message + assert f'run hm download --all in "{target}" to retry' in message assert Repo(target).dothm.head.commit.hexsha == source.dothm.head.commit.hexsha assert not (target / 'item1.txt').exists() assert (target / 'item2.txt').read_bytes() == b'science' @@ -489,3 +490,77 @@ def clone(git_url, destination, **kwargs): assert result.exit_code == 1 assert f"Source branch {name!r} has an invalid name" in result.output assert list(work.iterdir()) == [] + + +def _commit_on_branch(source, branch, files, message): + """Commit raw .hm files on a new branch, then return to main.""" + source.dothm.git.checkout("-b", branch) + for name, text in files.items(): + if text is None: + source.dothm.index.remove([name], working_tree=True) + else: + (source.dothm.path / name).write_text(text) + source.dothm.index.add([name]) + source.dothm.index.commit(message) + source.dothm.git.checkout("main") + + +def test_clone_copies_every_branch_pointer(tmp_path, monkeypatch): + source = _remote_catalog_source(tmp_path / "source") + for branch, name in (("experiment", "item2.txt"), ("notes", "notes.txt")): + source.dothm.git.checkout("-b", branch) + _catalog_remote_files(source, [(name, b"x")], f"Catalog {name}") + source.dothm.git.checkout("main") + repo = Repo.clone(str(source.dothm.path), tmp_path / "copy", download=False) + assert {head.name: head.commit.hexsha for head in repo.dothm.heads} == { + head.name: head.commit.hexsha for head in source.dothm.heads} + assert repo.dothm.active_branch.name == "main" + monkeypatch.chdir(repo.worktree) + result = CliRunner().invoke(hallmark, ["branch"]) + assert result.output.split() == ["experiment", "*", "main", "notes"] + assert repo.checkout("experiment") + assert repo.state.data["path"].tolist() == ["item2.txt"] + + +def test_clone_starts_on_the_source_selected_branch(tmp_path): + source = _remote_catalog_source(tmp_path / "source") + source.dothm.git.checkout("-b", "experiment") + _catalog_remote_files(source, [("item2.txt", b"x")], "Catalog item2") + repo = Repo.clone(str(source.dothm.path), tmp_path / "copy", download=False) + assert repo.dothm.active_branch.name == "experiment" + assert repo.state.data["path"].tolist() == ["item2.txt"] + assert {head.name for head in repo.dothm.heads} == {"main", "experiment"} + + +@pytest.mark.parametrize("files, message", [ + ({"data.tsv": "path\n../escape.txt\n"}, "safe relative path"), + ({"data.tsv": "path\tchecksum_algorithm\tchecksum\na.txt\tsha256\txyz\n"}, + "Invalid sha256 checksum"), + ({"data.tsv": "path\tchecksum_algorithm\tchecksum\na.txt\tcrc\tabcd\n"}, + "Unsupported checksum algorithm"), + ({"config.yml": "data: [\n"}, "config.yml"), + ({"meta.yml": "- not a mapping\n"}, "meta.yml"), + ({"meta.yml": None}, "meta.yml"), +]) +def test_clone_refuses_an_invalid_catalog_on_another_branch( + tmp_path, files, message): + source = _remote_catalog_source(tmp_path / "source") + _commit_on_branch(source, "broken", files, "Break the catalog") + target = tmp_path / "copy" + with pytest.raises(CloneError, match="broken") as error: + Repo.clone(str(source.dothm.path), target, download=False) + assert message in str(error.value) + assert not target.exists() + + +def test_incomplete_clone_suggests_retrying_the_same_selection( + clone_source, tmp_path): + source, calls, server = clone_source + server.add_file('item1.txt', b'invalid') + target = tmp_path / 'copy' + with pytest.raises(DownloadError) as error: + Repo.clone(str(source.dothm.path), target, filter='item*.txt', + fmt='item{n:d}.txt') + assert ("run hm download --all --filter 'item*.txt' --fmt 'item{n:d}.txt' " + f'in "{target}" to retry') in str(error.value) + assert (target / '.hm/data.tsv').is_file()