From 6c6827cbf27df9964f0c8a1fd069f2d31a10b210 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Thu, 6 Feb 2020 21:15:18 +0000 Subject: [PATCH] git-cl: Simplify FetchDescription and UpdateDescription - Merge GetDescription and FetchDescription, and remove unused `force` argument. - Merge UpdateDescription and UpdateDescriptionRemote. Bug: 1042324 Change-Id: Ia7a1b0aa1ea12a95bb6a19d9d3c9dd6aeb5bd2b8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2039968 Commit-Queue: Edward Lesmes Reviewed-by: Anthony Polito --- git_cl.py | 83 +++++++++++++++++++++++--------------------- tests/git_cl_test.py | 36 +++++++++---------- 2 files changed, 60 insertions(+), 59 deletions(-) diff --git a/git_cl.py b/git_cl.py index 23d6fe815..7ea25f220 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1389,18 +1389,23 @@ class Changelist(object): args = ['log', '--pretty=format:%s%n%n%b', '%s...' % (upstream_branch)] return RunGitWithCode(args)[1].strip() - def GetDescription(self, pretty=False, force=False): - if not self.has_description or force: - if self.GetIssue(): - self.description = self.FetchDescription(force=force) + def FetchDescription(self, pretty=False): + assert self.GetIssue(), 'issue is required to query Gerrit' + + if not self.has_description: + data = self._GetChangeDetail(['CURRENT_REVISION', 'CURRENT_COMMIT']) + current_rev = data['current_revision'] + self.description = data['revisions'][current_rev]['commit']['message'] self.has_description = True - if pretty: - # Set width to 72 columns + 2 space indent. - wrapper = textwrap.TextWrapper(width=74, replace_whitespace=True) - wrapper.initial_indent = wrapper.subsequent_indent = ' ' - lines = self.description.splitlines() - return '\n'.join([wrapper.fill(line) for line in lines]) - return self.description + + if not pretty: + return self.description + + # Set width to 72 columns + 2 space indent. + wrapper = textwrap.TextWrapper(width=74, replace_whitespace=True) + wrapper.initial_indent = wrapper.subsequent_indent = ' ' + lines = self.description.splitlines() + return '\n'.join([wrapper.fill(line) for line in lines]) def GetPatchset(self): """Returns the patchset number as a int or None if not set.""" @@ -1474,7 +1479,7 @@ class Changelist(object): issue = self.GetIssue() patchset = self.GetPatchset() if issue: - description = self.GetDescription() + description = self.FetchDescription() else: # If the change was never uploaded, use the log messages of all commits # up to the branch point, as git cl upload will prefill the description @@ -1493,7 +1498,22 @@ class Changelist(object): upstream=upstream_branch) def UpdateDescription(self, description, force=False): - self.UpdateDescriptionRemote(description, force=force) + assert self.GetIssue(), 'issue is required to update description' + + if gerrit_util.HasPendingChangeEdit( + self._GetGerritHost(), self._GerritChangeIdentifier()): + if not force: + confirm_or_exit( + 'The description cannot be modified while the issue has a pending ' + 'unpublished edit. Either publish the edit in the Gerrit web UI ' + 'or delete it.\n\n', action='delete the unpublished edit') + + gerrit_util.DeletePendingChangeEdit( + self._GetGerritHost(), self._GerritChangeIdentifier()) + gerrit_util.SetCommitMessage( + self._GetGerritHost(), self._GerritChangeIdentifier(), + description, notify='NONE') + self.description = description self.has_description = True @@ -1873,32 +1893,14 @@ class Changelist(object): return 'unsent' def GetMostRecentPatchset(self): + if not self.GetIssue(): + return None + data = self._GetChangeDetail(['CURRENT_REVISION']) patchset = data['revisions'][data['current_revision']]['_number'] self.SetPatchset(patchset) return patchset - def FetchDescription(self, force=False): - data = self._GetChangeDetail(['CURRENT_REVISION', 'CURRENT_COMMIT'], - no_cache=force) - current_rev = data['current_revision'] - return data['revisions'][current_rev]['commit']['message'] - - def UpdateDescriptionRemote(self, description, force=False): - if gerrit_util.HasPendingChangeEdit( - self._GetGerritHost(), self._GerritChangeIdentifier()): - if not force: - confirm_or_exit( - 'The description cannot be modified while the issue has a pending ' - 'unpublished edit. Either publish the edit in the Gerrit web UI ' - 'or delete it.\n\n', action='delete the unpublished edit') - - gerrit_util.DeletePendingChangeEdit( - self._GetGerritHost(), self._GerritChangeIdentifier()) - gerrit_util.SetCommitMessage( - self._GetGerritHost(), self._GerritChangeIdentifier(), - description, notify='NONE') - def AddComment(self, message, publish=None): gerrit_util.SetReview( self._GetGerritHost(), self._GerritChangeIdentifier(), @@ -2357,7 +2359,7 @@ class Changelist(object): self._GerritCommitMsgHookCheck(offer_removal=not options.force) if self.GetIssue(): # Try to get the message from a previous upload. - message = self.GetDescription() + message = self.FetchDescription() if not message: DieWithError( 'failed to fetch description from current Gerrit change %d\n' @@ -3818,12 +3820,13 @@ def CMDstatus(parser, args): parser.error('Unsupported args: %s' % args) if options.issue is not None and not options.field: - parser.error('--field must be specified with --issue.') + parser.error('--field must be given when --issue is set.') if options.field: cl = Changelist(issue=options.issue) if options.field.startswith('desc'): - print(cl.GetDescription()) + if cl.GetIssue(): + print(cl.FetchDescription()) elif options.field == 'id': issueid = cl.GetIssue() if issueid: @@ -3910,7 +3913,7 @@ def CMDstatus(parser, args): print('Issue number: %s (%s)' % (cl.GetIssue(), cl.GetIssueURL())) if not options.fast: print('Issue description:') - print(cl.GetDescription(pretty=True)) + print(cl.FetchDescription(pretty=True)) return 0 @@ -4098,7 +4101,7 @@ def CMDdescription(parser, args): if args and not args[0].isdigit(): logging.info('canonical issue/change URL: %s\n', cl.GetIssueURL()) - description = ChangeDescription(cl.GetDescription()) + description = ChangeDescription(cl.FetchDescription()) if options.display: print(description.description) @@ -4115,7 +4118,7 @@ def CMDdescription(parser, args): description.set_description(text) else: description.prompt() - if cl.GetDescription().strip() != description.description: + if cl.FetchDescription().strip() != description.description: cl.UpdateDescription(description.description, force=options.force) return 0 diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 746fc0ec8..78f4c8e0a 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -83,7 +83,7 @@ class ChangelistMock(object): pass def GetIssue(self): return 1 - def GetDescription(self, force=False): + def FetchDescription(self): return ChangelistMock.desc def UpdateDescription(self, desc, force=False): ChangelistMock.desc = desc @@ -169,11 +169,11 @@ class SystemExitMock(Exception): class TestGitClBasic(unittest.TestCase): - def test_get_description(self): + def test_fetch_description(self): cl = git_cl.Changelist(issue=1, codereview_host='host') cl.description = 'x' cl.has_description = True - self.assertEqual(cl.GetDescription(), 'x') + self.assertEqual(cl.FetchDescription(), 'x') def test_set_preserve_tryjobs(self): d = git_cl.ChangeDescription('Simple.') @@ -2150,7 +2150,8 @@ class TestGitCl(TestCase): self.assertEqual(git_cl.main(['status', '--issue', '1']), 0) except SystemExit as ex: self.assertEqual(ex.code, 2) - self.assertRegexpMatches(out.getvalue(), r'--field must be specified') + self.assertIn( + '--field must be given when --issue is set.', out.getvalue()) out = StringIO() self.mock(git_cl.sys, 'stderr', out) @@ -2159,7 +2160,8 @@ class TestGitCl(TestCase): self.assertEqual(git_cl.main(['status', '--issue', '1']), 0) except SystemExit as ex: self.assertEqual(ex.code, 2) - self.assertRegexpMatches(out.getvalue(), r'--field must be specified') + self.assertIn( + '--field must be given when --issue is set.', out.getvalue()) def test_StatusFieldOverrideIssue(self): out = StringIO() @@ -2169,7 +2171,7 @@ class TestGitCl(TestCase): self.assertEqual(cl_self.issue, 1) return 'foobar' - self.mock(git_cl.Changelist, 'GetDescription', assertIssue) + self.mock(git_cl.Changelist, 'FetchDescription', assertIssue) self.assertEqual( git_cl.main(['status', '--issue', '1', '--field', 'desc']), 0) @@ -2181,7 +2183,7 @@ class TestGitCl(TestCase): self.assertEqual(cl_self.issue, 1) return 'foobar' - self.mock(git_cl.Changelist, 'GetDescription', assertIssue) + self.mock(git_cl.Changelist, 'FetchDescription', assertIssue) self.mock(git_cl.Changelist, 'CloseIssue', lambda *_: None) self.assertEqual( git_cl.main(['set-close', '--issue', '1']), 0) @@ -2235,14 +2237,14 @@ class TestGitCl(TestCase): # Simulate user changing something. return 'Some.\n\nChange-Id: xxx\nBug: 123' - def UpdateDescriptionRemote(_, desc, force=False): + def UpdateDescription(_, desc, force=False): self.assertEqual(desc, 'Some.\n\nChange-Id: xxx\nBug: 123') self.mock(git_cl.sys, 'stdout', StringIO()) - self.mock(git_cl.Changelist, 'GetDescription', + self.mock(git_cl.Changelist, 'FetchDescription', lambda *args: current_desc) - self.mock(git_cl.Changelist, 'UpdateDescriptionRemote', - UpdateDescriptionRemote) + self.mock(git_cl.Changelist, 'UpdateDescription', + UpdateDescription) self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor) self.calls = [ @@ -2269,7 +2271,7 @@ class TestGitCl(TestCase): return desc self.mock(git_cl.sys, 'stdout', StringIO()) - self.mock(git_cl.Changelist, 'GetDescription', + self.mock(git_cl.Changelist, 'FetchDescription', lambda *args: current_desc) self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor) @@ -2428,7 +2430,7 @@ class TestGitCl(TestCase): def test_cmd_issue_erase_existing_with_change_id(self): out = StringIO() self.mock(git_cl.sys, 'stdout', out) - self.mock(git_cl.Changelist, 'GetDescription', + self.mock(git_cl.Changelist, 'FetchDescription', lambda _: 'This is a description\n\nChange-Id: Ideadbeef') self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), @@ -2594,18 +2596,14 @@ class TestGitCl(TestCase): (('GetChangeDetail', 'host', 'my%2Frepo~1', ['CURRENT_REVISION', 'CURRENT_COMMIT']), gen_detail('rev1', 'desc1')), - (('GetChangeDetail', 'host', 'my%2Frepo~1', - ['CURRENT_REVISION', 'CURRENT_COMMIT']), - gen_detail('rev2', 'desc2')), ] self._mock_gerrit_changes_for_detail_cache() cl = git_cl.Changelist(issue=1) cl._cached_remote_url = ( True, 'https://chromium.googlesource.com/a/my/repo.git/') - self.assertEqual(cl.GetDescription(), 'desc1') - self.assertEqual(cl.GetDescription(), 'desc1') # cache hit. - self.assertEqual(cl.GetDescription(force=True), 'desc2') + self.assertEqual(cl.FetchDescription(), 'desc1') + self.assertEqual(cl.FetchDescription(), 'desc1') # cache hit. def test_print_current_creds(self): class CookiesAuthenticatorMock(object):