Browse Source

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
Michael Johnson 4 years ago
parent
commit
7da7c37170
5 changed files with 286 additions and 13 deletions
  1. 44
    0
      git-review.1
  2. 101
    13
      git_review/cmd.py
  3. 10
    0
      git_review/tests/__init__.py
  4. 73
    0
      git_review/tests/test_git_review.py
  5. 58
    0
      git_review/tests/test_unit.py

+ 44
- 0
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
160 160
 Also can be used for
161 161
 .Fl \-compare
162 162
 to skip automatic rebase of fetched reviews.
163
+.It Fl \-track
164
+Choose the branch to submit the change against (and, if
165
+rebasing, to rebase against) from the branch being tracked
166
+(if a branch is being tracked), and set the tracking branch
167
+when downloading a change to point to the remote and branch
168
+against which patches should be submitted.
169
+See gitreview.track configuration.
170
+.It Fl \-no\-track
171
+Ignore any branch being tracked by the current branch,
172
+overriding gitreview.track.
173
+This option is implied by providing a specific branch name
174
+on the command line.
163 175
 .It Fl \-version
164 176
 Print the version number and exit.
165 177
 .El
@@ -199,6 +211,37 @@ This setting determines the default name to use for gerrit remote
199 211
 .It gitreview.branch
200 212
 This setting determines the default branch
201 213
 .Ed
214
+.It gitreview.track
215
+Determines whether to prefer the currently-tracked branch (if any)
216
+and the branch against which the changeset was submitted to Gerrit
217
+(if there is exactly one such branch) to the defaultremote and
218
+defaultbranch for submitting and rebasing against.
219
+If the local topic branch is tracking a remote branch, the remote
220
+and branch that the local topic branch is tracking should be used
221
+for submit and rebase operations, rather than the defaultremote
222
+and defaultbranch.
223
+.Pp
224
+When downloading a patch, creates the local branch to track the
225
+appropriate remote and branch in order to choose that branch by
226
+default when submitting modifications to that changeset.
227
+.Pp
228
+A value of 'true' or 'false' should be specified.
229
+.Bl -tag
230
+.It true
231
+Do prefer the currently-tracked branch (if any) \- equivalent
232
+to setting
233
+.Fl \-track
234
+when submitting changes.
235
+.It false
236
+Ignore tracking branches \- equivalent to setting
237
+.Fl \-no\-track
238
+(the default) or providing an explicit branch name when submitting
239
+changes. This is the default value unless overridden by
240
+.Pa .gitreview
241
+file, and is implied by providing a specific branch name on the
242
+command line.
243
+.El
244
+.Ed
202 245
 .It gitreview.rebase
203 246
 This setting determines whether changes submitted will
204 247
 be rebased to the newest state of the branch.
@@ -278,6 +321,7 @@ project=department/project.git
278 321
 defaultbranch=master
279 322
 defaultremote=review
280 323
 defaultrebase=0
324
+track=0
281 325
 .Ed
282 326
 .Pp
283 327
 When the same option is provided through FILES and CONFIGURATION, the

+ 101
- 13
git_review/cmd.py View File

@@ -56,7 +56,8 @@ CONFIGDIR = os.path.expanduser("~/.config/git-review")
56 56
 GLOBAL_CONFIG = "/etc/git-review/git-review.conf"
57 57
 USER_CONFIG = os.path.join(CONFIGDIR, "git-review.conf")
58 58
 DEFAULTS = dict(scheme='ssh', hostname=False, port=None, project=False,
59
-                branch='master', remote="gerrit", rebase="1")
59
+                branch='master', remote="gerrit", rebase="1",
60
+                track="0")
60 61
 
61 62
 _branch_name = None
62 63
 _has_color = None
@@ -653,6 +654,7 @@ def load_config_file(config_file):
653 654
         'branch': 'defaultbranch',
654 655
         'remote': 'defaultremote',
655 656
         'rebase': 'defaultrebase',
657
+        'track': 'track',
656 658
     }
657 659
     config = {}
658 660
     for config_key, option_name in options.items():
@@ -674,6 +676,39 @@ def update_remote(remote):
674 676
     return True
675 677
 
676 678
 
679
+def parse_tracking(ref=None):
680
+    """Return tracked (remote, branch) of current HEAD or other named
681
+       branch if tracking remote.
682
+    """
683
+    if ref is None:
684
+        ref = run_command_exc(
685
+            SymbolicRefFailed,
686
+            "git", "symbolic-ref", "-q", "HEAD")
687
+    tracked = run_command_exc(
688
+        ForEachRefFailed,
689
+        "git", "for-each-ref", "--format=%(upstream)", ref)
690
+
691
+    # Only on explicitly tracked remote branch do we diverge from default
692
+    if tracked and tracked.startswith('refs/remotes/'):
693
+        return tracked[13:].partition('/')[::2]
694
+
695
+    return None, None
696
+
697
+
698
+def resolve_tracking(remote, branch):
699
+    """Resolve tracked upstream remote/branch if current branch is tracked."""
700
+    tracked_remote, tracked_branch = parse_tracking()
701
+    # tracked_branch will be empty when tracking a local branch
702
+    if tracked_branch:
703
+        if VERBOSE:
704
+            print('Following tracked %s/%s rather than default %s/%s' % (
705
+                  tracked_remote, tracked_branch,
706
+                  remote, branch))
707
+        return tracked_remote, tracked_branch
708
+
709
+    return remote, branch
710
+
711
+
677 712
 def check_remote(branch, remote, scheme, hostname, port, project):
678 713
     """Check that a Gerrit Git remote repo exists, if not, set one."""
679 714
 
@@ -982,6 +1017,26 @@ class ResetHardFailed(CommandFailed):
982 1017
     EXIT_CODE = 66
983 1018
 
984 1019
 
1020
+class SetUpstreamBranchFailed(CommandFailed):
1021
+    "Cannot set upstream to remote branch"
1022
+    EXIT_CODE = 67
1023
+
1024
+
1025
+class SymbolicRefFailed(CommandFailed):
1026
+    "Cannot find symbolic reference"
1027
+    EXIT_CODE = 68
1028
+
1029
+
1030
+class ForEachRefFailed(CommandFailed):
1031
+    "Cannot process symbolic reference"
1032
+    EXIT_CODE = 69
1033
+
1034
+
1035
+class BranchTrackingMismatch(GitReviewException):
1036
+    "Branch exists but is tracking unexpected branch"
1037
+    EXIT_CODE = 70
1038
+
1039
+
985 1040
 def fetch_review(review, masterbranch, remote):
986 1041
     remote_url = get_remote_url(remote)
987 1042
 
@@ -1020,6 +1075,7 @@ def fetch_review(review, masterbranch, remote):
1020 1075
         author = re.sub('\W+', '_', review_info['owner']['name']).lower()
1021 1076
     except KeyError:
1022 1077
         author = 'unknown'
1078
+    remote_branch = review_info['branch']
1023 1079
 
1024 1080
     if patchset_number is None:
1025 1081
         branch_name = "review/%s/%s" % (author, topic)
@@ -1029,10 +1085,10 @@ def fetch_review(review, masterbranch, remote):
1029 1085
     print("Downloading %s from gerrit" % refspec)
1030 1086
     run_command_exc(PatchSetGitFetchFailed,
1031 1087
                     "git", "fetch", remote, refspec)
1032
-    return branch_name
1088
+    return branch_name, remote_branch
1033 1089
 
1034 1090
 
1035
-def checkout_review(branch_name):
1091
+def checkout_review(branch_name, remote, remote_branch):
1036 1092
     """Checkout a newly fetched (FETCH_HEAD) change
1037 1093
        into a branch
1038 1094
     """
@@ -1041,10 +1097,24 @@ def checkout_review(branch_name):
1041 1097
         run_command_exc(CheckoutNewBranchFailed,
1042 1098
                         "git", "checkout", "-b",
1043 1099
                         branch_name, "FETCH_HEAD")
1100
+        # --set-upstream-to is not supported in git 1.7
1101
+        run_command_exc(SetUpstreamBranchFailed,
1102
+                        "git", "branch", "--set-upstream",
1103
+                        branch_name,
1104
+                        '%s/%s' % (remote, remote_branch))
1044 1105
 
1045 1106
     except CheckoutNewBranchFailed as e:
1046 1107
         if re.search("already exists\.?", e.output):
1047
-            print("Branch already exists - reusing")
1108
+            print("Branch %s already exists - reusing" % branch_name)
1109
+            track_remote, track_branch = parse_tracking(
1110
+                ref='refs/heads/' + branch_name)
1111
+            if track_remote and not (track_remote == remote and
1112
+                                     track_branch == remote_branch):
1113
+                print("Branch %s incorrectly tracking %s/%s instead of %s/%s"
1114
+                      % (branch_name,
1115
+                         track_remote, track_branch,
1116
+                         remote, remote_branch))
1117
+                raise BranchTrackingMismatch
1048 1118
             run_command_exc(CheckoutExistingBranchFailed,
1049 1119
                             "git", "checkout", branch_name)
1050 1120
             run_command_exc(ResetHardFailed,
@@ -1101,8 +1171,8 @@ def compare_review(review_spec, branch, remote, rebase=False):
1101 1171
     old_review = build_review_number(review, old_ps)
1102 1172
     new_review = build_review_number(review, new_ps)
1103 1173
 
1104
-    old_branch = fetch_review(old_review, branch, remote)
1105
-    checkout_review(old_branch)
1174
+    old_branch, _ = fetch_review(old_review, branch, remote)
1175
+    checkout_review(old_branch, None, None)
1106 1176
 
1107 1177
     if rebase:
1108 1178
         print('Rebasing %s' % old_branch)
@@ -1111,8 +1181,8 @@ def compare_review(review_spec, branch, remote, rebase=False):
1111 1181
             print('Skipping rebase because of conflicts')
1112 1182
             run_command_exc(CommandFailed, 'git', 'rebase', '--abort')
1113 1183
 
1114
-    new_branch = fetch_review(new_review, branch, remote)
1115
-    checkout_review(new_branch)
1184
+    new_branch, remote_branch = fetch_review(new_review, branch, remote)
1185
+    checkout_review(new_branch, remote, remote_branch)
1116 1186
 
1117 1187
     if rebase:
1118 1188
         print('Rebasing also %s' % new_branch)
@@ -1186,6 +1256,14 @@ def _main():
1186 1256
                               action="store_true",
1187 1257
                               help="Force rebase even when not needed.")
1188 1258
 
1259
+    track_group = parser.add_mutually_exclusive_group()
1260
+    track_group.add_argument("--track", dest="track",
1261
+                             action="store_true",
1262
+                             help="Use tracked branch as default.")
1263
+    track_group.add_argument("--no-track", dest="track",
1264
+                             action="store_false",
1265
+                             help="Ignore tracked branch.")
1266
+
1189 1267
     fetch = parser.add_mutually_exclusive_group()
1190 1268
     fetch.set_defaults(download=False, compare=False, cherrypickcommit=False,
1191 1269
                        cherrypickindicate=False, cherrypickonly=False)
@@ -1273,8 +1351,8 @@ def _main():
1273 1351
     else:
1274 1352
         no_git_dir = False
1275 1353
         config = Config(os.path.join(top_dir, ".gitreview"))
1276
-        parser.set_defaults(branch=config['branch'],
1277
-                            rebase=convert_bool(config['rebase']),
1354
+        parser.set_defaults(rebase=convert_bool(config['rebase']),
1355
+                            track=convert_bool(config['track']),
1278 1356
                             remote=config['remote'])
1279 1357
     options = parser.parse_args()
1280 1358
     if no_git_dir:
@@ -1284,7 +1362,13 @@ def _main():
1284 1362
         print(COPYRIGHT)
1285 1363
         sys.exit(0)
1286 1364
 
1287
-    branch = options.branch
1365
+    if options.branch is None:
1366
+        branch = config['branch']
1367
+    else:
1368
+        # explicitly-specified branch on command line overrides options.track
1369
+        branch = options.branch
1370
+        options.track = False
1371
+
1288 1372
     global VERBOSE
1289 1373
     global UPDATE
1290 1374
     VERBOSE = options.verbose
@@ -1293,6 +1377,9 @@ def _main():
1293 1377
     yes = options.yes
1294 1378
     status = 0
1295 1379
 
1380
+    if options.track:
1381
+        remote, branch = resolve_tracking(remote, branch)
1382
+
1296 1383
     check_remote(branch, remote, config['scheme'],
1297 1384
                  config['hostname'], config['port'], config['project'])
1298 1385
 
@@ -1304,9 +1391,10 @@ def _main():
1304 1391
             compare_review(options.changeidentifier,
1305 1392
                            branch, remote, options.rebase)
1306 1393
             return
1307
-        local_branch = fetch_review(options.changeidentifier, branch, remote)
1394
+        local_branch, remote_branch = fetch_review(options.changeidentifier,
1395
+                                                   branch, remote)
1308 1396
         if options.download:
1309
-            checkout_review(local_branch)
1397
+            checkout_review(local_branch, remote, remote_branch)
1310 1398
         else:
1311 1399
             if options.cherrypickcommit:
1312 1400
                 cherrypick_review()

+ 10
- 0
git_review/tests/__init__.py View File

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

+ 73
- 0
git_review/tests/test_git_review.py View File

@@ -90,6 +90,12 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
90 90
         self.assertNotIn('test commit message',
91 91
                          self._run_git('show', 'HEAD^1'))
92 92
 
93
+        # and branch is tracking
94
+        head = self._run_git('symbolic-ref', '-q', 'HEAD')
95
+        self.assertIn(
96
+            'refs/remotes/gerrit/master',
97
+            self._run_git("for-each-ref", "--format='%(upstream)'", head))
98
+
93 99
     def test_multiple_changes(self):
94 100
         """Test git-review asks about multiple changes.
95 101
 
@@ -160,6 +166,73 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
160 166
                       review_res)
161 167
         self.assertEqual(self._run_git('rev-parse', 'HEAD^1'), head_1)
162 168
 
169
+    def test_uploads_with_nondefault_rebase(self):
170
+        """Test changes rebase against correct branches."""
171
+        # prepare maintenance branch that is behind master
172
+        self._create_gitreview_file(track='true',
173
+                                    defaultremote='origin')
174
+        self._run_git('add', '.gitreview')
175
+        self._run_git('commit', '-m', 'track=true.')
176
+        self._simple_change('diverge master from maint',
177
+                            'no conflict',
178
+                            self._dir('test', 'test_file_to_diverge.txt'))
179
+        self._run_git('push', 'origin', 'master')
180
+        self._run_git('push', 'origin', 'master', 'master:other')
181
+        self._run_git_review('-s')
182
+        head_1 = self._run_git('rev-parse', 'HEAD^1')
183
+        self._run_gerrit_cli('create-branch',
184
+                             'test/test_project',
185
+                             'maint', head_1)
186
+        self._run_git('fetch')
187
+
188
+        br_out = self._run_git('checkout',
189
+                               '-b', 'test_branch', 'origin/maint')
190
+        expected_track = 'Branch test_branch set up to track remote' + \
191
+                         ' branch maint from origin.'
192
+        self.assertIn(expected_track, br_out)
193
+        branches = self._run_git('branch', '-a')
194
+        expected_branch = '* test_branch'
195
+        observed = branches.split('\n')
196
+        self.assertIn(expected_branch, observed)
197
+
198
+        self._simple_change('some new message',
199
+                            'just another file (no conflict)',
200
+                            self._dir('test', 'new_tracked_test_file.txt'))
201
+        change_id = self._run_git('log', '-1').split()[-1]
202
+
203
+        review_res = self._run_git_review('-v')
204
+        # no rebase needed; if it breaks it would try to rebase to master
205
+        self.assertNotIn("Running: git rebase -p -i remotes/origin/master",
206
+                         review_res)
207
+        # Don't need to query gerrit for the branch as the second half
208
+        # of this test will work only if the branch was correctly
209
+        # stored in gerrit
210
+
211
+        # delete branch locally
212
+        self._run_git('checkout', 'master')
213
+        self._run_git('branch', '-D', 'test_branch')
214
+
215
+        # download, amend, submit
216
+        self._run_git_review('-d', change_id)
217
+        self._simple_amend('just another file (no conflict)',
218
+                           self._dir('test', 'new_tracked_test_file_2.txt'))
219
+        new_change_id = self._run_git('log', '-1').split()[-1]
220
+        self.assertEqual(change_id, new_change_id)
221
+        review_res = self._run_git_review('-v')
222
+        # caused the right thing to happen
223
+        self.assertIn("Running: git rebase -p -i remotes/origin/maint",
224
+                      review_res)
225
+
226
+        # track different branch than expected in changeset
227
+        branch = self._run_git('rev-parse', '--abbrev-ref', 'HEAD')
228
+        self._run_git('branch',
229
+                      '--set-upstream',
230
+                      branch,
231
+                      'remotes/origin/other')
232
+        self.assertRaises(
233
+            Exception,  # cmd.BranchTrackingMismatch inside
234
+            self._run_git_review, '-d', change_id)
235
+
163 236
     def test_no_rebase_check(self):
164 237
         """Test -R causes a change to be uploaded without rebase checking."""
165 238
         self._run_git_review('-s')

+ 58
- 0
git_review/tests/test_unit.py View File

@@ -176,6 +176,48 @@ password=pass
176 176
 """
177 177
 
178 178
 
179
+class ResolveTrackingUnitTest(testtools.TestCase):
180
+    """Class for testing resolve_tracking."""
181
+    def setUp(self):
182
+        testtools.TestCase.setUp(self)
183
+        patcher = mock.patch('git_review.cmd.run_command_exc')
184
+        self.addCleanup(patcher.stop)
185
+        self.run_command_exc = patcher.start()
186
+
187
+    def test_track_local_branch(self):
188
+        'Test that local tracked branch is not followed.'
189
+        self.run_command_exc.side_effect = [
190
+            '',
191
+            'refs/heads/other/branch',
192
+        ]
193
+        self.assertEqual(cmd.resolve_tracking(u'remote', u'rbranch'),
194
+                         (u'remote', u'rbranch'))
195
+
196
+    def test_track_untracked_branch(self):
197
+        'Test that local untracked branch is not followed.'
198
+        self.run_command_exc.side_effect = [
199
+            '',
200
+            '',
201
+        ]
202
+        self.assertEqual(cmd.resolve_tracking(u'remote', u'rbranch'),
203
+                         (u'remote', u'rbranch'))
204
+
205
+    def test_track_remote_branch(self):
206
+        'Test that remote tracked branch is followed.'
207
+        self.run_command_exc.side_effect = [
208
+            '',
209
+            'refs/remotes/other/branch',
210
+        ]
211
+        self.assertEqual(cmd.resolve_tracking(u'remote', u'rbranch'),
212
+                         (u'other', u'branch'))
213
+
214
+    def test_track_git_error(self):
215
+        'Test that local tracked branch is not followed.'
216
+        self.run_command_exc.side_effect = [cmd.CommandFailed(1, '', [], {})]
217
+        self.assertRaises(cmd.CommandFailed,
218
+                          cmd.resolve_tracking, u'remote', u'rbranch')
219
+
220
+
179 221
 class GitReviewUnitTest(testtools.TestCase):
180 222
     """Class for misc unit tests."""
181 223
 
@@ -236,3 +278,19 @@ class GitReviewUnitTest(testtools.TestCase):
236 278
                                          stdin='url=%s' % url)
237 279
         calls = [mock.call(url), mock.call(url, auth=('user', 'pass'))]
238 280
         mock_get.assert_has_calls(calls)
281
+
282
+    @mock.patch('sys.argv', ['argv0', '--track', 'branch'])
283
+    @mock.patch('git_review.cmd.check_remote')
284
+    @mock.patch('git_review.cmd.resolve_tracking')
285
+    def test_command_line_no_track(self, resolve_tracking, check_remote):
286
+        check_remote.side_effect = Exception()
287
+        self.assertRaises(Exception, cmd._main)
288
+        self.assertFalse(resolve_tracking.called)
289
+
290
+    @mock.patch('sys.argv', ['argv0', '--track'])
291
+    @mock.patch('git_review.cmd.check_remote')
292
+    @mock.patch('git_review.cmd.resolve_tracking')
293
+    def test_track(self, resolve_tracking, check_remote):
294
+        check_remote.side_effect = Exception()
295
+        self.assertRaises(Exception, cmd._main)
296
+        self.assertTrue(resolve_tracking.called)

Loading…
Cancel
Save