Edkrepo: Bug Fixes

- Fixed the out of order parameters in clone_repos function. - Changed names of methods from patchset_application_flow and is_collision to patchset_branch_creation_flow and is_branch_name_collision respectively. - Removed the manifest_path variable in sync_command as it was calculating the wrong path leading to a bug. - Brought back the exception while creating a local branch for a branch name that already exists. - Removed magic strings from the code. - While checking for collision, used 'git rev-parse' on the branch name to get the Head instead of checking out to that branch. - Implemented __eq__ method for _Patchset class. - Removed the check_patchset_similarity method and used the equality operator instead. - When creating new repos at sync, changed the flow from creating and then renaming to renaming and then creating which would prevent a potential bug. - If applying a patch fails, running a 'git am --abort' to cancel the operation and then raise an exception. - While applying cherry pick from a different remote, we now check to see if the url in the manifest exists in the 'git remote -v' list. - If a Cherry pick fails, we check for a merge conflict and execute 'git cherry-pick --abort' if there is a conflict. - Removed ':' from the humble strings to make it consistent. Signed-off-by: Harsh Vora [email protected] Reviewed-by: Ashley DeSimone <[email protected]> Reviewed-by: Kevin Sun <[email protected]> Reviewed-by: Nate DeSimone <[email protected]>
tianocore · Dec 20, 2022 · 2fb5003 · 2fb5003
1 parent 9102f86
commit 2fb5003
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 40 deletions.
diff --git a/edkrepo/commands/clone_command.py b/edkrepo/commands/clone_command.py
@@ -177,7 +177,7 @@ def run_command(self, args, config):
         cache_obj = get_repo_cache_obj(config)
         if cache_obj is not None:
             add_missing_cache_repos(cache_obj, manifest, args.verbose)
-        clone_repos(args, workspace_dir, repo_sources_to_clone, project_client_side_hooks, config, manifest, cache_obj, manifest_repository_path)
+        clone_repos(args, workspace_dir, repo_sources_to_clone, project_client_side_hooks, config, manifest, manifest_repository_path, cache_obj)
 
         # Init submodules
         if not args.skip_submodule:

diff --git a/edkrepo/commands/sync_command.py b/edkrepo/commands/sync_command.py
@@ -35,7 +35,7 @@
 from edkrepo.common.workspace_maintenance.humble.manifest_repos_maintenance_humble import SOURCE_MANIFEST_REPO_NOT_FOUND
 from edkrepo.common.pathfix import get_actual_path, expanduser
 from edkrepo.common.common_cache_functions import get_repo_cache_obj
-from edkrepo.common.common_repo_functions import check_patchset_similarity, clone_repos, create_repos, patchset_application_flow, patchset_operations_similarity, sparse_checkout_enabled
+from edkrepo.common.common_repo_functions import clone_repos, create_repos, patchset_branch_creation_flow, patchset_operations_similarity, sparse_checkout_enabled
 from edkrepo.common.common_repo_functions import reset_sparse_checkout, sparse_checkout, verify_single_manifest
 from edkrepo.common.common_repo_functions import checkout_repos, check_dirty_repos
 from edkrepo.common.common_repo_functions import update_editor_config
@@ -181,7 +181,7 @@ def run_command(self, args, config):
             #Fetch notes
             repo.remotes.origin.fetch("refs/notes/*:refs/notes/*")
             if repo_to_sync.patch_set:
-                patchset_application_flow(repo_to_sync, repo, workspace_path, manifest, global_manifest_path)
+                patchset_branch_creation_flow(repo_to_sync, repo, workspace_path, manifest, global_manifest_path)
             elif repo_to_sync.commit is None and repo_to_sync.tag is None:
                 local_commits = False
                 initial_active_branch = repo.active_branch
@@ -313,7 +313,6 @@ def __update_local_manifest(self, args, config, initial_manifest, workspace_path
         write_included_config(new_manifest_to_check.remotes, new_manifest_to_check.submodule_alternate_remotes, local_manifest_dir)
 
         self.__check_submodule_config(workspace_path, new_manifest_to_check, new_sources_for_current_combo)
-        manifest_path = get_manifest_repo_path(new_manifest_to_check.general_config.source_manifest_repo, config)
         # Check that the repo sources lists are the same. If they are not the same and the override flag is not set, throw an exception.
         if not args.override and set(initial_sources) != set(new_sources):
             raise EdkrepoManifestChangedException(SYNC_REPO_CHANGE.format(initial_manifest.project_info.codename))
@@ -397,7 +396,7 @@ def __update_local_manifest(self, args, config, initial_manifest, workspace_path
             if len(sources_to_remove) > 0:
                 ui_functions.print_warning_msg(SYNC_REMOVE_LIST_END_FORMATTING, header = False)
             # Clone any new Git repositories
-            clone_repos(args, workspace_path, sources_to_clone, new_manifest_to_check.repo_hooks, config, new_manifest_to_check, manifest_path)
+            clone_repos(args, workspace_path, sources_to_clone, new_manifest_to_check.repo_hooks, config, new_manifest_to_check, global_manifest_directory)
             # Make a list of and only checkout repos that were newly cloned. Sync keeps repos on their initial active branches
             # cloning the entire combo can prevent existing repos from correctly being returned to their proper branch
             repos_to_checkout = []
@@ -410,18 +409,18 @@ def __update_local_manifest(self, args, config, initial_manifest, workspace_path
             new_repos_to_checkout, repos_to_create = self.__check_combo_patchset_sha_tag_branch(workspace_path, initial_common, new_common, initial_manifest, new_manifest_to_check)
             repos_to_checkout.extend(new_repos_to_checkout)
             if repos_to_checkout:
-                checkout_repos(args.verbose, args.override, repos_to_checkout, workspace_path, new_manifest_to_check, manifest_path)
+                checkout_repos(args.verbose, args.override, repos_to_checkout, workspace_path, new_manifest_to_check, global_manifest_directory)
 
             if repos_to_create:
-                create_repos(repos_to_create, workspace_path, new_manifest_to_check, manifest_path)
+                create_repos(repos_to_create, workspace_path, new_manifest_to_check, global_manifest_directory)
 
         if set(initial_sources) == set(new_sources):
             repos_to_checkout, repos_to_create = self.__check_combo_patchset_sha_tag_branch(workspace_path, initial_sources, new_sources, initial_manifest, new_manifest_to_check)
             if repos_to_checkout:
-                checkout_repos(args.verbose, args.override, repos_to_checkout, workspace_path, new_manifest_to_check, manifest_path)
+                checkout_repos(args.verbose, args.override, repos_to_checkout, workspace_path, new_manifest_to_check, global_manifest_directory)
 
             if repos_to_create:
-                create_repos(repos_to_create, workspace_path, new_manifest_to_check, manifest_path)
+                create_repos(repos_to_create, workspace_path, new_manifest_to_check, global_manifest_directory)
 
         #remove the old manifest file and copy the new one
         ui_functions.print_info_msg(UPDATING_MANIFEST, header = False)
@@ -453,7 +452,7 @@ def __check_combo_patchset_sha_tag_branch(self, workspace_path, initial_sources,
                     if initial_source.patch_set:
                         initial_patchset = initial_manifest.get_patchset(initial_source.patch_set, initial_source.remote_name)
                         new_patchset = new_manifest_to_check.get_patchset(new_source.patch_set, new_source.remote_name)
-                        if check_patchset_similarity(initial_patchset, new_patchset):
+                        if initial_patchset == new_patchset:
                             if not patchset_operations_similarity(initial_patchset, new_patchset, initial_manifest, new_manifest_to_check):
                                 repos_to_create.append(new_source)
                         else:

diff --git a/edkrepo/common/common_repo_functions.py b/edkrepo/common/common_repo_functions.py
@@ -21,15 +21,15 @@
 from git import Repo
 import colorama
 
-from edkrepo.common.edkrepo_exception import EdkrepoRevertFailedException, EdkrepoCherryPickFailedException
+from edkrepo.common.edkrepo_exception import EdkrepoBranchExistsException, EdkrepoRevertFailedException, EdkrepoCherryPickFailedException
 from edkrepo.common.edkrepo_exception import EdkrepoFetchBranchNotFoundException
 from edkrepo.common.edkrepo_exception import EdkrepoPatchNotFoundException, EdkrepoPatchFailedException
 from edkrepo.common.edkrepo_exception import EdkrepoRemoteNotFoundException, EdkrepoRemoteAddException, EdkrepoRemoteRemoveException
 from edkrepo.common.edkrepo_exception import EdkrepoManifestInvalidException
 from edkrepo.common.edkrepo_exception import EdkrepoUncommitedChangesException
 from edkrepo.common.edkrepo_exception import EdkrepoInvalidParametersException
 from edkrepo.common.progress_handler import GitProgressHandler
-from edkrepo.common.humble import APPLYING_CHERRY_PICK_FAILED, APPLYING_PATCH_FAILED, APPLYING_REVERT_FAILED, CHECKING_OUT_DEFAULT, CHECKING_OUT_PATCHSET
+from edkrepo.common.humble import APPLYING_CHERRY_PICK_FAILED, APPLYING_PATCH_FAILED, APPLYING_REVERT_FAILED, BRANCH_EXISTS, CHECKING_OUT_DEFAULT, CHECKING_OUT_PATCHSET
 from edkrepo.common.humble import FETCH_BRANCH_DOES_NOT_EXIST, PATCHFILE_DOES_NOT_EXIST
 from edkrepo.common.humble import REMOTE_CREATION_FAILED, REMOTE_NOT_FOUND, REMOVE_REMOTE_FAILED
 from edkrepo.common.humble import MISSING_BRANCH_COMMIT
@@ -75,7 +75,8 @@
 CLEAR_LINE = '\x1b[K'
 DEFAULT_REMOTE_NAME = 'origin'
 PRIMARY_REMOTE_NAME = 'primary'
-
+PATCH = "Patch"
+REVERT = "Revert"
 
 def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, manifest, global_manifest_path, cache_obj=None):
     # created_patch_sets = []
@@ -382,7 +383,7 @@ def checkout_repos(verbose, override, repos_to_checkout, workspace_path, manifes
         # the source section for the manifest the order of priority is the followiwng 1)patchset 2)commit
         # 3) tag 4)branch with the highest priority attribute provided beinng checked out
         if repo_to_checkout.patch_set:
-            patchset_application_flow(repo_to_checkout, repo, workspace_path, manifest, global_manifest_path)
+            patchset_branch_creation_flow(repo_to_checkout, repo, workspace_path, manifest, global_manifest_path)
         else:
             if repo_to_checkout.commit:
                 if verbose and (repo_to_checkout.branch or repo_to_checkout.tag):
@@ -414,18 +415,20 @@ def checkout_repos(verbose, override, repos_to_checkout, workspace_path, manifes
             else:
                 raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-def patchset_application_flow(repo, repo_obj, workspace_path, manifest, global_manifest_path):
+def patchset_branch_creation_flow(repo, repo_obj, workspace_path, manifest, global_manifest_path):
     json_path = os.path.join(workspace_path, "repo")
     json_path = os.path.join(json_path, "patchset_{}.json".format(os.path.basename(repo_obj.working_dir)))
     patchset = manifest.get_patchset(repo.patch_set, repo.remote_name)
 
     if repo.patch_set in repo_obj.branches:
-        if is_collision(json_path, repo.patch_set, repo_obj, global_manifest_path):
+        if is_branch_name_collision(json_path, repo.patch_set, repo_obj, global_manifest_path):
             create_local_branch(repo.patch_set, patchset, global_manifest_path, manifest, repo_obj)
+        else:
+            repo_obj.git.checkout(repo.patch_set)
     else:
         create_local_branch(repo.patch_set, patchset, global_manifest_path, manifest, repo_obj)
 
-def is_collision(json_path, patch_set, repo, global_manifest_path):
+def is_branch_name_collision(json_path, patch_set, repo, global_manifest_path):
     repo_name = os.path.basename(repo.working_dir)
     COLLISION = False
     for branch in repo.branches:
@@ -435,8 +438,7 @@ def is_collision(json_path, patch_set, repo, global_manifest_path):
                 patchset_data = data[repo_name]
                 for patchset in patchset_data:
                     if patch_set in patchset.values():
-                        repo.git.checkout(patch_set)
-                        head = repo.git.execute(['git', 'rev-parse', 'HEAD'])
+                        head = repo.git.execute(['git', 'rev-parse', patch_set])
                         if patchset['head_sha'] != head:
                             patchset['head_sha'] = head
                             COLLISION = True
@@ -458,18 +460,6 @@ def patchset_operations_similarity(initial_patchset, new_patchset, initial_manif
     return initial_manifest.get_patchset_operations(initial_patchset.name, initial_patchset.remote) \
             == new_manifest.get_patchset_operations(new_patchset.name, new_patchset.remote)
 
-def check_patchset_similarity(initial_patchset, new_patchset):
-    if initial_patchset.remote != new_patchset.remote:
-        return False
-    elif initial_patchset.name != new_patchset.name:
-        return False
-    elif initial_patchset.parent_sha != new_patchset.parent_sha:
-        return False
-    elif initial_patchset.fetch_branch != new_patchset.fetch_branch:
-        return False
-
-    return True
-
 def create_repos(repos_to_create, workspace_path, manifest, global_manifest_path):
     for repo_to_create in repos_to_create:
         local_repo_path = os.path.join(workspace_path, repo_to_create.root)
@@ -486,9 +476,9 @@ def create_repos(repos_to_create, workspace_path, manifest, global_manifest_path
                     for patch_data in patchset_data:
                         if patch_set in patch_data.values():
                             patchset = manifest.get_patchset(repo_to_create.patch_set, repo_to_create.remote_name)
-                            create_local_branch(patch_set, patchset, global_manifest_path, manifest, repo)
                             branch.rename(patch_set + '_' + time.strftime("%Y/%m/%d_%H_%M_%S"))
                             patch_data[patch_set] = patch_set + '_' + time.strftime("%Y/%m/%d_%H_%M_%S")
+                            create_local_branch(patch_set, patchset, global_manifest_path, manifest, repo)
                             f.seek(0)
                             json.dump(data, f, indent=4)
                             break
@@ -861,19 +851,24 @@ def create_local_branch(name, patchset, global_manifest_path, manifest_obj, repo
             json.dump(data, f, indent=4)
         f.close()
 
+def is_merge_conflict(repo):
+    status = repo.git.status(porcelain=True).split()
+    return True if 'UU' in status else False
+
 def apply_patchset_operations(repo, remote, operations_list, global_manifest_path, remote_list):
     for operations in operations_list:
         for operation in operations:
-            if operation.type == "Patch":
+            if operation.type == PATCH:
                 path = os.path.normpath(os.path.join(global_manifest_path, operation.file))
                 if os.path.isfile(path):
                     try:
                         repo.git.execute(['git', 'am', path, '--ignore-whitespace'])
                     except:
+                        repo.git.execute(['git', 'am', '--abort'])
                         raise EdkrepoPatchFailedException(APPLYING_PATCH_FAILED.format(operation.file))
                 else:
                     raise EdkrepoPatchNotFoundException(PATCHFILE_DOES_NOT_EXIST.format(operation.file))
-            elif operation.type == "Revert":
+            elif operation.type == REVERT:
                 try:
                     repo.git.execute(['git', 'revert', operation.sha, '--no-edit'])
                 except:
@@ -882,7 +877,7 @@ def apply_patchset_operations(repo, remote, operations_list, global_manifest_pat
                 if operation.source_remote:
                     REMOTE_FOUND = False
                     for remote in remote_list:
-                        if operation.source_remote == remote.name:
+                        if operation.source_remote == remote.name and remote.url in repo.git.execute(['git', 'remote', '-v']):
                             REMOTE_FOUND = True
                             try:
                                 repo.git.execute(['git', 'remote', 'add', operation.source_remote, remote.url])
@@ -895,6 +890,8 @@ def apply_patchset_operations(repo, remote, operations_list, global_manifest_pat
                             try:
                                 repo.git.execute(['git', 'cherry-pick', operation.sha, '-x'])
                             except:
+                                if is_merge_conflict(repo):
+                                    repo.git.execute(['git', 'cherry-pick', '--abort'])
                                 raise EdkrepoCherryPickFailedException(APPLYING_CHERRY_PICK_FAILED.format(operation.sha))
                             try:
                                 repo.git.execute(['git', 'remote', 'remove', operation.source_remote])

diff --git a/edkrepo/common/humble.py b/edkrepo/common/humble.py
@@ -167,12 +167,13 @@
 SUBMODULE_DEINIT_FAILED = 'Warning: Unable to remove all submodule content'
 
 # Creating Local Branch Error Messages
-REMOTE_NOT_FOUND = "Could not find the remote: {}"
-REMOTE_CREATION_FAILED = "Failed to add the remote: {}"
+BRANCH_EXISTS = "The branch {} already exists."
+REMOTE_NOT_FOUND = "Could not find the remote {}"
+REMOTE_CREATION_FAILED = "Failed to add the remote {}"
 FETCH_BRANCH_DOES_NOT_EXIST = "The branch {} does not exist"
 PATCHFILE_DOES_NOT_EXIST = "The patch file {} does not exist"
 APPLYING_PATCH_FAILED = "Unable to apply the patch file {}"
-APPLYING_REVERT_FAILED = "Failed to revert to the commit: {}"
-APPLYING_CHERRY_PICK_FAILED = "Failed to cherry pick the commit: {}"
-REMOVE_REMOTE_FAILED = "Failed to remove the remote: {}"
+APPLYING_REVERT_FAILED = "Failed to revert to the commit {}"
+APPLYING_CHERRY_PICK_FAILED = "Failed to cherry pick the commit {}"
+REMOVE_REMOTE_FAILED = "Failed to remove the remote {}"
 CHECKING_OUT_DEFAULT = "Failed to apply one of the patchset operations. Checking out back to the default branch"
diff --git a/edkrepo_manifest_parser/edk_manifest.py b/edkrepo_manifest_parser/edk_manifest.py
@@ -847,6 +847,12 @@ def __init__(self, element):
         except KeyError as k:
             raise KeyError(REQUIRED_ATTRIB_ERROR_MSG.format(k, element.tag))
 
+    def __eq__(self, other_patchset):
+        if isinstance(other_patchset, _PatchSet):
+            return (self.name, self.remote, self.parentSha, self.fetchBranch) == \
+            (other_patchset.name, other_patchset.remote, other_patchset.parentSha, other_patchset.fetchBranch)
+        return False
+
     @property
     def tuple(self):
         return PatchSet(self.remote, self.name, self.parentSha, self.fetchBranch)