diff --git a/cadetrdm/logging.py b/cadetrdm/logging.py index 45c2d99..b092368 100644 --- a/cadetrdm/logging.py +++ b/cadetrdm/logging.py @@ -76,6 +76,12 @@ def fulfils_environment(self, environment: Environment): Instance of Environment class, with requirements as key: value pairs. :return: """ + # Environment matching is opt-in. Without requirements to check against, the + # recorded environment is not read at all, so loading results does not depend + # on the run_history files being present in the working tree. + if environment is None: + return True + if self._environment is None: self._load_environment() @@ -137,6 +143,31 @@ def n_entries(self) -> int: """int: Number of results stored in the repository.""" return len(self.entries) + @classmethod + def from_string(cls, content: str, filepath=None): + """ + Create an OutputLog from the raw contents of a log.tsv file. + + Used to read the log out of a git ref without checking that ref out. The + filepath is not read from, but is retained so that LogEntry can resolve the + run_history files next to it. + + :param content: + Raw tab-separated contents of a log.tsv file. + :param filepath: + Optional path the contents belong to. + """ + instance = cls() + instance._filepath = filepath + + lines = [line.split("\t") for line in content.splitlines() if line] + if not lines: + return instance + + instance._entry_list = lines + instance.entries: dict[str, LogEntry] = instance._entries_from_entry_list(instance._entry_list) + return instance + @classmethod def from_list(cls, entry_list: list[list[str]]): instance = cls() diff --git a/cadetrdm/repositories.py b/cadetrdm/repositories.py index 8bb454d..fbdc07d 100644 --- a/cadetrdm/repositories.py +++ b/cadetrdm/repositories.py @@ -1095,6 +1095,9 @@ def update_output_main_logs( Dumps all the metadata information about the project repositories state and the commit hash and branch name of the ouput repository into the main branch of the output repository. + This is the write path for the output log: it intentionally checks out the + output repository's main branch, updates the run history, commits it, and then + returns to the result branch. :param output_dict: Dictionary containing key-value pairs to be added to the log. """ @@ -1565,11 +1568,21 @@ def output_log_file_path(self): @property def output_log(self): - if self.has_uncomitted_changes: - self._reset_hard_to_head(force_entry=True) - if not self.active_branch == self.main_branch: - self.checkout(self.main_branch) - return OutputLog(filepath=self.output_log_file_path) + """ + OutputLog: The run history recorded on the main branch. + + Read directly from the main branch ref, so that inspecting the log neither + checks out that branch nor touches the working tree. Reading the log used to + discard uncommitted changes and check out the main branch, which made loading + results a destructive operation. + """ + try: + log_content = self._git.show(f"{self.main_branch}:log.tsv") + except git.GitCommandError: + # No log.tsv on the main branch yet, e.g. in a freshly initialized repo. + return OutputLog() + + return OutputLog.from_string(log_content, filepath=self.path / "log.tsv") def print_output_log(self): self.checkout(self.main_branch) diff --git a/tests/test_read_only_loading.py b/tests/test_read_only_loading.py new file mode 100644 index 0000000..eb291f1 --- /dev/null +++ b/tests/test_read_only_loading.py @@ -0,0 +1,99 @@ +"""Regression tests pinning that reading results does not mutate git state. + +Loading archived results must not move either repository. These tests snapshot the +observable git state of both repositories, perform a read, and assert the snapshot is +unchanged. +""" + +from pathlib import Path + +import pytest + +from cadetrdm import ProjectRepo, initialize_repo + + +def git_state(repo): + """Snapshot the observable git state of a repository. + + Covers everything a read operation must leave alone: the checked out commit and + branch, the set of local branches, and whether the working tree is dirty. + """ + return { + "commit": repo.current_commit_hash, + "branch": str(repo.active_branch), + "branches": sorted(head.name for head in repo._git_repo.heads), + "is_dirty": repo._git_repo.is_dirty(untracked_files=True), + } + + +@pytest.fixture +def repo_with_results(tmp_path): + """A project repo holding one recorded result, left on the result branch. + + Leaving the output repo on the result branch rather than on main is what makes the + mutation visible: a read that checks out main would change the active branch. + """ + path_to_repo = tmp_path / "project" + initialize_repo(path_to_repo, "results") + + repo = ProjectRepo(path_to_repo) + with repo.track_results(results_commit_message="Add result"): + (repo.output_path / "result.csv").write_text("1,2,3\n") + + assert str(repo.output_repo.active_branch) != repo.output_repo.main_branch + + return repo + + +def test_reading_output_log_leaves_output_repo_untouched(repo_with_results): + output_repo = repo_with_results.output_repo + state_before = git_state(output_repo) + + output_repo.output_log + + assert git_state(output_repo) == state_before + + +def test_reading_output_log_preserves_uncommitted_changes(repo_with_results): + output_repo = repo_with_results.output_repo + scratch_file = Path(output_repo.path) / "uncommitted.txt" + scratch_file.write_text("work in progress\n") + + output_repo.output_log + + assert scratch_file.exists(), "reading the log discarded uncommitted work" + assert scratch_file.read_text() == "work in progress\n" + + +def test_output_log_read_without_checkout_lists_the_result_branch(repo_with_results): + output_repo = repo_with_results.output_repo + result_branch = str(output_repo.active_branch) + + entries = output_repo.output_log.entries + + assert result_branch in entries + assert entries[result_branch].project_repo_commit_hash == repo_with_results.current_commit_hash + + +def test_matching_without_an_environment_does_not_read_run_history(repo_with_results): + """Without requirements to match, recorded environments must not be read. + + run_history is only populated on the main branch, so on a result branch the + conda_environment.yml a LogEntry would load is not in the working tree. Reading it + used to work only because reading the log checked out main first. + """ + output_repo = repo_with_results.output_repo + assert not (Path(output_repo.path) / "run_history").exists() + + entry = output_repo.output_log.entries[str(output_repo.active_branch)] + + assert entry.fulfils_environment(None) is True + + +def test_output_log_is_empty_when_no_results_are_recorded(tmp_path): + path_to_repo = tmp_path / "project" + initialize_repo(path_to_repo, "results") + + repo = ProjectRepo(path_to_repo) + + assert repo.output_repo.output_log.n_entries == 0