Support for Borg create & prune --stats via borgmatic command-line flag (#100) #115

Stunkymonkey · 2018-12-06T23:01:48Z

Stunkymonkey commented

2018-12-06 23:01:48 +00:00

i think this should fix the issue

witten reviewed 2018-12-07 05:59:12 +00:00

witten left a comment

Thank you for taking this on and sending a PR! I appreciate it.

borgmatic/borg/create.py

						
				@ -105,6 +105,7 @@ def create_archive(

				    local_path='borg',

				    remote_path=None,

				    progress=False,

				    stats=False,

witten commented

2018-12-07 05:46:15 +00:00

I think there's a minor edge case here where, if you specify borgmatic --create --stats --dry-run, Borg will explode. (--stats and --dry-run are not compatible, at least for --create.) See a few lines up in this file, where this is handled:

        + (('--stats',) if not dry_run and logger.isEnabledFor(logging.INFO) else ())

One way to deal with this for the new case would be just to disallow it explicitly.. For instance in your argument validation code in commands/borgmatic.py.

I think there's a minor edge case here where, if you specify `borgmatic --create --stats --dry-run`, Borg will explode. (`--stats` and `--dry-run` are not compatible, at least for `--create`.) See a few lines up in this file, where this is handled: ```python + (('--stats',) if not dry_run and logger.isEnabledFor(logging.INFO) else ()) ``` One way to deal with this for the new case would be just to disallow it explicitly.. For instance in your argument validation code in `commands/borgmatic.py`.

borgmatic/borg/create.py Outdated

						
				@ -153,6 +154,7 @@ def create_archive(

				        + (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) else ())

				        + (('--dry-run',) if dry_run else ())

				        + (('--progress',) if progress else ())

				        + (('--stats',) if stats else ())

witten commented

2018-12-07 05:58:44 +00:00

For this change in prune.py and the one in create.py, it looks like --stats will be passed to Borg twice in the case of any verbosity levels other than zero, because level 1 and 2 already pass in --stats. That won't break anything, but it does strike me as a little specific to add a whole borgmatic --stats parameter that's really only there for modifying the --verbosity 0 level. So what do you think of just making --verbosity 0 include --stats all the time without needing to specify it on borgmatic's command-line?

If we do that, then the next logical step, as much as it pains me to suggest it, would be to add a --verbosity -1 (corresponding to Python's logging.ERROR) that would be utterly silent and not include --stats. And then maybe that could even be the new default if no --verbosity is specified, so as to avoid actually changing the default behavior for users who are not specifying a --verbosity.

What do you think.. Am I overthinking this?

For this change in `prune.py` and the one in `create.py`, it looks like `--stats` will be passed to Borg twice in the case of any verbosity levels other than zero, because level 1 and 2 already pass in `--stats`. That won't break anything, but it does strike me as a little *specific* to add a whole borgmatic `--stats` parameter that's really only there for modifying the `--verbosity 0` level. So what do you think of just making `--verbosity 0` include `--stats` all the time without needing to specify it on borgmatic's command-line? If we do that, then the next logical step, as much as it pains me to suggest it, would be to add a `--verbosity -1` (corresponding to Python's `logging.ERROR`) that would be utterly silent and not include `--stats`. And then maybe that could even be the new default if no `--verbosity` is specified, so as to avoid actually changing the default behavior for users who are not specifying a `--verbosity`. What do you think.. Am I overthinking this?

Stunkymonkey commented

2018-12-10 11:25:07 +00:00

So what do you think of just making --verbosity 0 include --stats all the time without needing to specify it on borgmatic’s command-line?

Would fit my needs, but for people knowing borg and switching to borgmatic. Maybe they would be confused having different args.

This is why I prefer having a seperate arg for it.

> So what do you think of just making --verbosity 0 include --stats all the time without needing to specify it on borgmatic’s command-line? Would fit my needs, but for people knowing borg and switching to borgmatic. Maybe they would be confused having different args. This is why I prefer having a seperate arg for it.

witten commented

2018-12-10 19:29:20 +00:00

Okay. A related idea is to make the default verbosity pass --stats to Borg automatically (so presumably that's --verbosity 0). That way, the user doesn't need to specify --stats on the borgmatic command-line or even specify --verbosity to go get that stats output.

Okay. A related idea is to make the default verbosity pass `--stats` to Borg automatically (so presumably that's `--verbosity 0`). That way, the user doesn't need to specify `--stats` on the borgmatic command-line or even specify `--verbosity` to go get that stats output.

Stunkymonkey commented

2018-12-10 19:33:59 +00:00

sounds fine to me. but does a seperate arg hurt this much?

maybe you could have a second look on my commits.

sounds fine to me. but does a seperate arg hurt this much? maybe you could have a second look on my commits.

witten commented

2018-12-11 06:26:05 +00:00

Okay, I'll have a look when I get a chance and keep an open mind. I'm always trying to think hard about each new introduced command-line option and whether it's necessary. But I just added four more today so maybe I need to ease up. :)

Stunkymonkey commented

2018-12-17 00:26:31 +00:00

Why do you try to minimize the amount of cmd-line options?

I see borgmatic as a wrapper to eliminate the repo location. Which works perfectly.

Why do you try to minimize the amount of cmd-line options? I see borgmatic as a wrapper to eliminate the repo location. Which works perfectly.

borgmatic/commands/borgmatic.py Outdated

						
				@ -88,0 +90,4 @@

				        dest='stats',

				        default=False,

				        action='store_true',

				        help='Display status with --create or --prune option for each file as it is backed up',

witten commented

2018-12-07 05:39:48 +00:00

Is this description correct? Maybe "status" -> "stats"? And the statistics are displayed at the end rather than as each file is backed up, right?

Stunkymonkey commented

2018-12-10 11:25:25 +00:00

copy-paste error

Stunkymonkey commented

2018-12-22 22:40:56 +00:00

any updates, how we are continuing?

witten commented

2018-12-24 00:02:54 +00:00

Why do you try to minimize the amount of cmd-line options?

Just in the spirit of minimalism and a simple user experience. If borgmatic --help is several pages long, that can be overwhelming and make the tool harder to use.

But I think you've sold me on the value of --stats outweighing that concern here, especially for just one extra command-line flag.

I'll merge shortly. Please feel free to add yourself to the AUTHORS file if you like.

> Why do you try to minimize the amount of cmd-line options? Just in the spirit of minimalism and a simple user experience. If `borgmatic --help` is several pages long, that can be overwhelming and make the tool harder to use. But I think you've sold me on the value of `--stats` outweighing that concern here, especially for just one extra command-line flag. I'll merge shortly. Please feel free to add yourself to the `AUTHORS` file if you like.

witten closed this pull request

2018-12-24 00:04:23 +00:00

witten deleted branch stats

2018-12-24 00:06:43 +00:00

Stunkymonkey commented

2019-01-07 22:04:23 +00:00

keep the good work up. really appreciate all you have done.