borgmatic-collective/borgmatic
borgmatic-collective/borgmatic
9
74
Fork 55
Code Issues 52 Pull requests 4 Releases 159 Packages 1 Activity Actions 1

Closes #1175 #1272

Merged
witten merged 6 commits from slantsh/borgmatic:flags into main 2026-02-28 07:19:55 +00:00
Conversation 11 Commits 6 Files changed 7 +82 -45
slantsh commented 2026-02-23 00:44:02 +00:00
Contributor
Copy link

I've added the two flags, "--files-changed=MODE" and "BORG_MSGPACK_VERSION_CHECK".
Also added a test for "--files-changed=MODE" flag in unit tests, seems to work on my end.

I've added the two flags, "--files-changed=MODE" and "BORG_MSGPACK_VERSION_CHECK". Also added a test for "--files-changed=MODE" flag in unit tests, seems to work on my end.
added BORG_MSGPACK_VERSION_CHECK, no test
All checks were successful
build / test (pull_request) Successful in 7m42s
Details
build / docs (pull_request) Has been skipped
Details
7dcb4e3c3a
witten reviewed 2026-02-24 19:22:00 +00:00
witten left a comment
Owner
Copy link

Thanks for taking his on!

Thanks for taking his on!
borgmatic/borg/environment.py Outdated
@ -13,6 +13,7 @@ OPTION_TO_ENVIRONMENT_VARIABLE = {
'borg_key_file': 'BORG_KEY_FILE',
'ssh_command': 'BORG_RSH',
'temporary_directory': 'TMPDIR',
'msgpack_version_check': 'BORG_MSGPACK_VERSION_CHECK',
witten commented 2026-02-24 18:56:54 +00:00
Owner
Copy link

I think this should actually be in DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE instead of OPTION_TO_ENVIRONMENT_VARIABLE. That way, the True/False value (see comments below) will get transformed into the appropriate string representation (YES/NO) for passing to Borg.

I think this should actually be in `DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE` instead of `OPTION_TO_ENVIRONMENT_VARIABLE`. That way, the `True`/`False` value (see comments below) will get transformed into the appropriate string representation (`YES`/`NO`) for passing to Borg.
borgmatic/config/schema.yaml
@ -167,2 +167,4 @@
Defaults to true.
example: false
files_changed:
type: string
witten commented 2026-02-24 18:39:18 +00:00
Owner
Copy link

If there are really only three valid values, you could make this an enum string. See elsewhere in this file for examples of that.

Do not feel strongly.

If there are really only three valid values, you could make this an `enum` string. See elsewhere in this file for examples of that. Do not feel strongly.
slantsh marked this conversation as resolved
borgmatic/config/schema.yaml
@ -169,0 +171,4 @@
description: |
Threshold for considering a file as changed. See
https://borgbackup.readthedocs.io/en/stable/usage/create.html for
details. Defaults to "mtime" (i.e., a file is considered
witten commented 2026-02-24 18:33:35 +00:00
Owner
Copy link

Based on those docs, it looks like the default is actually ctime?

Based on those docs, it looks like the default is actually `ctime`?
slantsh marked this conversation as resolved
borgmatic/config/schema.yaml Outdated
@ -169,0 +173,4 @@
https://borgbackup.readthedocs.io/en/stable/usage/create.html for
details. Defaults to "mtime" (i.e., a file is considered
changed if its mtime has changed since the last backup).
example: mtime,size
witten commented 2026-02-24 18:37:54 +00:00
Owner
Copy link

If I'm reading the docs correctly, mtime is a valid value but mtime,size isn't.

If I'm reading the docs correctly, `mtime` is a valid value but `mtime,size` isn't.
slantsh marked this conversation as resolved
borgmatic/config/schema.yaml
@ -563,2 +571,4 @@
false and an interactive prompt from Borg.
example: true
msgpack_version_check:
type: string
witten commented 2026-02-24 19:08:31 +00:00
Owner
Copy link

IMO this should be a boolean rather than a string. See relocated_repo_access_is_ok for an example of another environment variable option that works this way.

IMO this should be a `boolean` rather than a `string`. See `relocated_repo_access_is_ok` for an example of another environment variable option that works this way.
witten marked this conversation as resolved
tests/unit/borg/test_create.py
@ -482,6 +482,7 @@ def test_make_base_create_command_with_store_config_false_omits_config_files():
('flags', True, False, ()),
('flags', False, True, ('--noflags',)),
('flags', False, False, ('--nobsdflags',)),
('files_changed', 'mtime', True, ('--files-changed', 'mtime')),
witten commented 2026-02-24 19:21:23 +00:00
Owner
Copy link

Nice! Glad to see this was the only testing change required for this.

Nice! Glad to see this was the only testing change required for this.
slantsh marked this conversation as resolved
slantsh commented 2026-02-24 21:18:51 +00:00
Author
Contributor
Copy link

Oh, my bad, I've corrected the documentation and changed it to an enum string (According to the docs it should be just those 3 values? That is ctime, mtime or disabled.)

The new problem I've been facing is that test_environment.py does fail and it seems that just adding BORG_MSGPACK_VERSION_CHECK is not enough.

Can we set BORG_MSGPACK_VERSION_CHECK 's default value to be True?
Right now I think every option in DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE is set to False by default if not specified, but I'm not sure..

    for (
        option_name,
        environment_variable_name,
    ) in DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE.items():
        if os.environ.get(environment_variable_name) is None:
            value = config.get(option_name)
            environment[environment_variable_name] = 'YES' if value else 'NO'

This part in environment.py.

Oh, my bad, I've corrected the documentation and changed it to an enum string (According to the docs it should be just those 3 values? That is ctime, mtime or disabled.) The new problem I've been facing is that `test_environment.py` does fail and it seems that just adding `BORG_MSGPACK_VERSION_CHECK` is not enough. Can we set `BORG_MSGPACK_VERSION_CHECK` 's default value to be True? Right now I think every option in `DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE` is set to False by default if not specified, but I'm not sure.. ```py for ( option_name, environment_variable_name, ) in DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE.items(): if os.environ.get(environment_variable_name) is None: value = config.get(option_name) environment[environment_variable_name] = 'YES' if value else 'NO' ``` This part in `environment.py`.
slantsh changed title from Closes #1175 to WIP: Closes #1175 2026-02-24 21:28:54 +00:00
witten commented 2026-02-24 21:54:05 +00:00
Owner
Copy link

Oh, my bad, I've corrected the documentation and changed it to an enum string (According to the docs it should be just those 3 values? That is ctime, mtime or disabled.)

Yup!

The new problem I've been facing is that test_environment.py does fail and it seems that just adding BORG_MSGPACK_VERSION_CHECK is not enough.

Can we set BORG_MSGPACK_VERSION_CHECK 's default value to be True?
Right now I think every option in DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE is set to False by default if not specified, but I'm not sure..

You are correct that this is the first example of a default-YES environment variable.

    for (
        option_name,
        environment_variable_name,
    ) in DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE.items():
        if os.environ.get(environment_variable_name) is None:
            value = config.get(option_name)
            environment[environment_variable_name] = 'YES' if value else 'NO'

This part in environment.py.

So you might be able to add if value is not None: to this code right before the environment variable gets set, just like in the for loop right below this one. What that would do is only set the environment variable if it's actually configured in borgmatic's config file. And if it's not, then Borg will have to use its own defaults. Which, as you've established, can vary by environment variable.

> Oh, my bad, I've corrected the documentation and changed it to an enum string (According to the docs it should be just those 3 values? That is ctime, mtime or disabled.) Yup! > The new problem I've been facing is that `test_environment.py` does fail and it seems that just adding `BORG_MSGPACK_VERSION_CHECK` is not enough. > > Can we set `BORG_MSGPACK_VERSION_CHECK` 's default value to be True? > Right now I think every option in `DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE` is set to False by default if not specified, but I'm not sure.. You are correct that this is the first example of a default-`YES` environment variable. > ```py > for ( > option_name, > environment_variable_name, > ) in DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE.items(): > if os.environ.get(environment_variable_name) is None: > value = config.get(option_name) > environment[environment_variable_name] = 'YES' if value else 'NO' > ``` > This part in `environment.py`. So you might be able to add `if value is not None:` to this code right before the environment variable gets set, just like in the `for` loop right below this one. What that would do is only set the environment variable if it's actually configured in borgmatic's config file. And if it's not, then Borg will have to use its own defaults. Which, as you've established, can vary by environment variable.
Fixed documentation in schema
All checks were successful
build / test (pull_request) Successful in 7m40s
Details
build / docs (pull_request) Has been skipped
Details
ceb487a6b9
slantsh commented 2026-02-25 14:43:21 +00:00
Author
Contributor
Copy link
for (
        option_name,
        environment_variable_name,
    ) in DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE.items():
        if os.environ.get(environment_variable_name) is None:
            value = config.get(option_name)
            if value is not None:
                environment[environment_variable_name] = 'YES' if value else 'NO'

This is what the proposed fix looks like right?
However, the test still fails as such

====================================== FAILURES =======================================
___ test_make_environment_without_configuration_sets_certain_environment_variables ____

    def test_make_environment_without_configuration_sets_certain_environment_variables():
        flexmock(module.os).should_receive('environ').and_return({'USER': 'root'})
        flexmock(module.borgmatic.hooks.credential.parse).should_receive(
            'resolve_credential',
        ).and_return(None)
        flexmock(module.os).should_receive('pipe').never()
        environment = module.make_environment({})
    
        # Default environment variables.
>       assert environment == {
            'USER': 'root',
            'BORG_EXIT_CODES': 'modern',
            'BORG_RELOCATED_REPO_ACCESS_IS_OK': 'NO',
            'BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK': 'NO',
            'BORG_USE_CHUNKS_ARCHIVE': 'NO',
            'BORG_DEBUG_PASSPHRASE': 'NO',
            'BORG_DISPLAY_PASSPHRASE': 'NO',
            'BORG_MSGPACK_VERSION_CHECK': 'YES',
        }
E       AssertionError: assert {'BORG_EXIT_C...USER': 'root'} == {'BORG_DEBUG_...': 'YES', ...}
E         
E         Omitting 2 identical items, use -vv to show
E         Right contains 6 more items:
E         {'BORG_DEBUG_PASSPHRASE': 'NO',
E          'BORG_DISPLAY_PASSPHRASE': 'NO',
E          'BORG_MSGPACK_VERSION_CHECK': 'YES',
E          'BORG_RELOCATED_REPO_ACCESS_IS_OK
E         
E         ...Full output truncated (4 lines hidden), use '-vv' to show

tests/unit/borg/test_environment.py:90: AssertionError

The only fix that I've tried is to just have an if statement to handle msgpack_version_check and set it to True like that.

```py for ( option_name, environment_variable_name, ) in DEFAULT_BOOL_OPTION_TO_ENVIRONMENT_VARIABLE.items(): if os.environ.get(environment_variable_name) is None: value = config.get(option_name) if value is not None: environment[environment_variable_name] = 'YES' if value else 'NO' ``` This is what the proposed fix looks like right? However, the test still fails as such ```py ====================================== FAILURES ======================================= ___ test_make_environment_without_configuration_sets_certain_environment_variables ____ def test_make_environment_without_configuration_sets_certain_environment_variables(): flexmock(module.os).should_receive('environ').and_return({'USER': 'root'}) flexmock(module.borgmatic.hooks.credential.parse).should_receive( 'resolve_credential', ).and_return(None) flexmock(module.os).should_receive('pipe').never() environment = module.make_environment({}) # Default environment variables. > assert environment == { 'USER': 'root', 'BORG_EXIT_CODES': 'modern', 'BORG_RELOCATED_REPO_ACCESS_IS_OK': 'NO', 'BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK': 'NO', 'BORG_USE_CHUNKS_ARCHIVE': 'NO', 'BORG_DEBUG_PASSPHRASE': 'NO', 'BORG_DISPLAY_PASSPHRASE': 'NO', 'BORG_MSGPACK_VERSION_CHECK': 'YES', } E AssertionError: assert {'BORG_EXIT_C...USER': 'root'} == {'BORG_DEBUG_...': 'YES', ...} E E Omitting 2 identical items, use -vv to show E Right contains 6 more items: E {'BORG_DEBUG_PASSPHRASE': 'NO', E 'BORG_DISPLAY_PASSPHRASE': 'NO', E 'BORG_MSGPACK_VERSION_CHECK': 'YES', E 'BORG_RELOCATED_REPO_ACCESS_IS_OK E E ...Full output truncated (4 lines hidden), use '-vv' to show tests/unit/borg/test_environment.py:90: AssertionError ``` The only fix that I've tried is to just have an if statement to handle `msgpack_version_check` and set it to True like that.
change BORG_MSGPACK_VERSION_CHECK to default bool
Some checks failed
build / test (pull_request) Failing after 1m50s
Details
build / docs (pull_request) Has been skipped
Details
ee32cc19e9
witten commented 2026-02-26 19:10:49 +00:00
Owner
Copy link

My take is that you can safely delete that test. What it's doing is ensuring that, if no configuration is provided for those particular options, then their corresponding environment variables still get set (with default values).

But with the change you've made with the addition of that if value is not None: statement, borgmatic no longer sets the environment variables—unless there are actually configuration options for them. And so the test isn't needed any longer.

My take is that you can safely delete that test. What it's doing is ensuring that, if no configuration is provided for those particular options, then their corresponding environment variables still get set (with default values). But with the change you've made with the addition of that `if value is not None:` statement, borgmatic no longer sets the environment variables—unless there are actually configuration options for them. And so the test isn't needed any longer.
witten commented 2026-02-26 19:11:59 +00:00
Owner
Copy link

Actually, instead of deleting the test outright, it might be nice to repurpose it to assert that only the following environment variables get set:

    assert environment == {
        'USER': 'root',
        'BORG_EXIT_CODES': 'modern',
    }

I believe that should pass even with the changes you have in place.

Actually, instead of deleting the test outright, it might be nice to repurpose it to assert that only the following environment variables get set: ```python assert environment == { 'USER': 'root', 'BORG_EXIT_CODES': 'modern', } ``` I believe that should pass even with the changes you have in place.
test reconfigured
Some checks failed
build / test (pull_request) Failing after 1m55s
Details
build / docs (pull_request) Has been skipped
Details
75dcd41050
slantsh commented 2026-02-28 04:02:50 +00:00
Author
Contributor
Copy link

Oh, that makes sense! Thank you for being patient and guiding me, by the way.
I've made the changes and it does seem to pass all tests on my end.

Oh, that makes sense! Thank you for being patient and guiding me, by the way. I've made the changes and it does seem to pass all tests on my end.
slantsh commented 2026-02-28 04:10:13 +00:00
Author
Contributor
Copy link

Also want to ask whether I should use ruff to properly format it, because there are 2 other files that need formatting.

Also want to ask whether I should use ruff to properly format it, because there are 2 other files that need formatting.
ruff formatting?
All checks were successful
build / test (pull_request) Successful in 7m44s
Details
build / docs (pull_request) Has been skipped
Details
1ada61a1d3
witten commented 2026-02-28 07:11:40 +00:00
Owner
Copy link

Oh, that makes sense! Thank you for being patient and guiding me, by the way.

Of course! Thank you for your patience in getting through this PR process.

I've made the changes and it does seem to pass all tests on my end.

Great, glad to hear tests are passing now.

Also want to ask whether I should use ruff to properly format it, because there are 2 other files that need formatting.

Yes! I imagine there are sometimes formatting differences across different versions of ruff, which would explain how it might introduce formatting changes to files you didn't edit. As long as things pass in CI, you're all good.

> Oh, that makes sense! Thank you for being patient and guiding me, by the way. Of course! Thank you for your patience in getting through this PR process. > I've made the changes and it does seem to pass all tests on my end. Great, glad to hear tests are passing now. > Also want to ask whether I should use ruff to properly format it, because there are 2 other files that need formatting. Yes! I imagine there are sometimes formatting differences across different versions of ruff, which would explain how it might introduce formatting changes to files you didn't edit. As long as things pass [in CI](https://projects.torsion.org/borgmatic-collective/borgmatic/actions), you're all good.
witten commented 2026-02-28 07:14:38 +00:00
Owner
Copy link

This looks fine to merge to me! When you're ready, please feel free to remove the WIP prefix.

This looks fine to merge to me! When you're ready, please feel free to remove the WIP prefix.
slantsh changed title from WIP: Closes #1175 to Closes #1175 2026-02-28 07:17:32 +00:00
slantsh commented 2026-02-28 07:19:19 +00:00
Author
Contributor
Copy link

All set, I've removed the WIP prefix.

All set, I've removed the WIP prefix.
👍 1
witten merged commit 12e92acd15 into main 2026-02-28 07:19:55 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked

   
breaking

   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No labels
blocked
breaking
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
borgmatic-collective/borgmatic!1272
No description provided.