Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make ignored_files support re #33

Closed
wants to merge 4 commits into from
Closed

make ignored_files support re #33

wants to merge 4 commits into from

Conversation

chinese-wzq
Copy link

No description provided.

@vercel Vercel
Copy link

vercel bot commented Jun 9, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
prime-backup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 12, 2024 5:41am

This was referenced Jun 9, 2024
Closed
Closed
Fallen-Breath
Fallen-Breath requested changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@Fallen-Breath Fallen-Breath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

该 PR 的修改破坏了存量部署的配置后向兼容性，请调整实现方式

除此之外，该实现未对文件/文件夹的嵌套关系进行处理，可能会造成意料外的非完整数据备份，这也是需要考虑的

@chinese-wzq
Copy link
Author

chinese-wzq commented Jun 10, 2024 •
edited
Loading

该 PR 的修改破坏了存量部署的配置后向兼容性，请调整实现方式

除此之外，该实现未对文件/文件夹的嵌套关系进行处理，可能会造成意料外的非完整数据备份，这也是需要考虑的

第二点可以具体举例说说嘛？什么情况下无法用正则表达式匹配？

@chinese-wzq chinese-wzq changed the title make ignored_files supports re make ignored_files support re Jun 10, 2024
@Fallen-Breath
Copy link
Contributor

第二点可以具体举例说说嘛

建议自行阅读相关代码实现

什么情况下无法用正则表达式匹配？

该观点我未曾提出过

@chinese-wzq
Copy link
Author

chinese-wzq commented Jun 10, 2024 •
edited
Loading

第一点可以考虑将原始的ignored_files保留，新增一个配置项ignored_files_re（考虑到*在re中是有特殊含义的），不过新增的配置项能否自动添加到用户config.json中呢？
第二点我实在没有理解你的意思，麻烦讲得再详细一点

@Fallen-Breath
Copy link
Contributor

自己看一下相关代码是怎么使用这个函数的吧

理解清楚修改的代码的用处功能，是交 PR 前必须的操作呀，不能连相关改动的代码是干什么的都不知道，就随便改下交 PR

@chinese-wzq
Copy link
Author

chinese-wzq commented Jul 12, 2024 •
edited
Loading

自己看一下相关代码是怎么使用这个函数的吧

理解清楚修改的代码的用处功能，是交 PR 前必须的操作呀，不能连相关改动的代码是干什么的都不知道，就随便改下交 PR

我现在仔细看了一下__scan_files，好像改动前后没有逻辑上没有什么差别呀，能指出来吗

Update: 'ignored_files' no longer supports regex.
4c22bcd 
Added 'ignored_files_regex' to support regex patterns.
@chinese-wzq
Copy link
Author

chinese-wzq commented Jul 12, 2024 •
edited
Loading

你的第一个问题现在解决了，麻烦看一下

Fallen-Breath
Fallen-Breath reviewed Jul 12, 2024
View reviewed changes
prime_backup/config/backup_config.py
'session.lock',
ignored_files: List[str] = []
ignored_files_regex: List[str] = [
'server/session.lock',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

错误配置的默认值

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是等效的

Copy link
Contributor

@Fallen-Breath Fallen-Breath Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不等效。多了解下相关逻辑吧，这里有 4 个不同种类错误，不清楚就别改了

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 正则表达式里未转义 .
  2. 路径分隔符硬编码为 posix 下的实现，不兼容 windows
  3. 假定了 MCDR 配置的服务端路径为 server
  4. session.lock 的路径错误。其应当位于存档文件夹中

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 正则表达式里未转义 .

    1. 路径分隔符硬编码为 posix 下的实现，不兼容 windows

    2. 假定了 MCDR 配置的服务端路径为 server

    3. session.lock 的路径错误。其应当位于存档文件夹中

好的

Fallen-Breath
Fallen-Breath reviewed Jul 12, 2024
View reviewed changes
docs/config.md
A list of file names to be ignored during backup. It contains `session.lock` by default
to solve the backup failure problem caused by `session.lock` being occupied by the server in Windows
The list of filenames to ignore during backup, does not support regex matching.
It is recommended to use `ignored_files_regex` to support more accurate and specific matching methods.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignored_files_regex 不是 ignored_files 的完全替代

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignored_files_regex 不是 ignored_files 的完全替代

那么将描述修改为“更强大的匹配”？

Fallen-Breath
Fallen-Breath reviewed Jul 12, 2024
View reviewed changes
prime_backup/config/backup_config.py
@@ -44,4 +45,7 @@ def is_file_ignore(self, full_path: Path) -> bool:
return True
if name == item:
return True
for item in self.ignored_files_regex:
if re.match(item, str(full_path)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议用 Path.as_posix

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的

@Fallen-Breath
Copy link
Contributor

我现在仔细看了一下__scan_files，好像改动前后没有逻辑上没有什么差别呀，能指出来吗

改动前后没有差别，复用了仅支持文件排除的 ignored_files 的逻辑就是问题所在

@Fallen-Breath
Copy link
Contributor

我之后自己实现吧

@chinese-wzq
Copy link
Author

chinese-wzq commented Jul 12, 2024 •
edited
Loading

我现在仔细看了一下__scan_files，好像改动前后没有逻辑上没有什么差别呀，能指出来吗

改动前后没有差别，复用了仅支持文件排除的 ignored_files 的逻辑就是问题所在

if not target_path.is_symlink() and target_path.is_dir():
	for dir_path, dir_names, file_names in os.walk(target_path):
		for name in file_names + dir_names:
			file_path = Path(dir_path) / name
			if not self.config.backup.is_file_ignore(file_path):
				collected.append(file_path)

你原本的代码的逻辑就是同时用ignored_files来对所有的文件夹和文件名进行匹配啊，并不是“仅支持文件排除”

Fallen-Breath added a commit that referenced this pull request Jul 17, 2024
@Fallen-Breath
Added gitignore-flavor ignore patterns support; Refactored file scann…
be5e21a 
…ing logic during backup creation,

resolved #31, closed #33
see also #32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fallen-Breath Fallen-Breath Fallen-Breath requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chinese-wzq @Fallen-Breath