opendev
/
git-restack
Code Issues Proposed changes

Choose tracked branch for rebase when submitting 

When choosing the branch to submit a changeset against, and
against which to rebase a changeset if it is being rebased, a new
gerrit.track configuration item and corresponding --track command
line option indicate to use the upstream branch being tracked as the
source of branch information, in preference to configuration. When
downloading a changeset, always set it up to track the matching
remote branch to make --track work for downloaded changesets.
If a branch name is provided explicitly on the command line, it
overrides the tracked branch.

Rationale:

Workflows with multiple active branches are common.  For example,
there may be one development branch (master) and multiple branches
representing prior releases to which bug fixes are still being
made.  Then a common workflow is to fix bugs in the earliest
affected branch still maintained and merge forward or cherry-pick
to master.  The commits being made to the earlier released branches
should not be rebased against master.

A typical usage pattern in this workflow is:

    git checkout -b my-feature origin/master
    ... implement feature ...
    git review -f

    git checkout -b my-bug-fix origin/maintenancebranch
    ... implement bug fix ...
    git review -f maintenancebranch

    git checkout -b my-bug-fix-merge origin/master
    git merge maintenancebranch / git cherry-pick -x ... / git review -x ...
    git review -f

The developer, who is usually implementing features and therefore
used to working against master, may accidentally forget to name
the release branch when running git review for the bug fix to the
release branch.  Mananging .gitreview files across branches and
repositories scales poorly with larger numbers of repositories
and branches and can be vulnerable to missed bad merges altering
configuration to point at wrong branches.

This change rebases changesets against the tracked remote and branch,
or if no branch is tracked, against the previously-specified branch,
instead of against <defaultremote>/master, only if gerrit.track has
been set to true.  With this change, the developer can safely omit
to specify the branch name when committing changes to non-default
branches such as "maintenancebranch" in the example.

When downloading a changeset, it will always be set up to track the
matching remote branch.  That way, whether or not the gerrit.track
configuration item is set when the changeset is downloaded, the right
branch will be chosen when it is submitted if gerrit.track is set
after the changeset is downloaded, or if the --track command line
option is specified.

Closes-Bug: #883176
Story: #883176
Story: #2000176
Change-Id: I25f22b9e3cda38598681d720a2f2ac534baec5a6
This commit is contained in:
Michael Johnson 2015-02-24 16:00:06 -05:00
parent 79262a5230
commit 7da7c37170
5 changed files with 286 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
git-review.1
View File

@ -160,6 +160,18 @@ When submitting a change for review, you will usually want it to be based on the
Also can be used for
.Fl \-compare
to skip automatic rebase of fetched reviews.
.It Fl \-track
Choose the branch to submit the change against (and, if
rebasing, to rebase against) from the branch being tracked
(if a branch is being tracked), and set the tracking branch
when downloading a change to point to the remote and branch
against which patches should be submitted.
See gitreview.track configuration.
.It Fl \-no\-track
Ignore any branch being tracked by the current branch,
overriding gitreview.track.
This option is implied by providing a specific branch name
on the command line.
.It Fl \-version
Print the version number and exit.
.El
@ -199,6 +211,37 @@ This setting determines the default name to use for gerrit remote
.It gitreview.branch
This setting determines the default branch
.Ed
.It gitreview.track
Determines whether to prefer the currently-tracked branch (if any)
and the branch against which the changeset was submitted to Gerrit
(if there is exactly one such branch) to the defaultremote and
defaultbranch for submitting and rebasing against.
If the local topic branch is tracking a remote branch, the remote
and branch that the local topic branch is tracking should be used
for submit and rebase operations, rather than the defaultremote
and defaultbranch.
.Pp
When downloading a patch, creates the local branch to track the
appropriate remote and branch in order to choose that branch by
default when submitting modifications to that changeset.
.Pp
A value of 'true' or 'false' should be specified.
.Bl -tag
.It true
Do prefer the currently-tracked branch (if any) \- equivalent
to setting
.Fl \-track
when submitting changes.
.It false
Ignore tracking branches \- equivalent to setting
.Fl \-no\-track
(the default) or providing an explicit branch name when submitting
changes. This is the default value unless overridden by
.Pa .gitreview
file, and is implied by providing a specific branch name on the
command line.
.El
.Ed
.It gitreview.rebase
This setting determines whether changes submitted will
be rebased to the newest state of the branch.
@ -278,6 +321,7 @@ project=department/project.git
defaultbranch=master
defaultremote=review
defaultrebase=0
track=0
.Ed
.Pp
When the same option is provided through FILES and CONFIGURATION, the

114
git_review/cmd.py
View File

@ -56,7 +56,8 @@ CONFIGDIR = os.path.expanduser("~/.config/git-review")
GLOBAL_CONFIG = "/etc/git-review/git-review.conf"
USER_CONFIG = os.path.join(CONFIGDIR, "git-review.conf")
DEFAULTS = dict(scheme='ssh', hostname=False, port=None, project=False,
branch='master', remote="gerrit", rebase="1")
branch='master', remote="gerrit", rebase="1",
track="0")
_branch_name = None
_has_color = None
@ -653,6 +654,7 @@ def load_config_file(config_file):
'branch': 'defaultbranch',
'remote': 'defaultremote',
'rebase': 'defaultrebase',
'track': 'track',
}
config = {}
for config_key, option_name in options.items():
@ -674,6 +676,39 @@ def update_remote(remote):
return True
def parse_tracking(ref=None):
"""Return tracked (remote, branch) of current HEAD or other named
branch if tracking remote.
"""
if ref is None:
ref = run_command_exc(
SymbolicRefFailed,
"git", "symbolic-ref", "-q", "HEAD")
tracked = run_command_exc(
ForEachRefFailed,
"git", "for-each-ref", "--format=%(upstream)", ref)
# Only on explicitly tracked remote branch do we diverge from default
if tracked and tracked.startswith('refs/remotes/'):
return tracked[13:].partition('/')[::2]
return None, None
def resolve_tracking(remote, branch):
"""Resolve tracked upstream remote/branch if current branch is tracked."""
tracked_remote, tracked_branch = parse_tracking()
# tracked_branch will be empty when tracking a local branch
if tracked_branch:
if VERBOSE:
print('Following tracked %s/%s rather than default %s/%s' % (
tracked_remote, tracked_branch,
remote, branch))
return tracked_remote, tracked_branch
return remote, branch
def check_remote(branch, remote, scheme, hostname, port, project):
"""Check that a Gerrit Git remote repo exists, if not, set one."""
@ -982,6 +1017,26 @@ class ResetHardFailed(CommandFailed):
EXIT_CODE = 66
class SetUpstreamBranchFailed(CommandFailed):
"Cannot set upstream to remote branch"
EXIT_CODE = 67
class SymbolicRefFailed(CommandFailed):
"Cannot find symbolic reference"
EXIT_CODE = 68
class ForEachRefFailed(CommandFailed):
"Cannot process symbolic reference"
EXIT_CODE = 69
class BranchTrackingMismatch(GitReviewException):
"Branch exists but is tracking unexpected branch"
EXIT_CODE = 70
def fetch_review(review, masterbranch, remote):
remote_url = get_remote_url(remote)
@ -1020,6 +1075,7 @@ def fetch_review(review, masterbranch, remote):
author = re.sub('\W+', '_', review_info['owner']['name']).lower()
except KeyError:
author = 'unknown'
remote_branch = review_info['branch']
if patchset_number is None:
branch_name = "review/%s/%s" % (author, topic)
@ -1029,10 +1085,10 @@ def fetch_review(review, masterbranch, remote):
print("Downloading %s from gerrit" % refspec)
run_command_exc(PatchSetGitFetchFailed,
"git", "fetch", remote, refspec)
return branch_name
return branch_name, remote_branch
def checkout_review(branch_name):
def checkout_review(branch_name, remote, remote_branch):
"""Checkout a newly fetched (FETCH_HEAD) change
into a branch
"""
@ -1041,10 +1097,24 @@ def checkout_review(branch_name):
run_command_exc(CheckoutNewBranchFailed,
"git", "checkout", "-b",
branch_name, "FETCH_HEAD")
# --set-upstream-to is not supported in git 1.7
run_command_exc(SetUpstreamBranchFailed,
"git", "branch", "--set-upstream",
branch_name,
'%s/%s' % (remote, remote_branch))
except CheckoutNewBranchFailed as e:
if re.search("already exists\.?", e.output):
print("Branch already exists - reusing")
print("Branch %s already exists - reusing" % branch_name)
track_remote, track_branch = parse_tracking(
ref='refs/heads/' + branch_name)
if track_remote and not (track_remote == remote and
track_branch == remote_branch):
print("Branch %s incorrectly tracking %s/%s instead of %s/%s"
% (branch_name,
track_remote, track_branch,
remote, remote_branch))
raise BranchTrackingMismatch
run_command_exc(CheckoutExistingBranchFailed,
"git", "checkout", branch_name)
run_command_exc(ResetHardFailed,
@ -1101,8 +1171,8 @@ def compare_review(review_spec, branch, remote, rebase=False):
old_review = build_review_number(review, old_ps)
new_review = build_review_number(review, new_ps)
old_branch = fetch_review(old_review, branch, remote)
checkout_review(old_branch)
old_branch, _ = fetch_review(old_review, branch, remote)
checkout_review(old_branch, None, None)
if rebase:
print('Rebasing %s' % old_branch)
@ -1111,8 +1181,8 @@ def compare_review(review_spec, branch, remote, rebase=False):
print('Skipping rebase because of conflicts')
run_command_exc(CommandFailed, 'git', 'rebase', '--abort')
new_branch = fetch_review(new_review, branch, remote)
checkout_review(new_branch)
new_branch, remote_branch = fetch_review(new_review, branch, remote)
checkout_review(new_branch, remote, remote_branch)
if rebase:
print('Rebasing also %s' % new_branch)
@ -1186,6 +1256,14 @@ def _main():
action="store_true",
help="Force rebase even when not needed.")
track_group = parser.add_mutually_exclusive_group()
track_group.add_argument("--track", dest="track",
action="store_true",
help="Use tracked branch as default.")
track_group.add_argument("--no-track", dest="track",
action="store_false",
help="Ignore tracked branch.")
fetch = parser.add_mutually_exclusive_group()
fetch.set_defaults(download=False, compare=False, cherrypickcommit=False,
cherrypickindicate=False, cherrypickonly=False)
@ -1273,8 +1351,8 @@ def _main():
else:
no_git_dir = False
config = Config(os.path.join(top_dir, ".gitreview"))
parser.set_defaults(branch=config['branch'],
rebase=convert_bool(config['rebase']),
parser.set_defaults(rebase=convert_bool(config['rebase']),
track=convert_bool(config['track']),
remote=config['remote'])
options = parser.parse_args()
if no_git_dir:
@ -1284,7 +1362,13 @@ def _main():
print(COPYRIGHT)
sys.exit(0)
branch = options.branch
if options.branch is None:
branch = config['branch']
else:
# explicitly-specified branch on command line overrides options.track
branch = options.branch
options.track = False
global VERBOSE
global UPDATE
VERBOSE = options.verbose
@ -1293,6 +1377,9 @@ def _main():
yes = options.yes
status = 0
if options.track:
remote, branch = resolve_tracking(remote, branch)
check_remote(branch, remote, config['scheme'],
config['hostname'], config['port'], config['project'])
@ -1304,9 +1391,10 @@ def _main():
compare_review(options.changeidentifier,
branch, remote, options.rebase)
return
local_branch = fetch_review(options.changeidentifier, branch, remote)
local_branch, remote_branch = fetch_review(options.changeidentifier,
branch, remote)
if options.download:
checkout_review(local_branch)
checkout_review(local_branch, remote, remote_branch)
else:
if options.cherrypickcommit:
cherrypick_review()

10
git_review/tests/__init__.py
View File

@ -239,6 +239,16 @@ class BaseGitReviewTestCase(testtools.TestCase, GerritHelpers):
self._run_git('add', file_)
self._run_git('commit', '-m', commit_message)
def _simple_amend(self, change_text, file_=None):
"""Helper method to amend existing commit with change."""
if file_ is None:
file_ = self._dir('test', 'test_file_new.txt')
utils.write_to_file(file_, change_text.encode())
self._run_git('add', file_)
# cannot use --no-edit because it does not exist in older git
message = self._run_git('log', '-1', '--format=%s\n\n%b')
self._run_git('commit', '--amend', '-m', message)
def _configure_ssh(self, ssh_addr, ssh_port):
"""Setup ssh and scp to run with special options."""

73
git_review/tests/test_git_review.py
View File

@ -90,6 +90,12 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
self.assertNotIn('test commit message',
self._run_git('show', 'HEAD^1'))
# and branch is tracking
head = self._run_git('symbolic-ref', '-q', 'HEAD')
self.assertIn(
'refs/remotes/gerrit/master',
self._run_git("for-each-ref", "--format='%(upstream)'", head))
def test_multiple_changes(self):
"""Test git-review asks about multiple changes.
@ -160,6 +166,73 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
review_res)
self.assertEqual(self._run_git('rev-parse', 'HEAD^1'), head_1)
def test_uploads_with_nondefault_rebase(self):
"""Test changes rebase against correct branches."""
# prepare maintenance branch that is behind master
self._create_gitreview_file(track='true',
defaultremote='origin')
self._run_git('add', '.gitreview')
self._run_git('commit', '-m', 'track=true.')
self._simple_change('diverge master from maint',
'no conflict',
self._dir('test', 'test_file_to_diverge.txt'))
self._run_git('push', 'origin', 'master')
self._run_git('push', 'origin', 'master', 'master:other')
self._run_git_review('-s')
head_1 = self._run_git('rev-parse', 'HEAD^1')
self._run_gerrit_cli('create-branch',
'test/test_project',
'maint', head_1)
self._run_git('fetch')
br_out = self._run_git('checkout',
'-b', 'test_branch', 'origin/maint')
expected_track = 'Branch test_branch set up to track remote' + \
' branch maint from origin.'
self.assertIn(expected_track, br_out)
branches = self._run_git('branch', '-a')
expected_branch = '* test_branch'
observed = branches.split('\n')
self.assertIn(expected_branch, observed)
self._simple_change('some new message',
'just another file (no conflict)',
self._dir('test', 'new_tracked_test_file.txt'))
change_id = self._run_git('log', '-1').split()[-1]
review_res = self._run_git_review('-v')
# no rebase needed; if it breaks it would try to rebase to master
self.assertNotIn("Running: git rebase -p -i remotes/origin/master",
review_res)
# Don't need to query gerrit for the branch as the second half
# of this test will work only if the branch was correctly
# stored in gerrit
# delete branch locally
self._run_git('checkout', 'master')
self._run_git('branch', '-D', 'test_branch')
# download, amend, submit
self._run_git_review('-d', change_id)
self._simple_amend('just another file (no conflict)',
self._dir('test', 'new_tracked_test_file_2.txt'))
new_change_id = self._run_git('log', '-1').split()[-1]
self.assertEqual(change_id, new_change_id)
review_res = self._run_git_review('-v')
# caused the right thing to happen
self.assertIn("Running: git rebase -p -i remotes/origin/maint",
review_res)
# track different branch than expected in changeset
branch = self._run_git('rev-parse', '--abbrev-ref', 'HEAD')
self._run_git('branch',
'--set-upstream',
branch,
'remotes/origin/other')
self.assertRaises(
Exception, # cmd.BranchTrackingMismatch inside
self._run_git_review, '-d', change_id)
def test_no_rebase_check(self):
"""Test -R causes a change to be uploaded without rebase checking."""
self._run_git_review('-s')

58
git_review/tests/test_unit.py
View File

@ -176,6 +176,48 @@ password=pass
"""
class ResolveTrackingUnitTest(testtools.TestCase):
"""Class for testing resolve_tracking."""
def setUp(self):
testtools.TestCase.setUp(self)
patcher = mock.patch('git_review.cmd.run_command_exc')
self.addCleanup(patcher.stop)
self.run_command_exc = patcher.start()
def test_track_local_branch(self):
'Test that local tracked branch is not followed.'
self.run_command_exc.side_effect = [
'',
'refs/heads/other/branch',
]
self.assertEqual(cmd.resolve_tracking(u'remote', u'rbranch'),
(u'remote', u'rbranch'))
def test_track_untracked_branch(self):
'Test that local untracked branch is not followed.'
self.run_command_exc.side_effect = [
'',
'',
]
self.assertEqual(cmd.resolve_tracking(u'remote', u'rbranch'),
(u'remote', u'rbranch'))
def test_track_remote_branch(self):
'Test that remote tracked branch is followed.'
self.run_command_exc.side_effect = [
'',
'refs/remotes/other/branch',
]
self.assertEqual(cmd.resolve_tracking(u'remote', u'rbranch'),
(u'other', u'branch'))
def test_track_git_error(self):
'Test that local tracked branch is not followed.'
self.run_command_exc.side_effect = [cmd.CommandFailed(1, '', [], {})]
self.assertRaises(cmd.CommandFailed,
cmd.resolve_tracking, u'remote', u'rbranch')
class GitReviewUnitTest(testtools.TestCase):
"""Class for misc unit tests."""
@ -236,3 +278,19 @@ class GitReviewUnitTest(testtools.TestCase):
stdin='url=%s' % url)
calls = [mock.call(url), mock.call(url, auth=('user', 'pass'))]
mock_get.assert_has_calls(calls)
@mock.patch('sys.argv', ['argv0', '--track', 'branch'])
@mock.patch('git_review.cmd.check_remote')
@mock.patch('git_review.cmd.resolve_tracking')
def test_command_line_no_track(self, resolve_tracking, check_remote):
check_remote.side_effect = Exception()
self.assertRaises(Exception, cmd._main)
self.assertFalse(resolve_tracking.called)
@mock.patch('sys.argv', ['argv0', '--track'])
@mock.patch('git_review.cmd.check_remote')
@mock.patch('git_review.cmd.resolve_tracking')
def test_track(self, resolve_tracking, check_remote):
check_remote.side_effect = Exception()
self.assertRaises(Exception, cmd._main)
self.assertTrue(resolve_tracking.called)