hunleyd / btrfs-auto-snapshot

BTRFS Automatic Snapshot Service for Linux
GNU General Public License v2.0
16 stars 4 forks source link

Calculate all BTRFS subvolumes when using // and use more functions for better readability. #7

Closed ams-tschoening closed 2 years ago

ams-tschoening commented 2 years ago

After creating the changes in #6, I've tried to make the code better readable by using additional functions. This was especially of interest because of your commandline arguments and how to parse them. So I consider this pretty much a PoC of how I might handle things ion future as well... :-)

Parsing arguments is essentially the same like before, only that an associative array is used to return settings from the function call in the subshell. Those values are afterwards stored in the original global variables, to not need to change too much more. Though, that array could be accessed instead of the global variables I guess.

Error handling and exiting from the commandline parser is a bit difficult currently using kill and trap because of the subshell. But in my opinion it's not that bad as well.

hunleyd commented 2 years ago

i really like this @ams-tschoening . However, associative arrays require Bash 4, so the code should check for that and fail if version is < 4. Mind adding that?

ams-tschoening commented 2 years ago

Do you really think it's worth it? Even my pretty old Ubuntu 18.04 LTS ships with version 4.4.20 already. In theory we would need to not only check for version 4 or 4.4, but really for 4.4.19+ if I understood the article about associative arrays correctly. Possible of course, but makes things unnecessary difficult in my opinion.

How about leaving that for a later PR if anyone complains? ;-)

hunleyd commented 2 years ago

hrm, wouldn't something like:

unset assoc
if ! declare -A assoc ; then
    echo "associative arrays not supported!"
    exit 1
fi

suffice?

ams-tschoening commented 2 years ago

I misunderstood the article regarding 4.4.19, some older versions are enough already:

user@host:~$ bash --version
GNU bash, version 4.3.11(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
user@host:~$ declare -A assoc
user@host:~$ declare -B assoc
-bash: declare: -B: invalid option
declare: usage: declare [-aAfFgilnrtux] [-p] [name[=value] ...]
user@host:~ > bash --version
GNU bash, version 2.05.0(1)-release (i386-suse-linux)
Copyright 2000 Free Software Foundation, Inc.
user@host:~ > declare -A test
bash: declare: unknown option: `-A'
declare: usage: declare [-afFrxi] [-p] name[=value] ...
user@host:~ >
hunleyd commented 2 years ago

thanks for the edit @ams-tschoening !