From: Simon Glass <simon.glass@canonical.com> When do_step() finds the number of open MRs is at the maximum, it returns immediately without processing subtree updates or advancing past fully-processed merges. These actions don't create MRs (subtree updates push to the target branch directly), so they should happen regardless. Currently the user must wait for the next poll interval and for an MR slot to open before subtree updates are applied. Move the _prepare_get_commits() call to before the max_mrs check in do_step(), so subtree updates and source-position advances happen unconditionally. Add an optional 'info' parameter to do_apply() and prepare_apply() so the already-fetched result can be passed through, avoiding a redundant call when below the max MR limit. Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/control.py | 27 +++++++++++---- tools/pickman/ftest.py | 72 ++++++++++++++++++++++++++-------------- 2 files changed, 67 insertions(+), 32 deletions(-) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index f4d4a43c292..a42927f991d 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -1663,7 +1663,7 @@ def _prepare_get_commits(dbs, source, args): return info, None -def prepare_apply(dbs, source, branch, args=None): +def prepare_apply(dbs, source, branch, args=None, info=None): """Prepare for applying commits from a source branch Get the next commits, set up the branch name and prints info about @@ -1676,15 +1676,18 @@ def prepare_apply(dbs, source, branch, args=None): branch (str): Branch name to use, or None to auto-generate args (Namespace): Parsed arguments with 'push', 'remote', 'target' (needed for subtree updates) + info (NextCommitsInfo): Pre-fetched commit info from + _prepare_get_commits(), or None to fetch it here Returns: tuple: (ApplyInfo, return_code) where ApplyInfo is set if there are commits to apply, or None with return_code indicating the result (0 for no commits, 1 for error) """ - info, ret = _prepare_get_commits(dbs, source, args) - if ret is not None: - return None, ret + if info is None: + info, ret = _prepare_get_commits(dbs, source, args) + if ret is not None: + return None, ret commits = info.commits @@ -1889,18 +1892,20 @@ def execute_apply(dbs, source, commits, branch_name, args, advance_to=None): # return ret, success, conv_log -def do_apply(args, dbs): +def do_apply(args, dbs, info=None): """Apply the next set of commits using Claude agent Args: args (Namespace): Parsed arguments with 'source' and 'branch' attributes dbs (Database): Database instance + info (NextCommitsInfo): Pre-fetched commit info from + _prepare_get_commits(), or None to fetch during prepare Returns: int: 0 on success, 1 on failure """ source = args.source - info, ret = prepare_apply(dbs, source, args.branch, args) + info, ret = prepare_apply(dbs, source, args.branch, args, info=info) if not info: return ret @@ -2851,6 +2856,14 @@ def do_step(args, dbs): process_pipeline_failures(remote, active_mrs, dbs, args.target, args.fix_retries) + # Process subtree updates and advance past fully-processed merges + # regardless of MR count, since these don't create MRs + info, ret = _prepare_get_commits(dbs, source, args) + if ret is not None: + if ret: + return ret + return 0 + # Only block new MR creation if we've reached the max allowed open MRs max_mrs = args.max_mrs if len(active_mrs) >= max_mrs: @@ -2869,7 +2882,7 @@ def do_step(args, dbs): tout.info('No pending pickman MRs, creating new one...') args.push = True args.branch = None # Let do_apply generate branch name - return do_apply(args, dbs) + return do_apply(args, dbs, info=info) def do_poll(args, dbs): diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index af26cbb4229..ced2c79ac87 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -2061,16 +2061,21 @@ class TestStep(unittest.TestCase): source_branch='cherry-test', description='Test', ) + mock_info = control.NextCommitsInfo( + commits=['fake'], merge_found=True, advance_to='abc123') with mock.patch.object(control, 'run_git'): with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[mock_mr]): - args = argparse.Namespace(cmd='step', source='us/next', - remote='ci', target='master', - max_mrs=1, fix_retries=3) - with terminal.capture(): - ret = control.do_step(args, None) + with mock.patch.object( + control, '_prepare_get_commits', + return_value=(mock_info, None)): + args = argparse.Namespace( + cmd='step', source='us/next', remote='ci', + target='master', max_mrs=1, fix_retries=3) + with terminal.capture(): + ret = control.do_step(args, None) self.assertEqual(ret, 0) @@ -2109,18 +2114,23 @@ class TestStep(unittest.TestCase): source_branch='cherry-test', description='Test', ) + mock_info = control.NextCommitsInfo( + commits=['fake'], merge_found=True, advance_to='abc123') with mock.patch.object(control, 'run_git'): with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[mock_mr]): - with mock.patch.object(control, 'do_apply', - return_value=0) as mock_apply: - args = argparse.Namespace(cmd='step', source='us/next', - remote='ci', target='master', - max_mrs=5, fix_retries=3) - with terminal.capture(): - ret = control.do_step(args, None) + with mock.patch.object( + control, '_prepare_get_commits', + return_value=(mock_info, None)): + with mock.patch.object(control, 'do_apply', + return_value=0) as mock_apply: + args = argparse.Namespace( + cmd='step', source='us/next', remote='ci', + target='master', max_mrs=5, fix_retries=3) + with terminal.capture(): + ret = control.do_step(args, None) # With 1 open MR and max_mrs=5, it should try to create a new one self.assertEqual(ret, 0) @@ -2138,17 +2148,23 @@ class TestStep(unittest.TestCase): ) for i in range(3) ] + mock_info = control.NextCommitsInfo( + commits=['fake'], merge_found=True, advance_to='abc123') with mock.patch.object(control, 'run_git'): with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=mock_mrs): - with mock.patch.object(control, 'do_apply') as mock_apply: - args = argparse.Namespace(cmd='step', source='us/next', - remote='ci', target='master', - max_mrs=3, fix_retries=3) - with terminal.capture(): - ret = control.do_step(args, None) + with mock.patch.object( + control, '_prepare_get_commits', + return_value=(mock_info, None)): + with mock.patch.object(control, 'do_apply') \ + as mock_apply: + args = argparse.Namespace( + cmd='step', source='us/next', remote='ci', + target='master', max_mrs=3, fix_retries=3) + with terminal.capture(): + ret = control.do_step(args, None) # With 3 open MRs and max_mrs=3, should not create new MR self.assertEqual(ret, 0) @@ -6120,19 +6136,25 @@ class TestProcessPipelineFailures(unittest.TestCase): pipeline_status='failed', pipeline_id=100, ) + mock_info = control.NextCommitsInfo( + commits=['fake'], merge_found=True, advance_to='abc123') with mock.patch.object(control, 'run_git'): with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[mock_mr]): with mock.patch.object( - control, 'process_pipeline_failures') as mock_ppf: - args = argparse.Namespace( - cmd='step', source='us/next', - remote='ci', target='master', - max_mrs=1, fix_retries=0) - with terminal.capture(): - control.do_step(args, None) + control, '_prepare_get_commits', + return_value=(mock_info, None)): + with mock.patch.object( + control, + 'process_pipeline_failures') as mock_ppf: + args = argparse.Namespace( + cmd='step', source='us/next', + remote='ci', target='master', + max_mrs=1, fix_retries=0) + with terminal.capture(): + control.do_step(args, None) mock_ppf.assert_not_called() -- 2.43.0