Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BORGBACKUP_COMPRESSION now handles Borg compression defaults correctly #1044

Merged
merged 1 commit into from Oct 21, 2016
Merged

Conversation

gozora
Copy link
Member

@gozora gozora commented Oct 20, 2016

Improvements in default compression handling discusses in #1041

@jsmeix jsmeix merged commit 8e1cef8 into rear:master Oct 21, 2016
@jsmeix jsmeix added fixed / solved / done minor bug An alternative or workaround exists labels Oct 21, 2016
@jsmeix jsmeix added this to the Rear v1.20 milestone Oct 21, 2016
@jsmeix jsmeix self-assigned this Oct 21, 2016
@jsmeix
Copy link
Member

jsmeix commented Oct 21, 2016

Now it behaves perfectly - as far as I can see.

@jsmeix
Copy link
Member

jsmeix commented Oct 21, 2016

@gozora
a general comment regarding specific variables
for specific backup program option settings:

I think you could completely remove
BORGBACKUP_COMPRESSION
and support instead two generic
BORGBACKUP_CREATE_OPTIONS=()
BORGBACKUP_EXTRACT_OPTIONS=()
where the user can specify any options he wants
for "borg create" and "borg extract".

This way you would be automatically future-proof and
avoid an possibly endless sequence of user requests
to support this and that and those further specific Borg
backup program option settings.

@gozora gozora deleted the borg branch October 21, 2016 09:10
@gozora
Copy link
Member Author

gozora commented Oct 21, 2016

@jsmeix actually I was thinking about such setup, but decided to go different way. Thinking of it now, it really could be easier.
I'll check that, thanks!

@jsmeix
Copy link
Member

jsmeix commented Oct 21, 2016

@gozora
FYI regarding how to use and implement multiple
program options settings via an array variable
you may have a look at
#912

I think I vaguely remember that quoting with " as in

COMMAND ... "${BACKUP_PROG_COMPRESS_OPTIONS[@]}" ...

could be crucial, cf.
http://stackoverflow.com/questions/3348443/a-confusion-about-array-versus-array-in-the-context-of-a-bash-comple

@gozora
Copy link
Member Author

gozora commented Oct 21, 2016

@jsmeix thanks for hint!

@gozora
Copy link
Member Author

gozora commented Oct 23, 2016

@jsmeix

I think you could completely remove
BORGBACKUP_COMPRESSION
and support instead two generic
BORGBACKUP_CREATE_OPTIONS=()
BORGBACKUP_EXTRACT_OPTIONS=()
where the user can specify any options he wants
for "borg create" and "borg extract".

Now I maybe know why I've abandoned this idea.

It would require to add 5 different variables for each type of Borg operations (init, create, prune, list, extract). The code I've wrote relies on some options in use (I did not tested what would happen if some options are omitted).
As an example let's have a look on borg create and borg prune.
I've used variable BORGBACKUP_ARCHIVE_PREFIX to define archives that have been created by ReaR. So every time you run rear mkbackup, archive with name BORGBACKUP_ARCHIVE_PREFIX_<incremented_archive_id> is created in Borg repository. User can add arbitrary new archives to this repository (and do restore from then with rear recover), but with such naming convention we will know which archives has been created using ReaR. This property is used e.g. during borg prune (which cleans repository according user defined settings) to avoid removing of user created archives. (ReaR will prune only archives with prefix BORGBACKUP_ARCHIVE_PREFIX_).
To cut the story short, from my point of view, current code is more straight forward to configure, easier to maintain and easier to look for a bugs ...
Let us just wait for first users to report some issues and flat down the code after we have some feedback from actual testing use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed / solved / done minor bug An alternative or workaround exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants