Is it difficult to run init only when needed, and fail on error? This seems to ignore errors, which gives me a bad feeling.

It is better to touch a file after borg init and only call borg init if this file does not exist.

I think it is better to call e.g. borg list to check if a repo exists. This will also cover the case where the repo has been created manually.

I think borg has some safety measures in case somebody creates backup on an external drive/remote filesystem and the mount is currently not active to avoid making backups on the underlying device.
Can we cover this as well?

The reason is that user might thing their backup works, while they backup the whole time on the same filesystem where the data is located.

I don't know of such measures. Maybe adding a doInit option is a good idea?

Missing default value.

daily is a good default.

Maybe say that this runs every time the job runs. Or change the backup job script template so that "borg init" is only invoked once, followed by postInit hook. I'd prefer the latter.

Is this "after" needed? (I don't see this pattern anywhere in nixpkgs.)

It isn't. Thanks for catching that.

I would prefer to have type just be "nonEmptyListOf path". Reasoning: the option and its documentation is in plural, so not using a list feels weird to me.

It's just for convenience. I usually back up just one directory and this way I don't have to write the brackets. I can write Path(s) to back up. if you want :)

It's fine. I'm just talking out loud :-)

thinking out loud

Doesn't this make postHook run twice?

You're right. Didn't think about that.

You can pass strings directly to "openssh.authorizedKeys.keys" instead of writing files and passing them to "...keyFiles".

I used keyFiles because I wasn't able to merge multiple keys options using mkMerge. But I tried again and it worked :D

Perhaps do something about this FIXME before merge?

The things necessary to use ProtectSystem = "strict":

• assert that createHome == true (or don't assert it and just try to create the directory)
• create $HOME/{.config,.cache}/borg

I went for the assertion, but root doesn't have an entry in config.users by default, so it is treated specially.

Dropped the assertion again. Home dirs can also be created in other ways:

Please add a warning that the passphrase will be world readable in the Nix store, and recommend to use the "passCommand" option instead.

Missing dot. Perhaps add "See 'borg help patterns' for pattern syntax"?

Missing dot. (Also on a couple of other option descriptions.)

Feature request. Add optional "cd ${cfg.path}" in front of that "borg server" (Inspired by http://borgbackup.readthedocs.io/en/stable/deployment/central-backup-server.html#restrictions.) This allows clients to ignore the server directory layout, and can use repo urls like backup@server:myrepo.

Nice idea!
Btw, the example you linked to uses --restrict-to-path instead of --restrict-to-repository, which I use. Therefore, as it is currently implemented there is no way of using multiple repos with the same key (except if you use multiple users on the storage server).

Do you have a use case for --restrict-to-path?
I implemented the working directory change, but noticed the way you have to specify the repo is quite ugly (user@machine:.).

Re: --restrict-to-path. I use that in my local setup (not yet pushed to github). I chose that option because it allowed the client to create the repository it needs. I have something like /data/borgbackup/hosts/machine1 and .../machine2, where each machine may have a number of repositories, managed by the client. I'm currently experimenting with the setup, hence it's not pushed yet. I don't have a good answer. I guess I can flip it around and ask "what's the use case of --restrict-to-repo"?

Ah, it should be "cd ${config.services.borgbackup.repos.${name}.path}". (Or whatever expands to the path where repos are stored, e.g. /var/lib/borgbackup.) Then we can specify repo correctly, I think, without the "." (dot).

Btw, I think it's a good idea to "cd dir && borg serve ...", so that borg isn't run if the cd fails. (Unless that's considered a feature in this setup -- I didn't check.)

You're right, there's not much use in --restrict-to-repository for restricting repository access. However, I think --restrict-to-path makes --storage-quota useless.

No, we can't leave out the dot:

$ BORG_REPO="machine:" borg create ::archive path
Repository /home/user/machine: does not exist.

I think the best solution is an option called allowSubRepos. Do you think it should be true or false by default?

Good point about the --storage-quota. Ok, now I think I understand why there are two --restrict-to-* options :-)

Re: the dot. I was thinking of this syntax: BORG_REPO="machine:repo.borg" borg create ::archive path.

I think the best solution is an option called allowSubRepos. Do you think it should be true or false by default?

No opinion yet. Do you plan to select the --restrict-to-* option based on allowSubRepos?

Yes exactly. See the third commit.

Sweet.

Dude.

A thing I just noticed: If you specify a relative path for cfg.repo, this will not trigger and borg won't have access to the repo. I think I should add a fancier test for isPath, e.g. check if no : is contained. It appears like all of

borg init -e none foo:bar
borg init -e none foo\:bar
borg init -e none foo\\:bar

try to use SSH. So I don't see a possibility for using a relative path with a colon. I however did not read completely through borgbackup/borg#1705 and borgbackup/borg#1710.

I'm stupid. Of course there is a way: You can use ./foo:bar.

That's the best I could come up with:

isLocalPath = x:
    builtins.substring 0 1 x == "/"      # Absolute path
    || builtins.substring 0 1 x == "."   # Relative path
    || builtins.match "[.*:.*]" == null; # not machine:path

I see that in the current version of this PR, "archiveName" is sometimes the base archive name, and sometimes the full archive name (with datetime suffix). On some level, this confuses me. How do you feel about having separate names? (I guess you didn't opt for my "archiveBaseName" before for a reason, but I'd like to hear your thoughts.)

Separate names are definitely less confusing 👍
The only reason I used archiveName is that archiveBaseName doesn't make sense when using placeholders.

The new name is $archive_name.

Why change naming style for this variable? (The other variables are lowerCamelCase.)

Stupid typo: $achiveName instead of $archiveName. Therefore grep didn't find it.

I thought that style was more common for bash variables. But I can indeed change that back if you want.

I agree about snake_case being more common, and it's what I use myself. But the module currently mixes styles: e.g. $archive_name and $extraInitArgs. That should be fixed, either way.

You know what, I think that since some variables are inherited from the NixOS config, with lowerCamelCase, it feels weird to convert them to $snake_case. How about making all shell variables that are exposed (documented) as part of the module interface $lowerCamelCase?

👍
I named them $archiveName, $archiveSuffix and $exitStatus.