From: Simon Glass <sjg@chromium.org>
The review command currently picks '<upstream>/next' as its base
branch whenever that ref exists, falling back to '<upstream>/master'
only if next is missing. That works during a release cycle, but it
mishandles the period right after a release: the next branch has
just been merged into master and contains no exclusive commits, yet
patman still applies onto it. The mirror-image case occurs when a
reviewer wants to apply onto master even though next has diverged.
Honour a new -b/--base-branch flag so the user can override the
choice explicitly. When the flag is unset, decide automatically by
asking gitutil.count_revs() for the count of commits in
'<upstream>/master..<upstream>/next'. If next is ahead of master,
use it; otherwise fall back to master. After a release, master
contains everything next had, so count_revs() returns zero and
patman picks master until next diverges again at the next cycle's
RC1.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
tools/patman/cmdline.py | 5 +++
tools/patman/patman.rst | 12 +++++++
tools/patman/review.py | 24 +++++++++----
tools/patman/test_cseries.py | 69 ++++++++++++++++++++++++++++++++++++
4 files changed, 103 insertions(+), 7 deletions(-)
@@ -589,6 +589,11 @@ def add_review_subparser(subparsers):
review.add_argument(
'-U', '--upstream', type=str, default=None,
help='Upstream name (for patchwork URL lookup)')
+ review.add_argument(
+ '-b', '--base-branch', type=str, default=None,
+ help="Base branch to apply review patches onto (e.g. 'us/master', "
+ "'us/next'). If unset, picks the upstream's '/next' branch "
+ "when it has commits ahead of '/master', otherwise '/master'.")
review.add_argument(
'--apply-only', action='store_true',
help='Only download and apply patches, skip AI review')
@@ -1297,6 +1297,18 @@ without calling the Gmail API.
Use ``--apply-only`` to download and apply patches without running the
AI review -- useful for checking that patches apply cleanly.
+By default the review branch is created from ``<upstream>/next`` when
+that branch has commits ahead of ``<upstream>/master``, and from
+``<upstream>/master`` otherwise. This tracks U-Boot's release cadence:
+right after a release ``next`` is merged into ``master`` and stays
+empty until the next cycle's RC1, so reviews land on ``master`` during
+that window and switch back to ``next`` automatically once it
+diverges. Use ``-b`` / ``--base-branch`` to override the choice for a
+particular run::
+
+ patman review -s 497923 -U us -b us/master \
+ --reviewer 'Your Name <your@email>'
+
Use ``-f`` / ``--force`` to re-review a series that has already been
reviewed. This deletes the old review records and runs the review
again::
@@ -1741,25 +1741,35 @@ def _draft_stored_reviews(args, reviews, series_data, pwork, cser):
def _get_upstream_branch(args, cser):
- """Determine the upstream branch for applying patches
+ """Determine the base branch for applying patches
- Prefers 'next' over 'master' if it exists.
+ Honours --base-branch if the user supplied one. Otherwise prefers
+ '<upstream>/next' when it has commits ahead of '<upstream>/master',
+ falling back to '<upstream>/master' when next is empty (e.g. just
+ after a release, when next has been merged into master and has not
+ yet been reopened for the next cycle).
Args:
args (Namespace): Command-line arguments
cser (Cseries): Open cseries instance
Returns:
- str: Upstream branch ref, e.g. 'us/next'
+ str: Base branch ref, e.g. 'us/next' or 'us/master'
"""
+ if args.base_branch:
+ return args.base_branch
ups = args.upstream
if not ups:
ups = cser.db.upstream_get_default()
if ups:
- branch = f'{ups}/next'
- if not gitutil.ref_exists(branch):
- branch = f'{ups}/master'
- return branch
+ next_branch = f'{ups}/next'
+ master_branch = f'{ups}/master'
+ if gitutil.ref_exists(next_branch):
+ ahead = gitutil.count_revs(
+ None, f'{master_branch}..{next_branch}')
+ if ahead:
+ return next_branch
+ return master_branch
return 'origin/master'
@@ -21,6 +21,7 @@ from u_boot_pylib import tools
from patman import cmdline
from patman import control
from patman import cser_helper
+from patman import review
from patman import cseries
from patman.database import Pcommit
from patman import database
@@ -5223,3 +5224,71 @@ VERDICT: changes_needed"""
self.assertEqual(2, len(notes))
self.assertEqual(1, notes[0][0])
self.assertEqual(3, notes[1][0])
+
+
+class TestGetUpstreamBranch(unittest.TestCase):
+ """Tests for review._get_upstream_branch() base-branch selection."""
+
+ def _cser(self, default_upstream=None):
+ cser = mock.Mock()
+ cser.db.upstream_get_default.return_value = default_upstream
+ return cser
+
+ def test_explicit_base_branch_wins(self):
+ """An explicit -b/--base-branch overrides every other check."""
+ args = Namespace(base_branch='custom/branch', upstream='us')
+ with mock.patch('patman.review.gitutil') as gu:
+ self.assertEqual(
+ 'custom/branch',
+ review._get_upstream_branch(args, self._cser()))
+ gu.ref_exists.assert_not_called()
+ gu.count_revs.assert_not_called()
+
+ def test_next_ahead_of_master_picks_next(self):
+ """When next has commits ahead of master, next is chosen."""
+ args = Namespace(base_branch=None, upstream='us')
+ with mock.patch('patman.review.gitutil.ref_exists',
+ return_value=True), \
+ mock.patch('patman.review.gitutil.count_revs',
+ return_value=3):
+ self.assertEqual(
+ 'us/next',
+ review._get_upstream_branch(args, self._cser()))
+
+ def test_next_empty_falls_back_to_master(self):
+ """When next exists but has no commits ahead, master is chosen."""
+ args = Namespace(base_branch=None, upstream='us')
+ with mock.patch('patman.review.gitutil.ref_exists',
+ return_value=True), \
+ mock.patch('patman.review.gitutil.count_revs',
+ return_value=0):
+ self.assertEqual(
+ 'us/master',
+ review._get_upstream_branch(args, self._cser()))
+
+ def test_next_missing_falls_back_to_master(self):
+ """When next does not exist at all, master is chosen."""
+ args = Namespace(base_branch=None, upstream='us')
+ with mock.patch('patman.review.gitutil.ref_exists',
+ return_value=False):
+ self.assertEqual(
+ 'us/master',
+ review._get_upstream_branch(args, self._cser()))
+
+ def test_default_upstream_used_when_unset(self):
+ """When args.upstream is unset, the cser default is consulted."""
+ args = Namespace(base_branch=None, upstream=None)
+ cser = self._cser(default_upstream='us')
+ with mock.patch('patman.review.gitutil.ref_exists',
+ return_value=True), \
+ mock.patch('patman.review.gitutil.count_revs',
+ return_value=5):
+ self.assertEqual(
+ 'us/next', review._get_upstream_branch(args, cser))
+
+ def test_no_upstream_returns_origin_master(self):
+ """With no upstream configured anywhere, return 'origin/master'."""
+ args = Namespace(base_branch=None, upstream=None)
+ self.assertEqual(
+ 'origin/master',
+ review._get_upstream_branch(args, self._cser()))