Add the borg recreate #1030

Merged
witten merged 19 commits from VandalByte/borgmatic:main into main 2025-03-29 22:07:52 +00:00
Contributor

This PR aims to add the borg recreate as a native borgmatic action.

Closes #610

This PR aims to add the `borg recreate` as a native borgmatic action. Closes #610
witten left a comment

This looks like a good start!

This looks like a good start!
@ -0,0 +4,4 @@
import borgmatic.borg.environment
import borgmatic.borg.feature
import borgmatic.borg.flags
import borgmatic.borg.repo_delete
Owner

This'll happen as part of running testing: Flaking will complain here about unused imports.

This'll happen as part of running testing: Flaking will complain here about unused imports.
VandalByte marked this conversation as resolved
@ -0,0 +32,4 @@
elif logger.isEnabledFor(logging.INFO):
verbosity_flags = ('--info',)
command = [local_path, 'recreate', repository]
Owner

It looks like Borg takes a repository or an archive for its recreate subcommand.

Additionally, for Borg 2 the repository/archive CLI syntax is completely different from Borg 1. Fortunately, many other borgmatic actions already deal with this so you don't have to reinvent the wheel. (See borg/flags.py:make_repository_archive_flags().)

It looks like Borg takes a repository *or* an archive for its `recreate` subcommand. Additionally, for Borg 2 the repository/archive CLI syntax is completely different from Borg 1. Fortunately, many other borgmatic actions already deal with this so you don't have to reinvent the wheel. (See `borg/flags.py:make_repository_archive_flags()`.)
VandalByte marked this conversation as resolved
@ -0,0 +33,4 @@
verbosity_flags = ('--info',)
command = [local_path, 'recreate', repository]
command.extend(verbosity_flags)
Owner

borgmatic (generally) uses a declarative syntax rather than imperative for constructing arguments like these.

borgmatic (generally) uses a declarative syntax rather than imperative for constructing arguments like these.
VandalByte marked this conversation as resolved
@ -0,0 +34,4 @@
command = [local_path, 'recreate', repository]
command.extend(verbosity_flags)
command.extend(global_arguments)
Owner

I don't think this will work as-is; global_arguments is usually an instance of argparse.Namespace() and represents the global arguments passed to borgmatic from the CLI. Check out some of the other borgmatic actions for how these are laboriously converted to Borg flags.

I don't think this will work as-is; `global_arguments` is usually an instance of `argparse.Namespace()` and represents the global arguments passed to borgmatic from the CLI. Check out some of the other borgmatic actions for how these are laboriously converted to Borg flags.
Owner

On the use of make_flags_from_arguments() here: You'll find out when it comes time to test this, but I'm guessing this will pull in way more arguments than you actually want converted into flags for this!

On the use of `make_flags_from_arguments()` here: You'll find out when it comes time to test this, but I'm guessing this will pull in way more arguments than you actually want converted into flags for this!
Author
Contributor

Yeah even I thought of that, I'll keep that in mind. 👍

Yeah even I thought of that, I'll keep that in mind. 👍
VandalByte marked this conversation as resolved
@ -0,0 +35,4 @@
command = [local_path, 'recreate', repository]
command.extend(verbosity_flags)
command.extend(global_arguments)
command.extend(recreate_arguments)
Owner

Similar here. This is by convention an argparse.Namepace() of borgmatic's own recreate arguments.

Similar here. This is by convention an `argparse.Namepace()` of borgmatic's own `recreate` arguments.
VandalByte marked this conversation as resolved
@ -0,0 +54,4 @@
remote_path=None,
):
'''
Recreate a Borg archive with the given repository and configuration.
Owner

Docstring convention is to document each function argument. See some of the other actions for examples.

Docstring convention is to document each function argument. See some of the other actions for examples.
VandalByte marked this conversation as resolved
@ -0,0 +69,4 @@
patterns_file = write_patterns_file(patterns, borgmatic.config.paths.get_runtime_directory())
if patterns_file:
command.extend(['--patterns-from', patterns_file.name])
Owner

Nice! Couple of thoughts:

  • Would it make sense to do this in make_recreate_command() since it's part of constructing that command?
  • Currently actions/create.py is responsible for collecting these patterns, specifically the process_patterns(collect_patterns(config), working_directory) call in run_create(). It might make sense to factor some of that out into a separate file for reuse in your code (say in an eventual actions/recreate.py).
Nice! Couple of thoughts: * Would it make sense to do this in `make_recreate_command()` since it's part of constructing that command? * Currently `actions/create.py` is responsible for collecting these patterns, specifically the `process_patterns(collect_patterns(config), working_directory)` call in `run_create()`. It might make sense to factor some of that out into a separate file for reuse in your code (say in an eventual `actions/recreate.py`).
Author
Contributor

I've removed the patterns for now, I'll add it later in actions/recreate.py

I've removed the patterns for now, I'll add it later in `actions/recreate.py`
VandalByte marked this conversation as resolved
@ -0,0 +79,4 @@
remote_path=remote_path,
borg_local_path=local_path,
borg_exit_codes=config.get('borg_exit_codes')
)
Owner

Also part of running tests: The black code formatter!

Also part of running tests: The `black` code formatter!
VandalByte marked this conversation as resolved
Author
Contributor

I've made the changes based on the feedback provided. Do let me know if everything looks good. I’m working on adding the recreate to borgmatic/commands/arguments.py next.

I've made the changes based on the feedback provided. Do let me know if everything looks good. I’m working on adding the recreate to `borgmatic/commands/arguments.py` next.
@ -0,0 +46,4 @@
+ global_flags
+ recreate_flags
+ exclude_flags
)
Owner

Nice and declarative!

Nice and declarative!
VandalByte marked this conversation as resolved
witten left a comment

Left a couple of minor comments, but I'll wait till there's more done for more. Thanks!

Left a couple of minor comments, but I'll wait till there's more done for more. Thanks!
Author
Contributor

I have made some new additions, few things extra that I need to look at are:

  • Check if I need to handle or use recreate_arguments in borgmatic/borg/recreate.py because right now it's not being used in there.
  • Handle path from recreate_arguments and add the PATH functionality as mentioned in docs here
  • Include pattern handling into borgmatic/actions/recreate.py ( any tips regarding this will be helpful :) )
  • Then finally, add the tests for the new additions.

I'll finish this by tomorrow.

I have made some new additions, few things extra that I need to look at are: - [x] Check if I need to handle or use `recreate_arguments` in `borgmatic/borg/recreate.py` because right now it's not being used in there. - [x] Handle path from `recreate_arguments` and add the PATH functionality as mentioned in docs [here](https://borgbackup.readthedocs.io/en/stable/usage/recreate.html) - [x] Include pattern handling into `borgmatic/actions/recreate.py` ( any tips regarding this will be helpful :) ) - [x] Then finally, add the tests for the new additions. I'll finish this by tomorrow.
Owner

Thanks, I'll have a look at this when I get the chance!

Thanks, I'll have a look at this when I get the chance!
witten left a comment

The overall approach looks good to me! Make sure you try running this though.. As mentioned in a comment, I'm not sure the patterns get passed through to Borg correctly right now, but it'd be hopefully straightforward to address that.

The overall approach looks good to me! Make sure you try running this though.. As mentioned in a comment, I'm not sure the patterns get passed through to Borg correctly right now, but it'd be hopefully straightforward to address that.
@ -0,0 +2,4 @@
import borgmatic.borg.recreate
import borgmatic.config.validate
from borgmatic.actions.create import collect_patterns, process_patterns
Owner

Just in terms of organization, it may make sense for some of this to be factored out of create.py since it's now used by multiple actions and isn't create-specific. Do not feel strongly.

Just in terms of organization, it may make sense for some of this to be factored out of `create.py` since it's now used by multiple actions and isn't `create`-specific. Do not feel strongly.
Author
Contributor

I went with the approach of just importing it to avoid code redundancy. But I do get what you mean, I'll add them in with recreate.py.

I went with the approach of just importing it to avoid code redundancy. But I do get what you mean, I'll add them in with recreate.py.
Owner

Oh I wasn't suggesting duplicating them! Just maybe putting them in a shared file that both create.py and recreate.py import and use. But again, I don't feel strongly about doing that. Just making the suggestion.

Oh I wasn't suggesting duplicating them! Just maybe putting them in a shared file that both `create.py` and `recreate.py` import and use. But again, I don't feel strongly about doing that. Just making the suggestion.
Author
Contributor

Oh, got it👍

Oh, got it👍
@ -0,0 +27,4 @@
else:
logger.info('Recreating repository')
# collect and process patterns
Owner

Super minor: Ideally comments are full sentences. (So "Collect and process patterns." in this case.)

Super minor: Ideally comments are full sentences. (So "Collect and process patterns." in this case.)
VandalByte marked this conversation as resolved
@ -0,0 +31,4 @@
patterns = collect_patterns(config)
processed_patterns = process_patterns(
patterns, borgmatic.config.paths.get_working_directory(config)
)
Owner

The general approach here with patterns for recreate looks good to me!

One thought though is you could call collect_patterns() inline since the patterns variable isn't used again. For that matter, you could even call process_patterns() inline below since processed_patterns is only used once. Do not feel at all strongly about either.

The general approach here with patterns for `recreate` looks good to me! One thought though is you could call `collect_patterns()` inline since the `patterns` variable isn't used again. For that matter, you could even call `process_patterns()` inline below since `processed_patterns` is only used once. Do not feel at all strongly about either.
VandalByte marked this conversation as resolved
@ -0,0 +43,4 @@
+ (('--log-json',) if global_arguments.log_json else ())
+ (('--lock-wait', str(lock_wait)) if lock_wait else ())
+ (('--info',) if logger.getEffectiveLevel() == logging.INFO else ())
+ (('--debug', '--show-rc', '--list') if logger.isEnabledFor(logging.DEBUG) else ())
Owner

What do you think of adding a --list flag to the accepted recreate command-line arguments and then making --list present here depending solely on that flag? And maybe that could also implicitly pass --filter ... to Borg? See the use of make_list_filter_flags() in borg/create.py.

This is basically how create works now.

What do you think of adding a `--list` flag to the accepted `recreate` command-line arguments and then making `--list` present here depending solely on that flag? And maybe that could also implicitly pass `--filter ...` to Borg? See the use of `make_list_filter_flags()` in `borg/create.py`. This is basically how `create` works now.
Author
Contributor

I did think of doing it that way but, if I remember correctly it was extract.py, where in borg docs --list was an optional arg, was just added along as a logging flag. I'll add it separately.

I did think of doing it that way but, if I remember correctly it was extract.py, where in borg docs `--list` was an optional arg, was just added along as a logging flag. I'll add it separately.
Owner

Yeah I see what you're saying about extract.py. My thinking is that recreate.py is maybe a little closer to create.py and users might expect it to work somewhat similarly. Also: It's not your concern at this point, but I'm adding a list configuration option for an unrelated feature (#303), and it could ultimately serve as a default for --list command-line arguments for actions like create and recreate.

Yeah I see what you're saying about `extract.py`. My thinking is that `recreate.py` is maybe a little closer to `create.py` and users might expect it to work somewhat similarly. Also: It's not your concern at this point, but I'm adding a `list` configuration option for an unrelated feature (#303), and it could ultimately serve as a default for `--list` command-line arguments for actions like `create` and `recreate`.
VandalByte marked this conversation as resolved
@ -0,0 +44,4 @@
+ (('--lock-wait', str(lock_wait)) if lock_wait else ())
+ (('--info',) if logger.getEffectiveLevel() == logging.INFO else ())
+ (('--debug', '--show-rc', '--list') if logger.isEnabledFor(logging.DEBUG) else ())
+ pattern_flags
Owner

The way you're constructing these pattern_flags unfortunately won't work, because (if I'm reading this correctly) you're passing raw patterns to --patterns-from, which instead expect a patterns filename.

The good news is I think the approach used for that in borgmatic/borg/create.py will work here too—calling write_patterns_file() to write all the patterns to a temporary file and then using that filename with --patterns-from.

The way you're constructing these `pattern_flags` unfortunately won't work, because (if I'm reading this correctly) you're passing raw patterns to `--patterns-from`, which instead expect a patterns filename. The good news is I think the approach used for that in `borgmatic/borg/create.py` will work here too—calling `write_patterns_file()` to write all the patterns to a temporary file and then using *that* filename with `--patterns-from`.
VandalByte marked this conversation as resolved
@ -0,0 +60,4 @@
remote_path=remote_path,
borg_local_path=local_path,
borg_exit_codes=config.get('borg_exit_codes'),
)
Owner

It might make more sense for the output_log_level to be logging.INFO instead of logging.ANSWER since this isn't a "answer my question by providing output" type of action like, say, list or info.

That change would also have the side effect of making tests slightly easier because you wouldn't have to deal with add_custom_log_levels().

It might make more sense for the `output_log_level` to be `logging.INFO` instead of `logging.ANSWER` since this isn't a "answer my question by providing output" type of action like, say, `list` or `info`. That change would also have the side effect of making tests slightly easier because you wouldn't have to deal with `add_custom_log_levels()`.
VandalByte marked this conversation as resolved
Author
Contributor

I've added most of the requested changes and tests, the big one tests/unit/borg/test_recreate.py is a WIP. I feel like I have addressed most of the requested changes, do let me know if something looks odd.

Also, should I add recreate to skip_actions and commands in borgmatic/config/schema.yaml?

skip_actions:

commands:
type: array
items:

EDIT:
I added it to skip_actions since the test_schema.py was failing without it. For now, I have added it to both skip_action and commands.

I've added most of the requested changes and tests, the big one `tests/unit/borg/test_recreate.py` is a WIP. I feel like I have addressed most of the requested changes, do let me know if something looks odd. Also, should I add `recreate` to skip_actions and commands in `borgmatic/config/schema.yaml`? https://projects.torsion.org/borgmatic-collective/borgmatic/src/commit/524ec6b3cbdabf198790c44c7904e4c761836336/borgmatic/config/schema.yaml#L760 https://projects.torsion.org/borgmatic-collective/borgmatic/src/commit/524ec6b3cbdabf198790c44c7904e4c761836336/borgmatic/config/schema.yaml#L947-L949 EDIT: I added it to `skip_actions` since the `test_schema.py` was failing without it. For now, I have added it to both `skip_action` and `commands`.
Owner

Yeah, adding it to the skip_actions and commands when enums sounds right to me!

Yeah, adding it to the `skip_actions` and `commands` `when` enums sounds right to me!
VandalByte changed title from WIP: Add the borg recreate to Add the borg recreate 2025-03-25 15:14:42 +00:00
Author
Contributor

All tests have been completed, and the tests passed locally.

All tests have been completed, and the tests passed locally.
VandalByte changed title from Add the borg recreate to WIP: Add the borg recreate 2025-03-25 17:31:03 +00:00
Author
Contributor

I still have few manual tests I want to do. Other than that testing only things left are:

Oh I wasn't suggesting duplicating them! Just maybe putting them in a shared file that both create.py and recreate.py import and use. But again, I don't feel strongly about doing that. Just making the suggestion.

Where do you say we move this to? Also any suggestion about the file name to use for the new file, pattern.py? in /borgmatic/action/ but one pattern.py already exist in /borgmatic/borg/

Yeah I see what you're saying about extract.py. My thinking is that recreate.py is maybe a little closer to create.py and users might expect it to work somewhat similarly. Also: It's not your concern at this point, but I'm adding a list configuration option for an unrelated feature (#303), and it could ultimately serve as a default for --list command-line arguments for actions like create and recreate.

Do you want me to add this --list to args? in /borgmatic/commands/arguments.py right now this is what i have in borgmatic/borg/recreate.py

      + (
            (
                '--list',
                '--filter',
                make_list_filter_flags(local_borg_version, global_arguments.dry_run),
            )
            if hasattr(recreate_arguments, 'list') and recreate_arguments.list
            else ()
        )
I still have few manual tests I want to do. Other than that testing only things left are: > Oh I wasn't suggesting duplicating them! Just maybe putting them in a shared file that both create.py and recreate.py import and use. But again, I don't feel strongly about doing that. Just making the suggestion. Where do you say we move this to? Also any suggestion about the file name to use for the new file, `pattern.py`? in `/borgmatic/action/` but one `pattern.py` already exist in `/borgmatic/borg/` > Yeah I see what you're saying about extract.py. My thinking is that recreate.py is maybe a little closer to create.py and users might expect it to work somewhat similarly. Also: It's not your concern at this point, but I'm adding a list configuration option for an unrelated feature (#303), and it could ultimately serve as a default for --list command-line arguments for actions like create and recreate. Do you want me to add this `--list` to args? in `/borgmatic/commands/arguments.py` right now this is what i have in `borgmatic/borg/recreate.py` ```python + ( ( '--list', '--filter', make_list_filter_flags(local_borg_version, global_arguments.dry_run), ) if hasattr(recreate_arguments, 'list') and recreate_arguments.list else () ) ```
Owner

Where do you say we move this to? Also any suggestion about the file name to use for the new file, pattern.py? in /borgmatic/action/ but one pattern.py already exist in /borgmatic/borg/

IMO it's fine to have same-named source files in different directories. In this particular case, actions/pattern.py seems like as good a spot as any for these functions.

Do you want me to add this --list to args? in /borgmatic/commands/arguments.py right now this is what i have in borgmatic/borg/recreate.py

That seems reasonable, yeah. Note that I don't think you need the hasattr() call.

> Where do you say we move this to? Also any suggestion about the file name to use for the new file, pattern.py? in /borgmatic/action/ but one pattern.py already exist in /borgmatic/borg/ IMO it's fine to have same-named source files in different directories. In this particular case, `actions/pattern.py` seems like as good a spot as any for these functions. > Do you want me to add this --list to args? in /borgmatic/commands/arguments.py right now this is what i have in borgmatic/borg/recreate.py That seems reasonable, yeah. Note that I don't think you need the `hasattr()` call.
witten left a comment

Just a few minor comments.

Just a few minor comments.
@ -0,0 +31,4 @@
if archive is None:
logger.error('Please provide a valid archive name.')
return
Owner

I don't think this is necessary if you make --archive required=True in arguments.py.

I don't think this is necessary if you make `--archive` `required=True` in `arguments.py`.
VandalByte marked this conversation as resolved
@ -0,0 +41,4 @@
patterns, borgmatic.config.paths.get_working_directory(config)
)
recreate_cmd = (
Owner

Minor code style nit: Spell out variable names (cmd -> command).

Minor code style nit: Spell out variable names (`cmd` -> `command`).
VandalByte marked this conversation as resolved
Author
Contributor

IMO it's fine to have same-named source files in different directories. In this particular case, actions/pattern.py seems like as good a spot as any for these functions.

The issue is, I'm using methods from both create.py from /borgmatic/action/ and borgmatic/borg/. And file borgmatic/borg/pattern.py (class) already exists, so will that be ok if I add it to that?

That seems reasonable, yeah. Note that I don't think you need the hasattr() call.

I added that because I was getting error on execution of borgmatic recreate saying complaining about no attribute list, I got the same for path as well. For path I'm probably going to set it as required, because when you run borg recreate with just repo::archive you get this:

image.png

EDIT:
Now that I explained about path, I just realized it's a positional argument and not an optional one, so need to change that as well.

> IMO it's fine to have same-named source files in different directories. In this particular case, actions/pattern.py seems like as good a spot as any for these functions. The issue is, I'm using methods from both `create.py` from `/borgmatic/action/` and `borgmatic/borg/`. And file [borgmatic/borg/pattern.py](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/borg/pattern.py) (class) already exists, so will that be ok if I add it to that? > That seems reasonable, yeah. Note that I don't think you need the hasattr() call. I added that because I was getting error on execution of `borgmatic recreate` saying complaining about no attribute `list`, I got the same for `path` as well. For `path` I'm probably going to set it as required, because when you run `borg recreate` with just repo::archive you get this: <img width="728" alt="image.png" src="attachments/38d7018c-253f-49f2-986f-338d946ffc30"> EDIT: Now that I explained about path, I just realized it's a positional argument and not an optional one, so need to change that as well.
Owner

The issue is, I'm using methods from both create.py from /borgmatic/actions/ and borgmatic/borg/. And file borgmatic/borg/pattern.py (class) already exists, so will that be ok if I add it to that?

I think it really depends on what those function are. The way I think about actions/ vs. borg/, the main distinction is that actions/ contains code (including shared utilities) for borgmatic's own action logic, while borg/ contains code (including shared utilities) for interacting with Borg. Sometimes that line can get a little blurry, but for instance I would only add functions to borg/pattern.py that deal in Borg patterns and not borgmatic's own data structures or concepts (configuration, etc.)

So if you're thinking about the existing parse_pattern() and collect_patterns() in particular:

  • collect_patterns(): This to me seems like it belongs at the actions/ level because it deals directly in processing borgmatic config, which seems like more of a borgmatic concern rather than a Borg concern.
  • parse_pattern(): I could see an argument for moving this into either borg/pattern.py or, say, actions/pattern.py. That's because it operates directly on the data structures currently defined in borg/pattern.py (as a kind of constructor for them), but it's a function that's typically called from collect_patterns() and so maybe should live alongside it.

I'm happy to weigh in on any other functions you're thinking about, but most likely wherever you put them will be fine. We can always move them around later. You could even leave them where there are for now in actions/create.py, etc. if all this seems like too big of a refactor for now.

I added that because I was getting error on execution of borgmatic recreate saying complaining about no attribute list, I got the same for path as well.

Hmm I'm not sure why the attribute (for list) wouldn't be there if it's not required. My understanding of argparse is the attribute is always there, but if it's not required then the value may just be None. Standalone example:

import argparse
parser = argparse.ArgumentParser()
parser.add_argument('--foo')
print(parser.parse_args([]))

... which produces:

Namespace(foo=None)

For path I'm probably going to set it as required, because when you run borg recreate with just repo::archive you get this:

Is it possible that whatever paths or patterns or excludes gets stuffed into --patterns-from are interfering and making Borg think there's nothing to recreate? The docs for recreate seem to indicate that path really is optional. This line from the docs is interesting:

Note that all paths in an archive are relative, therefore absolute patterns/paths will not match (--exclude, --exclude-from, PATHs).

Are absolute paths getting passed in, maybe?

> The issue is, I'm using methods from both create.py from /borgmatic/actions/ and borgmatic/borg/. And file borgmatic/borg/pattern.py (class) already exists, so will that be ok if I add it to that? I think it really depends on what those function are. The way I think about `actions/` vs. `borg/`, the main distinction is that `actions/` contains code (including shared utilities) for borgmatic's own action logic, while `borg/` contains code (including shared utilities) for interacting with Borg. Sometimes that line can get a little blurry, but for instance I would only add functions to `borg/pattern.py` that deal in Borg patterns and not borgmatic's own data structures or concepts (configuration, etc.) So if you're thinking about the existing `parse_pattern()` and `collect_patterns()` in particular: * `collect_patterns()`: This to me seems like it belongs at the `actions/` level because it deals directly in processing borgmatic config, which seems like more of a borgmatic concern rather than a Borg concern. * `parse_pattern()`: I could see an argument for moving this into either `borg/pattern.py` or, say, `actions/pattern.py`. That's because it operates directly on the data structures currently defined in `borg/pattern.py` (as a kind of constructor for them), *but* it's a function that's typically called from `collect_patterns()` and so maybe should live alongside it. I'm happy to weigh in on any other functions you're thinking about, but most likely wherever you put them will be fine. We can always move them around later. You could even leave them where there are for now in `actions/create.py`, etc. if all this seems like too big of a refactor for now. > I added that because I was getting error on execution of borgmatic recreate saying complaining about no attribute list, I got the same for path as well. Hmm I'm not sure why the attribute (for `list`) wouldn't be there if it's not required. My understanding of argparse is the attribute is always there, but if it's not required then the value may just be `None`. Standalone example: ```python import argparse parser = argparse.ArgumentParser() parser.add_argument('--foo') print(parser.parse_args([])) ``` ... which produces: ```python Namespace(foo=None) ``` > For path I'm probably going to set it as required, because when you run borg recreate with just repo::archive you get this: Is it possible that whatever paths or patterns or excludes gets stuffed into `--patterns-from` are interfering and making Borg think there's nothing to recreate? The [docs for recreate](https://borgbackup.readthedocs.io/en/stable/usage/recreate.html) seem to indicate that path really is optional. This line from the docs is interesting: > Note that all paths in an archive are relative, therefore absolute patterns/paths will not match (--exclude, --exclude-from, PATHs). Are absolute paths getting passed in, maybe?
Author
Contributor

I'm happy to weigh in on any other functions you're thinking about, but most likely wherever you put them will be fine. We can always move them around later. You could even leave them where there are for now in actions/create.py, etc. if all this seems like too big of a refactor for now.

Yeah that will be great. I think I'll probably work on that on a new PR after this one.

Hmm I'm not sure why the attribute (for list) wouldn't be there if it's not required. My understanding of argparse is the attribute is always there, but if it's not required then the value may just be None. Standalone example:

I figured it out, it was because I hadn't added it to the arguments.

Is it possible that whatever paths or patterns or excludes gets stuffed into --patterns-from are interfering and making Borg think there's nothing to recreate? The docs for recreate seem to indicate that path really is optional. This line from the docs is interesting

I still have to test out patterns and exclusions. Then only I will know for sure.

So right now I managed to make the basic recreate work, from the examples of borg, path is basically the patterns of archive inside repo::archive, I though of it as a path to place the newly created repo/archive. recreate also supports --pattern. So I'm thinking of removing the path entirely then use the patten flags instead. I still on the fence about this, whether to keep it in or remove it.

I'm also thinking of adding the archive options, or do I let user give them? ( factor in the flags from namespace excluding the repo, archive and list flags) which one do you think is the best? I'm thinking of 1st option. But I though of the second because for borg and borg2 the archive options are quite different except a few being common. So I was thinking of adding those common ones (example: --comment , --timestamp and a few others) and take up the rest, if any, from namespace args.

> I'm happy to weigh in on any other functions you're thinking about, but most likely wherever you put them will be fine. We can always move them around later. You could even leave them where there are for now in actions/create.py, etc. if all this seems like too big of a refactor for now. Yeah that will be great. I think I'll probably work on that on a new PR after this one. > Hmm I'm not sure why the attribute (for list) wouldn't be there if it's not required. My understanding of argparse is the attribute is always there, but if it's not required then the value may just be None. Standalone example: I figured it out, it was because I hadn't added it to the arguments. > Is it possible that whatever paths or patterns or excludes gets stuffed into --patterns-from are interfering and making Borg think there's nothing to recreate? The docs for recreate seem to indicate that path really is optional. This line from the docs is interesting I still have to test out patterns and exclusions. Then only I will know for sure. So right now I managed to make the basic recreate work, from the examples of borg, `path` is basically the patterns of archive inside `repo::archive`, I though of it as a path to place the newly created repo/archive. recreate also supports `--pattern`. So I'm thinking of removing the `path` entirely then use the patten flags instead. I still on the fence about this, whether to keep it in or remove it. I'm also thinking of adding the archive options, or do I let user give them? ( factor in the flags from namespace excluding the repo, archive and list flags) which one do you think is the best? I'm thinking of 1st option. But I though of the second because for [borg](https://borgbackup.readthedocs.io/en/stable/usage/recreate.html) and [borg2](https://borgbackup.readthedocs.io/en/master/usage/recreate.html) the archive options are quite different except a few being common. So I was thinking of adding those common ones (example: `--comment` , `--timestamp` and a few others) and take up the rest, if any, from namespace args.
Owner

Yeah that will be great. I think I'll probably work on that on a new PR after this one.

Sounds good.

I figured it out, it was because I hadn't added it to the arguments.

Ah yeah, that would do it!

So right now I managed to make the basic recreate work, from the examples of borg, path is basically the patterns of archive inside repo::archive, I though of it as a path to place the newly created repo/archive. recreate also supports --pattern. So I'm thinking of removing the path entirely then use the patten flags instead. I still on the fence about this, whether to keep it in or remove it.

Yeah, I could see an argument for either approach. I will say that if you start without path, I think we can always add it in later if there's a need. And if you start with path included, you could maybe just roll it into the patterns file like we do with everything else (source_directories, excludes, etc.)—although maybe that would be a pain just in terms of where that's done in the code.

I'm also thinking of adding the archive options, or do I let user give them? ( factor in the flags from namespace excluding the repo, archive and list flags) which one do you think is the best? I'm thinking of 1st option. But I though of the second because for borg and borg2 the archive options are quite different except a few being common. So I was thinking of adding those common ones (example: --comment , --timestamp and a few others) and take up the rest, if any, from namespace args.

Here are my thoughts on the archive options:

  • --match-archives (Borg 2): I think this is worth having, and the code for this exists for several other actions already so it should hopefully be straightforward. Although it might need to be tweaked a little since Borg 1 doesn't support this flag for this action.
  • --first/--last/--oldest/--newest/--older/--newer (Borg 2): IMO nice to have but not critical. Several existing borgmatic actions support these.
  • --sort-by (Borg 2): Not even sure why this is needed on this action? Maybe it influences the --list output?
  • --target: Should probably include.
  • --comment: Would be good to include.
  • --timestamp: Nice to have.
  • --compression/--chunker-params: Doesn't need to be on borgmatic's command-line, but should be passed to Borg from the relevant configuration option.
  • --recompress: Could be either command-line or a configuration option. (Note that once #303 is done, all configuration options will be override-able from the command-line at the global scope.)

These are just my takes on this. Feel free to make calls as you see fit.

> Yeah that will be great. I think I'll probably work on that on a new PR after this one. Sounds good. > I figured it out, it was because I hadn't added it to the arguments. Ah yeah, that would do it! > So right now I managed to make the basic recreate work, from the examples of borg, path is basically the patterns of archive inside repo::archive, I though of it as a path to place the newly created repo/archive. recreate also supports --pattern. So I'm thinking of removing the path entirely then use the patten flags instead. I still on the fence about this, whether to keep it in or remove it. Yeah, I could see an argument for either approach. I will say that if you start without `path`, I think we can always add it in later if there's a need. And if you start with `path` included, you could maybe just roll it into the patterns file like we do with everything else (`source_directories`, `excludes`, etc.)—although maybe that would be a pain just in terms of where that's done in the code. > I'm also thinking of adding the archive options, or do I let user give them? ( factor in the flags from namespace excluding the repo, archive and list flags) which one do you think is the best? I'm thinking of 1st option. But I though of the second because for borg and borg2 the archive options are quite different except a few being common. So I was thinking of adding those common ones (example: --comment , --timestamp and a few others) and take up the rest, if any, from namespace args. Here are my thoughts on the archive options: * `--match-archives` (Borg 2): I think this is worth having, and the code for this exists for several other actions already so it should hopefully be straightforward. Although it might need to be tweaked a little since Borg 1 doesn't support this flag for this action. * `--first`/`--last`/`--oldest`/`--newest`/`--older`/`--newer` (Borg 2): IMO nice to have but not critical. Several existing borgmatic actions support these. * `--sort-by` (Borg 2): Not even sure why this is needed on this action? Maybe it influences the `--list` output? * `--target`: Should probably include. * `--comment`: Would be good to include. * `--timestamp`: Nice to have. * `--compression`/`--chunker-params`: Doesn't need to be on borgmatic's command-line, but should be passed to Borg from the relevant configuration option. * `--recompress`: Could be either command-line or a configuration option. (Note that once #303 is done, all configuration options will be override-able from the command-line at the global scope.) These are just my takes on this. Feel free to make calls as you see fit.
@ -0,0 +37,4 @@
Executes the recreate command with the given arguments.
'''
if recreate_arguments.timestamp and not is_valid_timestamp(recreate_arguments.timestamp):
Owner

Is there a reason not to let Borg do this validation for you? Presumably if it accepts a timestamp value it also validates it.

Is there a reason not to let Borg do this validation for you? Presumably if it accepts a timestamp value it also validates it.
VandalByte marked this conversation as resolved
@ -0,0 +80,4 @@
if recreate_arguments.target and recreate_arguments.archive
else ()
)
+ (('--comment', f'"{recreate_arguments.comment}"') if recreate_arguments.comment else ())
Owner

I think the way to do this reliably would be shlex.quote()?

I think the way to do this reliably would be `shlex.quote()`?
VandalByte marked this conversation as resolved
Author
Contributor

Yeah, I could see an argument for either approach. I will say that if you start without path, I think we can always add it in later if there's a need. And if you start with path included, you could maybe just roll it into the patterns file like we do with everything else (source_directories, excludes, etc.)—although maybe that would be a pain just in terms of where that's done in the code.

For now, I'm not adding path since the patterns I tested, seem to work very well. And I think I'll leave the flag commented out, just in case we might have to revisit it in the future.

Now regarding the flags, I have added --target, --comment, --timestamp and --compression (compression I have to add action='store_true' since it's taken from config file)

Next I'll add --match-archives, --recompress and --chunker-params. I am skipping --sort-by. --first/--last/ - Since it doesn't seem that critically necessary, I'll try to include it, if not, later

Ok, so these (--oldest/--newest/--older/--newer), use the TIMESTAMP, there's something I came across while I was adding --timestamp yesterday, timestamp that I gave was not one one being added to archive.

Docs (borg):
--timestamp TIMESTAMP
manually specify the archive creation date/time (UTC, yyyy-mm-ddThh:mm:ss format). alternatively, give a reference file/directory.

Docs (borg2):
--timestamp TIMESTAMP
manually specify the archive creation date/time (yyyy-mm-ddThh:mm:ss[(+|-)HH:MM] format, (+|-)HH:MM is the UTC offset, default: local time zone). Alternatively, give a reference file/directory.

I tested it out in borg 1.2.8, and the issue is when we give the time say, 07:10 borg converts it to UTC by adding the offset of our local time. Example: give time in TIMESTAMP 07:10, adds the offset 7:10 + 5:30 = 12:40, so 12:40 is reflected when we do borg list, for me it felt a bit weird because I gave a time but something else was being reflected, in borg2 they handled it I guess, by giving option for offset. I thought of adjusting it, but since it's a borg issue I thought shouldn't.

Also that would mean different validation, but since you mentioned this, I guess I will go with letting borg handle it. Thanks :)

Is there a reason not to let Borg do this validation for you? Presumably if it accepts a timestamp value it also validates it.

> Yeah, I could see an argument for either approach. I will say that if you start without path, I think we can always add it in later if there's a need. And if you start with path included, you could maybe just roll it into the patterns file like we do with everything else (source_directories, excludes, etc.)—although maybe that would be a pain just in terms of where that's done in the code. For now, I'm not adding `path` since the patterns I tested, seem to work very well. And I think I'll leave the flag commented out, just in case we might have to revisit it in the future. Now regarding the flags, I have added `--target`, `--comment`, `--timestamp` and `--compression` (compression I have to add `action='store_true'` since it's taken from config file) Next I'll add `--match-archives`, `--recompress` and `--chunker-params`. I am skipping `--sort-by`. `--first`/`--last`/ - Since it doesn't seem that critically necessary, I'll try to include it, if not, later Ok, so these (`--oldest`/`--newest`/`--older`/`--newer`), use the TIMESTAMP, there's something I came across while I was adding `--timestamp` yesterday, timestamp that I gave was not one one being added to archive. > Docs (borg): > --timestamp TIMESTAMP > manually specify the archive creation date/time (UTC, yyyy-mm-ddThh:mm:ss format). alternatively, give a reference file/directory. > > Docs (borg2): > --timestamp TIMESTAMP > manually specify the archive creation date/time (yyyy-mm-ddThh:mm:ss[(+|-)HH:MM] format, (+|-)HH:MM is the UTC offset, default: local time zone). Alternatively, give a reference file/directory. I tested it out in borg 1.2.8, and the issue is when we give the time say, `07:10` borg converts it to UTC by adding the offset of our local time. Example: give time in TIMESTAMP `07:10`, adds the offset `7:10 + 5:30 = 12:40`, so `12:40` is reflected when we do `borg list`, for me it felt a bit weird because I gave a time but something else was being reflected, in borg2 they handled it I guess, by giving option for offset. I thought of adjusting it, but since it's a borg issue I thought shouldn't. Also that would mean different validation, but since you mentioned this, I guess I will go with letting borg handle it. Thanks :) > Is there a reason not to let Borg do this validation for you? Presumably if it accepts a timestamp value it also validates it.
Owner

For now, I'm not adding path since the patterns I tested, seem to work very well. And I think I'll leave the flag commented out, just in case we might have to revisit it in the future.

Leaving it out sounds fine for now, but I'd prefer removing the commented out code entirely, just as a matter of code cleanliness. You can always make a branch or a git stash if you want to save it off somewhere in case you need it for later.

Now regarding the flags, I have added --target, --comment, --timestamp and --compression (compression I have to add action='store_true' since it's taken from config file)

Sounds good, although I think you can probably omit --compression since: 1. It's available from the configuration, and 2. When #303 lands, all config options will be override-able from the command-line. Or if you prefer you can leave --compression for now and I can deduplicate flags as part of #303 work.

Next I'll add --match-archives, --recompress and --chunker-params. I am skipping --sort-by. --first/--last/ - Since it doesn't seem that critically necessary, I'll try to include it, if not, later

I'd put --chunker-params in the same category as --compression IMO. And leaving off --first/--last to start with seems fine to me.

Ok, so these (--oldest/--newest/--older/--newer), use the TIMESTAMP, there's something I came across while I was adding --timestamp yesterday, timestamp that I gave was not one one being added to archive.
...
I tested it out in borg 1.2.8, and the issue is when we give the time say, 07:10 borg converts it to UTC by adding the offset of our local time. Example: give time in TIMESTAMP 07:10, adds the offset 7:10 + 5:30 = 12:40, so 12:40 is reflected when we do borg list, for me it felt a bit weird because I gave a time but something else was being reflected, in borg2 they handled it I guess, by giving option for offset. I thought of adjusting it, but since it's a borg issue I thought shouldn't.

Also that would mean different validation, but since you mentioned this, I guess I will go with letting borg handle it. Thanks :)

Sounds reasonable!

> For now, I'm not adding path since the patterns I tested, seem to work very well. And I think I'll leave the flag commented out, just in case we might have to revisit it in the future. Leaving it out sounds fine for now, but I'd prefer removing the commented out code entirely, just as a matter of code cleanliness. You can always make a branch or a `git stash` if you want to save it off somewhere in case you need it for later. > Now regarding the flags, I have added --target, --comment, --timestamp and --compression (compression I have to add action='store_true' since it's taken from config file) Sounds good, although I think you can probably omit `--compression` since: 1. It's available from the configuration, and 2. When #303 lands, all config options will be override-able from the command-line. Or if you prefer you can leave `--compression` for now and I can deduplicate flags as part of #303 work. > Next I'll add --match-archives, --recompress and --chunker-params. I am skipping --sort-by. --first/--last/ - Since it doesn't seem that critically necessary, I'll try to include it, if not, later I'd put `--chunker-params` in the same category as `--compression` IMO. And leaving off `--first`/`--last` to start with seems fine to me. > Ok, so these (--oldest/--newest/--older/--newer), use the TIMESTAMP, there's something I came across while I was adding --timestamp yesterday, timestamp that I gave was not one one being added to archive. > ... > I tested it out in borg 1.2.8, and the issue is when we give the time say, 07:10 borg converts it to UTC by adding the offset of our local time. Example: give time in TIMESTAMP 07:10, adds the offset 7:10 + 5:30 = 12:40, so 12:40 is reflected when we do borg list, for me it felt a bit weird because I gave a time but something else was being reflected, in borg2 they handled it I guess, by giving option for offset. I thought of adjusting it, but since it's a borg issue I thought shouldn't. > > Also that would mean different validation, but since you mentioned this, I guess I will go with letting borg handle it. Thanks :) Sounds reasonable!
Author
Contributor

Sounds good, although I think you can probably omit --compression since: 1. It's available from the configuration, and 2. When #303 lands, all config options will be override-able from the command-line. Or if you prefer you can leave --compression for now and I can deduplicate flags as part of #303 work.

Yeah, so now --compression, --recompress, --chunker-params are taken from config. I should have mentioned that clearly in my last comment.

All local unit tests are passing, and I believe it's ready for a full review. It's been an interesting learning curve and experience, I should say :)

> Sounds good, although I think you can probably omit --compression since: 1. It's available from the configuration, and 2. When #303 lands, all config options will be override-able from the command-line. Or if you prefer you can leave --compression for now and I can deduplicate flags as part of #303 work. Yeah, so now `--compression`, `--recompress`, `--chunker-params` are taken from config. I should have mentioned that clearly in my last comment. All local unit tests are passing, and I believe it's ready for a full review. It's been an interesting learning curve and experience, I should say :)
VandalByte changed title from WIP: Add the borg recreate to Add the borg recreate 2025-03-28 20:05:38 +00:00
witten left a comment

Thanks for all the exhaustive testing and for sticking with this! I think this is almost ready to merge.

Thanks for all the exhaustive testing and for sticking with this! I think this is almost ready to merge.
@ -0,0 +36,4 @@
# Available recompress MODES: 'if-different' (default), 'always', 'never'
recompress = config.get('recompress', None)
# Write patterns to a temporary file and use that file with --patterns-from.
Owner
  1. I don't actually see recompress anywhere in the config schema. 2. If you do want to add it, you might consider making it an enum since it sounds like it can only have certain values.
1. I don't actually see `recompress` anywhere in the config schema. 2. If you do want to add it, you might consider making it an enum since it sounds like it can only have certain values.
VandalByte marked this conversation as resolved
@ -0,0 +61,4 @@
# Flag --target works only for a single archive
+ (('--target', recreate_arguments.target) if recreate_arguments.target and archive else ())
+ (
('--comment', shlex.quote(recreate_arguments.comment))
Owner

Good use of shlex.quote()!

Good use of `shlex.quote()`!
VandalByte marked this conversation as resolved
@ -0,0 +8,4 @@
from ..test_verbosity import insert_logging_mock
# from borgmatic.borg.pattern import Pattern, Pattern_type, Pattern_style, Pattern_source
# from borgmatic.borg.create import make_exclude_flags, make_list_filter_flags
Owner

Remove commented out code, please!

Remove commented out code, please!
Author
Contributor

Oops! might have missed that.

Oops! might have missed that.
witten marked this conversation as resolved
@ -0,0 +26,4 @@
def test_recreate_archive_dry_run_skips_execution():
flexmock(module.borgmatic.borg.flags).should_receive(
'make_repository_archive_flags'
).and_return(('repo::archive',))
Owner

This is a good start, but IMO all the test functions of recreate_archive() in this file also need to mock out:

  • make_exclude_flags()
  • write_patterns_file()
  • make_list_filter_flags()
  • make_match_archives_flags()
  • make_repository_flags() (if called)

The idea is to isolate the unit under test so we're only testing it and not all the other random borgmatic functions it calls. Python standard library calls usually don't have to be mocked out though (unless they hit disk or whatever).

This is a good start, but IMO all the test functions of `recreate_archive()` in this file also need to mock out: * `make_exclude_flags()` * `write_patterns_file()` * `make_list_filter_flags()` * `make_match_archives_flags()` * `make_repository_flags()` (if called) The idea is to isolate the unit under test so we're only testing it and not all the other random borgmatic functions it calls. Python standard library calls usually don't have to be mocked out though (unless they hit disk or whatever).
witten marked this conversation as resolved
Author
Contributor

Hey @witten
I have added the changes you mentioned. Thanks.

Hey @witten I have added the changes you mentioned. Thanks.
witten left a comment

Okay, hopefully some final UX/docs-level comments!

Okay, hopefully some final UX/docs-level comments!
@ -1548,0 +1568,4 @@
recreate_group.add_argument(
'--target',
metavar='TARGET',
help='Name of new archive name',
Owner

Do you think it's worth adding "to create instead of recreating the existing archive" or something similar? I ask because if I was just looking through the help as-is, I might be confused as to why there's both --archive and --target.

Do you think it's worth adding "to create instead of recreating the existing archive" or something similar? I ask because if I was just looking through the help as-is, I might be confused as to why there's both `--archive` and `--target`.
Author
Contributor

Yeah you're right, I will make it more clearer.

Yeah you're right, I will make it more clearer.
Author
Contributor

How's this?

help='Create a new archive from the specified archive (via --archive), without replacing any existing archive.'
How's this? ```python help='Create a new archive from the specified archive (via --archive), without replacing any existing archive.' ```
Owner

Looks good! You could even shorten the end to without replacing it if you wanted.

Looks good! You could even shorten the end to `without replacing it` if you wanted.
VandalByte marked this conversation as resolved
@ -1548,0 +1573,4 @@
recreate_group.add_argument(
'--comment',
metavar='COMMENT',
help='Add a comment text to the archive, if archive not provided, consider all archives',
Owner

I'm not following the ", if archive not provided, consider all archives" part here. Is that meant for a different flag maybe?

I'm not following the ", if archive not provided, consider all archives" part here. Is that meant for a different flag maybe?
Author
Contributor

I put that part there because during my testing I noticed that if you don't give an archive name to the borg command, every archive under that repo gets the comment that we just gave. I should make that a bit more clearer I guess.

I put that part there because during my testing I noticed that if you don't give an archive name to the borg command, every archive under that repo gets the comment that we just gave. I should make that a bit more clearer I guess.
Owner

Oh gotcha! In that case I might suggest tweaking the wording to something more explicit like:

 help='Add a comment text to the archive or, if an archive is not provided, to all matching archives',

... or even:

 help='Add a comment text to all matching archives',
Oh gotcha! In that case I might suggest tweaking the wording to something more explicit like: ```python help='Add a comment text to the archive or, if an archive is not provided, to all matching archives', ``` ... or even: ```python help='Add a comment text to all matching archives', ```
VandalByte marked this conversation as resolved
@ -1548,0 +1578,4 @@
recreate_group.add_argument(
'--timestamp',
metavar='TIMESTAMP',
help='Manually specify the archive creation date/time (UTC)',
Owner

Maybe "override" instead of "specify" here? Or just some way of indicating that you're replacing the existing/default timestamp by using this flag.

Maybe "override" instead of "specify" here? Or just some way of indicating that you're replacing the existing/default timestamp by using this flag.
VandalByte marked this conversation as resolved
@ -287,0 +300,4 @@
See https://borgbackup.readthedocs.io/en/stable/usage/recreate.html
for details.
Not passing --recompress is equivalent to "--recompress never".
Defaults to "if-different".
Owner

These two statements seem to be at odds...? The default can't be both "if-different" and "never". I might be misunderstanding.

These two statements seem to be at odds...? The default can't be both "if-different" and "never". I might be misunderstanding.
Author
Contributor

Yeah it's a bit confusing to be honest. I took it straight from the docs

recompress data chunks according to MODE and --compression. Possible modes are if-different: recompress if current compression is with a different compression algorithm (the level is not considered); always: recompress even if current compression is with the same compression algorithm (use this to change the compression level); and never: do not recompress (use this option to explicitly prevent recompression). If no MODE is given, if-different will be used. Not passing --recompress is equivalent to “--recompress never”.

I think what they meant is if recompress flag is not used, it's like using --recompress never, if it's used without a value then it's if-different

Yeah it's a bit confusing to be honest. I took it straight from the [docs](https://borgbackup.readthedocs.io/en/stable/usage/recreate.html) > recompress data chunks according to MODE and --compression. Possible modes are if-different: recompress if current compression is with a different compression algorithm (the level is not considered); always: recompress even if current compression is with the same compression algorithm (use this to change the compression level); and never: do not recompress (use this option to explicitly prevent recompression). If no MODE is given, if-different will be used. Not passing --recompress is equivalent to “--recompress never”. I think what they meant is if `recompress` flag is not used, it's like using `--recompress never`, if it's used without a value then it's `if-different`
Author
Contributor

I can remove that "never" part if it's too confusing.

I can remove that "never" part if it's too confusing.
Owner

Yeah I don't think it's possible to use without a value, given the current schema and the enum! If I'm reading it correctly, the possibilities are:

  • recompress is not present in the config at all, which I guess defaults to if-differentnever
  • recompress is set explicitly to one of if-different, always, or never
Yeah I don't think it's possible to use without a value, given the current schema and the enum! If I'm reading it correctly, the possibilities are: * `recompress` is not present in the config at all, which I guess defaults to ~~`if-different`~~`never` * `recompress` is set explicitly to one of `if-different`, `always`, or `never`
Author
Contributor

Using --recompress never is same as not using the flag at all, so it shouldn't be an issue right?

Right now how I have set it up is:

recompress = config.get('recompress', None)

So if it's not in config then, ignored completely, so same as using --recompress never

Using `--recompress never` is same as not using the flag at all, so it shouldn't be an issue right? Right now how I have set it up is: ```python recompress = config.get('recompress', None) ``` So if it's not in config then, ignored completely, so same as using `--recompress never`
Owner

Oops, yes, you are correct! So maybe change the description to Defaults to "never" and remove the bit about Not passing --recompress ...?

Oops, yes, you are correct! So maybe change the description to `Defaults to "never"` and remove the bit about `Not passing --recompress ...`?
Author
Contributor

I've made those changes.

I've made those changes.
Owner

Thank you so much!

Thank you so much!
witten merged commit 59f9d56aae into main 2025-03-29 22:07:52 +00:00
witten referenced this pull request from a commit 2025-03-29 22:07:53 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
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!1030
No description provided.