From: Simon Glass <simon.glass@canonical.com> Add a 'rewind' command that moves the source position back by N merges, deleting the corresponding commits from the database. This is useful when a cherry-pick run needs to be retried from an earlier point. By default the command performs a dry run, showing what would happen without modifying the database. Use --force to actually execute the rewind. The command also identifies cherry-pick branches on the remote that correspond to the rewound range, so the user knows which MRs to delete on GitLab. Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/__main__.py | 10 + tools/pickman/control.py | 214 ++++++++++++++++-- tools/pickman/database.py | 8 + tools/pickman/ftest.py | 425 ++++++++++++++++++++++++++++++++++-- tools/pickman/gitlab_api.py | 22 +- 5 files changed, 643 insertions(+), 36 deletions(-) diff --git a/tools/pickman/__main__.py b/tools/pickman/__main__.py index effb74a1db2..7814fd0fedc 100755 --- a/tools/pickman/__main__.py +++ b/tools/pickman/__main__.py @@ -100,6 +100,16 @@ def add_main_commands(subparsers): review_cmd.add_argument('-r', '--remote', default='ci', help='Git remote (default: ci)') + rewind_cmd = subparsers.add_parser( + 'rewind', help='Rewind source position back by N merges') + rewind_cmd.add_argument('source', help='Source branch name') + rewind_cmd.add_argument('-c', '--count', type=int, default=1, + help='Number of merges to rewind (default: 1)') + rewind_cmd.add_argument('-f', '--force', action='store_true', + help='Actually execute (default is dry run)') + rewind_cmd.add_argument('-r', '--remote', default='ci', + help='Git remote for MR lookup (default: ci)') + step_cmd = subparsers.add_parser('step', help='Create MR if none pending') step_cmd.add_argument('source', help='Source branch name') diff --git a/tools/pickman/control.py b/tools/pickman/control.py index 2e6f49f1816..8f7568e47bf 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -1124,9 +1124,46 @@ def do_next_merges(args, dbs): if len(merges) >= count: break - tout.info(f'Next {len(merges)} merges from {source}:') - for i, (_, chash, subject) in enumerate(merges, 1): - tout.info(f' {i}. {chash} {subject}') + # Build display list, expanding mega-merges into sub-merges + # Each entry is (chash, subject, is_mega, sub_list) where sub_list + # is a list of (chash, subject) for mega-merge sub-merges + display = [] + total_sub = 0 + for commit_hash, chash, subject in merges: + sub_merges = detect_sub_merges(commit_hash) + if sub_merges: + sub_list = [] + for sub_hash in sub_merges: + try: + info = run_git( + ['log', '-1', '--format=%h|%s', sub_hash]) + parts = info.strip().split('|', 1) + sub_chash = parts[0] + sub_subject = parts[1] if len(parts) > 1 else '' + except Exception: # pylint: disable=broad-except + sub_chash = sub_hash[:11] + sub_subject = '(unknown)' + sub_list.append((sub_chash, sub_subject)) + display.append((chash, subject, True, sub_list)) + total_sub += len(sub_list) + else: + display.append((chash, subject, False, None)) + + n_items = total_sub + len(merges) - len( + [d for d in display if d[2]]) + tout.info(f'Next merges from {source} ' + f'({n_items} from {len(merges)} first-parent):') + idx = 1 + for chash, subject, is_mega, sub_list in display: + if is_mega: + tout.info(f' {chash} {subject} ' + f'({len(sub_list)} sub-merges):') + for sub_chash, sub_subject in sub_list: + tout.info(f' {idx}. {sub_chash} {sub_subject}') + idx += 1 + else: + tout.info(f' {idx}. {chash} {subject}') + idx += 1 return 0 @@ -1446,12 +1483,9 @@ def prepare_apply(dbs, source, branch): branch_name = f'cherry-{commits[0].chash}' # Delete branch if it already exists - try: - run_git(['rev-parse', '--verify', branch_name]) + if run_git(['branch', '--list', branch_name]).strip(): tout.info(f'Deleting existing branch {branch_name}') run_git(['branch', '-D', branch_name]) - except Exception: # pylint: disable=broad-except - pass # Branch doesn't exist, which is fine if info.merge_found: tout.info(f'Applying next set from {source} ({len(commits)} commits):') @@ -1589,8 +1623,10 @@ def execute_apply(dbs, source, commits, branch_name, args, advance_to=None): # # Verify the branch actually exists - agent may have aborted and deleted it if success: try: - run_git(['rev-parse', '--verify', branch_name]) + exists = run_git(['branch', '--list', branch_name]).strip() except Exception: # pylint: disable=broad-except + exists = '' + if not exists: tout.warning(f'Branch {branch_name} does not exist - ' 'agent may have aborted') success = False @@ -1693,12 +1729,9 @@ def do_pick(args, dbs): # pylint: disable=unused-argument,too-many-locals branch_name = f'pick-{commits[0].chash}' # Delete branch if it already exists - try: - run_git(['rev-parse', '--verify', branch_name]) + if run_git(['branch', '--list', branch_name]).strip(): tout.info(f'Deleting existing branch {branch_name}') run_git(['branch', '-D', branch_name]) - except Exception: # pylint: disable=broad-except - pass # Branch doesn't exist, which is fine tout.info(f'Cherry-picking {len(commits)} commit(s):') tout.info(f' Branch: {branch_name}') @@ -1717,8 +1750,10 @@ def do_pick(args, dbs): # pylint: disable=unused-argument,too-many-locals # Verify the branch actually exists - agent may have aborted and deleted it if success: try: - run_git(['rev-parse', '--verify', branch_name]) + exists = run_git(['branch', '--list', branch_name]).strip() except Exception: # pylint: disable=broad-except + exists = '' + if not exists: tout.warning(f'Branch {branch_name} does not exist - ' 'agent may have aborted') success = False @@ -1801,6 +1836,154 @@ 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 + + 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 + + # We need to find merges *before* current. Use ancestry instead. + try: + out = run_git([ + 'log', '--first-parent', '--merges', '--format=%H|%h|%s', + f'-{count + 1}', current + ]) + except Exception: # pylint: disable=broad-except + tout.error(f'Could not read merge history for {current[:12]}') + return 1 + + if not out: + tout.error('No merges found in history') + return 1 + + # 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 '')) + + if len(merges) < 2: + tout.error(f'Not enough merges to rewind by {count}') + return 1 + + # 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] + + # Get all commits in the range target..current + try: + range_hashes = run_git([ + 'rev-list', f'{target_hash}..{current}' + ]) + except Exception: # pylint: disable=broad-except + tout.error(f'Could not list commits in range ' + f'{target_hash[:12]}..{current[:12]}') + return 1 + + # 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. + 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 + 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) + current_short = current[:12] + prefix = '' if force else '[dry run] ' + tout.info(f"{prefix}Rewind '{source}': " + f'{current_short} -> {target_chash}') + tout.info(f' Target: {target_chash} {target_subject}') + tout.info(f' Merges being rewound:') + for i in range(target_idx): + tout.info(f' {merges[i][1]} {merges[i][2]}') + tout.info(f' Commits to delete from database: {len(db_commits)}') + + if matched_mrs: + tout.info(f' MRs to delete on GitLab:') + for merge_req in matched_mrs: + tout.info(f' !{merge_req.iid}: {merge_req.title}') + tout.info(f' {merge_req.web_url}') + elif mr_branches: + tout.info(f' Branches to check for MRs:') + for branch in mr_branches: + tout.info(f' {branch}') + + 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() + + tout.info(f' Deleted {len(db_commits)} commit(s) from database') + + return 0 + + # pylint: disable=too-many-locals,too-many-branches,too-many-statements def process_single_mr(remote, merge_req, dbs, target): """Process review comments on a single MR @@ -2071,6 +2254,10 @@ def process_merged_mrs(remote, source, dbs): f"MR !{merge_req.iid}") continue + # Skip if already at this position + if full_hash == current: + continue + # Check if this commit is newer than current (current is ancestor of it) try: # Is current an ancestor of last_hash? (meaning last_hash is newer) @@ -2225,6 +2412,7 @@ COMMANDS = { 'poll': do_poll, 'push-branch': do_push_branch, 'review': do_review, + 'rewind': do_rewind, 'step': do_step, 'test': do_test, } diff --git a/tools/pickman/database.py b/tools/pickman/database.py index f584fe14c21..92bff7a5702 100644 --- a/tools/pickman/database.py +++ b/tools/pickman/database.py @@ -364,6 +364,14 @@ class Database: # pylint: disable=too-many-public-methods 'UPDATE pcommit SET mergereq_id = ? WHERE chash = ?', (mergereq_id, chash)) + def commit_delete(self, chash): + """Delete a commit from the database + + Args: + chash (str): Commit hash to delete + """ + self.execute('DELETE FROM pcommit WHERE chash = ?', (chash,)) + # mergereq functions # pylint: disable-next=too-many-arguments diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index eb69bde96a4..f07e4ecb0db 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -1025,6 +1025,23 @@ class TestNextMerges(unittest.TestCase): database.Database.instances.clear() command.TEST_RESULT = None + def _make_simple_merge_mock(self, log_output): + """Create a mock handler for merges with no sub-merges""" + def mock_git(pipe_list): + cmd = pipe_list[0] if pipe_list else [] + # Initial merge listing + if '--reverse' in cmd and '--format=%H|%h|%s' in cmd: + return command.CommandResult(stdout=log_output) + # Sub-merge detection: no sub-merges + if '--first-parent' in cmd and '--merges' in cmd: + return command.CommandResult(stdout='') + # Parent lookup for detect_sub_merges + if 'rev-parse' in cmd: + return command.CommandResult( + stdout='parent1\nparent2\n') + return command.CommandResult(stdout='') + return mock_git + def test_next_merges(self): """Test next-merges shows upcoming merges""" # Add source to database @@ -1037,20 +1054,19 @@ class TestNextMerges(unittest.TestCase): database.Database.instances.clear() - # Mock git log with merge commits log_output = ( 'aaa111|aaa111a|Merge branch feature-1\n' 'bbb222|bbb222b|Merge branch feature-2\n' 'ccc333|ccc333c|Merge branch feature-3\n' ) - command.TEST_RESULT = command.CommandResult(stdout=log_output) + command.TEST_RESULT = self._make_simple_merge_mock(log_output) args = argparse.Namespace(cmd='next-merges', source='us/next', count=10) with terminal.capture() as (stdout, _): ret = control.do_pickman(args) self.assertEqual(ret, 0) output = stdout.getvalue() - self.assertIn('Next 3 merges from us/next:', output) + self.assertIn('3 from 3 first-parent', output) self.assertIn('1. aaa111a Merge branch feature-1', output) self.assertIn('2. bbb222b Merge branch feature-2', output) self.assertIn('3. ccc333c Merge branch feature-3', output) @@ -1067,24 +1083,73 @@ class TestNextMerges(unittest.TestCase): database.Database.instances.clear() - # Mock git log with merge commits log_output = ( 'aaa111|aaa111a|Merge branch feature-1\n' 'bbb222|bbb222b|Merge branch feature-2\n' 'ccc333|ccc333c|Merge branch feature-3\n' ) - command.TEST_RESULT = command.CommandResult(stdout=log_output) + command.TEST_RESULT = self._make_simple_merge_mock(log_output) args = argparse.Namespace(cmd='next-merges', source='us/next', count=2) with terminal.capture() as (stdout, _): ret = control.do_pickman(args) self.assertEqual(ret, 0) output = stdout.getvalue() - self.assertIn('Next 2 merges from us/next:', output) + self.assertIn('2 from 2 first-parent', output) self.assertIn('1. aaa111a', output) self.assertIn('2. bbb222b', output) self.assertNotIn('3. ccc333c', output) + def test_next_merges_expands_mega_merge(self): + """Test next-merges expands mega-merges into sub-merges""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + dbs.source_set('us/next', 'abc123') + dbs.commit() + dbs.close() + + database.Database.instances.clear() + + def mock_git(pipe_list): + cmd = pipe_list[0] if pipe_list else [] + cmd_str = ' '.join(cmd) + # Initial merge listing - one mega-merge + if '--reverse' in cmd and '--format=%H|%h|%s' in cmd: + return command.CommandResult( + stdout='mega111|mega111a|Merge branch next\n') + # Parent lookup + if 'rev-parse' in cmd and '^@' in cmd_str: + return command.CommandResult( + stdout='first_parent\nsecond_parent\n') + # Sub-merge detection on second parent chain + if ('--first-parent' in cmd and '--merges' in cmd + and '--format=%H' in cmd): + return command.CommandResult( + stdout='sub_aaa\nsub_bbb\n') + # Sub-merge detail lookup + if 'log' in cmd and '-1' in cmd and '--format=%h|%s' in cmd: + if 'sub_aaa' in cmd_str: + return command.CommandResult( + stdout='sub_aaa1|Merge feature-A\n') + if 'sub_bbb' in cmd_str: + return command.CommandResult( + stdout='sub_bbb1|Merge feature-B\n') + return command.CommandResult(stdout='') + + command.TEST_RESULT = mock_git + + args = argparse.Namespace(cmd='next-merges', source='us/next', count=10) + with terminal.capture() as (stdout, _): + ret = control.do_pickman(args) + self.assertEqual(ret, 0) + output = stdout.getvalue() + self.assertIn('2 from 1 first-parent', output) + self.assertIn('mega111a Merge branch next', output) + self.assertIn('2 sub-merges', output) + self.assertIn('1. sub_aaa1 Merge feature-A', output) + self.assertIn('2. sub_bbb1 Merge feature-B', output) + def test_next_merges_no_merges(self): """Test next-merges with no merges remaining""" # Add source to database @@ -1496,16 +1561,24 @@ class TestPushBranch(unittest.TestCase): with mock.patch.object(gitlab, 'get_push_url', return_value=TEST_SHORT_OAUTH_URL): with mock.patch.object(command, 'output') as mock_output: + mock_output.side_effect = [ + None, # fetch succeeds + 'abc123def\n', # rev-parse returns OID + None, # push succeeds + ] result = gitlab.push_branch('ci', 'test-branch', force=True) self.assertTrue(result) - # Should fetch first, then push with --force-with-lease + # Should fetch, rev-parse, then push with --force-with-lease calls = mock_output.call_args_list - self.assertEqual(len(calls), 2) - self.assertEqual(calls[0], mock.call('git', 'fetch', 'ci', - 'test-branch')) - push_args = calls[1][0] - self.assertIn('--force-with-lease=refs/remotes/ci/test-branch', + self.assertEqual(len(calls), 3) + self.assertEqual(calls[0], mock.call( + 'git', 'fetch', 'ci', + '+refs/heads/test-branch:refs/remotes/ci/test-branch')) + self.assertEqual(calls[1], mock.call( + 'git', 'rev-parse', 'refs/remotes/ci/test-branch')) + push_args = calls[2][0] + self.assertIn('--force-with-lease=test-branch:abc123def', push_args) def test_push_branch_force_no_remote_ref(self): @@ -3477,6 +3550,330 @@ class TestDoCommitSourceResolveError(unittest.TestCase): self.assertIn('Could not resolve', stderr.getvalue()) +class TestRewind(unittest.TestCase): + """Tests for rewind command.""" + + def setUp(self): + """Set up test fixtures.""" + fd, self.db_path = tempfile.mkstemp(suffix='.db') + os.close(fd) + os.unlink(self.db_path) + self.old_db_fname = control.DB_FNAME + control.DB_FNAME = self.db_path + database.Database.instances.clear() + + def tearDown(self): + """Clean up test fixtures.""" + control.DB_FNAME = self.old_db_fname + if os.path.exists(self.db_path): + os.unlink(self.db_path) + database.Database.instances.clear() + command.TEST_RESULT = None + + def test_rewind_source_not_found(self): + """Test rewind with unknown source.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + dbs.close() + + database.Database.instances.clear() + + args = argparse.Namespace(cmd='rewind', source='unknown', count=1, + force=True, remote='ci') + with terminal.capture() as (_, stderr): + ret = control.do_pickman(args) + self.assertEqual(ret, 1) + self.assertIn("Source 'unknown' not found", stderr.getvalue()) + + def test_rewind_dry_run(self): + """Test rewind dry run shows what would happen without executing.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + dbs.source_set('us/next', 'current_hash') + source_id = dbs.source_get_id('us/next') + dbs.commit_add('commit_a', source_id, 'Commit A', 'Author') + dbs.commit_add('commit_b', source_id, 'Commit B', 'Author') + dbs.commit() + dbs.close() + + database.Database.instances.clear() + + def mock_git(pipe_list): + cmd = pipe_list[0] if pipe_list else [] + if '--merges' in cmd: + return command.CommandResult( + stdout='current_hash|current1|Current merge\n' + 'prev_hash|prev1234|Previous merge\n') + if 'rev-list' in cmd: + return command.CommandResult( + stdout='commit_a\ncommit_b\n') + if 'branch' in cmd and '--list' in cmd: + return command.CommandResult(stdout='') + return command.CommandResult(stdout='') + + command.TEST_RESULT = mock_git + + args = argparse.Namespace(cmd='rewind', source='us/next', count=1, + force=False, remote='ci') + with terminal.capture() as (stdout, _): + ret = control.do_pickman(args) + self.assertEqual(ret, 0) + output = stdout.getvalue() + self.assertIn('dry run', output) + self.assertIn('prev1234', output) + self.assertIn('2', output) # 2 commits to delete + self.assertIn('--force', output) + + # Verify database was NOT modified + database.Database.instances.clear() + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + self.assertEqual(dbs.source_get('us/next'), 'current_hash') + self.assertIsNotNone(dbs.commit_get('commit_a')) + self.assertIsNotNone(dbs.commit_get('commit_b')) + dbs.close() + + def test_rewind_one_merge(self): + """Test rewinding by one merge with --force.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + dbs.source_set('us/next', 'current_hash') + source_id = dbs.source_get_id('us/next') + # Add some commits that should be deleted + dbs.commit_add('commit_a', source_id, 'Commit A', 'Author') + dbs.commit_add('commit_b', source_id, 'Commit B', 'Author') + dbs.commit() + dbs.close() + + database.Database.instances.clear() + + def mock_git(pipe_list): + cmd = pipe_list[0] if pipe_list else [] + if '--merges' in cmd: + return command.CommandResult( + stdout='current_hash|current1|Current merge\n' + 'prev_hash|prev1234|Previous merge\n') + if 'rev-list' in cmd: + return command.CommandResult( + stdout='commit_a\ncommit_b\n') + if 'branch' in cmd and '--list' in cmd: + return command.CommandResult(stdout='') + return command.CommandResult(stdout='') + + command.TEST_RESULT = mock_git + + args = argparse.Namespace(cmd='rewind', source='us/next', count=1, + force=True, remote='ci') + with terminal.capture() as (stdout, _): + ret = control.do_pickman(args) + self.assertEqual(ret, 0) + output = stdout.getvalue() + self.assertIn('prev1234', output) + self.assertIn('Deleted 2 commit(s)', output) + + # Verify source was updated + database.Database.instances.clear() + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + self.assertEqual(dbs.source_get('us/next'), 'prev_hash') + # Commits should be deleted + self.assertIsNone(dbs.commit_get('commit_a')) + self.assertIsNone(dbs.commit_get('commit_b')) + dbs.close() + + def test_rewind_shows_mr_details(self): + """Test rewind shows MR numbers, titles and URLs.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + dbs.source_set('us/next', 'current_hash') + dbs.commit() + dbs.close() + + database.Database.instances.clear() + + def mock_git(pipe_list): + cmd = pipe_list[0] if pipe_list else [] + if '--merges' in cmd: + return command.CommandResult( + stdout='current_hash|current1|Current merge\n' + 'prev_hash|prev1234|Previous merge\n') + if 'rev-list' in cmd: + return command.CommandResult( + stdout='abc1234ffffff\ndef5678aaaaaa\n') + if 'branch' in cmd and '--list' in cmd: + return command.CommandResult( + stdout=' ci/cherry-abc1234f\n' + ' ci/cherry-other99\n') + return command.CommandResult(stdout='') + + command.TEST_RESULT = mock_git + + mock_mrs = [ + gitlab.PickmanMr( + iid=541, title='[pickman] Some cherry-pick', + web_url='https://gitlab.com/proj/-/merge_requests/541', + source_branch='cherry-abc1234f', + description='desc'), + gitlab.PickmanMr( + iid=540, title='[pickman] Unrelated MR', + web_url='https://gitlab.com/proj/-/merge_requests/540', + source_branch='cherry-zzz9999', + description='desc'), + ] + + args = argparse.Namespace(cmd='rewind', source='us/next', count=1, + force=False, remote='ci') + with mock.patch.object(gitlab, 'get_open_pickman_mrs', + return_value=mock_mrs): + with terminal.capture() as (stdout, _): + ret = control.do_pickman(args) + self.assertEqual(ret, 0) + output = stdout.getvalue() + self.assertIn('!541', output) + self.assertIn('[pickman] Some cherry-pick', output) + self.assertIn('merge_requests/541', output) + # Unrelated MR should not appear + self.assertNotIn('!540', output) + self.assertIn('MRs to delete', output) + + def test_rewind_shows_branches_when_api_unavailable(self): + """Test rewind falls back to branch names when GitLab unavailable.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + dbs.source_set('us/next', 'current_hash') + dbs.commit() + dbs.close() + + database.Database.instances.clear() + + def mock_git(pipe_list): + cmd = pipe_list[0] if pipe_list else [] + if '--merges' in cmd: + return command.CommandResult( + stdout='current_hash|current1|Current merge\n' + 'prev_hash|prev1234|Previous merge\n') + if 'rev-list' in cmd: + return command.CommandResult( + stdout='abc1234ffffff\n') + if 'branch' in cmd and '--list' in cmd: + return command.CommandResult( + stdout=' ci/cherry-abc1234f\n') + return command.CommandResult(stdout='') + + command.TEST_RESULT = mock_git + + args = argparse.Namespace(cmd='rewind', source='us/next', count=1, + force=False, remote='ci') + # GitLab API returns None (unavailable) + with mock.patch.object(gitlab, 'get_open_pickman_mrs', + return_value=None): + with terminal.capture() as (stdout, _): + ret = control.do_pickman(args) + self.assertEqual(ret, 0) + output = stdout.getvalue() + self.assertIn('cherry-abc1234f', output) + self.assertIn('Branches to check', output) + + def test_rewind_two_merges(self): + """Test rewinding by two merges with --force.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + dbs.source_set('us/next', 'current_hash') + dbs.commit() + dbs.close() + + database.Database.instances.clear() + + def mock_git(pipe_list): + cmd = pipe_list[0] if pipe_list else [] + if '--merges' in cmd: + return command.CommandResult( + stdout='current_hash|current1|Current merge\n' + 'mid_hash|mid12345|Middle merge\n' + 'old_hash|old12345|Old merge\n') + if 'rev-list' in cmd: + return command.CommandResult(stdout='') + if 'branch' in cmd and '--list' in cmd: + return command.CommandResult(stdout='') + return command.CommandResult(stdout='') + + command.TEST_RESULT = mock_git + + args = argparse.Namespace(cmd='rewind', source='us/next', count=2, + force=True, remote='ci') + with terminal.capture() as (stdout, _): + ret = control.do_pickman(args) + self.assertEqual(ret, 0) + self.assertIn('old12345', stdout.getvalue()) + + # Verify source was updated to old_hash + database.Database.instances.clear() + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + self.assertEqual(dbs.source_get('us/next'), 'old_hash') + dbs.close() + + def test_rewind_not_enough_merges(self): + """Test rewind fails when not enough merges in history.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + dbs.source_set('us/next', 'current_hash') + dbs.commit() + dbs.close() + + database.Database.instances.clear() + + def mock_git(pipe_list): + cmd = pipe_list[0] if pipe_list else [] + if '--merges' in cmd: + # Only one merge (the current position) + return command.CommandResult( + stdout='current_hash|current1|Current merge\n') + return command.CommandResult(stdout='') + + command.TEST_RESULT = mock_git + + args = argparse.Namespace(cmd='rewind', source='us/next', count=1, + force=True, remote='ci') + with terminal.capture() as (_, stderr): + ret = control.do_pickman(args) + self.assertEqual(ret, 1) + self.assertIn('Not enough merges', stderr.getvalue()) + + def test_parse_rewind(self): + """Test parsing rewind command.""" + args = pickman.parse_args(['rewind', 'us/next']) + self.assertEqual(args.cmd, 'rewind') + self.assertEqual(args.source, 'us/next') + self.assertEqual(args.count, 1) + self.assertFalse(args.force) + self.assertEqual(args.remote, 'ci') + + def test_parse_rewind_with_count(self): + """Test parsing rewind command with count.""" + args = pickman.parse_args(['rewind', 'us/next', '-c', '3']) + self.assertEqual(args.cmd, 'rewind') + self.assertEqual(args.source, 'us/next') + self.assertEqual(args.count, 3) + + def test_parse_rewind_with_force(self): + """Test parsing rewind command with --force.""" + args = pickman.parse_args(['rewind', 'us/next', '-c', '2', '-f']) + self.assertEqual(args.cmd, 'rewind') + self.assertEqual(args.count, 2) + self.assertTrue(args.force) + + class TestDoPushBranch(unittest.TestCase): """Tests for do_push_branch command.""" @@ -4369,8 +4766,8 @@ class TestDoPick(unittest.TestCase): push=False) def run_git_handler(args): - if '--verify' in args: - raise ValueError('branch not found') + if 'branch' in args and '--list' in args: + return '' # Branch doesn't exist return 'main' with mock.patch.object(control, 'get_commits_for_pick', diff --git a/tools/pickman/gitlab_api.py b/tools/pickman/gitlab_api.py index bfaf828edae..9262ac15a0d 100644 --- a/tools/pickman/gitlab_api.py +++ b/tools/pickman/gitlab_api.py @@ -196,24 +196,28 @@ def push_branch(remote, branch, force=False, skip_ci=True): push_target = push_url if push_url else remote # When using --force-with-lease with an HTTPS URL (not remote name), - # git can't find tracking refs automatically. Try to fetch first to - # update the tracking ref. If fetch fails (branch doesn't exist on - # remote yet), use regular --force instead of --force-with-lease. - have_remote_ref = False + # git can't find tracking refs automatically. Fetch and update the + # tracking ref explicitly so we can pass the expected value. + remote_oid = None if force and push_url: try: - command.output('git', 'fetch', remote, branch) - have_remote_ref = True + command.output( + 'git', 'fetch', remote, + f'+refs/heads/{branch}:' + f'refs/remotes/{remote}/{branch}') + remote_oid = command.output( + 'git', 'rev-parse', + f'refs/remotes/{remote}/{branch}').strip() except command.CommandExc: - pass # Branch doesn't exist on remote, will use --force + pass # Branch doesn't exist on remote args = ['git', 'push', '-u'] if skip_ci: args.extend(['-o', 'ci.skip']) if force: - if have_remote_ref: + if remote_oid: args.append( - f'--force-with-lease=refs/remotes/{remote}/{branch}') + f'--force-with-lease={branch}:{remote_oid}') else: args.append('--force') args.extend([push_target, f'HEAD:{branch}']) -- 2.43.0