From: Simon Glass <simon.glass@canonical.com> Add a --diff/-d option to the check command that displays the difference between the original commit and the cherry-picked commit for commits which show a large discrepancy. This helps identify what changed during the cherry-pick process. Also add a --no-colour option to disable coloured output when needed, e.g. when outputting to a log file. Features: - Shows unified diff between original and cherry-picked patch content - Uses coloured output by default, can be disabled with --no-colour - Works in both verbose and summary modes - Includes comprehensive tests for both colour modes Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/README.rst | 2 + tools/pickman/__main__.py | 4 ++ tools/pickman/control.py | 118 ++++++++++++++++++++++++++++++-------- tools/pickman/ftest.py | 93 ++++++++++++++++++++++++++++++ 4 files changed, 192 insertions(+), 25 deletions(-) diff --git a/tools/pickman/README.rst b/tools/pickman/README.rst index 3b280e718d3..cc1ce5f81d5 100644 --- a/tools/pickman/README.rst +++ b/tools/pickman/README.rst @@ -227,6 +227,8 @@ Options: - ``-t, --threshold``: Delta threshold as fraction (default: 0.2 = 20%) - ``-m, --min-lines``: Minimum lines changed to check (default: 10) - ``-v, --verbose``: Show detailed analysis for all commits +- ``--diff``: Show patch differences for problem commits +- ``--no-colour``: Disable color output in patch differences Example output:: diff --git a/tools/pickman/__main__.py b/tools/pickman/__main__.py index 87077d1d10a..e450a9142fc 100755 --- a/tools/pickman/__main__.py +++ b/tools/pickman/__main__.py @@ -53,6 +53,8 @@ def add_main_commands(subparsers): '(default: 10)') check_cmd.add_argument('-v', '--verbose', action='store_true', help='Show detailed stats for all commits') + check_cmd.add_argument('-d', '--diff', action='store_true', + help='Show source code diff for problem commits') check_gl = subparsers.add_parser('check-gitlab', help='Check GitLab permissions') @@ -148,6 +150,8 @@ def parse_args(argv): Namespace: Parsed arguments """ parser = argparse.ArgumentParser(description='Check commit differences') + parser.add_argument('--no-colour', action='store_true', + help='Disable colour output') subparsers = parser.add_subparsers(dest='cmd', required=True) add_main_commands(subparsers) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index 648fd66947a..ac9b57e10e3 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -11,6 +11,7 @@ from datetime import date import os import re import sys +import tempfile import time import unittest @@ -464,12 +465,87 @@ def get_branch_commits(): return current_branch, commits +def show_commit_diff(res, no_colour=False): + """Show the difference between original and cherry-picked commit patches + + Args: + res (CheckResult): Check result with commit hashes + no_colour (bool): Disable colour output + """ + tout.info(f'\n--- Patch diff between original {res.orig_hash[:8]} and ' + f'cherry-picked {res.chash[:8]} ---') + + # Get the patch content of each commit + orig_patch = run_git(['show', '--no-ext-diff', res.orig_hash]) + cherry_patch = run_git(['show', '--no-ext-diff', res.chash]) + + # Create temporary files and diff them + with tempfile.NamedTemporaryFile(mode='w', suffix='_orig.patch', + delete=False) as orig_file: + orig_file.write(orig_patch) + orig_path = orig_file.name + + with tempfile.NamedTemporaryFile(mode='w', suffix='_cherry.patch', + delete=False) as cherry_file: + cherry_file.write(cherry_patch) + cherry_path = cherry_file.name + + try: + # Diff the two patch files using system diff + diff_args = ['diff', '-u'] + if not no_colour: + diff_args.append('--color=always') + diff_args.extend([orig_path, cherry_path]) + + patch_diff = command.output(*diff_args, raise_on_error=False) + if patch_diff: + print(patch_diff) + else: + tout.info('(Patches are identical)') + finally: + # Clean up temporary files + os.unlink(orig_path) + os.unlink(cherry_path) + + tout.info('--- End patch diff ---\n') + + +def show_check_summary(bad, verbose, threshold, show_diff, no_colour): + """Show summary of check results + + Args: + bad (list): List of CheckResult objects with problems + verbose (bool): Whether to show verbose output + threshold (float): Delta threshold for problems + show_diff (bool): Whether to show diffs for problems + no_colour (bool): Whether to disable colour in diffs + + Returns: + int: 0 if no problems, 1 if problems found + """ + if bad: + if verbose: + tout.info(f'Found {len(bad)} commit(s) with large deltas:') + tout.info('') + print_check_header() + for res in bad: + tout.info(format_problem_commit(res, threshold)) + if show_diff: + show_commit_diff(res, no_colour) + else: + tout.info(f'{len(bad)} problem commit(s) found') + return 1 + if verbose: + tout.info('All cherry-picks have acceptable deltas ✓') + return 0 + + def do_check(args, dbs): # pylint: disable=unused-argument """Check current branch for cherry-picks with large deltas Args: args (Namespace): Parsed arguments with 'threshold', 'min_lines', - and 'verbose' attributes + 'verbose', and 'diff' attributes dbs (Database): Database instance Returns: @@ -478,6 +554,7 @@ def do_check(args, dbs): # pylint: disable=unused-argument threshold = args.threshold min_lines = args.min_lines verbose = args.verbose + show_diff = args.diff current_branch, commits = get_branch_commits() @@ -493,33 +570,24 @@ def do_check(args, dbs): # pylint: disable=unused-argument # Process commits using the generator for res in check_commits(commits, min_lines): + is_problem = not res.reason and res.delta_ratio > threshold + if verbose: check_verbose(res, threshold) - if not res.reason and res.delta_ratio > threshold: - bad.append(res) - else: + elif is_problem: # Non-verbose: only show problems on one line - if not res.reason and res.delta_ratio > threshold: - if not header_printed: - print_check_header() - header_printed = True - tout.info(format_problem_commit(res, threshold)) - bad.append(res) - - # Summary - if bad: - if verbose: - tout.info(f'Found {len(bad)} commit(s) with large deltas:') - tout.info('') - print_check_header() - for res in bad: - tout.info(format_problem_commit(res, threshold)) - else: - tout.info(f'{len(bad)} problem commit(s) found') - return 1 - if verbose: - tout.info('All cherry-picks have acceptable deltas ✓') - return 0 + if not header_printed: + print_check_header() + header_printed = True + tout.info(format_problem_commit(res, threshold)) + + if is_problem: + bad.append(res) + if show_diff: + show_commit_diff(res, args.no_colour) + + return show_check_summary(bad, verbose, threshold, show_diff, + args.no_colour) def do_check_gitlab(args, dbs): # pylint: disable=unused-argument diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 2b829e4c75a..5d88e65c7cb 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -3438,6 +3438,99 @@ This is a normal commit without cherry-pick info. result = results[0] self.assertEqual(result.reason, 'small_commit_3_lines') + @mock.patch('pickman.control.command') + @mock.patch('pickman.control.run_git') + @mock.patch('tempfile.NamedTemporaryFile') + @mock.patch('os.unlink') + def test_show_commit_diff_with_colour(self, unused_unlink, mock_temp, + mock_run_git, mock_command): + """Test show_commit_diff with colour enabled.""" + # Mock temporary files + mock_temp.side_effect = [ + mock.mock_open()(), # orig file + mock.mock_open()() # cherry file + ] + mock_temp.return_value.__enter__.return_value.name = '/tmp/test.patch' + + # Mock git show outputs + mock_run_git.side_effect = [ + 'orig patch content', + 'cherry patch content' + ] + + # Mock diff command output + mock_command.output.return_value = 'diff output' + + # Test data + res = control.CheckResult( + chash='abc123', + orig_hash='def456', + subject='Test', + delta_ratio=0.5, + orig_stats=None, + cherry_stats=None, + reason=None + ) + + with terminal.capture(): + control.show_commit_diff(res, no_colour=False) + + # Verify git show was called for both commits + expected_calls = [ + mock.call(['show', '--no-ext-diff', 'def456']), + mock.call(['show', '--no-ext-diff', 'abc123']) + ] + mock_run_git.assert_has_calls(expected_calls) + + # Verify diff was called with colour + mock_command.output.assert_called_once() + args, kwargs = mock_command.output.call_args + self.assertIn('--color=always', args) + self.assertEqual(kwargs.get('raise_on_error'), False) + + @mock.patch('pickman.control.command') + @mock.patch('pickman.control.run_git') + @mock.patch('tempfile.NamedTemporaryFile') + @mock.patch('os.unlink') + def test_show_commit_diff_no_colour(self, unused_unlink, mock_temp, + mock_run_git, mock_command): + """Test show_commit_diff with colour disabled.""" + # Mock temporary files + mock_temp.side_effect = [ + mock.mock_open()(), # orig file + mock.mock_open()() # cherry file + ] + mock_temp.return_value.__enter__.return_value.name = '/tmp/test.patch' + + # Mock git show outputs + mock_run_git.side_effect = [ + 'orig patch content', + 'cherry patch content' + ] + + # Mock diff command output + mock_command.output.return_value = 'diff output' + + # Test data + res = control.CheckResult( + chash='abc123', + orig_hash='def456', + subject='Test', + delta_ratio=0.5, + orig_stats=None, + cherry_stats=None, + reason=None + ) + + with terminal.capture(): + control.show_commit_diff(res, no_colour=True) + + # Verify diff was called without colour + mock_command.output.assert_called_once() + args, kwargs = mock_command.output.call_args + self.assertNotIn('--color=always', args) + self.assertEqual(kwargs.get('raise_on_error'), False) + if __name__ == '__main__': unittest.main() -- 2.43.0