Merge pull request #532 from openlawlibrary/rentav/updater-fixes

Various updater fixes - updating remote-tracking branches, additional tests, fix removal of local commits

CHANGELOG.md

@@ -41,10 +41,14 @@ and this project adheres to [Semantic Versioning][semver].
 
 ### Fixed
 
+- Fix update status when a target repo was updated and the auth repo was not ([532])
+- Fix merge-commit which wasn't updating the remote-tracking branch ([532])
+- Fix removal of additional local commits ([532])
 - Fix top-level authentication repository update to correctly update child auth repos ([528])
 - 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])
 
+[532]: https://github.com/openlawlibrary/taf/pull/532
 [529]: https://github.com/openlawlibrary/taf/pull/529
 [528]: https://github.com/openlawlibrary/taf/pull/528
 [525]: https://github.com/openlawlibrary/taf/pull/525

taf/api/targets.py

@@ -658,6 +658,11 @@ def _update_target_repos(
         return
     target_repo = GitRepository(path=target_repo_path)
     if target_repo.is_git_repository:
+        if target_repo.head_commit_sha() is None:
+            taf_logger.warning(
+                f"Repository {repo_path} does not have the HEAD reference"
+            )
+            return
         data = {"commit": target_repo.head_commit_sha()}
         if add_branch:
             data["branch"] = target_repo.get_current_branch()

taf/git.py

@@ -371,6 +371,11 @@ def all_commits_on_branch(
                 )
             latest_commit_id = branch_obj.target
         else:
+            if self.head_commit_sha() is None:
+                raise GitError(
+                    self,
+                    message=f"Error occurred while getting commits of branch {branch}. No HEAD reference",
+                )
             latest_commit_id = repo[repo.head.target].id
 
         sort = pygit2.GIT_SORT_REVERSE if reverse else pygit2.GIT_SORT_NONE
@@ -392,7 +397,11 @@ def all_commits_since_commit(
         """
 
         if since_commit is None:
-            return self.all_commits_on_branch(branch=branch, reverse=reverse)
+            try:
+                return self.all_commits_on_branch(branch=branch, reverse=reverse)
+            except GitError as e:
+                self._log_warning(str(e))
+                return []
 
         try:
             self.commit_exists(commit_sha=since_commit)
@@ -407,6 +416,8 @@ def all_commits_since_commit(
                 return []
             latest_commit_id = branch_obj.target
         else:
+            if self.head_commit_sha() is None:
+                return []
             latest_commit_id = repo[repo.head.target].id
 
         if repo.descendant_of(since_commit, latest_commit_id):
@@ -817,7 +828,7 @@ def checkout_commit(self, commit: str) -> None:
             reraise_error=True,
         )
 
-    def is_branch_with_unpushed_commits(self, branch_name):
+    def branch_unpushed_commits(self, branch_name):
         repo = self.pygit_repo
 
         local_branch = repo.branches.get(branch_name)
@@ -849,7 +860,7 @@ def is_branch_with_unpushed_commits(self, branch_name):
             else:
                 break
 
-        return bool(unpushed_commits)
+        return [commit.id for commit in unpushed_commits]
 
     def commit(self, message: str) -> str:
         self._git("add -A")
@@ -1322,9 +1333,15 @@ def merge_commit(
         commit: str,
         fast_forward_only: Optional[bool] = False,
         check_if_merge_completed: Optional[bool] = False,
+        update_remote_tracking: Optional[bool] = True,
     ) -> bool:
         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
         if check_if_merge_completed:
             try:
                 self._git("rev-parse -q --verify MERGE_HEAD")

taf/tests/test_git/conftest.py

@@ -41,3 +41,14 @@ def clone_repository():
     yield repo
     repo.cleanup()
     shutil.rmtree(path, onerror=on_rm_error)
+
+
+@fixture
+def empty_repository():
+    path = TEST_DIR / CLONE_REPO_NAME
+    path.mkdir(exist_ok=True, parents=True)
+    repo = GitRepository(path=path)
+    repo.init_repo()
+    yield repo
+    repo.cleanup()
+    shutil.rmtree(path, onerror=on_rm_error)

taf/tests/test_git/test_git.py

@@ -11,14 +11,14 @@ def test_clone_from_local(repository, clone_repository):
     assert len(commits)
 
 
-def test_is_branch_with_unpushed_commits(repository, clone_repository):
+def test_branch_unpushed_commits(repository, clone_repository):
     clone_repository.clone_from_disk(repository.path, keep_remote=True)
     branch = clone_repository.branches()[0]
     clone_repository.reset_num_of_commits(1, True)
-    assert not clone_repository.is_branch_with_unpushed_commits(branch)
+    assert not len(clone_repository.branch_unpushed_commits(branch))
     (clone_repository.path / "test3.txt").write_text("Updated test3")
     clone_repository.commit(message="Update test3.txt")
-    assert clone_repository.is_branch_with_unpushed_commits(branch)
+    assert len(clone_repository.branch_unpushed_commits(branch))
 
 
 def test_is_git_repository_root_bare(repository):
@@ -41,3 +41,9 @@ def test_head_commit_sha():
             match=f"Repo {repo.name}: The path '{repo.path.as_posix()}' is not a Git repository.",
         ):
             repo.head_commit_sha() is not None
+
+
+def test_all_commits_since_commit_when_repo_empty(empty_repository):
+    all_commits_empty = empty_repository.all_commits_since_commit()
+    assert isinstance(all_commits_empty, list)
+    assert len(all_commits_empty) == 0

taf/tests/test_updater/conftest.py

@@ -173,9 +173,15 @@ def execute_tasks(self):
 
 
 class RepositoryConfig:
-    def __init__(self, name: str, allow_unauthenticated_commits: bool = False):
+    def __init__(
+        self,
+        name: str,
+        allow_unauthenticated_commits: bool = False,
+        is_empty: bool = False,
+    ):
         self.name = name
         self.allow_unauthenticated_commits = allow_unauthenticated_commits
+        self.is_empty = is_empty
 
 
 @pytest.fixture
@@ -209,6 +215,7 @@ def origin_auth_repo(request, test_name: str, origin_dir: Path):
         RepositoryConfig(
             f"{test_name}/{targets_config['name']}",
             targets_config.get("allow_unauthenticated_commits", False),
+            targets_config.get("is_empty", False),
         )
         for targets_config in targets_config_list
     ]
@@ -332,6 +339,8 @@ def _init_auth_repo(
 
 def initialize_git_repo(library_dir: Path, repo_name: str) -> GitRepository:
     repo_path = Path(library_dir, repo_name)
+    if repo_path.is_dir():
+        shutil.rmtree(repo_path, onerror=on_rm_error)
     repo_path.mkdir(parents=True, exist_ok=True)
     repo = GitRepository(path=repo_path)
     repo.init_repo()
@@ -349,10 +358,11 @@ def initialize_target_repositories(
         else:
             target_repo = GitRepository(library_dir, target_config.name)
         # create some files, content of these repositories is not important
-        for i in range(1, 3):
-            random_text = _generate_random_text()
-            (target_repo.path / f"test{i}.txt").write_text(random_text)
-        target_repo.commit("Initial commit")
+        if not target_config.is_empty:
+            for i in range(1, 3):
+                random_text = _generate_random_text()
+                (target_repo.path / f"test{i}.txt").write_text(random_text)
+            target_repo.commit("Initial commit")
 
 
 def sign_target_repositories(library_dir: Path, repo_name: str, keystore: Path):
@@ -478,8 +488,12 @@ def setup_repository_no_target_repositories(
     return AuthenticationRepository(origin_dir, repo_name)
 
 
-def add_valid_target_commits(auth_repo: AuthenticationRepository, target_repos: list):
+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")
     sign_target_repositories(TEST_DATA_ORIGIN_PATH, auth_repo.name, KEYSTORE_PATH)
 
@@ -495,6 +509,17 @@ def add_unauthenticated_commits_to_all_target_repos(target_repos: list):
         update_target_files(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")
+
+
 def create_new_target_orphan_branches(
     auth_repo: AuthenticationRepository, target_repos: list, branch_name: str
 ):
@@ -598,7 +623,7 @@ def update_role_metadata_invalid_signature(
     content["signatures"][0]["sign"] = "invalid signature"
     version = content["signed"]["version"]
     content["signed"]["version"] = version + 1
-    role_metadata_path.write_text(json.dumps(content))
+    role_metadata_path.write_text(json.dumps(content, indent=4))
     auth_repo.commit("Invalid metadata update")
 
 
@@ -631,11 +656,17 @@ def update_and_sign_metadata_without_clean_check(
 def update_target_files(target_repo: GitRepository, commit_message: str):
     text_to_add = _generate_random_text()
     # Iterate over all files in the repository directory
+    is_empty = True
     for file_path in target_repo.path.iterdir():
         if file_path.is_file():
+            is_empty = False
             existing_content = file_path.read_text(encoding="utf-8")
             new_content = existing_content + "\n" + text_to_add
             file_path.write_text(new_content, encoding="utf-8")
+
+    if is_empty:
+        random_text = _generate_random_text()
+        (target_repo.path / "test.txt").write_text(random_text)
     target_repo.commit(commit_message)
 
 
@@ -660,8 +691,6 @@ def add_file_without_commit(repo_path: str, filename: str):
 
 
 def remove_commits(
-    auth_repo: AuthenticationRepository,
-    target_repos: list,
     repo_path: str,
     num_commits: int = 1,
 ):

taf/tests/test_updater/test_clone/test_clone_valid.py

@@ -1,6 +1,7 @@
 import pytest
 from taf.tests.test_updater.conftest import (
     SetupManager,
+    add_unauthenticated_commit_to_target_repo,
     add_unauthenticated_commits_to_all_target_repos,
     add_valid_target_commits,
     add_valid_unauthenticated_commits,
@@ -13,6 +14,7 @@
 from taf.tests.test_updater.update_utils import (
     clone_client_target_repos_without_updater,
     update_and_check_commit_shas,
+    verify_repos_eixst,
 )
 from taf.updater.types.update import OperationType, UpdateType
 
@@ -316,3 +318,55 @@ def test_clone_valid_when_no_upstream_top_commits_unsigned(
         expected_repo_type=UpdateType.EITHER,
         no_upstream=True,
     )
+
+
+@pytest.mark.parametrize(
+    "origin_auth_repo",
+    [
+        {
+            "targets_config": [
+                {"name": "notempty"},
+                {"name": "empty", "is_empty": True},
+            ],
+        },
+    ],
+    indirect=True,
+)
+def test_clone_when_target_empty(origin_auth_repo, client_dir):
+
+    update_and_check_commit_shas(
+        OperationType.CLONE,
+        origin_auth_repo,
+        client_dir,
+        expected_repo_type=UpdateType.EITHER,
+    )
+    verify_repos_eixst(client_dir, origin_auth_repo, exists=["notempty"])
+
+
+@pytest.mark.parametrize(
+    "origin_auth_repo",
+    [
+        {
+            "targets_config": [
+                {"name": "target1"},
+                {"name": "target2", "is_empty": True},
+            ],
+        },
+    ],
+    indirect=True,
+)
+def test_clone_when_no_target_file_and_commit(origin_auth_repo, client_dir):
+
+    setup_manager = SetupManager(origin_auth_repo)
+    setup_manager.add_task(
+        add_unauthenticated_commit_to_target_repo, kwargs={"target_name": "target2"}
+    )
+    setup_manager.execute_tasks()
+
+    update_and_check_commit_shas(
+        OperationType.CLONE,
+        origin_auth_repo,
+        client_dir,
+        expected_repo_type=UpdateType.EITHER,
+    )
+    verify_repos_eixst(client_dir, origin_auth_repo, exists=["target1"])

taf/tests/test_updater/test_update/test_update_invalid.py

@@ -1,6 +1,7 @@
 import pytest
 from taf.auth_repo import AuthenticationRepository
 from taf.tests.test_updater.conftest import (
+    INVALID_KEYS_PATTERN,
     TARGET_MISSMATCH_PATTERN,
     FORCED_UPDATE_PATTERN,
     UNCOMMITTED_CHANGES,
@@ -10,9 +11,12 @@
     add_unauthenticated_commits_to_all_target_repos,
     add_valid_target_commits,
     create_index_lock,
+    update_expiration_dates,
     update_file_without_commit,
+    update_role_metadata_invalid_signature,
 )
 from taf.tests.test_updater.update_utils import (
+    check_if_last_validated_commit_exists,
     clone_repositories,
     update_invalid_repos_and_check_if_repos_exist,
 )
@@ -237,3 +241,35 @@ def test_update_with_invalid_last_validated_commit(origin_auth_repo, client_dir)
         COMMIT_NOT_FOUND_PATTERN,
         True,
     )
+
+
+@pytest.mark.parametrize(
+    "origin_auth_repo",
+    [
+        {
+            "targets_config": [{"name": "target1"}, {"name": "target2"}],
+        }
+    ],
+    indirect=True,
+)
+def test_update_invalid_target_invalid_singature(origin_auth_repo, client_dir):
+
+    clone_repositories(origin_auth_repo, client_dir)
+
+    setup_manager = SetupManager(origin_auth_repo)
+    setup_manager.add_task(
+        update_role_metadata_invalid_signature, kwargs={"role": "targets"}
+    )
+    setup_manager.add_task(update_expiration_dates)
+    setup_manager.execute_tasks()
+
+    update_invalid_repos_and_check_if_repos_exist(
+        OperationType.UPDATE,
+        origin_auth_repo,
+        client_dir,
+        INVALID_KEYS_PATTERN,
+        True,
+    )
+    client_auth_repo = AuthenticationRepository(client_dir, origin_auth_repo.name)
+    # make sure that the last validated commit does not exist
+    check_if_last_validated_commit_exists(client_auth_repo, True)