[Concept,5/9] pickman: Return NextCommitsInfo from get_next_commits()

Message ID 20260212211626.167191-6-sjg@u-boot.org
State New
Headers
Series pickman: Improve handling of large merges and add rewind |

Commit Message

Simon Glass Feb. 12, 2026, 9:16 p.m. UTC
  From: Simon Glass <simon.glass@canonical.com>

Add a NextCommitsInfo named tuple with commits and merge_found
fields, and return (NextCommitsInfo, error_msg) from
get_next_commits() instead of a bare 3-tuple.

Update do_next_set() and prepare_apply() to unpack the new
return type, along with the corresponding tests.

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

 tools/pickman/control.py | 49 ++++++++++++++++++++++----------------
 tools/pickman/ftest.py   | 51 +++++++++++++++++-----------------------
 2 files changed, 51 insertions(+), 49 deletions(-)
  

Patch

diff --git a/tools/pickman/control.py b/tools/pickman/control.py
index 3f034d03d72..cd138612623 100644
--- a/tools/pickman/control.py
+++ b/tools/pickman/control.py
@@ -87,6 +87,13 @@  CommitInfo = namedtuple('CommitInfo',
 AgentCommit = namedtuple('AgentCommit',
                          ['hash', 'chash', 'subject', 'applied_as'])
 
+# Named tuple for get_next_commits() result
+#
+# commits: list of CommitInfo to cherry-pick
+# merge_found: True if these commits came from a merge on the source branch
+NextCommitsInfo = namedtuple('NextCommitsInfo',
+                             ['commits', 'merge_found'])
+
 # Named tuple for prepare_apply() result
 #
 # commits: list of AgentCommit to cherry-pick
@@ -751,16 +758,14 @@  def get_next_commits(dbs, source):
         source (str): Source branch name
 
     Returns:
-        tuple: (commits, merge_found, error_msg) where:
-            commits: list of CommitInfo tuples
-            merge_found: bool, True if stopped at a merge commit
-            error_msg: str or None, error message if failed
+        tuple: (NextCommitsInfo, error_msg) where error_msg is None
+            on success
     """
     # Get the last cherry-picked commit from database
     last_commit = dbs.source_get(source)
 
     if not last_commit:
-        return None, False, f"Source '{source}' not found in database"
+        return None, f"Source '{source}' not found in database"
 
     # Get all first-parent commits to find merges
     fp_output = run_git([
@@ -769,7 +774,7 @@  def get_next_commits(dbs, source):
     ])
 
     if not fp_output:
-        return [], False, None
+        return NextCommitsInfo([], False), None
 
     # Build list of merge hashes on the first-parent chain
     merge_hashes = []
@@ -799,7 +804,7 @@  def get_next_commits(dbs, source):
         commits = [c for c in all_commits if not dbs.commit_get(c.hash)]
 
         if commits:
-            return commits, True, None
+            return NextCommitsInfo(commits, True), None
 
         # All commits in this merge are processed, skip to next
         prev_commit = merge_hash
@@ -811,12 +816,12 @@  def get_next_commits(dbs, source):
     ])
 
     if not log_output:
-        return [], False, None
+        return NextCommitsInfo([], False), None
 
     all_commits = parse_log_output(log_output, has_parents=True)
     commits = [c for c in all_commits if not dbs.commit_get(c.hash)]
 
-    return commits, False, None
+    return NextCommitsInfo(commits, False), None
 
 
 def get_commits_for_pick(commit_spec):
@@ -890,23 +895,24 @@  def do_next_set(args, dbs):
         int: 0 on success, 1 if source not found
     """
     source = args.source
-    commits, merge_found, err = get_next_commits(dbs, source)
+    info, err = get_next_commits(dbs, source)
 
     if err:
         tout.error(err)
         return 1
 
-    if not commits:
+    if not info.commits:
         tout.info('No new commits to cherry-pick')
         return 0
 
-    if merge_found:
-        tout.info(f'Next set from {source} ({len(commits)} commits):')
+    if info.merge_found:
+        tout.info(f'Next set from {source} '
+                  f'({len(info.commits)} commits):')
     else:
-        tout.info(f'Remaining commits from {source} ({len(commits)} commits, '
-                  'no merge found):')
+        tout.info(f'Remaining commits from {source} '
+                  f'({len(info.commits)} commits, no merge found):')
 
-    for commit in commits:
+    for commit in info.commits:
         tout.info(f'  {commit.chash} {commit.subject}')
 
     return 0
@@ -1247,16 +1253,18 @@  def prepare_apply(dbs, source, branch):
             commits to apply, or None with return_code indicating the result
             (0 for no commits, 1 for error)
     """
-    commits, merge_found, err = get_next_commits(dbs, source)
+    info, err = get_next_commits(dbs, source)
 
     if err:
         tout.error(err)
         return None, 1
 
-    if not commits:
+    if not info.commits:
         tout.info('No new commits to cherry-pick')
         return None, 0
 
+    commits = info.commits
+
     # Save current branch to return to later
     original_branch = run_git(['rev-parse', '--abbrev-ref', 'HEAD'])
 
@@ -1274,7 +1282,7 @@  def prepare_apply(dbs, source, branch):
     except Exception:  # pylint: disable=broad-except
         pass  # Branch doesn't exist, which is fine
 
-    if merge_found:
+    if info.merge_found:
         tout.info(f'Applying next set from {source} ({len(commits)} commits):')
     else:
         tout.info(f'Applying remaining commits from {source} '
@@ -1285,7 +1293,8 @@  def prepare_apply(dbs, source, branch):
         tout.info(f'  {commit.chash} {commit.subject}')
     tout.info('')
 
-    return ApplyInfo(commits, branch_name, original_branch, merge_found), 0
+    return ApplyInfo(commits, branch_name, original_branch,
+                     info.merge_found), 0
 
 
 # pylint: disable=too-many-arguments
diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py
index b943804e6f8..38e5cef5306 100644
--- a/tools/pickman/ftest.py
+++ b/tools/pickman/ftest.py
@@ -1208,10 +1208,8 @@  class TestGetNextCommits(unittest.TestCase):
         with terminal.capture():
             dbs = database.Database(self.db_path)
             dbs.start()
-            commits, merge_found, err = control.get_next_commits(dbs,
-                                                                   'unknown')
-            self.assertIsNone(commits)
-            self.assertFalse(merge_found)
+            info, err = control.get_next_commits(dbs, 'unknown')
+            self.assertIsNone(info)
             self.assertIn('not found', err)
             dbs.close()
 
@@ -1243,13 +1241,12 @@  class TestGetNextCommits(unittest.TestCase):
 
             command.TEST_RESULT = mock_git
 
-            commits, merge_found, err = control.get_next_commits(dbs,
-                                                                   'us/next')
+            info, err = control.get_next_commits(dbs, 'us/next')
             self.assertIsNone(err)
-            self.assertTrue(merge_found)
-            self.assertEqual(len(commits), 2)
-            self.assertEqual(commits[0].chash, 'aaa111a')
-            self.assertEqual(commits[1].chash, 'bbb222b')
+            self.assertTrue(info.merge_found)
+            self.assertEqual(len(info.commits), 2)
+            self.assertEqual(info.commits[0].chash, 'aaa111a')
+            self.assertEqual(info.commits[1].chash, 'bbb222b')
             dbs.close()
 
 
@@ -2870,11 +2867,10 @@  class TestGetNextCommitsEmptyLine(unittest.TestCase):
             )
             command.TEST_RESULT = command.CommandResult(stdout=log_output)
 
-            commits, merge_found, err = control.get_next_commits(dbs,
-                                                                   'us/next')
+            info, err = control.get_next_commits(dbs, 'us/next')
             self.assertIsNone(err)
-            self.assertFalse(merge_found)
-            self.assertEqual(len(commits), 2)
+            self.assertFalse(info.merge_found)
+            self.assertEqual(len(info.commits), 2)
             dbs.close()
 
     def test_get_next_commits_skips_db_commits(self):
@@ -2897,13 +2893,12 @@  class TestGetNextCommitsEmptyLine(unittest.TestCase):
             )
             command.TEST_RESULT = command.CommandResult(stdout=log_output)
 
-            commits, merge_found, err = control.get_next_commits(dbs,
-                                                                   'us/next')
+            info, err = control.get_next_commits(dbs, 'us/next')
             self.assertIsNone(err)
-            self.assertFalse(merge_found)
+            self.assertFalse(info.merge_found)
             # Only second commit should be returned (first is in DB)
-            self.assertEqual(len(commits), 1)
-            self.assertEqual(commits[0].chash, 'bbb222b')
+            self.assertEqual(len(info.commits), 1)
+            self.assertEqual(info.commits[0].chash, 'bbb222b')
             dbs.close()
 
     def test_get_next_commits_all_in_db(self):
@@ -2928,12 +2923,11 @@  class TestGetNextCommitsEmptyLine(unittest.TestCase):
             )
             command.TEST_RESULT = command.CommandResult(stdout=log_output)
 
-            commits, merge_found, err = control.get_next_commits(dbs,
-                                                                   'us/next')
+            info, err = control.get_next_commits(dbs, 'us/next')
             self.assertIsNone(err)
-            self.assertFalse(merge_found)
+            self.assertFalse(info.merge_found)
             # No commits should be returned (all in DB)
-            self.assertEqual(len(commits), 0)
+            self.assertEqual(len(info.commits), 0)
             dbs.close()
 
     def test_get_next_commits_skips_processed_merge(self):
@@ -2987,14 +2981,13 @@  class TestGetNextCommitsEmptyLine(unittest.TestCase):
 
             command.TEST_RESULT = mock_git
 
-            commits, merge_found, err = control.get_next_commits(dbs,
-                                                                   'us/next')
+            info, err = control.get_next_commits(dbs, 'us/next')
             self.assertIsNone(err)
-            self.assertTrue(merge_found)
+            self.assertTrue(info.merge_found)
             # Should return commits from second merge (first was skipped)
-            self.assertEqual(len(commits), 2)
-            self.assertEqual(commits[0].chash, 'ccc333c')
-            self.assertEqual(commits[1].chash, 'merge2m')
+            self.assertEqual(len(info.commits), 2)
+            self.assertEqual(info.commits[0].chash, 'ccc333c')
+            self.assertEqual(info.commits[1].chash, 'merge2m')
             dbs.close()