Merge "Choose tracked branch for rebase when submitting"

2015-04-13 18:43:07 +00:00 · 2015-04-13 18:43:07 +00:00 · f4f6674c9c
commit f4f6674c9c
parent fbdff3245e 7da7c37170
5 changed files with 286 additions and 13 deletions
--- 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
--- 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()
--- 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."""

--- 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')
--- 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)