[RFC 2/2] b&m: Nuke _initializing_system_branch()

In its place is _atomic_subdirectory, which handles cleaning up the system branch if an exception is raised. Its api is designed such that it could create temporary directories in the future and rename them into place, but this is not implemented. Now, subcommands that create a system branch use _atomic_subdirectory to make the required path appear, then create the branch in that, instead of that being handled by a helper function, since the details between creating and being finished varies too much to be worth attempting to find common logic to factor out. --- morphlib/plugins/branch_and_merge_new_plugin.py | 96 ++++++++++++----------- 1 file changed, 49 insertions(+), 47 deletions(-) diff --git a/morphlib/plugins/branch_and_merge_new_plugin.py b/morphlib/plugins/branch_and_merge_new_plugin.py index 809699e..96d6829 100644 --- a/morphlib/plugins/branch_and_merge_new_plugin.py +++ b/morphlib/plugins/branch_and_merge_new_plugin.py @@ -108,37 +108,44 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin): self.app.output.write('%s\n' % ws.root) # TODO: Move this somewhere nicer + @staticmethod @contextlib.contextmanager - def _initializing_system_branch(self, ws, root_url, system_branch, - cached_repo, base_ref): - '''A context manager for system branches under construction. + def _atomic_subdirectory(root, subdir): + '''Context manager for creating a directory and its contents. + + It takes a root directory and the subdirectory which will have + been created after the context manager exits. + + The __enter__ method returns a path to where the subdirectory + can be initialized. - The purpose of this context manager is to factor out the branch - cleanup code for if an exception occurs while a branch is being - constructed. + The separation between the subdirectory passed and the + subdirectory to initialize is to allow the manager to use a + temporary directory and rename it into place after it has been + initialized. - This could be handled by a higher order function which takes - a function to initialize the branch as a parameter, but with - statements look nicer and are more obviously about resource - cleanup. + Renaming a tempdir into place is not yet implemented. ''' - root_dir = ws.get_default_system_branch_directory_name(system_branch) + os.makedirs(subdir) try: - sb = morphlib.sysbranchdir.create( - root_dir, root_url, system_branch) - gd = sb.clone_cached_repo(cached_repo, base_ref) + yield subdir + except BaseException as e: + logging.error('Caught exception: %s' % str(e)) - yield (sb, gd) + def handle_error(function, path, excinfo): + logging.warning ("Error while trying to clean up %s: %s" % + (path, excinfo)) + shutil.rmtree(subdir, onerror=handle_error) - gd.update_submodules(self.app) - gd.update_remotes() + # Remove parent directories that are empty too, avoiding exceptions + parent = os.path.dirname(subdir) + while parent != os.path.abspath(root): + if len(os.listdir(parent)) > 0 or os.path.islink(parent): + break + os.rmdir(parent) + parent = os.path.dirname(parent) - except BaseException as e: - # Oops. Clean up. - logging.error('Caught exception: %s' % str(e)) - logging.info('Removing half-finished branch %s' % system_branch) - self._remove_branch_dir_safe(ws.root, root_dir) raise def checkout(self, args): @@ -184,13 +191,17 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin): # Check the git branch exists. cached_repo.resolve_ref(system_branch) - with self._initializing_system_branch( - ws, root_url, system_branch, cached_repo, base_ref) as (sb, gd): + with self._atomic_subdirectory( + ws.root, + ws.get_default_system_branch_directory_name(system_branch)) \ + as path: + + sb = morphlib.sysbranchdir.create(path, root_url, system_branch) + gd = sb.clone_cached_repo(cached_repo, base_ref) if not self._checkout_has_systems(gd): raise BranchRootHasNoSystemsError(base_ref) - def branch(self, args): '''Create a new system branch. @@ -242,8 +253,13 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin): # Make sure the base_ref exists. cached_repo.resolve_ref(base_ref) - with self._initializing_system_branch( - ws, root_url, system_branch, cached_repo, base_ref) as (sb, gd): + with self._atomic_subdirectory( + ws.root, + ws.get_default_system_branch_directory_name(system_branch)) \ + as path: + + sb = morphlib.sysbranchdir.create(path, root_url, system_branch) + gd = sb.clone_cached_repo(cached_repo, base_ref) gd.branch(system_branch, base_ref) gd.checkout(system_branch) @@ -527,24 +543,6 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin): sb = morphlib.sysbranchdir.open_from_within('.') self.app.output.write('%s\n' % sb.get_config('branch.root')) - def _remove_branch_dir_safe(self, workspace_root, system_branch_root): - # This function avoids throwing any exceptions, so it is safe to call - # inside an 'except' block without altering the backtrace. - - def handle_error(function, path, excinfo): - logging.warning ("Error while trying to clean up %s: %s" % - (path, excinfo)) - - shutil.rmtree(system_branch_root, onerror=handle_error) - - # Remove parent directories that are empty too, avoiding exceptions - parent = os.path.dirname(system_branch_root) - while parent != os.path.abspath(workspace_root): - if len(os.listdir(parent)) > 0 or os.path.islink(parent): - break - os.rmdir(parent) - parent = os.path.dirname(parent) - def _require_git_user_config(self): '''Warn if the git user.name and user.email variables are not set.''' @@ -837,9 +835,13 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin): lrc, rrc = morphlib.util.new_repo_caches(self.app) cached_repo = lrc.get_updated_repo(root_url) + with self._atomic_subdirectory( + ws.root, + ws.get_default_system_branch_directory_name(system_branch)) \ + as path: - with self._initializing_system_branch( - ws, root_url, system_branch, cached_repo, base_ref) as (sb, gd): + sb = morphlib.sysbranchdir.create(path, root_url, system_branch) + gd = sb.clone_cached_repo(cached_repo, base_ref) # TODO: It's nasty to clone to a sha1 then create a branch # of that sha1 then check it out, a nicer API may be the -- 1.7.10.4