From 13101a6e42d159a7c9b4dddb2aaf916dab663b7e Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 9 Feb 2018 13:20:41 -0800 Subject: [PATCH] Remove similarity support from git-cl The purpose of this CL is to remove confusion around whether or not git-cl support the similarity flags when talking to Gerrit. It does not. However, because Rietveld support has not been fully ripped out of git-cl, this change does modify some of the Rietveld-specific code to remove similarity support from everywhere. Bug: 800902 Change-Id: I8703ed60d6889c7a36400b8a9f8f26de4669020a Reviewed-on: https://chromium-review.googlesource.com/912389 Reviewed-by: Andrii Shyshkalov Commit-Queue: Aaron Gable --- git_cl.py | 58 ++------------------------ tests/git_cl_test.py | 97 +++++++------------------------------------- 2 files changed, 18 insertions(+), 137 deletions(-) diff --git a/git_cl.py b/git_cl.py index 5a9db7eda1..cf49aaf785 100755 --- a/git_cl.py +++ b/git_cl.py @@ -294,44 +294,6 @@ def _git_amend_head(message, committer_timestamp): return RunGit(['commit', '--amend', '-m', message], env=env) -def add_git_similarity(parser): - parser.add_option( - '--similarity', metavar='SIM', type=int, action='store', - help='Sets the percentage that a pair of files need to match in order to' - ' be considered copies (default 50)') - parser.add_option( - '--find-copies', action='store_true', - help='Allows git to look for copies.') - parser.add_option( - '--no-find-copies', action='store_false', dest='find_copies', - help='Disallows git from looking for copies.') - - old_parser_args = parser.parse_args - - def Parse(args): - options, args = old_parser_args(args) - - if options.similarity is None: - options.similarity = _git_get_branch_config_value( - 'git-cl-similarity', default=50, value_type=int) - else: - print('Note: Saving similarity of %d%% in git config.' - % options.similarity) - _git_set_branch_config_value('git-cl-similarity', options.similarity) - - options.similarity = max(0, min(options.similarity, 100)) - - if options.find_copies is None: - options.find_copies = _git_get_branch_config_value( - 'git-find-copies', default=True, value_type=bool) - else: - _git_set_branch_config_value('git-find-copies', bool(options.find_copies)) - - return options, args - - parser.parse_args = Parse - - def _get_properties_from_options(options): properties = dict(x.split('=', 1) for x in options.properties) for key, val in properties.iteritems(): @@ -742,7 +704,7 @@ def write_try_results_json(output_file, builds): write_json(output_file, converted) -def print_stats(similarity, find_copies, args): +def print_stats(args): """Prints statistics about the change to the user.""" # --no-ext-diff is broken in some versions of Git, so try to work around # this by overriding the environment (but there is still a problem if the @@ -751,18 +713,12 @@ def print_stats(similarity, find_copies, args): if 'GIT_EXTERNAL_DIFF' in env: del env['GIT_EXTERNAL_DIFF'] - if find_copies: - similarity_options = ['-l100000', '-C%s' % similarity] - else: - similarity_options = ['-M%s' % similarity] - try: stdout = sys.stdout.fileno() except AttributeError: stdout = None return subprocess2.call( - ['git', - 'diff', '--no-ext-diff', '--stat'] + similarity_options + args, + ['git', 'diff', '--no-ext-diff', '--stat', '-l100000', '-C50'] + args, stdout=stdout, env=env) @@ -1668,7 +1624,7 @@ class Changelist(object): 'uploading now might not include those changes.') confirm_or_exit(action='upload') - print_stats(options.similarity, options.find_copies, git_diff_args) + print_stats(git_diff_args) ret = self.CMDUploadChange(options, git_diff_args, custom_cl_base, change) if not ret: if options.use_commit_queue: @@ -2285,10 +2241,6 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): if options.private or settings.GetDefaultPrivateFlag() == "True": upload_args.append('--private') - upload_args.extend(['--git_similarity', str(options.similarity)]) - if not options.find_copies: - upload_args.extend(['--git_no_find_copies']) - # Include the upstream repo's URL in the change -- this is useful for # projects that have their source spread across multiple repos. remote_url = self.GetGitBaseUrlFromConfig() @@ -5034,7 +4986,6 @@ def CMDupload(parser, args): help='email address to use to connect to Rietveld') orig_args = args - add_git_similarity(parser) auth.add_auth_options(parser) _add_codereview_select_options(parser) (options, args) = parser.parse_args(args) @@ -5134,7 +5085,6 @@ def CMDland(parser, args): help="external contributor for patch (appended to " + "description and used as author for git). Should be " + "formatted as 'First Last '") - add_git_similarity(parser) auth.add_auth_options(parser) (options, args) = parser.parse_args(args) auth_config = auth.extract_auth_config_from_options(options) @@ -5260,7 +5210,7 @@ def CMDland(parser, args): branches = [merge_base, cl.GetBranchRef()] if not options.force: - print_stats(options.similarity, options.find_copies, branches) + print_stats(branches) # We want to squash all this branch's commits into one commit with the proper # description. We do this by doing a "reset --soft" to the base branch (which diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 6a5de0f3c4..c7dbc09c5d 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -759,49 +759,22 @@ class TestGitCl(TestCase): ((['git', 'config', 'gerrit.host'],), 'True' if gerrit else '')] @classmethod - def _upload_calls(cls, similarity, find_copies, private): - return (cls._rietveld_git_base_calls(similarity, find_copies) + + def _upload_calls(cls, private): + return (cls._rietveld_git_base_calls() + cls._git_upload_calls(private)) @classmethod - def _rietveld_upload_no_rev_calls(cls, similarity, find_copies): - return (cls._rietveld_git_base_calls(similarity, find_copies) + [ + def _rietveld_upload_no_rev_calls(cls): + return (cls._rietveld_git_base_calls() + [ ((['git', 'config', 'core.editor'],), ''), ]) @classmethod - def _rietveld_git_base_calls(cls, similarity, find_copies): - if similarity is None: - similarity = '50' - similarity_call = ((['git', 'config', - 'branch.master.git-cl-similarity'],), CERR1) - else: - similarity_call = ((['git', 'config', - 'branch.master.git-cl-similarity', similarity],), '') - - if find_copies is None: - find_copies = True - find_copies_call = ((['git', 'config', '--bool', - 'branch.master.git-find-copies'],), CERR1) - else: - val = str(find_copies).lower() - find_copies_call = ((['git', 'config', '--bool', - 'branch.master.git-find-copies', val],), '') - - if find_copies: - stat_call = ((['git', 'diff', '--no-ext-diff', '--stat', - '-l100000', '-C'+similarity, - 'fake_ancestor_sha', 'HEAD'],), '+dat') - else: - stat_call = ((['git', 'diff', '--no-ext-diff', '--stat', - '-M'+similarity, 'fake_ancestor_sha', 'HEAD'],), '+dat') + def _rietveld_git_base_calls(cls): + stat_call = ((['git', 'diff', '--no-ext-diff', '--stat', + '-l100000', '-C50', 'fake_ancestor_sha', 'HEAD'],), '+dat') - return [ - ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - similarity_call, - ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - find_copies_call, - ] + cls._is_gerrit_calls() + [ + return cls._is_gerrit_calls() + [ ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'config', 'branch.master.rietveldissue'],), CERR1), ((['git', 'config', 'branch.master.gerritissue'],), CERR1), @@ -889,7 +862,7 @@ class TestGitCl(TestCase): ] @staticmethod - def _cmd_line(description, args, similarity, find_copies, private, cc): + def _cmd_line(description, args, private, cc): """Returns the upload command line passed to upload.RealMain().""" return [ 'upload', '--assume_yes', '--server', 'https://codereview.example.com', @@ -899,10 +872,7 @@ class TestGitCl(TestCase): ','.join( ['joe@example.com', 'chromium-reviews+test-more-cc@chromium.org'] + cc), - ] + (['--private'] - if private else []) + ['--git_similarity', similarity or '50'] + ( - ['--git_no_find_copies'] - if find_copies is False else []) + ['fake_ancestor_sha', 'HEAD'] + ] + (['--private'] if private else []) + ['fake_ancestor_sha', 'HEAD'] def _run_reviewer_test( self, @@ -915,22 +885,11 @@ class TestGitCl(TestCase): cc=None): """Generic reviewer test framework.""" self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) - try: - similarity = upload_args[upload_args.index('--similarity')+1] - except ValueError: - similarity = None - - if '--find-copies' in upload_args: - find_copies = True - elif '--no-find-copies' in upload_args: - find_copies = False - else: - find_copies = None private = '--private' in upload_args cc = cc or [] - self.calls = self._upload_calls(similarity, find_copies, private) + self.calls = self._upload_calls(private) def RunEditor(desc, _, **kwargs): self.assertEquals( @@ -945,8 +904,7 @@ class TestGitCl(TestCase): self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor) def check_upload(args): - cmd_line = self._cmd_line(final_description, reviewers, similarity, - find_copies, private, cc) + cmd_line = self._cmd_line(final_description, reviewers, private, cc) self.assertEquals(cmd_line, args) return 1, 2 self.mock(git_cl.upload, 'RealMain', check_upload) @@ -961,22 +919,6 @@ class TestGitCl(TestCase): 'desc\n\nBUG=', []) - def test_keep_similarity(self): - self._run_reviewer_test( - ['--similarity', '70'], - 'desc\n\nBUG=', - '# Blah blah comment.\ndesc\n\nBUG=', - 'desc\n\nBUG=', - []) - - def test_keep_find_copies(self): - self._run_reviewer_test( - ['--no-find-copies'], - 'desc\n\nBUG=', - '# Blah blah comment.\ndesc\n\nBUG=\n', - 'desc\n\nBUG=', - []) - def test_private(self): self._run_reviewer_test( ['--private'], @@ -1032,7 +974,7 @@ class TestGitCl(TestCase): def test_reviewer_send_mail_no_rev(self): # Fails without a reviewer. stdout = StringIO.StringIO() - self.calls = self._rietveld_upload_no_rev_calls(None, None) + [ + self.calls = self._rietveld_upload_no_rev_calls() + [ ((['DieWithError', 'Must specify reviewers to send email.'],), SystemExitMock()) ] @@ -1091,11 +1033,6 @@ class TestGitCl(TestCase): lambda i, c: self._mocked_call(['add_comment', i, c]))) self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), - ((['git', 'config', 'branch.feature.git-cl-similarity'],), CERR1), - ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), - ((['git', 'config', '--bool', 'branch.feature.git-find-copies'],), - CERR1), - ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), ((['git', 'config', 'branch.feature.rietveldissue'],), '123'), ((['git', 'config', 'rietveld.autoupdate'],), ''), ((['git', 'config', 'rietveld.server'],), @@ -1385,13 +1322,7 @@ class TestGitCl(TestCase): def _gerrit_base_calls(cls, issue=None, fetched_description=None, fetched_status=None, other_cl_owner=None, custom_cl_base=None): - calls = [ - ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', 'branch.master.git-cl-similarity'],), CERR1), - ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', '--bool', 'branch.master.git-find-copies'],), CERR1), - ] - calls += cls._is_gerrit_calls(True) + calls = cls._is_gerrit_calls(True) calls += [ ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'config', 'branch.master.rietveldissue'],), CERR1),