From 8067c59c6cc5ddbfcb4edcab182fcc8ec793f7cc Mon Sep 17 00:00:00 2001 From: Ben Andre Date: Wed, 17 Jan 2018 19:08:19 -0700 Subject: [PATCH] Refine verbose output Refine the verbose output based on PR feedback from Bill Sacks. * Verbose output is cumulative as the verbose count increases. * checkout and status have similar behavior * dump level status output is indented consistently. Testing: make test - all tests pass manual testing of cesm and standalone repos with new verbose checkout and status output. --- manic/checkout.py | 14 +++----- manic/externals_status.py | 32 ++++++++++------- manic/repository.py | 8 ----- manic/repository_git.py | 26 ++++---------- manic/repository_svn.py | 23 ++++--------- manic/sourcetree.py | 72 +++++++++++++++------------------------ 6 files changed, 64 insertions(+), 111 deletions(-) diff --git a/manic/checkout.py b/manic/checkout.py index 6862e2e..9229f69 100755 --- a/manic/checkout.py +++ b/manic/checkout.py @@ -23,7 +23,6 @@ from manic.sourcetree import SourceTree from manic.utils import printlog from manic.global_constants import VERSION_SEPERATOR, LOG_FILE_NAME -from manic.global_constants import VERBOSITY_DUMP if sys.hexversion < 0x02070000: print(70 * '*') @@ -219,8 +218,9 @@ def commandline_arguments(args=None): parser.add_argument('-v', '--verbose', action='count', default=0, help='Output additional information to ' - 'the screen and log file. This flag can be used ' - 'multiple times.') + 'the screen and log file. For --status, this flag ' + 'can be used a second time, increasing the verbosity' + 'level each time.') # # developer options @@ -277,12 +277,8 @@ def main(args): if args.status: # user requested status-only - if args.verbose != VERBOSITY_DUMP: - for comp in sorted(tree_status.keys()): - tree_status[comp].log_status_message(args.verbose) - else: # args.verbose == VERBOSITY_DUMP - # user requested verbose status dump of the git/svn status commands - source_tree.verbose_status_dump() + for comp in sorted(tree_status.keys()): + tree_status[comp].log_status_message(args.verbose) else: # checkout / update the external repositories. safe_to_update = check_safe_to_update_repos(tree_status) diff --git a/manic/externals_status.py b/manic/externals_status.py index 0f1216f..5f66890 100644 --- a/manic/externals_status.py +++ b/manic/externals_status.py @@ -9,8 +9,8 @@ from __future__ import print_function from .global_constants import EMPTY_STR -from .utils import printlog -from .global_constants import VERBOSITY_DEFAULT +from .utils import printlog, indent_string +from .global_constants import VERBOSITY_VERBOSE, VERBOSITY_DUMP class ExternalStatus(object): @@ -50,25 +50,26 @@ def __init__(self): self.path = EMPTY_STR self.current_version = EMPTY_STR self.expected_version = EMPTY_STR + self.status_output = EMPTY_STR def log_status_message(self, verbosity): """Write status message to the screen and log file """ - if verbosity == VERBOSITY_DEFAULT: - msg = self.default_status_message() - else: - msg = self.verbose_status_message() - printlog(msg) + self._default_status_message() + if verbosity >= VERBOSITY_VERBOSE: + self._verbose_status_message() + if verbosity >= VERBOSITY_DUMP: + self._dump_status_message() - def default_status_message(self): + def _default_status_message(self): """Return the default terse status message string """ msg = '{sync}{clean}{src_type} {path}'.format( sync=self.sync_state, clean=self.clean_state, src_type=self.source_type, path=self.path) - return msg + printlog(msg) - def verbose_status_message(self): + def _verbose_status_message(self): """Return the verbose status message string """ clean_str = self.DEFAULT @@ -81,9 +82,14 @@ def verbose_status_message(self): if self.sync_state != self.STATUS_OK: sync_str = '{current} --> {expected}'.format( current=self.current_version, expected=self.expected_version) - msg = '{path} :\n {clean}, {sync}'.format( - path=self.path, clean=clean_str, sync=sync_str) - return msg + msg = ' {clean}, {sync}'.format(clean=clean_str, sync=sync_str) + printlog(msg) + + def _dump_status_message(self): + """Return the dump status message string + """ + msg = indent_string(self.status_output, 12) + printlog(msg) def safe_to_update(self): """Report if it is safe to update a repository. Safe is defined as: diff --git a/manic/repository.py b/manic/repository.py index c5ccc27..9baa066 100644 --- a/manic/repository.py +++ b/manic/repository.py @@ -49,14 +49,6 @@ def status(self, stat, repo_dir_path): # pylint: disable=unused-argument 'repository classes! {0}'.format(self.__class__.__name__)) fatal_error(msg) - def verbose_status_dump(self, repo_dir_path): # pylint: disable=unused-argument - """Display the raw repo status to the user. - - """ - msg = ('DEV_ERROR: status method must be implemented in all ' - 'repository classes! {0}'.format(self.__class__.__name__)) - fatal_error(msg) - def url(self): """Public access of repo url. """ diff --git a/manic/repository_git.py b/manic/repository_git.py index c6f0f21..34aea0d 100644 --- a/manic/repository_git.py +++ b/manic/repository_git.py @@ -14,7 +14,7 @@ from .repository import Repository from .externals_status import ExternalStatus from .utils import expand_local_url, split_remote_url, is_remote_url -from .utils import log_process_output, fatal_error, printlog +from .utils import fatal_error, printlog from .utils import execute_subprocess @@ -80,13 +80,6 @@ def status(self, stat, repo_dir_path): if os.path.exists(repo_dir_path): self._status_summary(stat, repo_dir_path) - def verbose_status_dump(self, repo_dir_path): - """Display the raw repo status to the user. - - """ - if os.path.exists(repo_dir_path): - self._status_verbose_dump(repo_dir_path) - # ---------------------------------------------------------------- # # Internal work functions @@ -508,23 +501,16 @@ def _status_summary(self, stat, repo_dir_path): cwd = os.getcwd() os.chdir(repo_dir_path) git_output = self._git_status_porcelain_v1z() - os.chdir(cwd) is_dirty = self._status_v1z_is_dirty(git_output) if is_dirty: stat.clean_state = ExternalStatus.DIRTY else: stat.clean_state = ExternalStatus.STATUS_OK - def _status_verbose_dump(self, repo_dir_path): - """Display raw git status output to the user - - """ - cwd = os.getcwd() - os.chdir(repo_dir_path) - git_output = self._git_status_verbose() + # Now save the verbose status output incase the user wants to + # see it. + stat.status_output = self._git_status_verbose() os.chdir(cwd) - log_process_output(git_output) - print(git_output) @staticmethod def _status_v1z_is_dirty(git_output): @@ -648,7 +634,7 @@ def _git_clone(url, repo_dir_name, verbosity): """Run git clone for the side effect of creating a repository. """ cmd = ['git', 'clone', url, repo_dir_name] - if verbosity == VERBOSITY_VERBOSE: + if verbosity >= VERBOSITY_VERBOSE: printlog(' {0}'.format(' '.join(cmd))) execute_subprocess(cmd) @@ -675,6 +661,6 @@ def _git_checkout_ref(ref, verbosity): """ cmd = ['git', 'checkout', ref] - if verbosity == VERBOSITY_VERBOSE: + if verbosity >= VERBOSITY_VERBOSE: printlog(' {0}'.format(' '.join(cmd))) execute_subprocess(cmd) diff --git a/manic/repository_svn.py b/manic/repository_svn.py index e2c22ef..5c1a6e7 100644 --- a/manic/repository_svn.py +++ b/manic/repository_svn.py @@ -12,7 +12,7 @@ from .global_constants import EMPTY_STR, VERBOSITY_VERBOSE from .repository import Repository from .externals_status import ExternalStatus -from .utils import fatal_error, log_process_output, indent_string, printlog +from .utils import fatal_error, indent_string, printlog from .utils import execute_subprocess @@ -87,13 +87,6 @@ def status(self, stat, repo_dir_path): if os.path.exists(repo_dir_path): self._status_summary(stat, repo_dir_path) - def verbose_status_dump(self, repo_dir_path): - """Display the raw repo status to the user. - - """ - if os.path.exists(repo_dir_path): - self._status_verbose_dump(repo_dir_path) - # ---------------------------------------------------------------- # # Internal work functions @@ -187,13 +180,9 @@ def _status_summary(self, stat, repo_dir_path): else: stat.clean_state = ExternalStatus.STATUS_OK - def _status_verbose_dump(self, repo_dir_path): - """Display the raw svn status output to the user. - - """ - svn_output = self._svn_status_verbose(repo_dir_path) - log_process_output(svn_output) - print(svn_output) + # Now save the verbose status output incase the user wants to + # see it. + stat.status_output = self._svn_status_verbose(repo_dir_path) @staticmethod def xml_status_is_dirty(svn_output): @@ -265,7 +254,7 @@ def _svn_checkout(url, repo_dir_path, verbosity): Checkout a subversion repository (repo_url) to checkout_dir. """ cmd = ['svn', 'checkout', url, repo_dir_path] - if verbosity == VERBOSITY_VERBOSE: + if verbosity >= VERBOSITY_VERBOSE: printlog(' {0}'.format(' '.join(cmd))) execute_subprocess(cmd) @@ -275,6 +264,6 @@ def _svn_switch(url, verbosity): Switch branches for in an svn sandbox """ cmd = ['svn', 'switch', url] - if verbosity == VERBOSITY_VERBOSE: + if verbosity >= VERBOSITY_VERBOSE: printlog(' {0}'.format(' '.join(cmd))) execute_subprocess(cmd) diff --git a/manic/sourcetree.py b/manic/sourcetree.py index 9d54a45..4a8ea33 100644 --- a/manic/sourcetree.py +++ b/manic/sourcetree.py @@ -42,6 +42,7 @@ def __init__(self, root_dir, name, ext_description): self._repo = None self._externals = EMPTY_STR self._externals_sourcetree = None + self._stat = ExternalStatus() # Parse the sub-elements # _path : local path relative to the containing source tree @@ -86,38 +87,37 @@ def status(self): If load_all is True, also load all of the the externals sub-externals. """ - stat = ExternalStatus() - stat.path = self.get_local_path() + self._stat.path = self.get_local_path() if not self._required: - stat.source_type = ExternalStatus.OPTIONAL + self._stat.source_type = ExternalStatus.OPTIONAL elif self._local_path == LOCAL_PATH_INDICATOR: # LOCAL_PATH_INDICATOR, '.' paths, are standalone # component directories that are not managed by # checkout_externals. - stat.source_type = ExternalStatus.STANDALONE + self._stat.source_type = ExternalStatus.STANDALONE else: # managed by checkout_externals - stat.source_type = ExternalStatus.MANAGED + self._stat.source_type = ExternalStatus.MANAGED ext_stats = {} if not os.path.exists(self._repo_dir_path): - stat.sync_state = ExternalStatus.EMPTY + self._stat.sync_state = ExternalStatus.EMPTY msg = ('status check: repository directory for "{0}" does not ' 'exist.'.format(self._name)) logging.info(msg) - stat.current_version = 'not checked out' + self._stat.current_version = 'not checked out' # NOTE(bja, 2018-01) directory doesn't exist, so we cannot # use repo to determine the expected version. We just take # a best-guess based on the assumption that only tag or # branch should be set, but not both. if not self._repo: - stat.expected_version = 'unknown' + self._stat.expected_version = 'unknown' else: - stat.expected_version = self._repo.tag() + self._repo.branch() + self._stat.expected_version = self._repo.tag() + self._repo.branch() else: if self._repo: - self._repo.status(stat, self._repo_dir_path) + self._repo.status(self._stat, self._repo_dir_path) if self._externals and self._externals_sourcetree: # we expect externals and they exist @@ -134,29 +134,13 @@ def status(self): if self._local_path != LOCAL_PATH_INDICATOR: # store the stats under tha local_path, not comp name so # it will be sorted correctly - all_stats[stat.path] = stat + all_stats[self._stat.path] = self._stat if ext_stats: all_stats.update(ext_stats) return all_stats - def verbose_status_dump(self): - """Display the verbose status to the user. This is just the raw output - from the repository 'status' command. - - """ - if not os.path.exists(self._repo_dir_path): - msg = ('status check: repository directory for "{0}" does not ' - 'exist!'.format(self._name)) - logging.info(msg) - else: - cwd = os.getcwd() - os.chdir(self._repo_dir_path) - if self._repo: - self._repo.verbose_status_dump(self._repo_dir_path) - os.chdir(cwd) - def checkout(self, verbosity, load_all): """ If the repo destination directory exists, ensure it is correct (from @@ -180,11 +164,17 @@ def checkout(self, verbosity, load_all): self._base_dir_path) fatal_error(msg) + if self._stat.source_type != ExternalStatus.STANDALONE: + if verbosity >= VERBOSITY_VERBOSE: + # NOTE(bja, 2018-01) probably do not want to pass + # verbosity in this case, because if (verbosity == + # VERBOSITY_DUMP), then the previous status output would + # also be dumped, adding noise to the output. + self._stat.log_status_message(VERBOSITY_VERBOSE) + if self._repo: - self._repo.checkout( - self._base_dir_path, - self._repo_dir_name, - verbosity) + self._repo.checkout(self._base_dir_path, + self._repo_dir_name, verbosity) def checkout_externals(self, verbosity, load_all): """Checkout the sub-externals for this object @@ -255,6 +245,7 @@ def status(self, relative_path_base=LOCAL_PATH_INDICATOR): summary = {} for comp in load_comps: + printlog('{0}, '.format(comp), end='') stat = self._all_components[comp].status() for name in stat.keys(): # check if we need to append the relative_path_base to @@ -271,15 +262,6 @@ def status(self, relative_path_base=LOCAL_PATH_INDICATOR): return summary - def verbose_status_dump(self): - """Display verbose status to the user. This is just the raw output of - the git and svn status commands. - - """ - load_comps = self._all_components.keys() - for comp in load_comps: - self._all_components[comp].verbose_status_dump() - def checkout(self, verbosity, load_all, load_comp=None): """ Checkout or update indicated components into the the configured @@ -289,7 +271,7 @@ def checkout(self, verbosity, load_all, load_comp=None): If load_all is False, load_comp is an optional set of components to load. If load_all is True and load_comp is None, only load the required externals. """ - if verbosity == VERBOSITY_VERBOSE: + if verbosity >= VERBOSITY_VERBOSE: printlog('Checking out externals: ') else: printlog('Checking out externals: ', end='') @@ -303,10 +285,12 @@ def checkout(self, verbosity, load_all, load_comp=None): # checkout the primary externals for comp in load_comps: - if verbosity == VERBOSITY_VERBOSE: - printlog('\n {0} : '.format(comp)) - else: + if verbosity < VERBOSITY_VERBOSE: printlog('{0}, '.format(comp), end='') + else: + # verbose output handled by the _External object, just + # output a newline + printlog(EMPTY_STR) self._all_components[comp].checkout(verbosity, load_all) printlog('')