aiden
/
depot_tools
mirror of https://chromium.googlesource.com/chromium/tools/depot_tools
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Make gerrit 'git cl patch' use hard reset by default

Browse Source 
This CL changes the way that "git cl patch" behaves for Gerrit changes.

Previously, git-cl-patch behaved just like it did for Rietveld:
make sure you're on a branch, download the diff, apply it on top
of your branch. However, this causes problems with Gerrit. Namely,
when you upload a change to Gerrit, git-cl has to make sure that all
parents of your local change have previously been uploaded as well,
either as other changes or as commits already landed on the target
branch. But the method for "applying a patch" from Gerrit was to
cherry-pick it, and that changes the commit hash. So the resulting
commit would *not* have been uploaded to Gerrit. Thus, the
following routine didn't work with Gerrit:
$ git checkout -t origin/master -b your-work
$ git cl patch 123456
$ git checkout -tb my-work
$ #hack and commit
$ git cl upload
This would fail during the upload with a message saying that the
contents of 'your-work' hadn't been uploaded.

This CL fixes the situation by replacing the cherry-pick with
a hard reset. This means that the contents of the 'your-work'
branch will be *exactly* what was downloaded from Gerrit. Uploads
based on top of that commit will work just fine.

Finally, in a concession to some people who want 'git cl patch'
to actually apply a patch instead of performing a hard reset, if
the current branch contains local work, then rather than leaving
that work behind with a hard reset, we fall back to the old
cherry-pick behavior with a confirmation dialog and warning that
uploading will be hard.

Bug: 723787
Change-Id: I3ad164f6d3078bff00139d446bb8ce97738a1344
Reviewed-on: https://chromium-review.googlesource.com/527345
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
changes/45/527345/3
Aaron Gable 8 years ago committed by Commit Bot
parent 7303dcb9b3
commit 9387b4f0be
2 changed files with 74 additions and 14 deletions
Show Stats Download Patch File Download Diff File

29
git_cl.py
Unescape Escape View File

@ -2794,11 +2794,30 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
fetch_info = revision_info['fetch']['http']
RunGit(['fetch', fetch_info['url'], fetch_info['ref']])
self.SetIssue(self.GetIssue())
self.SetPatchset(patchset)
RunGit(['cherry-pick', 'FETCH_HEAD'])
print('Committed patch for change %i patchset %i locally.' %
(self.GetIssue(), self.GetPatchset()))
clean, _ = RunGitWithCode(
['merge-base', '--ancestor', 'HEAD', 'origin/master'])
if clean != 0:
clean, _ = RunGitWithCode(
['merge-base', '--ancestor', 'HEAD', 'FETCH_HEAD'])
if clean != 0:
confirm_or_exit(
'It looks like you\'re on a branch with some local commits.\n'
'If you apply this patch on top of your local content, you will not '
'be able to easily upload further changes based on it.\n'
'Would you like to proceed with applying this patch anyway?\n')
RunGit(['cherry-pick', 'FETCH_HEAD'])
print('Committed patch for change %i patchset %i locally.' %
(self._changelist.issue, patchset))
else:
RunGit(['reset', '--hard', 'FETCH_HEAD'])
self.SetIssue(self.GetIssue())
self.SetPatchset(patchset)
fetched_hash = RunGit(['rev-parse', 'FETCH_HEAD']).strip()
self._GitSetBranchConfigValue('last-upload-hash', fetched_hash)
self._GitSetBranchConfigValue('gerritsquashhash', fetched_hash)
print('Checked out commit for change %i patchset %i locally' %
(self.GetIssue(), self.GetPatchset()))
return 0
@staticmethod

59
tests/git_cl_test.py
Unescape Escape View File

@ -2127,12 +2127,16 @@ class TestGitCl(TestCase):
self.calls += [
((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
'refs/changes/56/123456/7'],), ''),
((['git', 'merge-base', '--ancestor', 'HEAD', 'origin/master'],), ''),
((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''),
((['git', 'config', 'branch.master.gerritissue', '123456'],),
''),
((['git', 'config', 'branch.master.gerritserver',
'https://chromium-review.googlesource.com'],), ''),
((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''),
((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
((['git', 'rev-parse', 'FETCH_HEAD'],), 'deadbeef'),
((['git', 'config', 'branch.master.last-upload-hash', 'deadbeef'],), ''),
((['git', 'config', 'branch.master.gerritsquashhash', 'deadbeef'],), ''),
]
self.assertEqual(git_cl.main(['patch', '123456']), 0)
@ -2142,12 +2146,16 @@ class TestGitCl(TestCase):
self.calls += [
((['git', 'fetch', 'https://host.googlesource.com/my/repo',
'refs/changes/56/123456/7'],), ''),
((['git', 'merge-base', '--ancestor', 'HEAD', 'origin/master'],), ''),
((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''),
((['git', 'config', 'branch.master.gerritissue', '123456'],),
''),
((['git', 'config', 'branch.master.gerritserver',
'https://host-review.googlesource.com'],), ''),
((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''),
((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
((['git', 'rev-parse', 'FETCH_HEAD'],), 'deadbeef'),
((['git', 'config', 'branch.master.last-upload-hash', 'deadbeef'],), ''),
((['git', 'config', 'branch.master.gerritsquashhash', 'deadbeef'],), ''),
]
self.assertEqual(git_cl.main(['patch', '--gerrit', '123456']), 0)
@ -2159,13 +2167,17 @@ class TestGitCl(TestCase):
self.calls += [
((['git', 'fetch', 'https://else.googlesource.com/my/repo',
'refs/changes/56/123456/1'],), ''),
((['git', 'merge-base', '--ancestor', 'HEAD', 'origin/master'],), ''),
((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''),
((['git', 'symbolic-ref', 'HEAD'],), 'master'),
((['git', 'config', 'branch.master.gerritissue', '123456'],),
''),
((['git', 'config', 'branch.master.gerritserver',
'https://else-review.googlesource.com'],), ''),
((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''),
((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
((['git', 'rev-parse', 'FETCH_HEAD'],), 'deadbeef'),
((['git', 'config', 'branch.master.last-upload-hash', 'deadbeef'],), ''),
((['git', 'config', 'branch.master.gerritsquashhash', 'deadbeef'],), ''),
]
self.assertEqual(git_cl.main(
['patch', 'https://else-review.googlesource.com/#/c/123456/1']), 0)
@ -2178,28 +2190,57 @@ class TestGitCl(TestCase):
self.calls += [
((['git', 'fetch', 'https://else.googlesource.com/my/repo',
'refs/changes/56/123456/1'],), ''),
((['git', 'merge-base', '--ancestor', 'HEAD', 'origin/master'],), ''),
((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''),
((['git', 'symbolic-ref', 'HEAD'],), 'master'),
((['git', 'config', 'branch.master.gerritissue', '123456'],),
''),
((['git', 'config', 'branch.master.gerritserver',
'https://else-review.googlesource.com'],), ''),
((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''),
((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
((['git', 'rev-parse', 'FETCH_HEAD'],), 'deadbeef'),
((['git', 'config', 'branch.master.last-upload-hash', 'deadbeef'],), ''),
((['git', 'config', 'branch.master.gerritsquashhash', 'deadbeef'],), ''),
]
self.assertEqual(git_cl.main(
['patch', 'https://else-review.googlesource.com/123456/1']), 0)
def test_patch_gerrit_local_content(self):
self._patch_common(default_codereview='gerrit', detect_gerrit_server=True,
git_short_host='chromium')
self.calls += [
((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
'refs/changes/56/123456/7'],), ''),
((['git', 'merge-base', '--ancestor', 'HEAD', 'origin/master'],),
CERR1),
((['git', 'merge-base', '--ancestor', 'HEAD', 'FETCH_HEAD'],),
CERR1),
(('ask_for_data',
'It looks like you\'re on a branch with some local commits.\n'
'If you apply this patch on top of your local content, you will not be '
'able to easily upload further changes based on it.\n'
'Would you like to proceed with applying this patch anyway?\n'
'Press Enter to confirm, or Ctrl+C to abort'), 'y'),
((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
]
self.assertEqual(git_cl.main(['patch', '123456']), 0)
def test_patch_gerrit_conflict(self):
self._patch_common(default_codereview='gerrit', detect_gerrit_server=True,
git_short_host='chromium')
self.calls += [
((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
'refs/changes/56/123456/7'],), ''),
((['git', 'config', 'branch.master.gerritissue', '123456'],),
''),
((['git', 'config', 'branch.master.gerritserver',
'https://chromium-review.googlesource.com'],), ''),
((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''),
((['git', 'merge-base', '--ancestor', 'HEAD', 'origin/master'],),
CERR1),
((['git', 'merge-base', '--ancestor', 'HEAD', 'FETCH_HEAD'],),
CERR1),
(('ask_for_data',
'It looks like you\'re on a branch with some local commits.\n'
'If you apply this patch on top of your local content, you will not be '
'able to easily upload further changes based on it.\n'
'Would you like to proceed with applying this patch anyway?\n'
'Press Enter to confirm, or Ctrl+C to abort'), 'y'),
((['git', 'cherry-pick', 'FETCH_HEAD'],), CERR1),
((['DieWithError', 'Command "git cherry-pick FETCH_HEAD" failed.\n'],),
SystemExitMock()),

Write Preview
Loading…
Cancel
Save