docs(conf): Explain dry run behavior correctly (#1)

README.md

@@ -20,7 +20,7 @@ Get started like so:
     sudo ln -s <repo>/pacman-zfs-snapshot-upgrade.hook /usr/share/libalpm/hooks/pacman-zfs-snapshot-upgrade.hook
     sudo ln -s <repo>/pacman-zfs-snapshot.conf /etc/pacman-zfs-snapshot.conf
     ```
-    Note that while you may choose arbitrary locations for symlinks the `pacman-zfs-snapshot.hook` file references `/usr/local/bin/pacman-zfs-snapshot`. Change that accordingly if you need to.
+    Note that while you may choose arbitrary locations for symlinks the `pacman-zfs-snapshot-*.hook` files reference `/usr/local/bin/pacman-zfs-snapshot`. Change that accordingly if you need to.
 1. For datasets you want auto-snapshotted add property `space.quico:auto-snapshot=true`
     ```
     zfs set space.quico:auto-snapshot=true zpool/root/archlinux
@@ -32,7 +32,7 @@ Get started like so:
 
 In `pacman` on every `PreTransaction`, meaning right before any actual operation on a package begins, we trigger a ZFS snapshot. By default we identify the active system dataset by doing `findmnt / --noheadings --output source`. If exactly one source returns that is the exact name of a ZFS dataset in an imported zpool we create a snapshot on it. If no source returns we silently exit. If more than one source returns we raise an error and halt the `pacman` transaction.
 
-We retain two different snapshot chains, one for `pacman` transactions that only affect what we are calling _trivial_ packages and a separate chain for _important_ packages. By default only the exact regular expression package name match `^linux$` is considered important. Whenever an important package is affected by a transaction a snapshot goes into the corresponding chain. In all other cases - when an important package is not affected - snapshots go into the trivial chain.
+We retain two different snapshot chains, one for `pacman` transactions that only affect what we are calling _trivial_ packages and a separate chain for _important_ packages. By default only the exact regular expression package name match `^(linux)$` is considered important. Whenever an important package is affected by a transaction a snapshot goes into the corresponding chain. In all other cases - when an important package is not affected - snapshots go into the trivial chain.
 
 The _trivial_ snapshot chain by default keeps 15 snapshots, the _important_ chain keeps 5. The thought process here is that you will likely not futz around with a kernel every day whereas you may very well install arbitrary packages multiple times a day. Snapshots should keep you safe for a couple of days hence the defaults of 5 and 15 snapshots, respectively.
 

pacman-zfs-snapshot-install.hook

@@ -4,7 +4,7 @@ Type = Package
 Target = *
 
 [Action]
-Description = Create ZFS snapshot on active system dataset
+Description = Create ZFS snapshot(s)
 When = PreTransaction
 Exec = /bin/sh -c 'while read -r f; do echo "$f"; done | /usr/local/bin/pacman-zfs-snapshot install'
 Depends = jq

pacman-zfs-snapshot-remove.hook

@@ -4,7 +4,7 @@ Type = Package
 Target = *
 
 [Action]
-Description = Create ZFS snapshot on active system dataset
+Description = Create ZFS snapshot(s)
 When = PreTransaction
 Exec = /bin/sh -c 'while read -r f; do echo "$f"; done | /usr/local/bin/pacman-zfs-snapshot remove'
 Depends = jq

pacman-zfs-snapshot-upgrade.hook

@@ -4,7 +4,7 @@ Type = Package
 Target = *
 
 [Action]
-Description = Create ZFS snapshot on active system dataset
+Description = Create ZFS snapshot(s)
 When = PreTransaction
 Exec = /bin/sh -c 'while read -r f; do echo "$f"; done | /usr/local/bin/pacman-zfs-snapshot upgrade'
 Depends = jq

pacman-zfs-snapshot.conf

@@ -1,20 +1,28 @@
-do_dry_run='true'
+# Set to 'true' to do nothing and just print messages during pacman
+# operations. Helpful to get a feel for what these hooks do. This defaults
+# to 'false' so if you set this to an empty string or remove or uncomment it
+# in this conf file it'll equal 'false'.
+do_dry_run='false'
 
-# Pipe-separated list of kernel names. Will be matched against regular
-# expression ^this_var_here$. Snapshots taken before a pacman transaction on
-# an important package have a separate retention from snapshots for trivial
-# packages. Lends itself to keeping kernel updates separate from everything
-# else.
-important_names='tmux'
-# zfs pkgs, systemd?
+# Pipe-separated list of package names we consider important. Will be
+# matched against regular expression ^(this_var_here)$. Snapshots taken
+# before a pacman transaction on an important package have a separate
+# retention from snapshots for trivial packages. Lends itself to keeping
+# high-risk updates separate from everything else.
+important_names='linux|systemd|zfs-(dkms|utils)'
 
+# Number snapshots to keep
 snaps_trivial_keep='15'
 snaps_important_keep='5'
 
+# Which suffix to use in snapshot names to identify snapshots before a
+# trivial pacman operation and before important pacman operations.
 snaps_trivial_suffix='trv'
 snaps_important_suffix='imp'
 
-pkgs_list_max_length='24'
+# Snapshot name will contain list of affected packages trimmed to this many
+# max characters.
+pkgs_list_max_length='30'
 
 # Hook will by default snapshot all datasets that have the property
 # 'space.quico:auto-snapshot=true' set, even the ones that are not currently
@@ -26,10 +34,17 @@ pkgs_list_max_length='24'
 # jq --raw-output '.[][] | select(.fstype=="zfs") | .source'
 snap_only_local_datasets='true'
 
+# Which characters do we want to use to separate snapshot name fields
 snap_field_separator='_'
-snap_name_prefix='pac'
-snap_date_format='+F-%H%M'
+# Prefix all our snapshots with this string to keep them separate from
+# snapshots done by any other means
+snap_name_prefix='pacman'
+# We do "$(date +<whatever>)" to put a timestamp into snapshot names.
+# Defaults to "$(date +<whatever>)" which returns '2023-03-07-0050'.
+snap_date_format='%F-%H%M'
 
+# Which strings do we want to diffferentiate pacman operations Install,
+# Remove, Upgrade
 snap_op_installation_suffix='inst'
 snap_op_remove_suffix='rmvl'
 snap_op_upgrade_suffix='upgr'

pacman-zfs-snapshot.sh

@@ -5,9 +5,6 @@ while read pkg; do
     pkgs+=("${pkg}")
 done
 
-declare operation
-operation="${1}"
-
 declare conf_file
 conf_file='/etc/pacman-zfs-snapshot.conf'
 
@@ -16,20 +13,43 @@ if [[ -r "${conf_file}" ]]; then
     source "${conf_file}"
 fi
 
-if [[ ! "${do_dry_run}" ]]; then do_dry_run='true'; fi
-if [[ ! "${important_names}" ]]; then important_names='linux'; fi
-if [[ ! "${snaps_trivial_keep}" ]]; then snaps_trivial_keep='15'; fi
-if [[ ! "${snaps_important_keep}" ]]; then snaps_important_keep='5'; fi
-if [[ ! "${snaps_trivial_suffix}" ]]; then snaps_trivial_suffix='trv'; fi
-if [[ ! "${snaps_important_suffix}" ]]; then snaps_important_suffix='imp'; fi
-if [[ ! "${pkgs_list_max_length}" ]]; then pkgs_list_max_length='24'; fi
-if [[ ! "${snap_only_local_datasets}" ]]; then snap_only_local_datasets='true'; fi
-if [[ ! "${snap_field_separator}" ]]; then snap_field_separator='_'; fi
-if [[ ! "${snap_name_prefix}" ]]; then snap_name_prefix='pac'; fi
-if [[ ! "${snap_date_format}" ]]; then snap_date_format='+F-%H%M'; fi
-if [[ ! "${snap_op_installation_suffix}" ]]; then snap_op_installation_suffix='inst'; fi
-if [[ ! "${snap_op_remove_suffix}" ]]; then snap_op_remove_suffix='rmvl'; fi
-if [[ ! "${snap_op_upgrade_suffix}" ]]; then snap_op_upgrade_suffix='upgr'; fi
+# User-defined
+do_dry_run="${do_dry_run:-false}"
+important_names="${important_names:-linux|systemd|zfs-(dkms|utils)}"
+snaps_trivial_keep="${snaps_trivial_keep:-15}"
+snaps_important_keep="${snaps_important_keep:-5}"
+snaps_trivial_suffix="${snaps_trivial_suffix:-trv}"
+snaps_important_suffix="${snaps_important_suffix:-imp}"
+pkgs_list_max_length="${pkgs_list_max_length:-30}"
+snap_only_local_datasets="${snap_only_local_datasets:-true}"
+snap_field_separator="${snap_field_separator:-_}"
+snap_name_prefix="${snap_name_prefix:-pacman}"
+snap_date_format="${snap_date_format:-%F-%H%M}"
+snap_op_installation_suffix="${snap_op_installation_suffix:-inst}"
+snap_op_remove_suffix="${snap_op_remove_suffix:-rmvl}"
+snap_op_upgrade_suffix="${snap_op_upgrade_suffix:-upgr}"
+
+# Internal
+declare pkg_separator max_zfs_snapshot_name_length color_reset color_lyellow color_red
+pkg_separator=':'
+max_zfs_snapshot_name_length='255'
+color_reset='\e[0m'
+color_lyellow='\e[93m'
+color_red='\e[31m'
+
+declare operation conf_op_suffix
+operation="${1}"
+case "${operation}" in
+    install)
+        conf_op_suffix="${snap_op_installation_suffix}"
+        ;;
+    remove)
+        conf_op_suffix="${snap_op_remove_suffix}"
+        ;;
+    upgrade)
+        conf_op_suffix="${snap_op_upgrade_suffix}"
+        ;;
+esac
 
 function pprint () {
     local style msg exit_code
@@ -37,11 +57,6 @@ function pprint () {
     msg="${2:?}"
     exit_code="${3}"
 
-    local color_reset color_lyellow
-    color_reset='\e[0m'
-    color_lyellow='\e[93m'
-    color_red='\e[31m'
-
     case "${style}" in
         warn)
             printf -- "${color_lyellow}"'[WARN]'"${color_reset}"' %s\n' "${msg}"
@@ -61,7 +76,7 @@ function split_pkgs_by_importance () {
     local pkgs_in_transaction
     pkgs_in_transaction=("${@}")
     for pkg in "${pkgs_in_transaction[@]}"; do
-        if grep -Piq -- '^'"${important_names}"'$' <<<"${pkg}"; then
+        if grep -Piq -- '^('"${important_names}"')$' <<<"${pkg}"; then
             important_pkgs_in_transaction+=("${pkg}")
         else
             trivial_pkgs_in_transaction+=("${pkg}")
@@ -86,18 +101,18 @@ function get_globally_snappable_datasets () {
     # from list, i.e. dataset names that contain an '@' character.
     datasets_list="$(zfs get -H -o 'name,value' 'space.quico:auto-snapshot' | \
                      awk -F'\t' '{if($2 ~ /^true$/ && $1 !~ /@/) print $1}')"
-    while IFS= read -r dataset; do
+    while IFS= read -u10 -r dataset; do
         globally_snappable_datasets+=("${dataset}")
-    done <<<"${datasets_list}"
+    done 10<<<"${datasets_list}"
 }
 
 function get_local_snappable_datasets () {
     local datasets_list
     datasets_list="$(findmnt --json --list --output 'fstype,source,target' | \
                      jq --raw-output '.[][] | select(.fstype=="zfs") | .source')"
-    while IFS= read -r dataset; do
+    while IFS= read -u10 -r dataset; do
         local_snappable_datasets+=("${dataset}")
-    done <<<"${datasets_list}"
+    done 10<<<"${datasets_list}"
 }
 
 function trim_globally_snappable_datasets () {
@@ -111,11 +126,10 @@ function trim_globally_snappable_datasets () {
 }
 
 function write_pkg_list_oneline () {
-    local unabridged_pkg_list_oneline
     if [[ "${severity}" == 'imp' ]]; then
         for pkg in "${important_pkgs_in_transaction[@]}"; do
             if [[ "${unabridged_pkg_list_oneline}" ]]; then
-                unabridged_pkg_list_oneline="${unabridged_pkg_list_oneline}"','"${pkg}"
+                unabridged_pkg_list_oneline="${unabridged_pkg_list_oneline}${pkg_separator}${pkg}"
             else
                 unabridged_pkg_list_oneline="${pkg}"
             fi
@@ -124,7 +138,7 @@ function write_pkg_list_oneline () {
     if [[ "${#trivial_pkgs_in_transaction[@]}" -ge '1' ]]; then
         for pkg in "${trivial_pkgs_in_transaction[@]}"; do
             if [[ "${unabridged_pkg_list_oneline}" ]]; then
-                unabridged_pkg_list_oneline="${unabridged_pkg_list_oneline}"','"${pkg}"
+                unabridged_pkg_list_oneline="${unabridged_pkg_list_oneline}${pkg_separator}${pkg}"
             else
                 unabridged_pkg_list_oneline="${pkg}"
             fi
@@ -151,7 +165,7 @@ function find_max_dataset_name_length () {
     done
     sev_suffix_string="$(head -c "${longest_sev_suffix}" '/dev/zero' | tr '\0' '_')"
 
-    local max_dataset_name_length dataset_name_no_pkgs
+    local dataset_name_no_pkgs
     max_dataset_name_length='0'
     for dataset in "${snappable_datasets[@]}"; do
         dataset_name_no_pkgs="${dataset}"'@'"${snap_name_prefix}${snap_field_separator}${date_string}${snap_field_separator}"'op:'"${op_suffix_string}${snap_field_separator}"'sev:'"${sev_suffix_string}${snap_field_separator}"'pkgs:'
@@ -168,41 +182,43 @@ function find_max_dataset_name_length () {
 function trim_pkg_list_oneline () {
     local available_pkg_list_length
     available_pkg_list_length="$((${max_zfs_snapshot_name_length} - ${max_dataset_name_length}))"
-    if [[ "${available_pkg_list_length}" -lt "{pkgs_list_max_length}" ]]; then
+    if [[ "${available_pkg_list_length}" -lt "${pkgs_list_max_length}" ]]; then
         # If we have fewer characters available than the user wants limit
         # package list length
         pkgs_list_max_length="${available_pkg_list_length}"
     fi
 
     local shorter_pkg_list
-    shorter_pkg_list="${pkg_list_oneline}"
+    shorter_pkg_list="${unabridged_pkg_list_oneline}"
     while [[ "${#shorter_pkg_list}" -gt "${pkgs_list_max_length}" ]]; do
-        shorter_pkg_list="${shorter_pkg_list%,*}"
-        if ! grep -Piq ',' <<<"${shorter_pkg_list}"; then
+        shorter_pkg_list="${shorter_pkg_list%${pkg_separator}*}"
+        if ! grep -Piq "${pkg_separator}" <<<"${shorter_pkg_list}"; then
             # Only one package remains in package list, no need to continue
             break
         fi
     done
-    if [[ "${#shorter_pkg_list}" -gt "${pkgs_list_max_length}" ]]; do
+    if [[ "${#shorter_pkg_list}" -gt "${pkgs_list_max_length}" ]]; then
         # If this is still too long we empty the package list
         shorter_pkg_list=''
-    done
+    fi
     trimmed_pkg_list_oneline="${shorter_pkg_list}"
 }
 
 function do_snaps () {
-    local snap_name
+    local snap_name snap_return_code
     for snappable_dataset in "${snappable_datasets[@]}"; do
-        snap_name="${snappable_dataset}"'@'"${snap_name_prefix}${snap_field_separator}${date_string}${snap_field_separator}"'op:'"${operation}${snap_field_separator}"'sev:'"${severity}${snap_field_separator}"'pkgs:'"${trimmed_pkg_list_oneline}"
+        snap_name="${snappable_dataset}"'@'"${snap_name_prefix}${snap_field_separator}${date_string}${snap_field_separator}"'op:'"${conf_op_suffix}${snap_field_separator}"'sev:'"${severity}${snap_field_separator}"'pkgs:'"${trimmed_pkg_list_oneline}"
         if [[ "${do_dry_run}" == 'true' ]]; then
             pprint 'info' 'Dry-run, pretending to run zfs snapshot '"${snap_name}"
         else
-            zfs snapshot "${snap_name}" && {
+            zfs snapshot "${snap_name}"
+            snap_return_code="${?}"
+            if [[ "${snap_return_code}" -eq '0' ]]; then
                 successfully_snapped_datasets+=("${snappable_dataset}")
                 pprint 'info' 'Snapshot done: '"${snap_name}"
-            } || {
+            else
                 pprint 'warn' 'Snapshot failed: '"${snap_name}"
-            }
+            fi
         fi
     done
 }
@@ -213,11 +229,11 @@ function get_snaps_in_cur_sev () {
     snap_list="$(zfs list -H -o 'name' -t snapshot "${dataset_to_query}")"
     snaps_done_by_us="$(grep -Pi -- '@'"${snap_name_prefix}${snap_field_separator}" <<<"${snap_list}")"
     snaps_in_cur_sev="$(grep -Pi -- "${snap_field_separator}"'sev:'"${severity}${snap_field_separator}" <<<"${snaps_done_by_us}")"
-    printf -- '%s' "${snaps_in_cur_sev}"
+    printf -- '%s\n' "${snaps_in_cur_sev}"
 }
 
 function do_retention () {
-    local snap_list snaps_done_by_us snaps_in_cur_sev snaps_limit oldest_snap
+    local snap_list snaps_done_by_us snaps_in_cur_sev snaps_limit oldest_snap snap_return_code
     if [[ "${do_dry_run}" == 'true' ]]; then
         pprint 'info' 'Dry-run, skipping potential zfs destroy operations ...'
     else
@@ -231,6 +247,12 @@ function do_retention () {
             while [[ "$(get_snaps_in_cur_sev "${successfully_snapped_dataset}" | wc -l)" -gt "${snaps_limit}" ]]; do
                 oldest_snap="$(get_snaps_in_cur_sev "${successfully_snapped_dataset}" | head -n1)"
                 zfs destroy "${oldest_snap}"
+                snap_return_code="${?}"
+                if [[ "${snap_return_code}" -eq '0' ]]; then
+                    pprint 'info' 'Oldest in chain '"'"'sev:'"${severity}"''"'"' destroyed: '"${oldest_snap}"
+                else
+                    pprint 'warn' 'Snapshot destruction failed: '"${oldest_snap}"
+                fi
             done
         done
     fi
@@ -258,12 +280,11 @@ function main () {
         snappable_datasets=("${globally_snappable_datasets}")
     fi
 
-    local pkg_list_oneline
+    local unabridged_pkg_list_oneline
     write_pkg_list_oneline
 
-    local date_string max_zfs_snapshot_name_length max_dataset_name_length
+    local date_string max_dataset_name_length
     date_string="$(date +"${snap_date_format}")"
-    max_zfs_snapshot_name_length='255'
     find_max_dataset_name_length
 
     local trimmed_pkg_list_oneline