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

5 files changed, 286 insertions, 13 deletions

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 3abf327..17d4d98 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
@@ -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()
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)