diff --git a/git-review.1 b/git-review.1 index 237212b..8515835 100644 --- a/git-review.1 +++ b/git-review.1 @@ -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 diff --git a/git_review/cmd.py b/git_review/cmd.py index 5f2ea98..efcd549 100755 --- a/git_review/cmd.py +++ b/git_review/cmd.py @@ -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 @@ -654,6 +655,7 @@ def load_config_file(config_file): 'branch': 'defaultbranch', 'remote': 'defaultremote', 'rebase': 'defaultrebase', + 'track': 'track', } config = {} for config_key, option_name in options.items(): @@ -675,6 +677,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.""" @@ -983,6 +1018,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) @@ -1021,6 +1076,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) @@ -1030,10 +1086,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 """ @@ -1042,10 +1098,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, @@ -1102,8 +1172,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) @@ -1112,8 +1182,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) @@ -1187,6 +1257,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) @@ -1274,8 +1352,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: @@ -1285,7 +1363,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 @@ -1294,6 +1378,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']) @@ -1305,9 +1392,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() diff --git a/git_review/tests/__init__.py b/git_review/tests/__init__.py index bcd7c73..dc2d894 100644 --- a/git_review/tests/__init__.py +++ b/git_review/tests/__init__.py @@ -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.""" diff --git a/git_review/tests/test_git_review.py b/git_review/tests/test_git_review.py index 9044911..94bd39b 100644 --- a/git_review/tests/test_git_review.py +++ b/git_review/tests/test_git_review.py @@ -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') diff --git a/git_review/tests/test_unit.py b/git_review/tests/test_unit.py index 30c4fc4..05113fe 100644 --- a/git_review/tests/test_unit.py +++ b/git_review/tests/test_unit.py @@ -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)