Refine Bash practices for oi0

5 files changed, 59 insertions, 51 deletions

diff --git a/AGENTS.md b/AGENTS.md
new file mode 100644
index 0000000..253f68f
--- /dev/null
+++ b/AGENTS.md
@@ -0,0 +1,17 @@
+# Agent Guidance
+
+For Bash changes in this repository, load and follow the
+`bash-best-practices` skill before editing. Keep changes focused, preserve the
+public `photoalbum` CLI, and run the project checks before committing.
+
+`src/photoalbum.sh` is the source for the generated `bin/photoalbum` script. If
+the source changes, run `just build` and keep `bin/photoalbum` synchronized.
+
+Expected checks for Bash changes:
+
+- `just test`
+- `just shellcheck`
+- `just check-generated`
+- `make test`
+- `make shellcheck`
+- `git diff --check`
diff --git a/Justfile b/Justfile
index a4cf996..19db28e 100644
--- a/Justfile
+++ b/Justfile
@@ -80,23 +80,7 @@ clean:
         2>/dev/null || true
 
 shellcheck:
-    # SC1090: ShellCheck can't follow non-constant source.
-    # SC2001: See if you can use ${variable//search/replace} instead.
-    # SC2010: Don't use ls | grep. Use a glob or a for loop.
-    # SC2012: Use find instead of ls for unusual filenames.
-    # SC2103: Use a subshell to avoid having to cd back.
-    # SC2155: Declare and assign separately to avoid masking return values.
-    # SC2164: Use 'cd ... || exit' or 'cd ... || return'.
-    # SC2207: Prefer mapfile or read -a to split command output.
     shellcheck \
-        --exclude SC1090 \
-        --exclude SC2001 \
-        --exclude SC2010 \
-        --exclude SC2012 \
-        --exclude SC2103 \
-        --exclude SC2155 \
-        --exclude SC2164 \
-        --exclude SC2207 \
         ./src/photoalbum.sh \
         ./tests/cli.sh \
         ./tests/helpers.sh
diff --git a/bin/photoalbum b/bin/photoalbum
index 355c32a..4944d39 100755
--- a/bin/photoalbum
+++ b/bin/photoalbum
@@ -141,14 +141,14 @@ init_config() {
     local source_template_dir
 
     if [ -f "$rc_file" ]; then
-        echo "Error: $rc_file already exists" >&2
+        printf 'Error: %s already exists\n' "$rc_file" >&2
         exit 1
     fi
 
     default_rc_file=$(resolve_default_rc_file)
 
     if [ ! -f "$default_rc_file" ]; then
-        echo "Error: Can not find config file $default_rc_file" >&2
+        printf 'Error: Can not find config file %s\n' "$default_rc_file" >&2
         exit 1
     fi
 
@@ -171,7 +171,7 @@ imagemagick() {
     elif command -v convert >/dev/null 2>&1; then
         convert "$@"
     else
-        echo 'ERROR: ImageMagick is required; install magick or convert' >&2
+        printf 'ERROR: ImageMagick is required; install magick or convert\n' >&2
         return 127
     fi
 }
@@ -188,7 +188,7 @@ imagemagick_identify() {
             convert "$@" info:
         fi
     else
-        echo 'ERROR: ImageMagick is required; install magick or convert' >&2
+        printf 'ERROR: ImageMagick is required; install magick or convert\n' >&2
         return 127
     fi
 }
@@ -1232,7 +1232,7 @@ randomphoto() {
     )
 
     if (( ${#photos[@]} == 0 )); then
-        echo "ERROR: No photos found in" \
+        printf 'ERROR: No photos found in %s\n' \
             "$(_display_path "$DIST_DIR/$photos_dir")" >&2
         return 1
     fi
@@ -1586,11 +1586,13 @@ replace_dist_with_staging() {
 
     if [ -n "$backup_dist" ] && [ -e "$backup_dist" ]; then
         if [ -e "$final_dist" ] && ! rm -rf "$final_dist"; then
-            echo "ERROR: Failed to restore $final_dist from $backup_dist" >&2
+            printf 'ERROR: Failed to restore %s from %s\n' \
+                "$final_dist" "$backup_dist" >&2
             return "$status"
         fi
         if ! mv "$backup_dist" "$final_dist"; then
-            echo "ERROR: Failed to restore $final_dist from $backup_dist" >&2
+            printf 'ERROR: Failed to restore %s from %s\n' \
+                "$final_dist" "$backup_dist" >&2
             return "$status"
         fi
         rm -rf "$backup_parent"
@@ -1664,8 +1666,8 @@ resolve_config_file() {
 missing_config() {
     local -r config_file="$1"; shift
 
-    echo "Error: Can not find config file $config_file" >&2
-    echo 'Run photoalbum --init to create ./photoalbum.conf.' >&2
+    printf 'Error: Can not find config file %s\n' "$config_file" >&2
+    printf 'Run photoalbum --init to create ./photoalbum.conf.\n' >&2
     exit 1
 }
 
@@ -1685,7 +1687,7 @@ option_value() {
     local -r option="$1"; shift
 
     if (( $# == 0 )) || [ -z "$1" ]; then
-        echo "Error: $option requires a value" >&2
+        printf 'Error: %s requires a value\n' "$option" >&2
         usage
         exit 1
     fi
@@ -1729,7 +1731,7 @@ apply_cli_overrides() {
 config_error() {
     local -r message="$1"; shift
 
-    echo "ERROR: $message" >&2
+    printf 'ERROR: %s\n' "$message" >&2
     return 1
 }
 
@@ -1948,7 +1950,7 @@ parse_cli_arguments() {
         case "$option" in
             --config)
                 if (( $# == 0 )) || [ -z "$1" ]; then
-                    echo 'Error: --config requires a path' >&2
+                    printf 'Error: --config requires a path\n' >&2
                     usage
                     exit 1
                 fi
@@ -2003,7 +2005,7 @@ run_simple_action() {
                 exit 1
             fi
 
-            echo "This is Photoalbum Version $VERSION"
+            printf 'This is Photoalbum Version %s\n' "$VERSION"
             ;;
         --init)
             if [[ -n "$config_file" || "$has_config_overrides" = 'yes' ]]; then
@@ -2023,6 +2025,7 @@ load_configured_action() {
         missing_config "$rc_file"
     fi
 
+    # shellcheck source=/dev/null
     source "$rc_file"
     apply_config_defaults
     apply_template_dir_default
diff --git a/src/photoalbum.sh b/src/photoalbum.sh
index 8ea4c36..03f730e 100755
--- a/src/photoalbum.sh
+++ b/src/photoalbum.sh
@@ -141,14 +141,14 @@ init_config() {
     local source_template_dir
 
     if [ -f "$rc_file" ]; then
-        echo "Error: $rc_file already exists" >&2
+        printf 'Error: %s already exists\n' "$rc_file" >&2
         exit 1
     fi
 
     default_rc_file=$(resolve_default_rc_file)
 
     if [ ! -f "$default_rc_file" ]; then
-        echo "Error: Can not find config file $default_rc_file" >&2
+        printf 'Error: Can not find config file %s\n' "$default_rc_file" >&2
         exit 1
     fi
 
@@ -171,7 +171,7 @@ imagemagick() {
     elif command -v convert >/dev/null 2>&1; then
         convert "$@"
     else
-        echo 'ERROR: ImageMagick is required; install magick or convert' >&2
+        printf 'ERROR: ImageMagick is required; install magick or convert\n' >&2
         return 127
     fi
 }
@@ -188,7 +188,7 @@ imagemagick_identify() {
             convert "$@" info:
         fi
     else
-        echo 'ERROR: ImageMagick is required; install magick or convert' >&2
+        printf 'ERROR: ImageMagick is required; install magick or convert\n' >&2
         return 127
     fi
 }
@@ -1232,7 +1232,7 @@ randomphoto() {
     )
 
     if (( ${#photos[@]} == 0 )); then
-        echo "ERROR: No photos found in" \
+        printf 'ERROR: No photos found in %s\n' \
             "$(_display_path "$DIST_DIR/$photos_dir")" >&2
         return 1
     fi
@@ -1586,11 +1586,13 @@ replace_dist_with_staging() {
 
     if [ -n "$backup_dist" ] && [ -e "$backup_dist" ]; then
         if [ -e "$final_dist" ] && ! rm -rf "$final_dist"; then
-            echo "ERROR: Failed to restore $final_dist from $backup_dist" >&2
+            printf 'ERROR: Failed to restore %s from %s\n' \
+                "$final_dist" "$backup_dist" >&2
             return "$status"
         fi
         if ! mv "$backup_dist" "$final_dist"; then
-            echo "ERROR: Failed to restore $final_dist from $backup_dist" >&2
+            printf 'ERROR: Failed to restore %s from %s\n' \
+                "$final_dist" "$backup_dist" >&2
             return "$status"
         fi
         rm -rf "$backup_parent"
@@ -1664,8 +1666,8 @@ resolve_config_file() {
 missing_config() {
     local -r config_file="$1"; shift
 
-    echo "Error: Can not find config file $config_file" >&2
-    echo 'Run photoalbum --init to create ./photoalbum.conf.' >&2
+    printf 'Error: Can not find config file %s\n' "$config_file" >&2
+    printf 'Run photoalbum --init to create ./photoalbum.conf.\n' >&2
     exit 1
 }
 
@@ -1685,7 +1687,7 @@ option_value() {
     local -r option="$1"; shift
 
     if (( $# == 0 )) || [ -z "$1" ]; then
-        echo "Error: $option requires a value" >&2
+        printf 'Error: %s requires a value\n' "$option" >&2
         usage
         exit 1
     fi
@@ -1729,7 +1731,7 @@ apply_cli_overrides() {
 config_error() {
     local -r message="$1"; shift
 
-    echo "ERROR: $message" >&2
+    printf 'ERROR: %s\n' "$message" >&2
     return 1
 }
 
@@ -1948,7 +1950,7 @@ parse_cli_arguments() {
         case "$option" in
             --config)
                 if (( $# == 0 )) || [ -z "$1" ]; then
-                    echo 'Error: --config requires a path' >&2
+                    printf 'Error: --config requires a path\n' >&2
                     usage
                     exit 1
                 fi
@@ -2003,7 +2005,7 @@ run_simple_action() {
                 exit 1
             fi
 
-            echo "This is Photoalbum Version $VERSION"
+            printf 'This is Photoalbum Version %s\n' "$VERSION"
             ;;
         --init)
             if [[ -n "$config_file" || "$has_config_overrides" = 'yes' ]]; then
@@ -2023,6 +2025,7 @@ load_configured_action() {
         missing_config "$rc_file"
     fi
 
+    # shellcheck source=/dev/null
     source "$rc_file"
     apply_config_defaults
     apply_template_dir_default
diff --git a/tests/cli.sh b/tests/cli.sh
index e59cf86..9cec762 100755
--- a/tests/cli.sh
+++ b/tests/cli.sh
@@ -1,7 +1,8 @@
 #!/usr/bin/env bash
 set -euo pipefail
 
-declare -r TEST_REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+TEST_REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+declare -r TEST_REPO_ROOT
 declare -r TEST_PHOTOALBUM="${PHOTOALBUM:-$TEST_REPO_ROOT/bin/photoalbum}"
 
 # shellcheck source=tests/helpers.sh
@@ -174,7 +175,7 @@ test_init_existing_config_fails_without_overwrite() {
     )
 
     test::assert_contains 'Error: photoalbum.conf already exists' "$output"
-    test "$(cat "$TEST_TMPDIR/photoalbum.conf")" = 'sentinel'
+    test "$(<"$TEST_TMPDIR/photoalbum.conf")" = 'sentinel'
     test::teardown
 }
 
@@ -1864,8 +1865,8 @@ test_generate_imagemagick_failure_preserves_dist() {
     )
 
     test::assert_contains 'simulated ImageMagick failure' "$output"
-    test "$(cat "$TEST_TMPDIR/dist/index.html")" = 'old dist'
-    test "$(cat "$TEST_TMPDIR/dist/sentinel")" = 'keep me'
+    test "$(<"$TEST_TMPDIR/dist/index.html")" = 'old dist'
+    test "$(<"$TEST_TMPDIR/dist/sentinel")" = 'keep me'
     test::assert_path_absent "$TEST_TMPDIR/dist/photos/01.jpg"
     test::assert_path_absent "$TEST_TMPDIR/dist/photoalbum.json"
     test::assert_no_staging_dirs "$TEST_TMPDIR"
@@ -1902,7 +1903,7 @@ test_generate_template_failure_preserves_dist() {
     )
 
     test::assert_contains 'Generating' "$output"
-    test "$(cat "$TEST_TMPDIR/dist/index.html")" = 'old index'
+    test "$(<"$TEST_TMPDIR/dist/index.html")" = 'old index'
     test::assert_path_absent "$TEST_TMPDIR/dist/photos/01-landscape.jpg"
     test::assert_path_absent "$TEST_TMPDIR/dist/photoalbum.json"
     test::assert_no_staging_dirs "$TEST_TMPDIR"
@@ -1941,7 +1942,7 @@ test_generate_templates_cannot_read_generation_locals() {
     )
 
     test::assert_contains 'num: unbound variable' "$output"
-    test "$(cat "$TEST_TMPDIR/dist/index.html")" = 'old index'
+    test "$(<"$TEST_TMPDIR/dist/index.html")" = 'old index'
     test::assert_path_absent "$TEST_TMPDIR/dist/photos/01-landscape.jpg"
     test::assert_path_absent "$TEST_TMPDIR/dist/photoalbum.json"
     test::assert_no_staging_dirs "$TEST_TMPDIR"
@@ -1980,7 +1981,7 @@ test_generate_templates_cannot_read_renderer_internals() {
     )
 
     test::assert_contains 'context_key: unbound variable' "$output"
-    test "$(cat "$TEST_TMPDIR/dist/index.html")" = 'old index'
+    test "$(<"$TEST_TMPDIR/dist/index.html")" = 'old index'
     test::assert_path_absent "$TEST_TMPDIR/dist/photos/01-landscape.jpg"
     test::assert_path_absent "$TEST_TMPDIR/dist/photoalbum.json"
     test::assert_no_staging_dirs "$TEST_TMPDIR"
@@ -2018,8 +2019,8 @@ test_generate_swap_failure_restores_dist() {
     )
 
     test::assert_contains 'simulated mv failure' "$output"
-    test "$(cat "$TEST_TMPDIR/dist/index.html")" = 'old index'
-    test "$(cat "$TEST_TMPDIR/dist/sentinel")" = 'old sentinel'
+    test "$(<"$TEST_TMPDIR/dist/index.html")" = 'old index'
+    test "$(<"$TEST_TMPDIR/dist/sentinel")" = 'old sentinel'
     test::assert_path_absent "$TEST_TMPDIR/dist/photos/01-landscape.jpg"
     test::assert_path_absent "$TEST_TMPDIR/dist/photoalbum.json"
     test::assert_no_staging_dirs "$TEST_TMPDIR"