Warn if wrong argument order with borgmatic borg #575

New Issue

remyabel · 2022-08-25T17:16:37Z

remyabel commented

2022-08-25 17:16:37 +00:00

What I'm trying to do and why

Steps to reproduce (if a bug)

If you intermix positional arguments with switches, because Borgmatic prepends the repository to borgmatic borg, it can result in surprising behavior. Example:

borgmatic -v 2 -n borg -- create repo::archive -s --progress /home/test
#                                ^ injected by borgmatic

Actual behavior (if a bug)

borg: error: unrecognized arguments: /home/test
Command 'borg create /home/backups/backup-borg::hostname-2022-08-25T00:10:55.951954 -s --progress /home/test --debug --show-rc' returned non-zero exit status 2.

Expected behavior (if a bug)

Show a more useful error message (maybe point to man borg). Or automatically fix it for the user (maybe more difficult to do).

Other notes / implementation ideas

See man borg for details:


     Borg only supports taking options (-s and --progress in the example) to the left or right of all positional  arguments  (repo::archive  and path in the example), but not in between them:
      borg create -s --progress repo::archive path  # good and preferred
      borg create repo::archive path -s --progress  # also works
      borg create -s repo::archive path --progress  # works, but ugly
      borg create repo::archive -s --progress path  # BAD

Environment

borgmatic version: [version here]

1.6.6

borgmatic installation method: RPM package

Borg version: 1.2.2

Python version: 3.10.6

Database version (if applicable):

operating system and version: Fedora 36

#### What I'm trying to do and why #### Steps to reproduce (if a bug) If you intermix positional arguments with switches, because Borgmatic prepends the repository to `borgmatic borg`, it can result in surprising behavior. Example: ``` borgmatic -v 2 -n borg -- create repo::archive -s --progress /home/test # ^ injected by borgmatic ``` #### Actual behavior (if a bug) ``` borg: error: unrecognized arguments: /home/test Command 'borg create /home/backups/backup-borg::hostname-2022-08-25T00:10:55.951954 -s --progress /home/test --debug --show-rc' returned non-zero exit status 2. ``` #### Expected behavior (if a bug) Show a more useful error message (maybe point to `man borg`). Or automatically fix it for the user (maybe more difficult to do). #### Other notes / implementation ideas See `man borg` for details: ``` Borg only supports taking options (-s and --progress in the example) to the left or right of all positional arguments (repo::archive and path in the example), but not in between them: borg create -s --progress repo::archive path # good and preferred borg create repo::archive path -s --progress # also works borg create -s repo::archive path --progress # works, but ugly borg create repo::archive -s --progress path # BAD ``` #### Environment **borgmatic version:** [version here] 1.6.6 **borgmatic installation method:** RPM package **Borg version:** 1.2.2 **Python version:** 3.10.6 **Database version (if applicable):** **operating system and version:** Fedora 36

witten commented

2023-03-26 22:24:45 +00:00

Thanks for filing this, and I apologize for the delay in getting to it. I think there are a couple of things going wrong here:

The argument ordering issue you've identified. borg wants flags to come after the repository/archive and path, at least with Borg 1.x. E.g., borg create repo::test /home/test -s --progress --debug --show-rc works fine but the borgmatic-generated borg create 1.2.borg::newtest -s --progress /home/test --debug --show-rc (with positional arguments and flags interleaved) doesn't. This would be possible to fix, but it'd require more invasive parsing of Borg options so that borgmatic-added options like --debug could get injected into the right spot. Alternatively, borgmatic could regress the ability to set Borg's verbosity with borgmatic borg.
You've tried to specify the repository and archive with borg parameters instead of doing so with borgmatic. From the docs:

borgmatic supplies the repository/archive name to Borg for you (based on your borgmatic configuration or the borgmatic borg --repository/--archive arguments), so do not specify the repository/archive otherwise.

Besides me unhelpfully saying "just don't do that," it's possible that parsing borg arguments could allow borgmatic to helpfully provide an error message that says "just don't do that" whenever a repo or archive is provided directly to borg in a borgmatic borg command.

Thanks for filing this, and I apologize for the delay in getting to it. I think there are a couple of things going wrong here: * The argument ordering issue you've identified. `borg` wants flags to come *after* the repository/archive and path, at least with Borg 1.x. E.g., `borg create repo::test /home/test -s --progress --debug --show-rc` works fine but the borgmatic-generated `borg create 1.2.borg::newtest -s --progress /home/test --debug --show-rc` (with positional arguments and flags interleaved) doesn't. This would be possible to fix, but it'd require more invasive parsing of Borg options so that borgmatic-added options like `--debug` could get injected into the right spot. Alternatively, borgmatic could regress the ability to set Borg's verbosity with `borgmatic borg`. * You've tried to specify the repository and archive with `borg` parameters instead of doing so with borgmatic. From [the docs](https://torsion.org/borgmatic/docs/how-to/run-arbitrary-borg-commands/#limitations): > borgmatic supplies the repository/archive name to Borg for you (based on your borgmatic configuration or the borgmatic borg --repository/--archive arguments), so do not specify the repository/archive otherwise. Besides me unhelpfully saying "just don't do that," it's possible that parsing `borg` arguments could allow borgmatic to helpfully provide an error message that says "just don't do that" whenever a repo or archive is provided directly to borg in a `borgmatic borg` command.

witten referenced this issue

2023-06-26 16:32:36 +00:00

Borg 1.7.15 breaks `borg key export` command #719

witten commented

2023-06-26 16:54:20 +00:00

Some more recent thoughts on this: Looking at the code, I'm beginning to think it's increasingly untenable for borgmatic to try to inject the repository and archive into the right place in the Borg command-line. That's because:

Borg has very particular ideas about argument ordering and will error if you stray too far from what it wants, and:
borgmatic can't always automatically inject the repository/archive into the right spot without a full understanding of the Borg command-line. For instance, it's not sufficient to sort arguments into --flags and positional arguments, because borgmatic would have to keep any given --flag next to its value.. And to do that, borgmatic would have to know if a particular Borg --flag even has a value—for every single Borg sub-command in every single supported version of Borg. This is not a road I want to go down.

So, instead, here's what I propose. borgmatic could drop the whole repository/archive injection logic. In its place, borgmatic would simply pass a REPOSITORY environment variable to Borg so that you could use those values in your borgmatic borg command-line. That means it would be the user's responsibility to make use of that REPOSITORY in their borgmatic borg commands. Example:

borgmatic borg delete --dry-run '$REPOSITORY::myarchive'

Or:

borgmatic borg key export '$REPOSITORY'

(Yes, the single quotes are necessary to prevent the shell borgmatic is running in from trying to resolve the environment variables.)

What about $ARCHIVE? Well, since the archive comes from the command-line today (via a borgmatic --archive flag), it would be a little silly for the user to have to specify an --archive flag to borgmatic only for borgmatic to turn around and pass that value to Borg via an environment variable. So for archive, I propose to cut out the middleman and require the user to specify it directly as needed for particular Borg sub-commands. And then I'd remove the --archive flag from borgmatic borg. (EDIT: The only problem with this approach is we'd lose support for borgmatic's --archive latest on the borg action. Maybe it would be best to leave --archive and make it plumb through to $ARCHIVE for optional use in the command?)

borgmatic could still continue injecting certain flags like --debug and --lock-wait, because those can unconditionally go at the beginning of the Borg command-line and I think in all cases those will continue to get parsed correctly by Borg. (If not, this part of the plan kind of falls apart.)

And yes, all of this would be a breaking change. But given that borgmatic borg is hopefully mostly used for one-off commands rather than run in scripts (😬🤞), that might not be too onerous of a change.

Some more recent thoughts on this: Looking at the code, I'm beginning to think it's increasingly untenable for borgmatic to try to inject the repository and archive into the right place in the Borg command-line. That's because: 1. Borg has [very particular ideas](https://borgbackup.readthedocs.io/en/master/usage/general.html#positional-arguments-and-options-order-matters) about argument ordering and will error if you stray too far from what it wants, and: 2. borgmatic can't always automatically inject the repository/archive into the right spot without a full understanding of the Borg command-line. For instance, it's not sufficient to sort arguments into --flags and positional arguments, because borgmatic would have to keep any given --flag next to its value.. And to do that, borgmatic would have to know if a particular Borg --flag even *has* a value—for every single Borg sub-command in every single supported version of Borg. This is not a road I want to go down. So, instead, here's what I propose. borgmatic could drop the whole repository/archive injection logic. In its place, borgmatic would simply pass a `REPOSITORY` environment variable to Borg so that you could use those values in your `borgmatic borg` command-line. That means it would be the user's responsibility to make use of that `REPOSITORY` in their `borgmatic borg` commands. Example: ``` borgmatic borg delete --dry-run '$REPOSITORY::myarchive' ``` Or: ``` borgmatic borg key export '$REPOSITORY' ``` (Yes, the single quotes are necessary to prevent the shell borgmatic is running in from trying to resolve the environment variables.) What about `$ARCHIVE`? Well, since the archive comes from the command-line today (via a borgmatic `--archive` flag), it would be a little silly for the user to have to specify an `--archive` flag to borgmatic only for borgmatic to turn around and pass that value to Borg via an environment variable. So for archive, I propose to cut out the middleman and require the user to specify it directly as needed for particular Borg sub-commands. And then I'd remove the `--archive` flag from `borgmatic borg`. (EDIT: The only problem with this approach is we'd lose support for borgmatic's `--archive latest` on the `borg` action. Maybe it would be best to leave `--archive` and make it plumb through to `$ARCHIVE` for optional use in the command?) borgmatic could still continue injecting certain flags like `--debug` and `--lock-wait`, because those can unconditionally go at the beginning of the Borg command-line and I *think* in all cases those will continue to get parsed correctly by Borg. (If not, this part of the plan kind of falls apart.) And yes, all of this would be a breaking change. But given that `borgmatic borg` is hopefully mostly used for one-off commands rather than run in scripts (😬🤞), that might not be too onerous of a change.

witten referenced this issue from a commit

2023-06-26 21:37:37 +00:00

Revamp "borg" action to support REPOSITORY and ARCHIVE env vars instead of implicitly injecting repository/archive into the Borg command (#575).

witten referenced this issue from a commit

2023-06-26 21:37:37 +00:00

Fix typos (#575).

witten commented

2023-06-26 21:45:52 +00:00

Well, I went ahead and implemented this in main. See the documentation for the details of what I settled on, which is very similar to the proposal above. I'm still very keen on receiving feedback though—especially before this gets released in the next version of borgmatic! Now you've just got a complete implementation + docs to react to.

Well, I went ahead and implemented this in main. See [the documentation](https://torsion.org/borgmatic/docs/how-to/run-arbitrary-borg-commands/) for the details of what I settled on, which is very similar to the proposal above. I'm still very keen on receiving feedback though—especially before this gets released in the next version of borgmatic! Now you've just got a complete implementation + docs to react to.

witten closed this issue

2023-06-26 21:45:52 +00:00

witten reopened this issue

2023-07-02 15:11:19 +00:00

witten commented

2023-07-02 15:13:10 +00:00

From Thomas Waldmann:

@borgmatic If you'ld set the BORG_REPO env var instead, borg would just use that (without the user having to use $BORG_REPO).

Also, repo::archive can be abbreviated as ::archive.

If a command requires giving a repo as position argument (e.g. because of following pos. other args), one can use :: then."

This probably means we can lose the whole '$REPOSITORY' env var and change it to $BORG_REPO which Borg will read directly.

From [Thomas Waldmann](https://chaos.social/@ThomasWaldmann/110644172693491243): > @borgmatic If you'ld set the BORG_REPO env var instead, borg would just use that (without the user having to use $BORG_REPO). > > Also, repo::archive can be abbreviated as ::archive. > > If a command requires giving a repo as position argument (e.g. because of following pos. other args), one can use :: then." This probably means we can lose the whole `'$REPOSITORY'` env var and change it to `$BORG_REPO` which Borg will read directly.

witten referenced this issue from a commit

2023-07-03 07:09:45 +00:00

For "borgmatic borg", pass the repository to Borg via a Borg-supported environment variable (#575).

witten commented

2023-07-03 07:10:10 +00:00

Implemented. Now commands are even simpler! Documentation update will be published shortly.

witten closed this issue

2023-07-03 07:10:10 +00:00

witten commented

2023-07-19 06:03:06 +00:00

Released in borgmatic 1.8.0!

Sign in to join this conversation.