borgmatic-collective/borgmatic
9
73
Fork 51
Code Issues 52 Pull Requests 3 Actions 1 Packages Releases 158 Activity

Fix patterns parsing #976

Merged
witten merged 2 commits from pasha132/borgmatic:fix-exclude-parser into main 2025-01-23 20:06:12 +00:00
Conversation 4 Commits 2 Files Changed 2 +28 -23
pasha132 commented 2025-01-23 16:39:17 +00:00
Contributor
Copy Link

Hello!

It seems that after patterns refactoring original borg patterns stopped to work: they always passed with "fn:" style prefix.

My variant of fix is in this PR.

Hello! It seems that after patterns refactoring original borg patterns stopped to work: they always passed with "fn:" style prefix. My variant of fix is in this PR.
pasha132 added 1 commit 2025-01-23 16:39:18 +00:00
Fix patterns parsing 5d390d7953
pasha132 reviewed 2025-01-23 16:42:12 +00:00
tests/unit/actions/test_create.py Outdated
@@ -88,11 +89,6 @@ def test_collect_patterns_reads_config_exclude_from_file():
builtins.should_receive('open').with_args('file2.txt').and_return(
io.StringIO('/bar\n# comment\n\n \n/baz')
)
flexmock(module).should_receive('parse_pattern').with_args('/bar').and_return(Pattern('/bar'))
pasha132 commented 2025-01-23 16:40:36 +00:00
Author
Contributor
Copy Link

It seems that this is dead code. Anyway it's nice to have real parse_pattern function tested here to.

It seems that this is dead code. Anyway it's nice to have real `parse_pattern` function tested here to.
witten commented 2025-01-23 18:33:30 +00:00
Owner
Copy Link

I don't think it's dead...? Although maybe I'm missing something.

In any case, unit tests in this codebase generally only test a single unit (function) at a time and don't integrate multiple units under test. So parse_pattern() is intended to be mocked out here. Any tests integrating multiple units can generally be found under tests/integration or tests/end-to-end.

I don't think it's dead...? Although maybe I'm missing something. In any case, unit tests in this codebase generally only test a single unit (function) at a time and don't integrate multiple units under test. So `parse_pattern()` is intended to be mocked out here. Any tests integrating multiple units can generally be found under `tests/integration` or `tests/end-to-end`.
pasha132 commented 2025-01-23 20:54:31 +00:00
Author
Contributor
Copy Link

Sorry, my mistake: never used flexmock before. Just saw that without this code test didn't fail.

Sorry, my mistake: never used flexmock before. Just saw that without this code test didn't fail.
witten commented 2025-01-23 21:38:17 +00:00
Owner
Copy Link

That's understandable!

That's understandable!
pasha132 marked this conversation as resolved
witten requested changes 2025-01-23 18:34:29 +00:00
witten left a comment
Owner
Copy Link

The solution here looks good to me! I just left a few comments about expanding scope a bit and also one of the tests. Thank you!

The solution here looks good to me! I just left a few comments about expanding scope a bit and also one of the tests. Thank you!
borgmatic/actions/create.py
@@ -75,11 +73,8 @@ def collect_patterns(config):
if pattern_line.strip()
witten commented 2025-01-23 18:26:22 +00:00
Owner
Copy Link

Since you now have the handy default_style parameter, what do you think of adding default_sytle=borgmatic.borg.pattern.Pattern_style.SHELL for the patterns and patterns_from options here? Based on these docs, that'd be more correct than the existing behavior prior to your changes.

Since you now have the handy `default_style` parameter, what do you think of adding `default_sytle=borgmatic.borg.pattern.Pattern_style.SHELL` for the `patterns` and `patterns_from` options here? Based on [these docs](https://borgbackup.readthedocs.io/en/stable/usage/help.html#borg-help-patterns), that'd be more correct than the existing behavior prior to your changes.
witten commented 2025-01-25 16:19:18 +00:00
Owner
Copy Link

Turns out this was a terrible idea.. My bad for suggesting it. The problem was that tacking on "sh:" unconditionally breaks root patterns in particular (e.g. R /foo/bar), because they don't have pattern style prefixes.

borgmatic 1.9.8 fixes that.

Turns out this was [a terrible idea](https://projects.torsion.org/borgmatic-collective/borgmatic/issues/979).. My bad for suggesting it. The problem was that tacking on "sh:" unconditionally breaks root patterns in particular (e.g. `R /foo/bar`), because they don't have pattern style prefixes. borgmatic 1.9.8 fixes that.
witten reviewed 2025-01-23 19:01:58 +00:00
tests/unit/actions/test_create.py Outdated
@@ -88,11 +89,6 @@ def test_collect_patterns_reads_config_exclude_from_file():
builtins.should_receive('open').with_args('file2.txt').and_return(
io.StringIO('/bar\n# comment\n\n \n/baz')
witten commented 2025-01-23 19:01:58 +00:00
Owner
Copy Link

Also: Do you think this test could benefit from a sh: pattern added here like you did for the test above?

Also: Do you think this test could benefit from a `sh:` pattern added here like you did for the test above?
pasha132 added 1 commit 2025-01-23 19:49:20 +00:00
Fix according to review comments 73c196aa70
witten commented 2025-01-23 20:05:01 +00:00
Owner
Copy Link

Looks great! Thank you.

Looks great! Thank you.
witten merged commit 92b8c0230e into main 2025-01-23 20:06:12 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
pasha132
witten
Labels
Clear labels
blocked
breaking
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
witten
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#976
Delete Branch "pasha132/borgmatic:fix-exclude-parser"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?