From 83669ee54bdaa991230c9a14f19db5773f6161d4 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Fri, 17 Oct 2014 16:17:35 +0100 Subject: [PATCH] Add support and CLI option to search multiple refs for last import When auto-detecting the most recent point in the history of the current branch to be updated that came from upstream, need to support searching more than the given upstream reference where the user is switching the upstream branch being tracked. Use a default of 'upstream/*' and add a CLI option to set a different pattern to be searched in cases where the repository is not using the same naming. The CLI option '--search-refs' may be specified multiple times to provide multiple ref patterns to be searched using simple globbing. The default if none provided by the user is 'upstream/*'. Change-Id: If890d1bee015d6b495a5caa60dd8f7783cf7c4e0 Closes-Bug: #1380652 --- git_upstream/commands/__init__.py | 19 +++ git_upstream/commands/import.py | 28 ++-- git_upstream/lib/searchers.py | 30 ++-- git_upstream/tests/base.py | 3 + git_upstream/tests/test_import_cmd.py | 202 ++++++++++++++++++++++++++ git_upstream/tests/test_searchers.py | 9 +- git_upstream/tests/test_strategies.py | 2 +- 7 files changed, 268 insertions(+), 25 deletions(-) diff --git a/git_upstream/commands/__init__.py b/git_upstream/commands/__init__.py index 2908c14..a48cecf 100644 --- a/git_upstream/commands/__init__.py +++ b/git_upstream/commands/__init__.py @@ -16,10 +16,28 @@ # limitations under the License. # +import argparse import os import sys +class AppendReplaceAction(argparse._AppendAction): + """Allows setting of a default value which is overriden by the first use + of the option, while subsequent calls will then append. + """ + def __init__(self, *args, **kwargs): + super(AppendReplaceAction, self).__init__(*args, **kwargs) + self._reset_default = False + self.default = list(self.default) + + def __call__(self, parser, namespace, values, option_string=None): + if not self._reset_default: + setattr(namespace, self.dest, []) + self._reset_default = True + super(AppendReplaceAction, self).__call__(parser, namespace, values, + option_string) + + def get_subcommands(subparsers): subcommands = _find_actions(subparsers, os.path.dirname(__file__)) @@ -45,6 +63,7 @@ def _find_actions(subparsers, module_path): command, help=help, description=desc) + subparser.register('action', 'append_replace', AppendReplaceAction) for (args, kwargs) in args: subparser.add_argument(*args, **kwargs) diff --git a/git_upstream/commands/import.py b/git_upstream/commands/import.py index 2bd276a..672a9f3 100644 --- a/git_upstream/commands/import.py +++ b/git_upstream/commands/import.py @@ -498,24 +498,29 @@ class LocateChangesWalk(LocateChangesStrategy): _strategy = "drop" - def __init__(self, branch="HEAD", search_ref=None, *args, **kwargs): + def __init__(self, branch="HEAD", upstream="upstream/master", + search_refs=None, *args, **kwargs): + + if not search_refs: + search_refs = [] + search_refs.insert(0, upstream) + self.searcher = UpstreamMergeBaseSearcher(branch=branch, - pattern=search_ref) - self.search_ref = search_ref + patterns=search_refs) + self.upstream = upstream super(LocateChangesWalk, self).__init__(*args, **kwargs) def filtered_iter(self): # may wish to make class used to remove duplicate objects configurable # through git-upstream specific 'git config' settings - if self.search_ref: - self.filters.append( - DiscardDuplicateGerritChangeId(self.search_ref, - limit=self.searcher.commit)) + self.filters.append( + DiscardDuplicateGerritChangeId(self.upstream, + limit=self.searcher.commit)) self.filters.append(NoMergeCommitFilter()) self.filters.append(ReverseCommitFilter()) self.filters.append(DroppedCommitFilter()) self.filters.append( - SupersededCommitFilter(self.search_ref, + SupersededCommitFilter(self.upstream, limit=self.searcher.commit)) return super(LocateChangesWalk, self).filtered_iter() @@ -542,6 +547,10 @@ class LocateChangesWalk(LocateChangesStrategy): default=LocateChangesWalk.get_strategy_name(), help='Use the given strategy to re-apply locally carried ' 'changes to the import branch. (default: %(default)s)') +@subcommand.arg('--search-refs', action='append_replace', metavar='', + default=['upstream/*'], dest='search_refs', + help='Refs to search for previous import commit. May be ' + 'specified multiple times.') @subcommand.arg('--into', dest='branch', metavar='', default='HEAD', help='Branch to take changes from, and replace with imported ' 'branch.') @@ -579,7 +588,8 @@ def do_import(args): logger.notice("Searching for previous import") strategy = ImportStrategiesFactory.create_strategy( - args.strategy, branch=args.branch, search_ref=args.upstream_branch) + args.strategy, branch=args.branch, upstream=args.upstream_branch, + search_refs=args.search_refs) if len(strategy) == 0: raise ImportUpstreamError("Cannot find previous import") diff --git a/git_upstream/lib/searchers.py b/git_upstream/lib/searchers.py index 25c3b52..3999c6d 100644 --- a/git_upstream/lib/searchers.py +++ b/git_upstream/lib/searchers.py @@ -124,34 +124,40 @@ class UpstreamMergeBaseSearcher(LogDedentMixin, Searcher): base available. """ - def __init__(self, pattern="upstream/*", search_tags=False, remotes=None, + def __init__(self, branch, patterns=None, search_tags=False, remotes=None, *args, **kwargs): + super(UpstreamMergeBaseSearcher, self).__init__(branch, *args, + **kwargs) + + if not patterns: + patterns = ["upstream/*"] + self._patterns = patterns + self._references = ["refs/heads/{0}".format(ref) + for ref in self.patterns] + if not remotes: remotes = [] - self._pattern = pattern - self._references = ["refs/heads/{0}".format(self.pattern)] - - super(UpstreamMergeBaseSearcher, self).__init__(*args, **kwargs) if remotes: self._references.extend( - ["refs/remotes/{0}/{1}".format(s, self.pattern) - for s in remotes]) + ["refs/remotes/{0}/{1}".format(s, ref) + for s in remotes for ref in self.patterns]) else: - self._references.append( - "refs/remotes/*/{0}".format(self.pattern)) + self._references.extend( + ["refs/remotes/*/{0}".format(ref) for ref in self.patterns]) if search_tags: - self._references.append("refs/tags/{0}".format(self.pattern)) + self._references.append( + ["refs/tags/{0}".format(ref) for ref in self.patterns]) @property - def pattern(self): + def patterns(self): """ Pattern to limit which references are searched when looking for a merge base commit. """ - return self._pattern + return self._patterns def find(self): """ diff --git a/git_upstream/tests/base.py b/git_upstream/tests/base.py index 4f5ad3c..737efbf 100644 --- a/git_upstream/tests/base.py +++ b/git_upstream/tests/base.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import logging import os import tempfile @@ -130,6 +131,8 @@ class GitRepo(fixtures.Fixture): class BaseTestCase(testtools.TestCase): """Base Test Case for all tests.""" + logging.basicConfig() + def setUp(self): super(BaseTestCase, self).setUp() diff --git a/git_upstream/tests/test_import_cmd.py b/git_upstream/tests/test_import_cmd.py index 1f76ea6..863361b 100644 --- a/git_upstream/tests/test_import_cmd.py +++ b/git_upstream/tests/test_import_cmd.py @@ -21,6 +21,7 @@ import mock from testtools.matchers import Equals from git_upstream import main +from git_upstream.lib.pygitcompat import Commit from git_upstream.tests.base import BaseTestCase from string import lower @@ -158,3 +159,204 @@ class TestImportCommand(BaseTestCase): mock_logger.warning.assert_called_with( SubstringMatcher( containing="Previous import merged additional")) + + def test_import_switch_branches_search(self): + """Test that the import sub-command can correctly switch branches when + importing from upstream when given a usable search-ref. + + Repository layout being checked (assumed already replayed) + + E---F local/master + / + C---D upstream/stable + / + A---B---G---H upstream/master + + New branch to be tracked will be upstream/master, so the resulting + commits found should just be E & F. + + Test that result is as follows + + E---F---I local/master + / / + C---D / upstream/stable + / / + / E1--F1 + / / + A---B---G---H upstream/master + + """ + + tree = [ + ('A', []), + ('B', ['A']), + ('C', ['B']), + ('D', ['C']), + ('E', ['D']), + ('F', ['E']), + ('G', ['B']), + ('H', ['G']) + ] + + branches = { + 'head': ('master', 'F'), + 'upstream': ('upstream/master', 'H'), + 'stable': ('upstream/stable', 'D') + } + + self._build_git_tree(tree, branches.values()) + self.git.tag(inspect.currentframe().f_code.co_name, 'upstream/master') + args = self.parser.parse_args(['-q', 'import']) + self.assertThat(args.func(args), Equals(True), + "import command failed to complete succesfully") + changes = list(Commit.iter_items( + self.repo, 'upstream/master..master^2')) + self.assertThat(len(changes), Equals(2), + "should only have seen two changes, got: %s" % + ", ".join(["%s:%s" % (commit.hexsha, + commit.message.splitlines()[0]) + for commit in changes])) + for commit, node in zip(changes, ['F', 'E']): + subject = commit.message.splitlines()[0] + node_subject = self._graph[node].message.splitlines()[0] + self.assertThat(subject, Equals(node_subject), + "subject '%s' of commit '%s' does not match " + "subject '%s' of node '%s'" % ( + subject, commit.hexsha, node_subject, node)) + + def test_import_switch_branches_fails_without_search_ref(self): + """Test that the import sub-command finds additional changes when + not given a search-ref to look under. + + Repository layout being checked (assumed already replayed) + + E---F local/master + / + C---D upstream/stable + / + A---B---G---H upstream/master + + New branch to be tracked will be upstream/master, so the resulting + commits found will be C, D, E & F because of not telling the searcher + to look under all of the namespace. + + Test that result is as follows + + E---F-------------I local/master + / / + C---D / upstream/stable + / / + / C1---D1---E1--F1 + / / + A---B---G---H upstream/master + + """ + + tree = [ + ('A', []), + ('B', ['A']), + ('C', ['B']), + ('D', ['C']), + ('E', ['D']), + ('F', ['E']), + ('G', ['B']), + ('H', ['G']) + ] + + # use 'custom/*' to ensure defaults are overriden correctly + branches = { + 'head': ('master', 'F'), + 'upstream': ('custom/master', 'H'), + 'stable': ('custom/stable', 'D') + } + + self._build_git_tree(tree, branches.values()) + + self.git.tag(inspect.currentframe().f_code.co_name, 'custom/master') + args = self.parser.parse_args(['-q', 'import', + '--into=master', 'custom/master']) + self.assertThat(args.func(args), Equals(True), + "import command failed to complete succesfully") + changes = list(Commit.iter_items( + self.repo, 'custom/master..master^2')) + local_rebased = ['F', 'E', 'D', 'C'] + self.assertThat(len(changes), Equals(len(local_rebased)), + "should only have seen two changes, got: %s" % + ", ".join(["%s:%s" % (commit.hexsha, + commit.message.splitlines()[0]) + for commit in changes])) + for commit, node in zip(changes, local_rebased): + subject = commit.message.splitlines()[0] + node_subject = self._graph[node].message.splitlines()[0] + self.assertThat(subject, Equals(node_subject), + "subject '%s' of commit '%s' does not match " + "subject '%s' of node '%s'" % ( + subject, commit.hexsha, node_subject, node)) + + def test_import_switch_branches_search_ref_custom_namespace(self): + """Test that the import sub-command can correctly switch branches when + importing from upstream when given a usable search-ref. + + Repository layout being checked (assumed already replayed) + + E---F local/master + / + C---D upstream/stable + / + A---B---G---H upstream/master + + New branch to be tracked will be upstream/master, so the resulting + commits found should just be E & F. + + Test that result is as follows + + E---F---I local/master + / / + C---D / upstream/stable + / / + / E1--F1 + / / + A---B---G---H upstream/master + + """ + + tree = [ + ('A', []), + ('B', ['A']), + ('C', ['B']), + ('D', ['C']), + ('E', ['D']), + ('F', ['E']), + ('G', ['B']), + ('H', ['G']) + ] + + # use 'custom/*' to ensure defaults are overriden correctly + branches = { + 'head': ('master', 'F'), + 'upstream': ('custom/master', 'H'), + 'stable': ('custom/stable', 'D') + } + + self._build_git_tree(tree, branches.values()) + self.git.tag(inspect.currentframe().f_code.co_name, 'custom/master') + args = self.parser.parse_args(['-q', 'import', + '--search-refs=custom/*', + '--search-refs=custom-d/*', + '--into=master', 'custom/master']) + self.assertThat(args.func(args), Equals(True), + "import command failed to complete succesfully") + changes = list(Commit.iter_items( + self.repo, 'custom/master..master^2')) + self.assertThat(len(changes), Equals(2), + "should only have seen two changes, got: %s" % + ", ".join(["%s:%s" % (commit.hexsha, + commit.message.splitlines()[0]) + for commit in changes])) + for commit, node in zip(changes, ['F', 'E']): + subject = commit.message.splitlines()[0] + node_subject = self._graph[node].message.splitlines()[0] + self.assertThat(subject, Equals(node_subject), + "subject '%s' of commit '%s' does not match " + "subject '%s' of node '%s'" % ( + subject, commit.hexsha, node_subject, node)) diff --git a/git_upstream/tests/test_searchers.py b/git_upstream/tests/test_searchers.py index 36f3907..c8fca70 100644 --- a/git_upstream/tests/test_searchers.py +++ b/git_upstream/tests/test_searchers.py @@ -23,8 +23,10 @@ class TestUpstreamMergeBaseSearcher(BaseTestCase): def _verify_expected(self, tree, branches, expected_nodes, pattern=None): self._build_git_tree(tree, branches.values()) - searcher = UpstreamMergeBaseSearcher( - pattern=pattern or branches['upstream'][0], repo=self.repo) + if pattern: + pattern = [pattern] + searcher = UpstreamMergeBaseSearcher(branch=branches['head'][0], + patterns=pattern, repo=self.repo) self.assertEquals(self._commits_from_nodes(reversed(expected_nodes)), searcher.list()) @@ -312,4 +314,5 @@ class TestUpstreamMergeBaseSearcher(BaseTestCase): } expected_changes = ['E', 'I', 'B1', 'J'] - self._verify_expected(tree, branches, expected_changes) + self._verify_expected(tree, branches, expected_changes, + pattern='upstream/master') diff --git a/git_upstream/tests/test_strategies.py b/git_upstream/tests/test_strategies.py index 3d49d42..e47971d 100644 --- a/git_upstream/tests/test_strategies.py +++ b/git_upstream/tests/test_strategies.py @@ -28,7 +28,7 @@ class TestStrategies(BaseTestCase): self._build_git_tree(tree, branches.values()) strategy = LocateChangesWalk(branch=branches['head'][0], - search_ref=branches['upstream'][0]) + search_refs=[branches['upstream'][0]]) self.assertEquals(self._commits_from_nodes(expected_nodes), [c for c in strategy.filtered_iter()])