From: Simon Glass <sjg@chromium.org> push_branch() catches CommandExc and returns False on failure, but callers like apply_subtree_update() use try/except to detect push failures rather than checking the return value. This means push failures are silently ignored and the source is advanced even though the branch was not pushed. Change push_branch() to re-raise the CommandExc instead of returning False, and update all callers to use exception handling consistently. This ensures apply_subtree_update() properly aborts when a push fails. Signed-off-by: Simon Glass <sjg@chromium.org> --- tools/pickman/control.py | 9 ++++++--- tools/pickman/ftest.py | 6 ++++-- tools/pickman/gitlab_api.py | 9 +++++++-- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index ed6825c2333..6d787d02163 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -2080,9 +2080,12 @@ def do_push_branch(args, dbs): # pylint: disable=unused-argument int: 0 on success, 1 on failure """ skip_ci = not args.run_ci - success = gitlab_api.push_branch(args.remote, args.branch, args.force, - skip_ci=skip_ci) - return 0 if success else 1 + try: + gitlab_api.push_branch(args.remote, args.branch, args.force, + skip_ci=skip_ci) + except command.CommandExc: + return 1 + return 0 def do_commit_source(args, dbs): diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 590677d6b5b..95053e78d21 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -4956,8 +4956,10 @@ class TestDoPushBranch(unittest.TestCase): tout.init(tout.INFO) args = argparse.Namespace(cmd='push-branch', branch='test-branch', remote='ci', force=False, run_ci=False) - with mock.patch.object(gitlab, 'push_branch', - return_value=False): + with mock.patch.object( + gitlab, 'push_branch', + side_effect=command.CommandExc( + 'push failed', command.CommandResult())): with terminal.capture(): ret = control.do_push_branch(args, None) self.assertEqual(ret, 1) diff --git a/tools/pickman/gitlab_api.py b/tools/pickman/gitlab_api.py index 81918c80c3c..4d90d622dbc 100644 --- a/tools/pickman/gitlab_api.py +++ b/tools/pickman/gitlab_api.py @@ -196,6 +196,9 @@ def push_branch(remote, branch, force=False, skip_ci=True): Returns: bool: True on success + + Raises: + command.CommandExc: If the push fails """ try: # Use token-authenticated URL if available @@ -232,7 +235,7 @@ def push_branch(remote, branch, force=False, skip_ci=True): return True except command.CommandExc as exc: tout.error(f'Failed to push branch: {exc}') - return False + raise # pylint: disable=too-many-arguments @@ -630,7 +633,9 @@ def push_and_create_mr(remote, branch, target, title, desc=''): return None tout.info(f'Pushing {branch} to {remote}...') - if not push_branch(remote, branch, force=True): + try: + push_branch(remote, branch, force=True) + except command.CommandExc: return None tout.info(f'Creating merge request to {target}...') -- 2.43.0