[Concept,02/16] pickman: Refactor do_rewind() into smaller helpers

Message ID 20260222154303.2851319-3-sjg@u-boot.org
State New
Headers
Series pickman: Support monitoring and fixing pipeline failures |

Commit Message

Simon Glass Feb. 22, 2026, 3:42 p.m. UTC
  From: Simon Glass <simon.glass@canonical.com>

do_rewind() is ~145 lines with 8 distinct sequential phases, making it
hard to follow. Extract five private helpers that each handle one phase:

- _rewind_fetch_merges(): fetch merge history and locate the target
- _rewind_get_range_commits(): list commits in the range and filter to
  those in the database
- _rewind_find_branches(): match cherry-pick branches against the range
- _rewind_find_mrs(): look up MR details for matching branches
- _rewind_show_summary(): display the dry-run / execution summary

This reduces do_rewind() to ~40 lines of sequential calls with clear
control flow, while each helper is independently understandable.

Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Simon Glass <simon.glass@canonical.com>
---

 tools/pickman/control.py | 196 +++++++++++++++++++++++++--------------
 1 file changed, 124 insertions(+), 72 deletions(-)
  

Patch

diff --git a/tools/pickman/control.py b/tools/pickman/control.py
index 40bcaaef120..868e03df8c9 100644
--- a/tools/pickman/control.py
+++ b/tools/pickman/control.py
@@ -1836,33 +1836,13 @@  def do_commit_source(args, dbs):
     return 0
 
 
-def do_rewind(args, dbs):
-    """Rewind the source position back by N merges
-
-    By default performs a dry run, showing what would happen. Use --force
-    to actually execute the rewind.
-
-    Walks back N merges on the first-parent chain from the current source
-    position, deletes the commits in that range from the database, and
-    resets the source to the earlier position.
-
-    Args:
-        args (Namespace): Parsed arguments with 'source', 'count', 'force'
-        dbs (Database): Database instance
+def _rewind_fetch_merges(current, count):
+    """Fetch first-parent merges and find target index.
 
     Returns:
-        int: 0 on success, 1 on failure
+        tuple: (merges, target_idx) where merges is a list of
+            (hash, short_hash, subject) tuples, or None on error
     """
-    source = args.source
-    count = args.count
-    force = args.force
-
-    current = dbs.source_get(source)
-    if not current:
-        tout.error(f"Source '{source}' not found in database")
-        return 1
-
-    # We need to find merges *before* current. Use ancestry instead.
     try:
         out = run_git([
             'log', '--first-parent', '--merges', '--format=%H|%h|%s',
@@ -1870,31 +1850,34 @@  def do_rewind(args, dbs):
         ])
     except Exception:  # pylint: disable=broad-except
         tout.error(f'Could not read merge history for {current[:12]}')
-        return 1
+        return None
 
     if not out:
         tout.error('No merges found in history')
-        return 1
+        return None
 
-    # Parse merges - first line is current (or nearest merge), last is target
     merges = []
     for line in out.strip().split('\n'):
         if not line:
             continue
         parts = line.split('|', 2)
-        merges.append((parts[0], parts[1], parts[2] if len(parts) > 2 else ''))
+        merges.append((parts[0], parts[1],
+                        parts[2] if len(parts) > 2 else ''))
 
     if len(merges) < 2:
         tout.error(f'Not enough merges to rewind by {count}')
-        return 1
+        return None
 
-    # The target is count merges back from the first entry
     target_idx = min(count, len(merges) - 1)
-    target_hash = merges[target_idx][0]
-    target_chash = merges[target_idx][1]
-    target_subject = merges[target_idx][2]
+    return merges, target_idx
+
 
-    # Get all commits in the range target..current
+def _rewind_get_range_commits(dbs, target_hash, current):
+    """Get commits in range and filter to those in database.
+
+    Returns:
+        tuple: (range_hashes_str, db_commits_list) or None on error
+    """
     try:
         range_hashes = run_git([
             'rev-list', f'{target_hash}..{current}'
@@ -1902,52 +1885,79 @@  def do_rewind(args, dbs):
     except Exception:  # pylint: disable=broad-except
         tout.error(f'Could not list commits in range '
                    f'{target_hash[:12]}..{current[:12]}')
-        return 1
+        return None
 
-    # Count commits that exist in the database
     db_commits = []
     if range_hashes:
         for chash in range_hashes.strip().split('\n'):
             if chash and dbs.commit_get(chash):
                 db_commits.append(chash)
 
-    # Find cherry-pick branches that match commits in the range.
-    # List all ci/cherry-* remote branches, then check if the hash in
-    # the branch name matches any commit in the rewound range.
+    return range_hashes, db_commits
+
+
+def _rewind_find_branches(range_hashes, remote):
+    """Find cherry-pick branches matching commits in the range.
+
+    Returns:
+        list: Branch names (without remote prefix) that match
+    """
+    if not range_hashes:
+        return []
+
+    hash_set = set(range_hashes.strip().split('\n'))
+    try:
+        branch_out = run_git(
+            ['branch', '-r', '--list', f'{remote}/cherry-*'])
+    except Exception:  # pylint: disable=broad-except
+        branch_out = ''
+
     mr_branches = []
-    if range_hashes:
-        hash_set = set(range_hashes.strip().split('\n'))
-        try:
-            branch_out = run_git(
-                ['branch', '-r', '--list', f'{args.remote}/cherry-*'])
-        except Exception:  # pylint: disable=broad-except
-            branch_out = ''
-        remote_prefix = f'{args.remote}/'
-        for line in branch_out.strip().split('\n'):
-            branch = line.strip()
-            if not branch:
-                continue
-            # Branch is like 'ci/cherry-abc1234'; extract the hash part
-            short = branch.removeprefix(f'{remote_prefix}cherry-')
-            # Check if any commit in the range starts with this hash
-            for chash in hash_set:
-                if chash.startswith(short):
-                    mr_branches.append(
-                        branch.removeprefix(remote_prefix))
-                    break
-
-    # Look up MR details from GitLab for matching branches
+    remote_prefix = f'{remote}/'
+    for line in branch_out.strip().split('\n'):
+        branch = line.strip()
+        if not branch:
+            continue
+        # Branch is like 'ci/cherry-abc1234'; extract the hash part
+        short = branch.removeprefix(f'{remote_prefix}cherry-')
+        # Check if any commit in the range starts with this hash
+        for chash in hash_set:
+            if chash.startswith(short):
+                mr_branches.append(
+                    branch.removeprefix(remote_prefix))
+                break
+
+    return mr_branches
+
+
+def _rewind_find_mrs(mr_branches, remote):
+    """Look up MR details for matching branches.
+
+    Returns:
+        list: PickmanMr objects whose source_branch matches
+    """
+    if not mr_branches:
+        return []
+
     matched_mrs = []
-    if mr_branches:
-        mrs = gitlab_api.get_open_pickman_mrs(args.remote)
-        if mrs:
-            branch_set = set(mr_branches)
-            for merge_req in mrs:
-                if merge_req.source_branch in branch_set:
-                    matched_mrs.append(merge_req)
-
-    # Show what would happen (or what is happening)
+    mrs = gitlab_api.get_open_pickman_mrs(remote)
+    if mrs:
+        branch_set = set(mr_branches)
+        for merge_req in mrs:
+            if merge_req.source_branch in branch_set:
+                matched_mrs.append(merge_req)
+
+    return matched_mrs
+
+
+def _rewind_show_summary(source, current, merges, target_idx,
+                         db_commits, matched_mrs, mr_branches,
+                         force):
+    """Display rewind summary."""
     current_short = current[:12]
+    target_chash = merges[target_idx][1]
+    target_subject = merges[target_idx][2]
+
     prefix = '' if force else '[dry run] '
     tout.info(f"{prefix}Rewind '{source}': "
               f'{current_short} -> {target_chash}')
@@ -1967,15 +1977,57 @@  def do_rewind(args, dbs):
         for branch in mr_branches:
             tout.info(f'    {branch}')
 
+
+def do_rewind(args, dbs):
+    """Rewind the source position back by N merges
+
+    By default performs a dry run, showing what would happen. Use --force
+    to actually execute the rewind.
+
+    Walks back N merges on the first-parent chain from the current source
+    position, deletes the commits in that range from the database, and
+    resets the source to the earlier position.
+
+    Args:
+        args (Namespace): Parsed arguments with 'source', 'count', 'force'
+        dbs (Database): Database instance
+
+    Returns:
+        int: 0 on success, 1 on failure
+    """
+    source = args.source
+    count = args.count
+    force = args.force
+
+    current = dbs.source_get(source)
+    if not current:
+        tout.error(f"Source '{source}' not found in database")
+        return 1
+
+    result = _rewind_fetch_merges(current, count)
+    if not result:
+        return 1
+    merges, target_idx = result
+    target_hash = merges[target_idx][0]
+
+    result = _rewind_get_range_commits(dbs, target_hash, current)
+    if not result:
+        return 1
+    range_hashes, db_commits = result
+
+    mr_branches = _rewind_find_branches(range_hashes, args.remote)
+    matched_mrs = _rewind_find_mrs(mr_branches, args.remote)
+
+    _rewind_show_summary(source, current, merges, target_idx,
+                         db_commits, matched_mrs, mr_branches, force)
+
     if not force:
         tout.info('Use --force to execute this rewind')
         return 0
 
-    # Delete commits from database
     for chash in db_commits:
         dbs.commit_delete(chash)
 
-    # Update source position
     dbs.source_set(source, target_hash)
     dbs.commit()