Merge pull request #543 from openlawlibrary/renatav/updater-fixes-no-…

…million

Handle various LVC states and other updater fixes

CHANGELOG.md

@@ -34,6 +34,8 @@ and this project adheres to [Semantic Versioning][semver].
 
 ### Changed
 
+- If in detached head state or an older branch, do not automatically checkout the newest one without force ([543])
+- Move validation of the last validated commit to the pipeline from the update handler ([543])
 - Default verbosity to 0 (NOTICE) level; add notice level update outcome logging ([538])
 - Raise a more descriptive error if `pygit2` repository cannot be instantiated  ([485], [489])
 - Enhanced commit_and_push for better error logging and update the last validated commit ([469])
@@ -43,6 +45,7 @@ and this project adheres to [Semantic Versioning][semver].
 
 ### Fixed
 
+- Handle invalid last validated commit ([543])
 - Fixes to executing taf handler scripts from a pyinstaller executable ([535])
 - Fix `persisent` and `transient` NoneType error when running taf handlers ([535])
 - Fix update status when a target repo was updated and the auth repo was not ([532])
@@ -52,6 +55,8 @@ and this project adheres to [Semantic Versioning][semver].
 - Fix setup role when specifying public keys in keys-description ([511])
 - `check_if_repositories_clean` error now returns a list of repositories which aren't clean, instead of a single repository ([525])
 
+
+[543]: https://github.com/openlawlibrary/taf/pull/543
 [538]: https://github.com/openlawlibrary/taf/pull/538
 [535]: https://github.com/openlawlibrary/taf/pull/535
 [532]: https://github.com/openlawlibrary/taf/pull/532

taf/git.py

@@ -161,6 +161,11 @@ def remotes(self) -> List[str]:
             self._remotes = [remote.name for remote in repo.remotes]
         return self._remotes
 
+    @property
+    def is_detached_head(self) -> bool:
+        repo = self.pygit_repo
+        return repo.head_is_detached
+
     @property
     def is_git_repository(self) -> bool:
         """Check if the given path is the root of a Git repository."""
@@ -914,6 +919,17 @@ def commit_before_commit(self, commit: str) -> Optional[str]:
                 return hex
         return None
 
+    def create_local_branch_from_remote_tracking(self, branch, remote="origin"):
+        repo = self.pygit_repo
+        remote_branch_name = f"refs/remotes/{remote}/{branch}"
+        remote_branch = repo.lookup_reference(remote_branch_name)
+        if remote_branch is not None:
+            local_branch = repo.lookup_branch(branch, pygit2.GIT_BRANCH_LOCAL)
+            if local_branch is None:
+                # Create a new local branch from the remote branch
+                target_commit = repo[remote_branch.target]
+                repo.create_branch(branch, target_commit)
+
     def delete_local_branch(self, branch_name: str) -> None:
         """Deletes local branch."""
         try:
@@ -1061,14 +1077,7 @@ def fetch_from_disk(self, local_repo_path, branches):
         remote.fetch()
 
         for branch in branches:
-            remote_branch_name = f"refs/remotes/{temp_remote_name}/{branch}"
-            remote_branch = repo.lookup_reference(remote_branch_name)
-            if remote_branch is not None:
-                local_branch = repo.lookup_branch(branch, pygit2.GIT_BRANCH_LOCAL)
-                if local_branch is None:
-                    # Create a new local branch from the remote branch
-                    target_commit = repo[remote_branch.target]
-                    repo.create_branch(branch, target_commit)
+            self.create_local_branch_from_remote_tracking(branch, temp_remote_name)
 
         repo.remotes.delete(temp_remote_name)
 
@@ -1331,17 +1340,18 @@ def list_worktrees(self) -> Dict[Path, Tuple[Path, str, str]]:
     def merge_commit(
         self,
         commit: str,
+        target_branch: Optional[str] = None,
         fast_forward_only: Optional[bool] = False,
         check_if_merge_completed: Optional[bool] = False,
-        update_remote_tracking: Optional[bool] = True,
     ) -> bool:
+        # Determine the branch to merge into, defaulting to the current branch if not provided
+        branch = target_branch or self.get_current_branch()
+
         fast_forward_only_flag = "--ff-only" if fast_forward_only else ""
-        self._git("merge {} {}", commit, fast_forward_only_flag, log_error=True)
-        if update_remote_tracking:
-            current_branch = self.get_current_branch()
-            self._git(
-                f"update-ref refs/remotes/origin/{current_branch} HEAD", log_error=True
-            )  # Update remote tracking
+
+        self._git(f"merge {commit} {fast_forward_only_flag}", log_error=True)
+
+        self.reset_to_commit(commit, branch, hard=True)
         if check_if_merge_completed:
             try:
                 self._git("rev-parse -q --verify MERGE_HEAD")
@@ -1425,9 +1435,22 @@ def reset_num_of_commits(
         flag = "--hard" if hard else "--soft"
         self._git(f"reset {flag} HEAD~{num_of_commits}")
 
-    def reset_to_commit(self, commit: str, hard: Optional[bool] = False) -> None:
+    def reset_to_commit(
+        self, commit: str, branch: Optional[str] = None, hard: Optional[bool] = False
+    ) -> None:
         flag = "--hard" if hard else "--soft"
-        self._git(f"reset {flag} {commit}")
+
+        if branch is None:
+            branch = self.get_current_branch()
+        self.update_branch_refs(branch, commit)
+        if hard:
+            self._git(f"reset {flag} HEAD")
+
+    def update_branch_refs(self, branch: str, commit: str) -> None:
+        # Update the local branch reference to the specific commit
+        self._git(f"update-ref refs/heads/{branch} {commit}")
+        # Update the remote-tracking branch
+        self._git(f"update-ref refs/remotes/origin/{branch} {commit}", log_error=True)
 
     def update_ref_for_bare_repository(self, branch: str, commit_sha: str) -> None:
         """

taf/tests/test_updater/conftest.py

@@ -1,6 +1,7 @@
 import enum
 import os
 import re
+from typing import Optional
 import pytest
 import inspect
 import random
@@ -62,6 +63,10 @@
 UNCOMMITTED_CHANGES = r"Update of (\w+\/\w+) failed due to error: Repository (\w+\/\w+) should contain only committed changes\. \nPlease update the repository at (.+) manually and try again\."
 UPDATE_ERROR_PATTERN = r"Update of (\w+\/\w+) failed due to error: Validation of authentication repository (\w+\/\w+) failed at revision ([0-9a-f]+) due to error: .*"
 FORCED_UPDATE_PATTERN = r"Update of (\w+\/\w+) failed due to error: Repositories ([\w/,\s-]+) have uncommitted changes. Commit and push or use --force to revert and run the command again."
+BEHIND_LVC_PATTERN = r"Update of (\w+\/\w+) failed due to error: Top commit of repository \1 ([0-9a-f]{40}) is not equal to or newer than the last successful commit."
+LVC_NOT_IN_REPO_PATTERN = r"Update of (\w+\/\w+) failed due to error: \1: Last validated commit (\w{40}) is not in repository \1\s*Run the updater with the --force flag to run the validation from the first commit"
+LVC_NOT_IN_REMOTE_PATTERN = r"Update of ([\w_\/]+) failed due to error: Last validated commit ([\da-f]{40}) is no longer on (\w+) of the remote ([\w_\/]+) repository.*"
+UNPUSHED_COMMITS_PATTERN = r"Update of (\w+\/\w+) failed due to error:\s*\nThe following repository has unpushed commits on branches: ([\w\/]+): \(([\w,-]+)\)"
 REMOVED_COMMITS_PATTERN = r"Update of (\w+/\w+) failed due to error: Last validated commit ([0-9a-f]{40}) is not in the remote repository."
 INVALID_TIMESTAMP_PATTERN = r"^Update of (\w+\/\w+) failed due to error: Update of (\w+\/\w+) failed. One or more referenced authentication repositories could not be validated:\n Validation of authentication repository (\w+\/\w+) failed at revision ([0-9a-f]{40}) due to error: timestamp was signed by (\d+)\/(\d+) keys$"
 CANNOT_CLONE_TARGET_PATTERN = r"^Update of (\w+/\w+) failed due to error: Update of (\w+/\w+) failed. One or more referenced authentication repositories could not be validated:\n Cannot clone (\w+/\w+) from any of the following URLs: \['.*'\]$"
@@ -72,7 +77,6 @@
     r"([0-9a-f]{40}) committed on (\d{4}-\d{2}-\d{2}): data repository (\w+\/\w+) was "
     r"supposed to be at commit ([0-9a-f]{40}) but (repo was at|commit not on branch) (\w+)"
 )
-COMMIT_NOT_FOUND_PATTERN = r"Update of (\w+\/\w+) failed due to error: object not found - no match for id \((\w{40})\)"
 
 
 # Disable console logging for all tests
@@ -488,36 +492,51 @@ def setup_repository_no_target_repositories(
     return AuthenticationRepository(origin_dir, repo_name)
 
 
+def add_file_to_repository(
+    target_repo: GitRepository, filename: str, commit_message: Optional[str] = None
+):
+    content = _generate_random_text()
+    file_path = target_repo.path / filename
+    file_path.write_text(content)
+    if commit_message is not None:
+        target_repo.commit(commit_message)
+
+
 def add_valid_target_commits(
     auth_repo: AuthenticationRepository, target_repos: list, add_if_empty: bool = True
 ):
     for target_repo in target_repos:
         if not add_if_empty and target_repo.head_commit_sha() is None:
             continue
-        update_target_files(target_repo, "Update target files")
+        update_target_repository(target_repo, "Update target files")
     sign_target_repositories(TEST_DATA_ORIGIN_PATH, auth_repo.name, KEYSTORE_PATH)
 
 
+def add_file_to_target_repo_without_committing(target_repos: list, target_name: str):
+    for target_repo in target_repos:
+        if target_name in target_repo.name:
+            add_file_to_repository(target_repo, "dirty.txt")
+
+
+def add_file_to_auth_repo_without_committing(auth_repo: AuthenticationRepository):
+    add_file_to_repository(auth_repo, "dirty.txt")
+
+
 def add_valid_unauthenticated_commits(target_repos: list):
     for target_repo in target_repos:
         if target_repo.custom.get("allow-unauthenticated-commits", False):
-            update_target_files(target_repo, "Update target files")
+            update_target_repository(target_repo, "Update target files")
 
 
 def add_unauthenticated_commits_to_all_target_repos(target_repos: list):
     for target_repo in target_repos:
-        update_target_files(target_repo, "Update target files")
+        update_target_repository(target_repo, "Update target files")
 
 
 def add_unauthenticated_commit_to_target_repo(target_repos: list, target_name: str):
     for target_repo in target_repos:
         if target_name in target_repo.name:
-            update_target_files(target_repo, "Update target files")
-
-
-def add_unauthenticated_commits_to_target_repo(target_repos: list):
-    for target_repo in target_repos:
-        update_target_files(target_repo, "Update target files")
+            update_target_repository(target_repo, "Update target files")
 
 
 def create_new_target_orphan_branches(
@@ -580,10 +599,26 @@ def swap_last_two_commits(auth_repo: AuthenticationRepository):
 
 
 def update_expiration_dates(
+    auth_repo: AuthenticationRepository, roles=["snapshot", "timestamp"], push=True
+):
+    update_metadata_expiration_date(
+        str(auth_repo.path),
+        roles=roles,
+        keystore=KEYSTORE_PATH,
+        interval=None,
+        push=push,
+    )
+
+
+def update_auth_repo_without_committing(
     auth_repo: AuthenticationRepository, roles=["snapshot", "timestamp"]
 ):
     update_metadata_expiration_date(
-        str(auth_repo.path), roles=roles, keystore=KEYSTORE_PATH, interval=None
+        str(auth_repo.path),
+        roles=roles,
+        keystore=KEYSTORE_PATH,
+        interval=None,
+        commit=False,
     )
 
 
@@ -602,17 +637,10 @@ def update_role_metadata_without_signing(
     )
 
 
-def update_existing_file(repo: GitRepository, filename: str, commit_message: str):
-    text_to_add = _generate_random_text()
-    file_path = repo.path / filename
-    if file_path.exists():
-        with file_path.open("a") as file:
-            file.write(f"\n{text_to_add}")
-        repo.commit(commit_message)
-    else:
-        raise FileNotFoundError(
-            f"The file {filename} does not exist in the repository {repo.path}"
-        )
+def update_target_repo_without_committing(target_repos: list, target_name: str):
+    for target_repo in target_repos:
+        if target_name in target_repo.name:
+            update_target_repository(target_repo)
 
 
 def update_role_metadata_invalid_signature(
@@ -653,7 +681,9 @@ def update_and_sign_metadata_without_clean_check(
     commit_and_push(auth_repo, commit_msg=commit_msg, push=False)
 
 
-def update_target_files(target_repo: GitRepository, commit_message: str):
+def update_target_repository(
+    target_repo: GitRepository, commit_message: Optional[str] = None
+):
     text_to_add = _generate_random_text()
     # Iterate over all files in the repository directory
     is_empty = True
@@ -667,27 +697,9 @@ def update_target_files(target_repo: GitRepository, commit_message: str):
     if is_empty:
         random_text = _generate_random_text()
         (target_repo.path / "test.txt").write_text(random_text)
-    target_repo.commit(commit_message)
-
-
-def update_file_without_commit(repo_path: str, filename: str):
-    text_to_add = _generate_random_text()
-    file_path = Path(repo_path) / filename
-    file_path.parent.mkdir(parents=True, exist_ok=True)  # Ensure the directory exists
-    if file_path.exists():
-        with file_path.open("a") as file:
-            file.write(text_to_add)
-    else:
-        with file_path.open("w") as file:
-            file.write(text_to_add)
 
-
-def add_file_without_commit(repo_path: str, filename: str):
-    text_to_add = _generate_random_text()
-    file_path = Path(repo_path) / filename
-    file_path.parent.mkdir(parents=True, exist_ok=True)  # Ensure the directory exists
-    with file_path.open("w") as file:
-        file.write(text_to_add)
+    if commit_message is not None:
+        target_repo.commit(commit_message)
 
 
 def remove_commits(