lm0 de-duplicate imagemagick detection and config-array parsing

Introduce a shared resolve_config_array helper in bootstrap.source.sh that
parses a config variable declared as either a Bash array or a
whitespace-separated scalar into a named output array. resolve_tar_opts and
resolve_sync_destinations now both delegate to it, with resolve_tar_opts
keeping its "-c" default for the empty/unset case.

validate_imagemagick now reuses resolve_imagemagick_command instead of
duplicating the magick/convert probing, reporting failures through
config_error so the validation output is unchanged.

Add a print-config test covering the empty TAR_OPTS (scalar and array)
fallback to the default.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

6 files changed, 157 insertions, 80 deletions

diff --git a/bin/shuriken b/bin/shuriken
index 2efd49a..75f8fd3 100755
--- a/bin/shuriken
+++ b/bin/shuriken
@@ -144,6 +144,48 @@ log_warning() {
     printf 'WARNING: %s\n' "$*" >&2
 }
 
+# Read a configuration value into the named array, accepting both Bash array
+# and whitespace-separated scalar declarations of the same variable.
+# This is shared by resolve_tar_opts and resolve_sync_destinations so the
+# "array or scalar config" parsing lives in exactly one place.
+#
+# Arguments:
+#   $1  name of the source config variable (e.g. TAR_OPTS)
+#   $2  name of the destination array variable (nameref)
+# Returns:
+#   0 if the variable was declared (the destination may still be empty),
+#   1 if the variable was not declared at all (lets callers apply defaults).
+resolve_config_array() {
+    local -r config_var="$1"; shift
+    local -n config_array_ref="$1"; shift
+    local config_decl
+
+    config_array_ref=()
+
+    # declare -p fails when the variable was never set; callers use the
+    # non-zero return to distinguish "unset" from "set but empty".
+    if ! config_decl=$(declare -p "$config_var" 2>/dev/null); then
+        return 1
+    fi
+
+    case "$config_decl" in
+        declare\ -a*\ "$config_var"=*)
+            # Already a real array: copy it element by element.
+            local -n config_source_ref="$config_var"
+            # shellcheck disable=SC2034
+            config_array_ref=("${config_source_ref[@]}")
+            ;;
+        *)
+            # Scalar string: word-split it into the destination array.
+            local -n config_scalar_ref="$config_var"
+            if [ -n "${config_scalar_ref:-}" ]; then
+                # shellcheck disable=SC2034
+                read -r -a config_array_ref <<< "$config_scalar_ref"
+            fi
+            ;;
+    esac
+}
+
 resolve_default_rc_file() {
     local source_root
 
@@ -402,27 +444,14 @@ tarball() {
 
 resolve_tar_opts() {
     local -n options_ref="$1"; shift
-    local tar_opts_decl
-
-    options_ref=()
 
-    if ! tar_opts_decl=$(declare -p TAR_OPTS 2>/dev/null); then
-        options_ref=(-c)
-        return
-    fi
-
-    case "$tar_opts_decl" in
-        declare\ -a*\ TAR_OPTS=*)
-            options_ref=("${TAR_OPTS[@]}")
-            ;;
-        *)
-            if [ -n "${TAR_OPTS:-}" ]; then
-                read -r -a options_ref <<< "$TAR_OPTS"
-            fi
-            ;;
-    esac
+    # Parse TAR_OPTS (array or scalar) via the shared config-array helper,
+    # then fall back to a plain "-c" whenever no options were configured,
+    # whether TAR_OPTS was unset or set to an empty value.
+    resolve_config_array TAR_OPTS options_ref
 
     if (( ${#options_ref[@]} == 0 )); then
+        # shellcheck disable=SC2034
         options_ref=(-c)
     fi
 }
@@ -2758,26 +2787,14 @@ print_config() {
 
 # Inlined from src/lib/config.sync.source.sh
 resolve_sync_destinations() {
+    # destinations_ref is a nameref output filled by resolve_config_array.
+    # shellcheck disable=SC2034
     local -n destinations_ref="$1"; shift
-    local destinations_decl
-
-    destinations_ref=()
 
-    if ! destinations_decl=$(declare -p SYNC_DESTINATIONS 2>/dev/null); then
-        return
-    fi
-
-    case "$destinations_decl" in
-        declare\ -a*\ SYNC_DESTINATIONS=*)
-            destinations_ref=("${SYNC_DESTINATIONS[@]}")
-            ;;
-        *)
-            if [ -n "${SYNC_DESTINATIONS:-}" ]; then
-                # shellcheck disable=SC2034
-                read -r -a destinations_ref <<< "$SYNC_DESTINATIONS"
-            fi
-            ;;
-    esac
+    # Parse SYNC_DESTINATIONS (array or scalar) via the shared config-array
+    # helper. Unlike TAR_OPTS there is no default: an unset or empty value
+    # simply yields an empty destinations array for callers to validate.
+    resolve_config_array SYNC_DESTINATIONS destinations_ref
 }
 
 sync_dist() {
@@ -3170,10 +3187,16 @@ validate_refresh_splash_config() {
 }
 
 validate_imagemagick() {
-    if command -v magick >/dev/null 2>&1; then
-        return
-    fi
-    if command -v convert >/dev/null 2>&1; then
+    # Reuse the canonical detection in resolve_imagemagick_command instead of
+    # duplicating the magick/convert probing here. Its own error message is
+    # suppressed so we report the failure through config_error, keeping the
+    # validation output consistent (single "ERROR: ..." line, return code 1).
+    # imagemagick_command is a nameref output filled by the resolver; we only
+    # care about the exit status here, not the resolved command.
+    # shellcheck disable=SC2034
+    local -a imagemagick_command=()
+
+    if resolve_imagemagick_command convert imagemagick_command 2>/dev/null; then
         return
     fi
 
diff --git a/src/lib/archive.source.sh b/src/lib/archive.source.sh
index 127a0c5..3b173b7 100644
--- a/src/lib/archive.source.sh
+++ b/src/lib/archive.source.sh
@@ -26,27 +26,14 @@ tarball() {
 
 resolve_tar_opts() {
     local -n options_ref="$1"; shift
-    local tar_opts_decl
 
-    options_ref=()
-
-    if ! tar_opts_decl=$(declare -p TAR_OPTS 2>/dev/null); then
-        options_ref=(-c)
-        return
-    fi
-
-    case "$tar_opts_decl" in
-        declare\ -a*\ TAR_OPTS=*)
-            options_ref=("${TAR_OPTS[@]}")
-            ;;
-        *)
-            if [ -n "${TAR_OPTS:-}" ]; then
-                read -r -a options_ref <<< "$TAR_OPTS"
-            fi
-            ;;
-    esac
+    # Parse TAR_OPTS (array or scalar) via the shared config-array helper,
+    # then fall back to a plain "-c" whenever no options were configured,
+    # whether TAR_OPTS was unset or set to an empty value.
+    resolve_config_array TAR_OPTS options_ref
 
     if (( ${#options_ref[@]} == 0 )); then
+        # shellcheck disable=SC2034
         options_ref=(-c)
     fi
 }
diff --git a/src/lib/bootstrap.source.sh b/src/lib/bootstrap.source.sh
index 244cd55..67ba090 100644
--- a/src/lib/bootstrap.source.sh
+++ b/src/lib/bootstrap.source.sh
@@ -60,6 +60,48 @@ log_warning() {
     printf 'WARNING: %s\n' "$*" >&2
 }
 
+# Read a configuration value into the named array, accepting both Bash array
+# and whitespace-separated scalar declarations of the same variable.
+# This is shared by resolve_tar_opts and resolve_sync_destinations so the
+# "array or scalar config" parsing lives in exactly one place.
+#
+# Arguments:
+#   $1  name of the source config variable (e.g. TAR_OPTS)
+#   $2  name of the destination array variable (nameref)
+# Returns:
+#   0 if the variable was declared (the destination may still be empty),
+#   1 if the variable was not declared at all (lets callers apply defaults).
+resolve_config_array() {
+    local -r config_var="$1"; shift
+    local -n config_array_ref="$1"; shift
+    local config_decl
+
+    config_array_ref=()
+
+    # declare -p fails when the variable was never set; callers use the
+    # non-zero return to distinguish "unset" from "set but empty".
+    if ! config_decl=$(declare -p "$config_var" 2>/dev/null); then
+        return 1
+    fi
+
+    case "$config_decl" in
+        declare\ -a*\ "$config_var"=*)
+            # Already a real array: copy it element by element.
+            local -n config_source_ref="$config_var"
+            # shellcheck disable=SC2034
+            config_array_ref=("${config_source_ref[@]}")
+            ;;
+        *)
+            # Scalar string: word-split it into the destination array.
+            local -n config_scalar_ref="$config_var"
+            if [ -n "${config_scalar_ref:-}" ]; then
+                # shellcheck disable=SC2034
+                read -r -a config_array_ref <<< "$config_scalar_ref"
+            fi
+            ;;
+    esac
+}
+
 resolve_default_rc_file() {
     local source_root
 
diff --git a/src/lib/config.sync.source.sh b/src/lib/config.sync.source.sh
index 68bb759..1707fe6 100644
--- a/src/lib/config.sync.source.sh
+++ b/src/lib/config.sync.source.sh
@@ -1,24 +1,12 @@
 resolve_sync_destinations() {
+    # destinations_ref is a nameref output filled by resolve_config_array.
+    # shellcheck disable=SC2034
     local -n destinations_ref="$1"; shift
-    local destinations_decl
 
-    destinations_ref=()
-
-    if ! destinations_decl=$(declare -p SYNC_DESTINATIONS 2>/dev/null); then
-        return
-    fi
-
-    case "$destinations_decl" in
-        declare\ -a*\ SYNC_DESTINATIONS=*)
-            destinations_ref=("${SYNC_DESTINATIONS[@]}")
-            ;;
-        *)
-            if [ -n "${SYNC_DESTINATIONS:-}" ]; then
-                # shellcheck disable=SC2034
-                read -r -a destinations_ref <<< "$SYNC_DESTINATIONS"
-            fi
-            ;;
-    esac
+    # Parse SYNC_DESTINATIONS (array or scalar) via the shared config-array
+    # helper. Unlike TAR_OPTS there is no default: an unset or empty value
+    # simply yields an empty destinations array for callers to validate.
+    resolve_config_array SYNC_DESTINATIONS destinations_ref
 }
 
 sync_dist() {
diff --git a/src/lib/config.validate.source.sh b/src/lib/config.validate.source.sh
index 7988821..b858144 100644
--- a/src/lib/config.validate.source.sh
+++ b/src/lib/config.validate.source.sh
@@ -152,10 +152,16 @@ validate_refresh_splash_config() {
 }
 
 validate_imagemagick() {
-    if command -v magick >/dev/null 2>&1; then
-        return
-    fi
-    if command -v convert >/dev/null 2>&1; then
+    # Reuse the canonical detection in resolve_imagemagick_command instead of
+    # duplicating the magick/convert probing here. Its own error message is
+    # suppressed so we report the failure through config_error, keeping the
+    # validation output consistent (single "ERROR: ..." line, return code 1).
+    # imagemagick_command is a nameref output filled by the resolver; we only
+    # care about the exit status here, not the resolved command.
+    # shellcheck disable=SC2034
+    local -a imagemagick_command=()
+
+    if resolve_imagemagick_command convert imagemagick_command 2>/dev/null; then
         return
     fi
 
diff --git a/tests/cli.sh b/tests/cli.sh
index d6fbbd4..b8b56f6 100755
--- a/tests/cli.sh
+++ b/tests/cli.sh
@@ -1677,6 +1677,34 @@ test_print_config_normalizes_scalar_and_array_tar_opts() {
     test::teardown
 }
 
+test_print_config_empty_tar_opts_falls_back_to_default() {
+    local config_file
+    local empty_array_output
+    local empty_scalar_output
+
+    test::setup
+    config_file="$TEST_TMPDIR/shuriken.conf"
+    test::write_album_config \
+        "$config_file" "$TEST_TMPDIR/incoming" "$TEST_TMPDIR/dist" \
+        'Empty tar opts' 40
+
+    # An empty scalar TAR_OPTS must fall back to the default "-c" just like an
+    # unset value; the shared resolve_config_array helper yields an empty array
+    # and resolve_tar_opts supplies the default.
+    printf 'TAR_OPTS=%q\n' '' >> "$config_file"
+    empty_scalar_output=$("$TEST_SHURIKEN" --print-config --config "$config_file")
+    test::assert_contains 'TAR_OPTS=( -c )' "$empty_scalar_output"
+
+    # An empty array declaration must behave identically.
+    test::write_album_config \
+        "$config_file" "$TEST_TMPDIR/incoming" "$TEST_TMPDIR/dist" \
+        'Empty array tar opts' 40
+    printf 'TAR_OPTS=()\n' >> "$config_file"
+    empty_array_output=$("$TEST_SHURIKEN" --print-config --config "$config_file")
+    test::assert_contains 'TAR_OPTS=( -c )' "$empty_array_output"
+    test::teardown
+}
+
 test_print_config_quiet_and_verbose_keep_machine_output() {
     local config_file
     local plain_output
@@ -4856,6 +4884,9 @@ main() {
         '--print-config normalizes scalar and array TAR_OPTS' \
         test_print_config_normalizes_scalar_and_array_tar_opts
     test::run_case \
+        '--print-config empty TAR_OPTS falls back to default' \
+        test_print_config_empty_tar_opts_falls_back_to_default
+    test::run_case \
         '--print-config quiet and verbose keep machine output' \
         test_print_config_quiet_and_verbose_keep_machine_output
     test::run_case \