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

git cl: use project~change_number when querying Gerrit for change Info.

Browse Source 
R=ehmaldonado

Bug: 876910
Change-Id: I218a8198d06e51ac2eb57636bbccca9aadc7d81a
Reviewed-on: https://chromium-review.googlesource.com/1194312
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
changes/12/1194312/2
Andrii Shyshkalov 7 years ago committed by Commit Bot
parent 0d14d0d85b
commit 03e0ed26ea
2 changed files with 54 additions and 29 deletions
Show Stats Download Patch File Download Diff File

17
git_cl.py
Unescape Escape View File

@ -2704,8 +2704,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
thus new data will be fetched from Gerrit. thus new data will be fetched from Gerrit.
""" """
options = options or [] options = options or []
issue = self.GetIssue() assert self.GetIssue(), 'issue is required to query Gerrit'
assert issue, 'issue is required to query Gerrit'
# Optimization to avoid multiple RPCs: # Optimization to avoid multiple RPCs:
if (('CURRENT_REVISION' in options or 'ALL_REVISIONS' in options) and if (('CURRENT_REVISION' in options or 'ALL_REVISIONS' in options) and
@ -2713,15 +2712,15 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
options.append('CURRENT_COMMIT') options.append('CURRENT_COMMIT')
# Normalize issue and options for consistent keys in cache. # Normalize issue and options for consistent keys in cache.
issue = str(self.GetIssue()) cache_key = str(self.GetIssue())
options = [o.upper() for o in options] options = [o.upper() for o in options]
# Check in cache first unless no_cache is True. # Check in cache first unless no_cache is True.
if no_cache: if no_cache:
self._detail_cache.pop(issue, None) self._detail_cache.pop(cache_key, None)
else: else:
options_set = frozenset(options) options_set = frozenset(options)
for cached_options_set, data in self._detail_cache.get(issue, []): for cached_options_set, data in self._detail_cache.get(cache_key, []):
# Assumption: data fetched before with extra options is suitable # Assumption: data fetched before with extra options is suitable
# for return for a smaller set of options. # for return for a smaller set of options.
# For example, if we cached data for # For example, if we cached data for
@ -2732,13 +2731,15 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
return data return data
try: try:
data = gerrit_util.GetChangeDetail(self._GetGerritHost(), issue, options) data = gerrit_util.GetChangeDetail(
self._GetGerritHost(), self._GerritChangeIdentifier(), options)
except gerrit_util.GerritError as e: except gerrit_util.GerritError as e:
if e.http_status == 404: if e.http_status == 404:
raise GerritChangeNotExists(issue, self.GetCodereviewServer()) raise GerritChangeNotExists(self.GetIssue(), self.GetCodereviewServer())
raise raise
self._detail_cache.setdefault(issue, []).append((frozenset(options), data)) self._detail_cache.setdefault(cache_key, []).append(
(frozenset(options), data))
return data return data
def _GetChangeCommit(self): def _GetChangeCommit(self):

66
tests/git_cl_test.py
Unescape Escape View File

@ -1124,7 +1124,7 @@ class TestGitCl(TestCase):
if issue: if issue:
calls += [ calls += [
(('GetChangeDetail', 'chromium-review.googlesource.com', (('GetChangeDetail', 'chromium-review.googlesource.com',
'123456', 'my%2Frepo~123456',
['DETAILED_ACCOUNTS', 'CURRENT_REVISION', 'CURRENT_COMMIT']), ['DETAILED_ACCOUNTS', 'CURRENT_REVISION', 'CURRENT_COMMIT']),
{ {
'owner': {'email': (other_cl_owner or 'owner@example.com')}, 'owner': {'email': (other_cl_owner or 'owner@example.com')},
@ -1334,8 +1334,8 @@ class TestGitCl(TestCase):
] ]
if tbr: if tbr:
calls += [ calls += [
(('GetChangeDetail', 'chromium-review.googlesource.com', '123456', (('GetChangeDetail', 'chromium-review.googlesource.com',
['LABELS']), { 'my%2Frepo~123456', ['LABELS']), {
'labels': { 'labels': {
'Code-Review': { 'Code-Review': {
'default_value': 0, 'default_value': 0,
@ -1838,7 +1838,7 @@ class TestGitCl(TestCase):
if actual_codereview == 'gerrit': if actual_codereview == 'gerrit':
self.calls += [ self.calls += [
(('GetChangeDetail', git_short_host + '-review.googlesource.com', (('GetChangeDetail', git_short_host + '-review.googlesource.com',
'123456', ['ALL_REVISIONS', 'CURRENT_COMMIT']), 'my%2Frepo~123456', ['ALL_REVISIONS', 'CURRENT_COMMIT']),
{ {
'current_revision': '7777777777', 'current_revision': '7777777777',
'revisions': { 'revisions': {
@ -1931,11 +1931,11 @@ class TestGitCl(TestCase):
self.assertEqual(git_cl.main(['patch', '--gerrit', '123456', '--force']), 0) self.assertEqual(git_cl.main(['patch', '--gerrit', '123456', '--force']), 0)
def test_patch_gerrit_guess_by_url(self): def test_patch_gerrit_guess_by_url(self):
self.calls += self._get_gerrit_codereview_server_calls(
'master', git_short_host='else', detect_server=False)
self._patch_common( self._patch_common(
actual_codereview='gerrit', git_short_host='else', actual_codereview='gerrit', git_short_host='else',
codereview_in_url=True, detect_gerrit_server=False) codereview_in_url=True, detect_gerrit_server=False)
self.calls += self._get_gerrit_codereview_server_calls(
'master', git_short_host='else', detect_server=False)
self.calls += [ self.calls += [
((['git', 'fetch', 'https://else.googlesource.com/my/repo', ((['git', 'fetch', 'https://else.googlesource.com/my/repo',
'refs/changes/56/123456/1'],), ''), 'refs/changes/56/123456/1'],), ''),
@ -1953,11 +1953,11 @@ class TestGitCl(TestCase):
['patch', 'https://else-review.googlesource.com/#/c/123456/1']), 0) ['patch', 'https://else-review.googlesource.com/#/c/123456/1']), 0)
def test_patch_gerrit_guess_by_url_with_repo(self): def test_patch_gerrit_guess_by_url_with_repo(self):
self.calls += self._get_gerrit_codereview_server_calls(
'master', git_short_host='else', detect_server=False)
self._patch_common( self._patch_common(
actual_codereview='gerrit', git_short_host='else', actual_codereview='gerrit', git_short_host='else',
codereview_in_url=True, detect_gerrit_server=False) codereview_in_url=True, detect_gerrit_server=False)
self.calls += self._get_gerrit_codereview_server_calls(
'master', git_short_host='else', detect_server=False)
self.calls += [ self.calls += [
((['git', 'fetch', 'https://else.googlesource.com/my/repo', ((['git', 'fetch', 'https://else.googlesource.com/my/repo',
'refs/changes/56/123456/1'],), ''), 'refs/changes/56/123456/1'],), ''),
@ -2240,8 +2240,13 @@ class TestGitCl(TestCase):
out = StringIO.StringIO() out = StringIO.StringIO()
self.mock(git_cl.sys, 'stdout', out) self.mock(git_cl.sys, 'stdout', out)
self.calls = [ self.calls = [
((['git', 'symbolic-ref', 'HEAD'],), 'feature'),
((['git', 'config', 'branch.feature.merge'],), 'feature'),
((['git', 'config', 'branch.feature.remote'],), 'origin'),
((['git', 'config', 'remote.origin.url'],),
'https://chromium.googlesource.com/my/repo'),
(('GetChangeDetail', 'code.review.org', (('GetChangeDetail', 'code.review.org',
'123123', ['CURRENT_REVISION', 'CURRENT_COMMIT']), 'my%2Frepo~123123', ['CURRENT_REVISION', 'CURRENT_COMMIT']),
{ {
'current_revision': 'sha1', 'current_revision': 'sha1',
'revisions': {'sha1': { 'revisions': {'sha1': {
@ -2500,7 +2505,12 @@ class TestGitCl(TestCase):
((['git', 'config', 'branch.feature.gerritissue'],), '123456'), ((['git', 'config', 'branch.feature.gerritissue'],), '123456'),
((['git', 'config', 'branch.feature.gerritserver'],), ((['git', 'config', 'branch.feature.gerritserver'],),
'https://chromium-review.googlesource.com'), 'https://chromium-review.googlesource.com'),
(('GetChangeDetail', 'chromium-review.googlesource.com', '123456', ((['git', 'config', 'branch.feature.merge'],), 'feature'),
((['git', 'config', 'branch.feature.remote'],), 'origin'),
((['git', 'config', 'remote.origin.url'],),
'https://chromium.googlesource.com/depot_tools'),
(('GetChangeDetail', 'chromium-review.googlesource.com',
'depot_tools~123456',
['DETAILED_ACCOUNTS', 'ALL_REVISIONS', 'CURRENT_COMMIT']), { ['DETAILED_ACCOUNTS', 'ALL_REVISIONS', 'CURRENT_COMMIT']), {
'project': 'depot_tools', 'project': 'depot_tools',
'status': 'OPEN', 'status': 'OPEN',
@ -2608,7 +2618,12 @@ class TestGitCl(TestCase):
((['git', 'config', 'branch.feature.gerritissue'],), '123456'), ((['git', 'config', 'branch.feature.gerritissue'],), '123456'),
((['git', 'config', 'branch.feature.gerritserver'],), ((['git', 'config', 'branch.feature.gerritserver'],),
'https://chromium-review.googlesource.com'), 'https://chromium-review.googlesource.com'),
(('GetChangeDetail', 'chromium-review.googlesource.com', '123456', ((['git', 'config', 'branch.feature.merge'],), 'feature'),
((['git', 'config', 'branch.feature.remote'],), 'origin'),
((['git', 'config', 'remote.origin.url'],),
'https://chromium.googlesource.com/depot_tools'),
(('GetChangeDetail', 'chromium-review.googlesource.com',
'depot_tools~123456',
['DETAILED_ACCOUNTS', 'ALL_REVISIONS', 'CURRENT_COMMIT']), { ['DETAILED_ACCOUNTS', 'ALL_REVISIONS', 'CURRENT_COMMIT']), {
'project': 'depot_tools', 'project': 'depot_tools',
'status': 'OPEN', 'status': 'OPEN',
@ -3024,12 +3039,16 @@ class TestGitCl(TestCase):
def test_gerrit_change_detail_cache_simple(self): def test_gerrit_change_detail_cache_simple(self):
self._mock_gerrit_changes_for_detail_cache() self._mock_gerrit_changes_for_detail_cache()
self.calls = [ self.calls = [
(('GetChangeDetail', 'host', '1', []), 'a'), (('GetChangeDetail', 'host', 'my%2Frepo~1', []), 'a'),
(('GetChangeDetail', 'host', '2', []), 'b'), (('GetChangeDetail', 'host', 'ab%2Frepo~2', []), 'b'),
(('GetChangeDetail', 'host', '2', []), 'b2'), (('GetChangeDetail', 'host', 'ab%2Frepo~2', []), 'b2'),
] ]
cl1 = git_cl.Changelist(issue=1, codereview='gerrit') cl1 = git_cl.Changelist(issue=1, codereview='gerrit')
cl1._cached_remote_url = (
True, 'https://chromium.googlesource.com/a/my/repo.git/')
cl2 = git_cl.Changelist(issue=2, codereview='gerrit') cl2 = git_cl.Changelist(issue=2, codereview='gerrit')
cl2._cached_remote_url = (
True, 'https://chromium.googlesource.com/ab/repo')
self.assertEqual(cl1._GetChangeDetail(), 'a') # Miss. self.assertEqual(cl1._GetChangeDetail(), 'a') # Miss.
self.assertEqual(cl1._GetChangeDetail(), 'a') self.assertEqual(cl1._GetChangeDetail(), 'a')
self.assertEqual(cl2._GetChangeDetail(), 'b') # Miss. self.assertEqual(cl2._GetChangeDetail(), 'b') # Miss.
@ -3040,12 +3059,14 @@ class TestGitCl(TestCase):
def test_gerrit_change_detail_cache_options(self): def test_gerrit_change_detail_cache_options(self):
self._mock_gerrit_changes_for_detail_cache() self._mock_gerrit_changes_for_detail_cache()
self.calls = [ self.calls = [
(('GetChangeDetail', 'host', '1', ['C', 'A', 'B']), 'cab'), (('GetChangeDetail', 'host', 'repo~1', ['C', 'A', 'B']), 'cab'),
(('GetChangeDetail', 'host', '1', ['A', 'D']), 'ad'), (('GetChangeDetail', 'host', 'repo~1', ['A', 'D']), 'ad'),
(('GetChangeDetail', 'host', '1', ['A']), 'a'), # no_cache=True (('GetChangeDetail', 'host', 'repo~1', ['A']), 'a'), # no_cache=True
(('GetChangeDetail', 'host', '1', ['B']), 'b'), # no longer in cache. # no longer in cache.
(('GetChangeDetail', 'host', 'repo~1', ['B']), 'b'),
] ]
cl = git_cl.Changelist(issue=1, codereview='gerrit') cl = git_cl.Changelist(issue=1, codereview='gerrit')
cl._cached_remote_url = (True, 'https://chromium.googlesource.com/repo/')
self.assertEqual(cl._GetChangeDetail(options=['C', 'A', 'B']), 'cab') self.assertEqual(cl._GetChangeDetail(options=['C', 'A', 'B']), 'cab')
self.assertEqual(cl._GetChangeDetail(options=['A', 'B', 'C']), 'cab') self.assertEqual(cl._GetChangeDetail(options=['A', 'B', 'C']), 'cab')
self.assertEqual(cl._GetChangeDetail(options=['B', 'A']), 'cab') self.assertEqual(cl._GetChangeDetail(options=['B', 'A']), 'cab')
@ -3069,16 +3090,18 @@ class TestGitCl(TestCase):
'revisions': {rev: {'commit': {'message': desc}}} 'revisions': {rev: {'commit': {'message': desc}}}
} }
self.calls = [ self.calls = [
(('GetChangeDetail', 'host', '1', (('GetChangeDetail', 'host', 'my%2Frepo~1',
['CURRENT_REVISION', 'CURRENT_COMMIT']), ['CURRENT_REVISION', 'CURRENT_COMMIT']),
gen_detail('rev1', 'desc1')), gen_detail('rev1', 'desc1')),
(('GetChangeDetail', 'host', '1', (('GetChangeDetail', 'host', 'my%2Frepo~1',
['CURRENT_REVISION', 'CURRENT_COMMIT']), ['CURRENT_REVISION', 'CURRENT_COMMIT']),
gen_detail('rev2', 'desc2')), gen_detail('rev2', 'desc2')),
] ]
self._mock_gerrit_changes_for_detail_cache() self._mock_gerrit_changes_for_detail_cache()
cl = git_cl.Changelist(issue=1, codereview='gerrit') cl = git_cl.Changelist(issue=1, codereview='gerrit')
cl._cached_remote_url = (
True, 'https://chromium.googlesource.com/a/my/repo.git/')
self.assertEqual(cl.GetDescription(), 'desc1') self.assertEqual(cl.GetDescription(), 'desc1')
self.assertEqual(cl.GetDescription(), 'desc1') # cache hit. self.assertEqual(cl.GetDescription(), 'desc1') # cache hit.
self.assertEqual(cl.GetDescription(force=True), 'desc2') self.assertEqual(cl.GetDescription(force=True), 'desc2')
@ -3264,7 +3287,8 @@ class TestGitCl(TestCase):
'origin/master'), 'origin/master'),
((['git', 'config', 'remote.origin.url'],), ((['git', 'config', 'remote.origin.url'],),
'https://chromium.googlesource.com/infra/infra'), 'https://chromium.googlesource.com/infra/infra'),
(('GetChangeDetail', 'chromium-review.googlesource.com', '1', (('GetChangeDetail', 'chromium-review.googlesource.com',
'infra%2Finfra~1',
['MESSAGES', 'DETAILED_ACCOUNTS']), { ['MESSAGES', 'DETAILED_ACCOUNTS']), {
'owner': {'email': 'owner@example.com'}, 'owner': {'email': 'owner@example.com'},
'messages': [ 'messages': [

Write Preview
Loading…
Cancel
Save