file:// paths for repositories #576

New Issue

witten · 2022-08-26T18:23:22Z

witten commented

2022-08-26 18:23:22 +00:00

What I'm trying to do and why

Borg supports file:// style paths for repositories, but borgmatic currently doesn't. That's because borgmatic assumes if a : character occurs in a repository path, it must be a remote repository. When borgmatic thinks a repository is remote, it:

Won't make its path absolute before passing it to Borg, which causes problems if a working directory is set.
Will incorrectly normalize/rewrite the path when converting it from a old-style "remote" repository to a new-style ssh:// repository.

Why add support for this: Given that Borg supports these paths, and given that Borg 2 is moving to only ssh://-style paths for remote repositories, users may have the expectation that file://-style paths will work as well. Additionally, the current failure mode when they're used is pretty obnoxious / non-obvious.

Other notes / implementation ideas

Effected files: config/validate.py, config/normalize.py, borg/extract.py, export_tar.py.

It would be nice if, as part of this work, all of this logic could be moved into one place so it's not scattered across several source files. Maybe it could be part of the existing normalization step.

#### What I'm trying to do and why Borg supports `file://` style paths for repositories, but borgmatic currently doesn't. That's because borgmatic assumes if a `:` character occurs in a repository path, it must be a remote repository. When borgmatic thinks a repository is remote, it: * Won't make its path absolute before passing it to Borg, which causes problems if a working directory is set. * Will incorrectly normalize/rewrite the path when converting it from a old-style "remote" repository to a new-style `ssh://` repository. Why add support for this: Given that Borg supports these paths, and given that Borg 2 is moving to only `ssh://`-style paths for remote repositories, users may have the expectation that `file://`-style paths will work as well. Additionally, the current failure mode when they're used is pretty obnoxious / non-obvious. #### Other notes / implementation ideas Effected files: `config/validate.py`, `config/normalize.py`, `borg/extract.py`, `export_tar.py`. It would be nice if, as part of this work, all of this logic could be moved into one place so it's not scattered across several source files. Maybe it could be part of the existing normalization step.

witten changed title from ~~file:// paths in repositories~~ to file:// paths for repositories

2022-08-26 18:23:33 +00:00

witten added the

good first issue

label 2023-02-04 17:13:42 +00:00

diivi commented

2023-03-15 20:02:04 +00:00

Converting the file:// paths to absolute paths that Borgmatic currently uses in the validate.py and normalize.py files should be enough right?
Other than this do only extract.py and export_tar.py use the actual repository path from the configuration? Specifically these lines :

 flags.make_repository_archive_flags(
    repository if ':' in repository else os.path.abspath(repository),
    archive,
    local_borg_version,
)

If so, I can take a look at this.

Converting the `file://` paths to absolute paths that Borgmatic currently uses in the `validate.py` and `normalize.py` files should be enough right? Other than this do only `extract.py` and `export_tar.py` use the actual repository path from the configuration? Specifically these lines : ```python flags.make_repository_archive_flags( repository if ':' in repository else os.path.abspath(repository), archive, local_borg_version, ) ``` If so, I can take a look at this.

witten commented

2023-03-16 21:13:23 +00:00

Thanks for taking a look!

Converting the file:// paths to absolute paths that Borgmatic currently uses in the validate.py and normalize.py files should be enough right?

That sounds like a totally reasonable approach. One thing you'll need to account for though is that if the user has file:///foo/bar in repositories in their configuration file and types --repository file:///foo/bar on the command-line to select that repository, it should ideally still match.

Other than this do only extract.py and export_tar.py use the actual repository path from the configuration?

Besides those two, the other borgmatic actions don't look at the repository path directly, but they call functions that do. (And they do pass the repository path to Borg.) For instance, several of the actions in borgmatic/actions/*.py call borgmatic.config.validate.repositories_match() which calls normalize_repository_path() which does the ':' repository path introspection trick.

Thanks for taking a look! > Converting the file:// paths to absolute paths that Borgmatic currently uses in the validate.py and normalize.py files should be enough right? That sounds like a totally reasonable approach. One thing you'll need to account for though is that if the user has `file:///foo/bar` in `repositories` in their configuration file and types `--repository file:///foo/bar` on the command-line to select that repository, it should ideally still match. > Other than this do only extract.py and export_tar.py use the actual repository path from the configuration? Besides those two, the other borgmatic actions don't look at the repository path directly, but they call functions that do. (And they do pass the repository path to Borg.) For instance, several of the actions in `borgmatic/actions/*.py` call `borgmatic.config.validate.repositories_match()` which calls `normalize_repository_path()` which does the `':'` repository path introspection trick.

diivi commented

2023-03-18 12:11:47 +00:00

if the user has file:///foo/bar in repositories in their configuration file and types --repository file:///foo/bar on the command-line to select that repository, it should ideally still match.

This should be taken care of in borgmatic.py::run_actions() right?

def run_actions(
    *,
    arguments,
    config_filename,
    location,
    storage,
    retention,
    consistency,
    hooks,
    local_path,
    remote_path,
    local_borg_version,
    repository_path,
):
	...
	repository = os.path.expanduser(repository_path)

and for

several of the actions in borgmatic/actions/*.py call borgmatic.config.validate.repositories_match() which calls normalize_repository_path() which does the ':' repository path introspection trick.

I would have already edited that function when handling this conversion for config files, so the 4 files you mentioned above and borgmatic.py will be the only files I'll touch.

Just making sure I don't miss anything.

>if the user has file:///foo/bar in repositories in their configuration file and types --repository file:///foo/bar on the command-line to select that repository, it should ideally still match. This should be taken care of in `borgmatic.py::run_actions()` right? ```python def run_actions( *, arguments, config_filename, location, storage, retention, consistency, hooks, local_path, remote_path, local_borg_version, repository_path, ): ... repository = os.path.expanduser(repository_path) ``` and for > several of the actions in borgmatic/actions/*.py call borgmatic.config.validate.repositories_match() which calls normalize_repository_path() which does the ':' repository path introspection trick. I would have already edited that function when handling this conversion for config files, so the 4 files you mentioned above and `borgmatic.py` will be the only files I'll touch. Just making sure I don't miss anything.

witten commented

2023-03-18 16:57:12 +00:00

This should be taken care of in borgmatic.py::run_actions() right?

Are you asking if the start of run_actions() is where the two repository paths should be matched? Or where they should be normalized? Because I think the matching should occur in borgmatic/config/validate.py:repositories_match() and normalization should happen in borgmatic/config/normalize.py:normalize(). Or am I misunderstanding?

I would have already edited that function when handling this conversion for config files, so the 4 files you mentioned above and borgmatic.py will be the only files I'll touch.

That sounds right.

> This should be taken care of in borgmatic.py::run_actions() right? Are you asking if the start of `run_actions()` is where the two repository paths should be matched? Or where they should be normalized? Because I think the matching should occur in `borgmatic/config/validate.py:repositories_match()` and normalization should happen in `borgmatic/config/normalize.py:normalize()`. Or am I misunderstanding? > I would have already edited that function when handling this conversion for config files, so the 4 files you mentioned above and borgmatic.py will be the only files I'll touch. That sounds right.

diivi commented

2023-03-18 17:08:51 +00:00

Are you asking if the start of run_actions() is where the two repository paths should be matched? Or where they should be normalized?

I wanted to ask if --repository file:///foo/bar be changed to --repository /foo/bar in borgmatic.py::run_actions() (internally changing the param)?
Or will this be automatically handled after I make changes to the other files?

> Are you asking if the start of `run_actions()` is where the two repository paths should be matched? Or where they should be normalized? I wanted to ask if `--repository file:///foo/bar` be changed to `--repository /foo/bar` in `borgmatic.py::run_actions()` (internally changing the param)? Or will this be automatically handled after I make changes to the other files?

witten commented

2023-03-18 20:43:16 +00:00

Ah, gotcha. The repository_path in run_actions() comes from a borgmatic configuration file originally, so at that point it would presumably already be changed to a non-file:// path (after the changes you make to the other files).

However, that doesn't handle the --repository file:///foo/bar case! That's because that repository path is part of each action's arguments. But if you rewrite the command-line repository path as part of borgmatic/config/validate.py:repositories_match() (or alternatively, in each function that calls it), that would take care of the --repository flag.

Ah, gotcha. The `repository_path` in `run_actions()` comes from a borgmatic configuration file originally, so at that point it would presumably already be changed to a non-`file://` path (after the changes you make to the other files). However, that doesn't handle the `--repository file:///foo/bar` case! That's because *that* repository path is part of each action's arguments. But if you rewrite the command-line repository path as part of `borgmatic/config/validate.py:repositories_match()` (or alternatively, in each function that calls it), that would take care of the `--repository` flag.

👍 1

witten referenced this issue from a commit

2023-03-21 17:29:42 +00:00

Support file:// paths for repositories (#576).

witten referenced this issue from a commit

2023-03-21 17:29:42 +00:00

Add "file://" paths to NEWS (#576).

witten referenced this issue from a commit

2023-03-21 17:29:42 +00:00

Remove unused module and outdated test expectations (#576).

witten commented

2023-03-21 17:31:42 +00:00