From 505b69fd8b0c2150370a0c1740e4188514e9b271 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Sun, 3 May 2020 18:00:12 +0800 Subject: [PATCH 1/9] fixed #3599 1. add new command `dvc remote rename `. 2. add tests for this new command. --- dvc/command/remote.py | 39 +++++++++++++++++++++ tests/func/test_remote.py | 71 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index c5f46a1a08..4afc66776a 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -108,6 +108,34 @@ def run(self): return 0 +class CmdRemoteRename(CmdRemote): + def run(self): + conf = self.config.load_one(self.args.level) + self._check_exists(conf) + + all_config = self.config.load_config_to_level("all") + if self.args.new in all_config.get("remote", {}): + raise ConfigError( + "Rename failed.Remote name {} already exists.".format( + {self.args.new} + ) + ) + + with self.config.edit(self.args.level) as conf: + conf["remote"][self.args.new] = conf["remote"][self.args.name] + del conf["remote"][self.args.name] + + for level in reversed(self.config.LEVELS): + with self.config.edit(level) as conf: + if conf["core"].get("remote") == self.args.name: + conf["core"]["remote"] = self.args.new + + if level == self.args.level: + break + + return 0 + + def add_parser(subparsers, parent_parser): from dvc.command.config import parent_config_parser @@ -224,3 +252,14 @@ def add_parser(subparsers, parent_parser): "name", help="Name of the remote to remove." ) remote_remove_parser.set_defaults(func=CmdRemoteRemove) + REMOTE_RENAME_HELP = "Rename a data remote" + remote_rename_parser = remote_subparsers.add_parser( + "rename", + parents=[parent_config_parser, parent_parser], + description=append_doc_link(REMOTE_RENAME_HELP, "remote/rename"), + help=REMOTE_RENAME_HELP, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + remote_rename_parser.add_argument("name", help="Remote to be renamed") + remote_rename_parser.add_argument("new", help="New name of the remote") + remote_rename_parser.set_defaults(func=CmdRemoteRename) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 8e349efb98..f447cb6942 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -285,3 +285,74 @@ def test_remote_modify_default(dvc): assert repo_config["core"]["remote"] == remote_repo assert local_config["core"]["remote"] == remote_local + + +def test_remote_rename(dvc): + remote_name = "drive" + remote_url = "gdrive://test/test" + new_name = "new" + other_name = "other" + # prepare + assert main(["remote", "add", remote_name, remote_url]) == 0 + config = dvc.config.load_one("repo") + assert config["remote"][remote_name]["url"] == remote_url + assert new_name not in config.get("remote", {}) + + # rename failed + assert main(["remote", "rename", remote_name]) == 254 + assert main(["remote", "rename", new_name, other_name]) == 251 + config = dvc.config.load_one("repo") + assert config["remote"][remote_name]["url"] == remote_url + assert new_name not in config.get("remote", {}) + + # rename success + assert main(["remote", "rename", remote_name, new_name]) == 0 + config = dvc.config.load_one("repo") + assert remote_name not in config.get("remote", {}) + assert config["remote"][new_name]["url"] == remote_url + + +def test_remote_duplicated(dvc): + remote_name = "drive" + remote_url = "gdrive://test/test" + used_name = "overlap" + another_url = "gdrive://test/test1" + # prepare + assert main(["remote", "add", remote_name, remote_url]) == 0 + assert main(["remote", "add", "--local", used_name, another_url]) == 0 + config = dvc.config.load_one("repo") + assert config["remote"][remote_name]["url"] == remote_url + local_config = dvc.config.load_one("local") + assert local_config["remote"][used_name]["url"] == another_url + + # rename duplicated + assert main(["remote", "rename", remote_name, used_name]) == 251 + config = dvc.config.load_one("repo") + assert config["remote"][remote_name]["url"] == remote_url + local_config = dvc.config.load_one("local") + assert local_config["remote"][used_name]["url"] == another_url + + +def test_remote_default(dvc): + remote_name = "drive" + remote_url = "gdrive://test/test" + new_name = "new" + # prepare + assert main(["remote", "add", "-d", remote_name, remote_url]) == 0 + assert main(["remote", "default", "--local", remote_name]) == 0 + config = dvc.config.load_one("repo") + assert config["core"]["remote"] == remote_name + assert config["remote"][remote_name]["url"] == remote_url + assert new_name not in config.get("remote", {}) + local_config = dvc.config.load_one("local") + assert local_config["core"]["remote"] == remote_name + + # rename success + assert main(["remote", "rename", remote_name, new_name]) == 0 + config = dvc.config.load_one("repo") + assert remote_name not in config.get("remote", {}) + assert config["core"]["remote"] == new_name + assert config["remote"][new_name]["url"] == remote_url + assert remote_name not in config.get("remote", {}) + local_config = dvc.config.load_one("local") + assert local_config["core"]["remote"] == new_name From 200a2f34cf2b6e2ecbec745e2c4f18100e878b86 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Tue, 19 May 2020 22:44:38 +0800 Subject: [PATCH 2/9] Update dvc/command/remote.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Peter Rowlands (변기호) --- dvc/command/remote.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index 4afc66776a..e5a68aec5d 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -116,7 +116,7 @@ def run(self): all_config = self.config.load_config_to_level("all") if self.args.new in all_config.get("remote", {}): raise ConfigError( - "Rename failed.Remote name {} already exists.".format( + "Rename failed. Remote name '{}' already exists.".format( {self.args.new} ) ) From 50abb10ad64d116cc8cd1e5ebfe518b9073e9e5e Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Tue, 19 May 2020 23:13:05 +0800 Subject: [PATCH 3/9] Change about code review --- dvc/command/remote.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index 4afc66776a..5864420454 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -110,10 +110,7 @@ def run(self): class CmdRemoteRename(CmdRemote): def run(self): - conf = self.config.load_one(self.args.level) - self._check_exists(conf) - - all_config = self.config.load_config_to_level("all") + all_config = self.config.load_config_to_level(None) if self.args.new in all_config.get("remote", {}): raise ConfigError( "Rename failed.Remote name {} already exists.".format( @@ -122,17 +119,19 @@ def run(self): ) with self.config.edit(self.args.level) as conf: + self._check_exists(conf) conf["remote"][self.args.new] = conf["remote"][self.args.name] del conf["remote"][self.args.name] + if conf["core"].get("remote") == self.args.name: + conf["core"]["remote"] = self.args.new for level in reversed(self.config.LEVELS): + if level == self.args.level: + break with self.config.edit(level) as conf: if conf["core"].get("remote") == self.args.name: conf["core"]["remote"] = self.args.new - if level == self.args.level: - break - return 0 From bf1a27a442033d0a7849537b001d06a2209a360d Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Tue, 19 May 2020 23:28:08 +0800 Subject: [PATCH 4/9] For deepsource --- dvc/command/remote.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/dvc/command/remote.py b/dvc/command/remote.py index 04c11926f5..b4e9052376 100644 --- a/dvc/command/remote.py +++ b/dvc/command/remote.py @@ -109,6 +109,10 @@ def run(self): class CmdRemoteRename(CmdRemote): + def _rename_default(self, conf): + if conf["core"].get("remote") == self.args.name: + conf["core"]["remote"] = self.args.new + def run(self): all_config = self.config.load_config_to_level(None) if self.args.new in all_config.get("remote", {}): @@ -122,15 +126,13 @@ def run(self): self._check_exists(conf) conf["remote"][self.args.new] = conf["remote"][self.args.name] del conf["remote"][self.args.name] - if conf["core"].get("remote") == self.args.name: - conf["core"]["remote"] = self.args.new + self._rename_default(conf) for level in reversed(self.config.LEVELS): if level == self.args.level: break - with self.config.edit(level) as conf: - if conf["core"].get("remote") == self.args.name: - conf["core"]["remote"] = self.args.new + with self.config.edit(level) as level_conf: + self._rename_default(level_conf) return 0 From 12eb8097c55abdb6babdc698f416b947ee68f65c Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Sun, 7 Jun 2020 20:59:43 +0800 Subject: [PATCH 5/9] Performance improvement of dvcignore fix#3869 1.Use big regex. --- dvc/ignore.py | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/dvc/ignore.py b/dvc/ignore.py index 481327e827..ba738d7868 100644 --- a/dvc/ignore.py +++ b/dvc/ignore.py @@ -1,8 +1,8 @@ import logging import os +import re from funcy import cached_property -from pathspec import PathSpec from pathspec.patterns import GitWildMatchPattern from dvc.scm.tree import BaseTree @@ -26,7 +26,22 @@ def __init__(self, ignore_file_path, tree): self.dirname = os.path.normpath(os.path.dirname(ignore_file_path)) with tree.open(ignore_file_path, encoding="utf-8") as fobj: - self.ignore_spec = PathSpec.from_lines(GitWildMatchPattern, fobj) + path_spec_lines = fobj.readlines() + regex_pattern_list = list( + map(GitWildMatchPattern.pattern_to_regex, path_spec_lines) + ) + ignore_pattern_list = [i for i, j in regex_pattern_list if j is True] + include_pattern_list = [i for i, j in regex_pattern_list if j is False] + self.ignore_spec = ( + re.compile("|".join(ignore_pattern_list)) + if ignore_pattern_list + else None + ) + self.include_spec = ( + re.compile("|".join(include_pattern_list)) + if include_pattern_list + else None + ) def __call__(self, root, dirs, files): files = [f for f in files if not self.matches(root, f)] @@ -40,7 +55,17 @@ def matches(self, dirname, basename): if os.pardir + os.sep in rel_path: return False - return self.ignore_spec.match_file(rel_path) + return self.ignore(rel_path) and not self.include(rel_path) + + def ignore(self, path): + if self.ignore_spec and self.ignore_spec.match(path): + return True + return False + + def include(self, path): + if self.include_spec and self.include_spec.match(path): + return True + return False def __hash__(self): return hash(self.ignore_file_path) From cf57eceb77032cde505ec7cc60b04c93d6f0cc30 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Thu, 11 Jun 2020 22:34:57 +0800 Subject: [PATCH 6/9] Solve windows --- dvc/ignore.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dvc/ignore.py b/dvc/ignore.py index 09a22ff9bb..562a73edcd 100644 --- a/dvc/ignore.py +++ b/dvc/ignore.py @@ -4,9 +4,11 @@ from funcy import cached_property from pathspec.patterns import GitWildMatchPattern +from pathspec.util import normalize_file from dvc.path_info import PathInfo from dvc.scm.tree import BaseTree +from dvc.system import System from dvc.utils import relpath logger = logging.getLogger(__name__) @@ -63,6 +65,8 @@ def matches(self, dirname, basename): else: return False + if not System.is_unix(): + path = normalize_file(path) return self.ignore(path) and not self.include(path) def ignore(self, path): From 285f01a4a358d529170a3a95b2e145d20b8becdf Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Sat, 13 Jun 2020 18:30:25 +0800 Subject: [PATCH 7/9] add rule order test --- tests/unit/test_ignore.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/unit/test_ignore.py b/tests/unit/test_ignore.py index 1f7c4d8fc3..40f936d091 100644 --- a/tests/unit/test_ignore.py +++ b/tests/unit/test_ignore.py @@ -99,3 +99,18 @@ def test_should_ignore_dir(omit_dir): new_dirs, _ = ignore(root, dirs, files) assert set(new_dirs) == {"dir1", "dir2"} + + +def test_ignore_order(): + dvcignore_path = os.path.join(os.path.sep, "ignore_order", ".dvcignore") + + patterns = ["!ac*", "a*", "!ab*"] + + root = os.path.dirname(dvcignore_path) + dirs = ["ignore_order"] + files = ["ac", "ab", "aa"] + + ignore = mock_dvcignore(dvcignore_path, patterns) + _, new_files = ignore(root, dirs, files) + + assert {"ab"} == set(new_files) From d32b4035be86906578bbae58247c120975af748c Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Sat, 13 Jun 2020 18:57:39 +0800 Subject: [PATCH 8/9] Solve ignore order --- dvc/ignore.py | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/dvc/ignore.py b/dvc/ignore.py index 562a73edcd..41d195c9cc 100644 --- a/dvc/ignore.py +++ b/dvc/ignore.py @@ -1,6 +1,7 @@ import logging import os import re +from itertools import groupby from funcy import cached_property from pathspec.patterns import GitWildMatchPattern @@ -30,21 +31,15 @@ def __init__(self, ignore_file_path, tree): with tree.open(ignore_file_path, encoding="utf-8") as fobj: path_spec_lines = fobj.readlines() - regex_pattern_list = list( - map(GitWildMatchPattern.pattern_to_regex, path_spec_lines) + regex_pattern_list = map( + GitWildMatchPattern.pattern_to_regex, path_spec_lines ) - ignore_pattern_list = [i for i, j in regex_pattern_list if j is True] - include_pattern_list = [i for i, j in regex_pattern_list if j is False] - self.ignore_spec = ( - re.compile("|".join(ignore_pattern_list)) - if ignore_pattern_list - else None - ) - self.include_spec = ( - re.compile("|".join(include_pattern_list)) - if include_pattern_list - else None - ) + self.ignore_spec = [ + (ignore, re.compile("|".join([item[0] for item in group]))) + for ignore, group in groupby( + regex_pattern_list, lambda x: x[1] + ) + ] def __call__(self, root, dirs, files): files = [f for f in files if not self.matches(root, f)] @@ -67,17 +62,14 @@ def matches(self, dirname, basename): if not System.is_unix(): path = normalize_file(path) - return self.ignore(path) and not self.include(path) + return self.ignore(path) def ignore(self, path): - if self.ignore_spec and self.ignore_spec.match(path): - return True - return False - - def include(self, path): - if self.include_spec and self.include_spec.match(path): - return True - return False + result = False + for ignore, pattern in self.ignore_spec: + if pattern.match(path): + result = ignore + return result def __hash__(self): return hash(self.ignore_file_path) From a86cedbf39919923d54b0754b64c412433a8f5b1 Mon Sep 17 00:00:00 2001 From: karajan1001 Date: Sat, 13 Jun 2020 19:55:50 +0800 Subject: [PATCH 9/9] remove list comprehensions --- dvc/ignore.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/ignore.py b/dvc/ignore.py index 41d195c9cc..53436e226f 100644 --- a/dvc/ignore.py +++ b/dvc/ignore.py @@ -35,7 +35,7 @@ def __init__(self, ignore_file_path, tree): GitWildMatchPattern.pattern_to_regex, path_spec_lines ) self.ignore_spec = [ - (ignore, re.compile("|".join([item[0] for item in group]))) + (ignore, re.compile("|".join(item[0] for item in group))) for ignore, group in groupby( regex_pattern_list, lambda x: x[1] )