results: refactor return values to include the full set of resultsub-automotive-master-20230622

Rather than normalize everything into a bool early on, and then make
it hard to keep track of results across projects, and force creation
of a parallel structure to hold fixup commands, create a new results
object to hold everything and pass that back up the chain.

For now, this is just restructuring, and upload continues to behave
the same.  But this will make it much easier to change the UX.

Bug: None
Test: unittests
Change-Id: Idfa545f02f3d26a893d6cbbdef6d9a2409e72813

2 files changed, 68 insertions, 25 deletions

diff --git a/pre-upload.py b/pre-upload.py
index 5c820c6..5d57a71 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -242,8 +242,11 @@ def _get_project_config(from_git=False):
     return rh.config.PreUploadSettings(paths=paths, global_paths=global_paths)
 
 
-def _attempt_fixes(fixup_list, commit_list, cwd):
-    """Attempts to run |fixup_list| given |commit_list|."""
+def _attempt_fixes(project_results, commit_list):
+    """Attempts to fix fixable results."""
+    cwd = project_results.workdir
+    fixup_list = [(x.hook, x.commit, x.fixup_cmd, x.files)
+                  for x in project_results.fixups]
     if len(fixup_list) != 1:
         # Only single fixes will be attempted, since various fixes might
         # interact with each other.
@@ -280,7 +283,13 @@ def _attempt_fixes(fixup_list, commit_list, cwd):
               'attempting to upload again.\n', file=sys.stderr)
 
 
-def _run_project_hooks_in_cwd(project_name, proj_dir, output, from_git=False, commit_list=None):
+def _run_project_hooks_in_cwd(
+    project_name: str,
+    proj_dir: str,
+    output: Output,
+    from_git: bool = False,
+    commit_list: Optional[List[str]] = None,
+) -> rh.results.ProjectResults:
     """Run the project-specific hooks in the cwd.
 
     Args:
@@ -294,18 +303,21 @@ def _run_project_hooks_in_cwd(project_name, proj_dir, output, from_git=False, co
           uploaded.
 
     Returns:
-      True if everything passed, else False.
+      All the results for this project.
     """
+    ret = rh.results.ProjectResults(project_name, proj_dir)
+
     try:
         config = _get_project_config(from_git)
     except rh.config.ValidationError as e:
         output.error('Loading config files', str(e))
-        return False
+        ret.internal_failure = True
+        return ret
 
     # If the repo has no pre-upload hooks enabled, then just return.
     hooks = list(config.callable_hooks())
     if not hooks:
-        return True
+        return ret
 
     output.set_num_hooks(len(hooks))
 
@@ -316,7 +328,8 @@ def _run_project_hooks_in_cwd(project_name, proj_dir, output, from_git=False, co
     except rh.utils.CalledProcessError as e:
         output.error('Upstream remote/tracking branch lookup',
                      f'{e}\nDid you run repo start?  Is your HEAD detached?')
-        return False
+        ret.internal_failure = True
+        return ret
 
     project = rh.Project(name=project_name, dir=proj_dir)
     rel_proj_dir = os.path.relpath(proj_dir, rh.git.find_repo_root())
@@ -334,9 +347,6 @@ def _run_project_hooks_in_cwd(project_name, proj_dir, output, from_git=False, co
             ignore_merged_commits=config.ignore_merged_commits)
     output.set_num_commits(len(commit_list))
 
-    ret = True
-    fixup_list = []
-
     for commit in commit_list:
         # Mix in some settings for our hooks.
         os.environ['PREUPLOAD_COMMIT'] = commit
@@ -353,25 +363,25 @@ def _run_project_hooks_in_cwd(project_name, proj_dir, output, from_git=False, co
             output.hook_start(name)
             hook_results = hook(project, commit, desc, diff)
             output.hook_finish()
+            ret.add_results(hook_results)
             (error, warning) = _process_hook_results(hook_results)
             if error is not None or warning is not None:
                 if warning is not None:
                     output.hook_warning(warning)
                 if error is not None:
-                    ret = False
                     output.hook_error(error)
-                for result in hook_results:
-                    if result.fixup_cmd:
-                        fixup_list.append(
-                            (name, commit, result.fixup_cmd, result.files))
 
-    if fixup_list:
-        _attempt_fixes(fixup_list, commit_list, proj_dir)
+    _attempt_fixes(ret, commit_list)
 
     return ret
 
 
-def _run_project_hooks(project_name, proj_dir=None, from_git=False, commit_list=None):
+def _run_project_hooks(
+    project_name: str,
+    proj_dir: Optional[str] = None,
+    from_git: bool = False,
+    commit_list: Optional[List[str]] = None,
+) -> rh.results.ProjectResults:
     """Run the project-specific hooks in |proj_dir|.
 
     Args:
@@ -385,7 +395,7 @@ def _run_project_hooks(project_name, proj_dir=None, from_git=False, commit_list=
           uploaded.
 
     Returns:
-      True if everything passed, else False.
+      All the results for this project.
     """
     output = Output(project_name)
 
@@ -437,20 +447,21 @@ def _run_projects_hooks(
     Returns:
       True if everything passed, else False.
     """
-    ret = True
+    results = []
     for project, worktree in zip(project_list, worktree_list):
-        if not _run_project_hooks(
+        result = _run_project_hooks(
             project,
             proj_dir=worktree,
             from_git=from_git,
             commit_list=commit_list,
-        ):
-            ret = False
+        )
+        results.append(result)
+        if result:
             # If a repo had failures, add a blank line to help break up the
             # output.  If there were no failures, then the output should be
             # very minimal, so we don't add it then.
             print('', file=sys.stderr)
-    return ret
+    return not any(results)
 
 
 def main(project_list, worktree_list=None, **_kwargs):
diff --git a/rh/results.py b/rh/results.py
index a754cef..8adcf6b 100644
--- a/rh/results.py
+++ b/rh/results.py
@@ -16,7 +16,7 @@
 
 import os
 import sys
-from typing import List, Optional
+from typing import List, NamedTuple, Optional
 
 _path = os.path.realpath(__file__ + '/../..')
 if sys.path[0] != _path:
@@ -50,9 +50,11 @@ class HookResult(object):
         self.fixup_cmd = fixup_cmd
 
     def __bool__(self):
+        """Whether this result is an error."""
         return bool(self.error)
 
     def is_warning(self):
+        """Whether this result is a non-fatal warning."""
         return False
 
 
@@ -67,7 +69,37 @@ class HookCommandResult(HookResult):
         self.result = result
 
     def __bool__(self):
+        """Whether this result is an error."""
         return self.result.returncode not in (None, 0)
 
     def is_warning(self):
+        """Whether this result is a non-fatal warning."""
         return self.result.returncode == 77
+
+
+class ProjectResults(NamedTuple):
+    """All results for a single project."""
+
+    project: str
+    workdir: str
+
+    # All the results from running all the hooks.
+    results: List[HookResult] = []
+
+    # Whether there were any non-hook related errors.  For example, trying to
+    # parse the project configuration.
+    internal_failure: bool = False
+
+    def add_results(self, results: Optional[List[HookResult]]) -> None:
+        """Add |results| to our results."""
+        if results:
+            self.results.extend(results)
+
+    @property
+    def fixups(self):
+        """Yield results that have a fixup available."""
+        yield from (x for x in self.results if x and x.fixup_cmd)
+
+    def __bool__(self):
+        """Whether there are any errors in this set of results."""
+        return self.internal_failure or any(self.results)