From: Simon Glass <simon.glass@canonical.com> Extract get_history() from write_history() to separate the history content generation and file I/O from the git operations. The function now: - Takes a filename parameter - Reads existing content from the file - Writes updated content to the file - Returns both the content and the git commit message This makes the history generation logic fully testable without needing to mock git operations. Add tests for get_history(): - test_get_history_empty: Test with no existing file - test_get_history_with_existing: Test appending to existing content - test_get_history_replaces_existing_branch: Test replacing entry - test_get_history_multiple_commits: Test commit message with multiple commits Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/control.py | 44 +++++++++++---- tools/pickman/ftest.py | 114 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 10 deletions(-) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index 474315f7db0..190f92fc57a 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -258,14 +258,19 @@ Commits: {commit_list}""" -def write_history(source, commits, branch_name, conversation_log): - """Write an entry to the pickman history file +def get_history(fname, source, commits, branch_name, conversation_log): + """Read, update and write history file for a cherry-pick operation Args: + fname (str): History filename to read/write source (str): Source branch name commits (list): list of CommitInfo tuples branch_name (str): Name of the cherry-pick branch conversation_log (str): The agent's conversation output + + Returns: + tuple: (content, commit_msg) where content is the updated history + and commit_msg is the git commit message """ summary = format_history_summary(source, commits, branch_name) entry = f"""{summary} @@ -277,24 +282,43 @@ def write_history(source, commits, branch_name, conversation_log): """ - # Read existing content and remove any entry for this branch + # Read existing content existing = '' - if os.path.exists(HISTORY_FILE): - with open(HISTORY_FILE, 'r', encoding='utf-8') as fhandle: + if os.path.exists(fname): + with open(fname, 'r', encoding='utf-8') as fhandle: existing = fhandle.read() # Remove existing entry for this branch (from ## header to ---) pattern = rf'## [^\n]+\n\nBranch: {re.escape(branch_name)}\n.*?---\n\n' existing = re.sub(pattern, '', existing, flags=re.DOTALL) + content = existing + entry + # Write updated history file - with open(HISTORY_FILE, 'w', encoding='utf-8') as fhandle: - fhandle.write(existing + entry) + with open(fname, 'w', encoding='utf-8') as fhandle: + fhandle.write(content) + + # Generate commit message + commit_msg = f'pickman: Record cherry-pick of {len(commits)} commits from {source}\n\n' + commit_msg += '\n'.join(f'- {c.short_hash} {c.subject}' for c in commits) + + return content, commit_msg + + +def write_history(source, commits, branch_name, conversation_log): + """Write an entry to the pickman history file and commit it + + Args: + source (str): Source branch name + commits (list): list of CommitInfo tuples + branch_name (str): Name of the cherry-pick branch + conversation_log (str): The agent's conversation output + """ + _, commit_msg = get_history(HISTORY_FILE, source, commits, branch_name, + conversation_log) # Commit the history file (use -f in case .gitignore patterns match) run_git(['add', '-f', HISTORY_FILE]) - msg = f'pickman: Record cherry-pick of {len(commits)} commits from {source}\n\n' - msg += '\n'.join(f'- {c.short_hash} {c.subject}' for c in commits) - run_git(['commit', '-m', msg]) + run_git(['commit', '-m', commit_msg]) tout.info(f'Updated {HISTORY_FILE}') diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 8865296ff11..d50be16fea7 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -1404,6 +1404,120 @@ class TestFormatHistorySummary(unittest.TestCase): self.assertIn('- bbb222b Second commit', result) +class TestGetHistory(unittest.TestCase): + """Tests for get_history function.""" + + def setUp(self): + """Set up test fixtures.""" + fd, self.history_file = tempfile.mkstemp(suffix='.history') + os.close(fd) + os.unlink(self.history_file) # Remove so we start fresh + + def tearDown(self): + """Clean up test fixtures.""" + if os.path.exists(self.history_file): + os.unlink(self.history_file) + + def test_get_history_empty(self): + """Test get_history with no existing file.""" + commits = [ + control.CommitInfo('aaa111', 'aaa111a', 'First commit', 'Author 1'), + ] + content, commit_msg = control.get_history( + self.history_file, 'us/next', commits, 'cherry-abc', + 'Conversation output') + + self.assertIn('us/next', content) + self.assertIn('Branch: cherry-abc', content) + self.assertIn('- aaa111a First commit', content) + self.assertIn('### Conversation log', content) + self.assertIn('Conversation output', content) + self.assertIn('---', content) + + # Verify commit message + self.assertIn('pickman: Record cherry-pick of 1 commits', commit_msg) + self.assertIn('- aaa111a First commit', commit_msg) + + # Verify file was written + with open(self.history_file, 'r', encoding='utf-8') as fhandle: + file_content = fhandle.read() + self.assertEqual(file_content, content) + + def test_get_history_with_existing(self): + """Test get_history appends to existing content.""" + # Create existing file + with open(self.history_file, 'w', encoding='utf-8') as fhandle: + fhandle.write('Previous history content\n') + + commits = [ + control.CommitInfo('bbb222', 'bbb222b', 'New commit', 'Author 2'), + ] + content, commit_msg = control.get_history( + self.history_file, 'us/next', commits, 'cherry-new', + 'New conversation') + + self.assertIn('Previous history content', content) + self.assertIn('cherry-new', content) + self.assertIn('New conversation', content) + self.assertIn('- bbb222b New commit', commit_msg) + + def test_get_history_replaces_existing_branch(self): + """Test get_history removes existing entry for same branch.""" + # Create existing file with an entry for cherry-abc + existing = """## 2025-01-01: us/next + +Branch: cherry-abc + +Commits: +- aaa111a Old commit + +### Conversation log +Old conversation + +--- + +Other content +""" + with open(self.history_file, 'w', encoding='utf-8') as fhandle: + fhandle.write(existing) + + commits = [ + control.CommitInfo('ccc333', 'ccc333c', 'Updated commit', 'Author'), + ] + content, _ = control.get_history(self.history_file, 'us/next', commits, + 'cherry-abc', 'New conversation') + + # Old entry should be removed + self.assertNotIn('Old commit', content) + self.assertNotIn('Old conversation', content) + # New entry should be present + self.assertIn('Updated commit', content) + self.assertIn('New conversation', content) + # Other content should be preserved + self.assertIn('Other content', content) + + def test_get_history_multiple_commits(self): + """Test get_history with multiple commits.""" + commits = [ + control.CommitInfo('aaa111', 'aaa111a', 'First commit', 'Author 1'), + control.CommitInfo('bbb222', 'bbb222b', 'Second commit', 'Author 2'), + control.CommitInfo('ccc333', 'ccc333c', 'Third commit', 'Author 3'), + ] + content, commit_msg = control.get_history( + self.history_file, 'us/next', commits, 'cherry-abc', 'Log') + + # Verify all commits in content + self.assertIn('- aaa111a First commit', content) + self.assertIn('- bbb222b Second commit', content) + self.assertIn('- ccc333c Third commit', content) + + # Verify commit message + self.assertIn('pickman: Record cherry-pick of 3 commits', commit_msg) + self.assertIn('- aaa111a First commit', commit_msg) + self.assertIn('- bbb222b Second commit', commit_msg) + self.assertIn('- ccc333c Third commit', commit_msg) + + class TestGetNextCommitsEmptyLine(unittest.TestCase): """Tests for get_next_commits with empty lines.""" -- 2.43.0