#2408 PR merged: Work on selecting the Borg archive to restore from.¶
Labels: enhancement, cleanup, fixed / solved / done
flyinggreenfrog opened issue at 2020-05-25 16:56:¶
Pull Request Details:¶
-
Type: Enhancement
-
Impact: Normal
-
Reference to related issue (URL): N/A
-
How was this pull request tested? openSUSE Leap 15.1 Baremetal via USB rescue medium
-
Brief description of the changes in this pull request:
Up until now while recover from Borg backup it shows all rear archives to the user to select from. That can be a very long list with hundreds of archives. Only the last items are visble.
With this PR a number (configurable, default is 10) of recent commits is shown. A option is added for the user to set that a next set (default 10) of older archives is presented to the user. If all available archives are shown, and the user didn't select anything valid, we cycle through available archives again from the beginning.
More over output format was adjusted a little bit: before it was
something like [<NR>] <ARCHIVE_NAME> <DATE> ..., we change it to
[<NR>] <DATE> <ARCHIVE_NAME>, since archive names are not fixed with
their width, that was resulting in not aligned output. <DATE> has
fixed width, so last column with archive name takes then whatever space
the name needs.
gozora commented at 2020-05-25 19:48:¶
Hello @jsmeix,
I know that you are not Borg user (although one day you you will
certainly run out of disk space and start using it :-) ), but could you
please take a quick look here if you don't see anything obviously wrong?
Thanks
V.
flyinggreenfrog commented at 2020-05-25 22:27:¶
But since there might be people who don't like to "scroll" or need to stick to current way for what ever reason, could you consider to implement something like:
BORGBACKUP_RESTORE_ARCHIVES_SHOW_NUMBER=-1where pagination will not be applied?
Good idea. However I used value 0 to disable pagination, because
showing zero entries doesn't make sense imho.
I also tested, that BORGBACKUP_RESTORE_ARCHIVES_SHOW_NUMBER >= 0 in
prep step.
My original attempt to change the order of shown entries (from newest to oldest), I reverted back to from oldest to newest, so that in case pagination is disabled, even with a lot of entries, the newest ones are shown last and are visible to the user, as to resemble behaviour before my PR.
If BORGBACKUP_RESTORE_ARCHIVES_SHOW_NUMBER is set to a positive
number, then show archives starting with
<NEWEST> - BORGBACKUP_RESTORE_ARCHIVES_SHOW_NUMBER + 1 to <NEWEST>
with pagination enabled.
EDIT:
In case BORGBACKUP_RESTORE_ARCHIVES_SHOW_NUMBER is set to a number
greater or equal to the number of available archives, pagination is
effectively also disabled. But for disabling pagination by the user,
setting to zero should be the way to go, since otherwise you have to
know the number of available archives before, and the number of archives
will also change over time, when backups are made.
flyinggreenfrog commented at 2020-05-26 04:13:¶
Instead of the
if [[ $BORGBACKUP_RESTORE_ARCHIVES_SHOW_NUMBER -eq 0 ]]; clause
setting
$BORGBACKUP_RESTORE_ARCHIVES_SHOW_NUMBER=$(( archive_cache_lines )) in
case BORGBACKUP_RESTORE_ARCHIVES_SHOW_NUMBER=0 before the while
loop,should do the same, since the code in the current else clause
should handle properly that case then.
But either way there is a if construct, so I would just leave the PR
like it is now, if no other objections/opinions about that.
flyinggreenfrog commented at 2020-05-26 05:25:¶
On a second thought, using same code, just resetting
BORGBACKUP_RESTORE_ARCHIVES_SHOW_NUMBER to the number of available
archives $archive_cache_lines seems cleaner and better imho.
Added commit 779ee6dfcf10b0e1d6a543fb0f5c9dbedd84bdb5.
Tested with pagination and without it (disabled: set to zero). Both worked liked expected.
jsmeix commented at 2020-05-26 08:44:¶
@gozora
don't worry because of my
https://github.com/rear/rear/pull/2402#issuecomment-633508778
I will have a look here for generic "possibly suspicious code".
What I cannot review is whether or not Borg related code
does actually do what is intended because as non-Borg user
I do not understand how Borg behaves.
flyinggreenfrog commented at 2020-05-26 09:09:¶
When this part of the code is already reviewed currently, just one
question. This particular break for the while loop after the Error
seems to me dead code? As it is never reached, because with the Error
it would leave with that Error already? Or it has to be there for some
reasons? @jsmeix @gozora
gozora commented at 2020-05-26 09:17:¶
@flyinggreenfrog
As it is never reached, because with the Error it would leave with that Error already
Correct, I guess the break statement can be removed since Error will
exit the code either way.
V.
flyinggreenfrog commented at 2020-05-26 14:20:¶
From plain looking at the code (as non-Borg user)
I did not notice something that could not work
so I approve it.
Tried to work on all your suggestions.
gozora commented at 2020-05-26 17:44:¶
@jsmeix thanks for your review and inputs, as always it is much appreciated!
V.
gozora commented at 2020-05-26 17:45:¶
@flyinggreenfrog I'll not have time to do review today. I need to postpone until tomorrow.
Sorry for that!
V.
flyinggreenfrog commented at 2020-05-27 04:31:¶
@flyinggreenfrog I'll not have time to do review today. I need to postpone until tomorrow.
Sorry for that!
V.
No problem, take your time.
gozora commented at 2020-05-27 06:14:¶
Everything works fine for me.
If there are no further objections I'll merge this PR later today.
V.
[Export of Github issue for rear/rear.]