Fix results ordering, control file whitespace, ENABLE_* override, debug levels

.claude/development.md

@@ -0,0 +1,48 @@
+# pgxntool Development Guidelines
+
+**THIS FILE IS FOR PGXNTOOL DEVELOPERS ONLY.**
+
+If you are an extension developer using pgxntool in your project, this file does not
+apply to you. See the top-level `CLAUDE.md` instead.
+
+## Critical: Work from pgxntool-test, Not Here
+
+**NEVER make changes to pgxntool directly from this repository.**
+
+pgxntool development must be done from a checkout of **pgxntool-test**, which contains
+the full test infrastructure. Working here directly means you cannot run tests, and
+any changes you commit cannot be validated before merging.
+
+**Correct workflow:**
+1. Clone or use an existing checkout of `pgxntool-test`
+2. Work in a worktree: both `pgxntool/` and `pgxntool-test/` will be siblings
+3. Make changes to `pgxntool/` from within that pgxntool-test context
+4. Run the test suite via `make test` in pgxntool-test before committing
+
+**See:** https://github.com/Postgres-Extensions/pgxntool-test for the full development
+workflow.
+
+---
+
+## Makefile Variable Assignment Rules
+
+**RULE: Do not use `:=` (simply expanded) unless you have a specific need for immediate evaluation.**
+
+Use `=` (recursively expanded) for standard variable assignments. Reserve `:=` for cases where the right-hand side must be evaluated exactly once at assignment time — for example, when assigning the result of a `$(call ...)` function that references the variable being set (which would cause infinite recursion with `=`).
+
+When a variable must also override command-line values, combine `override` with `:=` — but only where `override` is genuinely needed.
+
+## Debug Level Rules (lib.sh `debug` function)
+
+The `debug` function in `lib.sh` uses a range-based numeric level scheme. The order of magnitude gives a rough sense of verbosity; room within each decade allows fine-tuning without renumbering.
+
+- **1–9**: High-level script context — script arguments (1), top-level entry/exit (2–3), lower-level helper calls (4–5). Use sparingly.
+- **10–19**: Per-loop-iteration detail — exit with status (10), entry (11), intermediate results (15).
+- **20–29**: Nesting inside loop-level detail.
+- Higher ranges follow the same pattern.
+
+A non-trivial script with loops should have debug calls spanning multiple ranges.
+
+The commit skill checks for unusual distributions (e.g., a script with loops that only uses single-digit levels).
+
+Note: The BATS test helper `debug` function (in `tests/lib/helpers.bash` in pgxntool-test) uses a separate 1–5 scale controlled by `$TESTDEBUG`. The two systems are independent.

CLAUDE.md

@@ -4,10 +4,17 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
 
 ## Scope of This File
 
-The guidance in this CLAUDE.md applies to pgxntool's own source files and provides
-recommended best practices for projects using pgxntool. However, any agent working in
-an extension project should always defer to that project's own CLAUDE.md and
-instructions over anything stated here.
+**CLAUDE.md is for people USING pgxntool** — extension developers who have embedded
+pgxntool into their project via `git subtree`. It documents the build system, available
+commands, and how pgxntool works.
+
+**If you are making changes to pgxntool itself**, stop — you are in the wrong place.
+See `.claude/` in this directory for developer guidelines. More importantly, pgxntool
+development must be done from the **pgxntool-test** repository, not from here. See the
+`Development Workflow` section below.
+
+Any agent working in an extension project should always defer to that project's own
+CLAUDE.md and instructions over anything stated here.
 
 ## Git Commit Guidelines
 

HISTORY.asc

@@ -1,3 +1,14 @@
+STABLE
+------
+== Fix `verify-results` checking stale results in `make results`
+`make results` ran `verify-results` before `make test`, so it checked stale
+`regression.diffs` from a prior run. Reordered so `verify-results` always
+checks the fresh results.
+
+== Fix `PGXNTOOL_ENABLE_TEST_BUILD`/`PGXNTOOL_ENABLE_TEST_INSTALL` ignoring command-line values
+Without `override`, the `pgxntool_validate_yesno` normalization was silently
+skipped when these variables were set on the command line.
+
 2.0.2
 -----
 == Fix parse_control_file corrupting values with trailing comments

base.mk

@@ -93,7 +93,9 @@ pgxntool_validate_yesno = $(strip \
 TEST_BUILD_SQL_FILES = $(wildcard $(TESTDIR)/build/*.sql)
 TEST_BUILD_FILES = $(TEST_BUILD_SQL_FILES)
 ifdef PGXNTOOL_ENABLE_TEST_BUILD
-  PGXNTOOL_ENABLE_TEST_BUILD := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_TEST_BUILD),PGXNTOOL_ENABLE_TEST_BUILD)
+  # override needed so command-line values (make VAR=YES) are normalized, not silently ignored.
+  # := needed for immediate evaluation of the function call (avoids infinite recursion with =).
+  override PGXNTOOL_ENABLE_TEST_BUILD := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_TEST_BUILD),PGXNTOOL_ENABLE_TEST_BUILD)
 else
   # Auto-detect: enable if test/build/ directory has SQL files
   ifneq ($(strip $(TEST_BUILD_FILES)),)
@@ -135,7 +137,9 @@ endif
 # Either approach would eliminate the ~10-line block repeated for each feature.
 TEST_INSTALL_SQL_FILES = $(wildcard $(TESTDIR)/install/*.sql)
 ifdef PGXNTOOL_ENABLE_TEST_INSTALL
-  PGXNTOOL_ENABLE_TEST_INSTALL := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_TEST_INSTALL),PGXNTOOL_ENABLE_TEST_INSTALL)
+  # override needed so command-line values (make VAR=YES) are normalized, not silently ignored.
+  # := needed for immediate evaluation of the function call (avoids infinite recursion with =).
+  override PGXNTOOL_ENABLE_TEST_INSTALL := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_TEST_INSTALL),PGXNTOOL_ENABLE_TEST_INSTALL)
 else
   # Auto-detect: enable if test/install/ directory has SQL files
   ifneq ($(strip $(TEST_INSTALL_SQL_FILES)),)
@@ -292,9 +296,12 @@ endif
 
 # make results: runs `make test` and copies all result files to expected.
 # DO NOT RUN THIS UNLESS YOU'RE CERTAIN ALL YOUR TESTS ARE PASSING!
+# verify-results runs AFTER test (not before) so it checks the fresh regression.diffs
+# from this run, not stale results from a prior run. The old order (verify-results test)
+# checked pre-run state, which would miss failures introduced by the re-run itself.
 .PHONY: results
 ifeq ($(PGXNTOOL_ENABLE_VERIFY_RESULTS),yes)
-results: verify-results test
+results: test verify-results
 else
 results: test
 endif

lib.sh

@@ -60,13 +60,18 @@ array_not_empty() {
 # Debug function
 # Usage: debug LEVEL "message"
 # Outputs message to stderr if DEBUG >= LEVEL
-# Debug levels use multiples of 10 (10, 20, 30, 40, etc.) to allow for easy expansion
-#   - 10: Critical errors, important warnings
-#   - 20: Warnings, significant state changes
-#   - 30: General debugging, function entry/exit, array operations
-#   - 40: Verbose details, loop iterations
-#   - 50+: Maximum verbosity
-# Enable with: DEBUG=30 scriptname.sh
+#
+# Debug levels use ranges, not strict multiples, to leave room for fine-tuning
+# without renumbering. The order of magnitude gives a rough sense of verbosity:
+#   - 1-9:   High-level script context: arguments (1), top-level entry/exit (2-3),
+#            lower-level helper calls (4-5)
+#   - 10-19: Per-iteration detail inside loops: exit+status (10), entry (11),
+#            intermediate results (15)
+#   - 20-29: Nesting inside loop-level detail
+#   - Higher ranges follow the same pattern
+#
+# A non-trivial script with loops should have debug calls in multiple ranges.
+# Enable with: DEBUG=15 scriptname.sh
 debug() {
     local level=$1
     shift

pgtle.sh

@@ -156,7 +156,7 @@ MODULE_PATHNAME=""
 VERSION_FILES=()
 UPGRADE_FILES=()
 
-debug 1 "Global arrays initialized: VERSION_FILES=${#VERSION_FILES[@]}, UPGRADE_FILES=${#UPGRADE_FILES[@]}"
+debug 30 "Global arrays initialized: VERSION_FILES=${#VERSION_FILES[@]}, UPGRADE_FILES=${#UPGRADE_FILES[@]}"
 PGTLE_VERSION=""  # Empty = generate all
 GET_DIR_VERSION=""  # For --get-dir option
 
@@ -463,7 +463,15 @@ parse_control_file() {
             # Order matters: stripping quotes before removing comments leaves a rogue
             # trailing quote for values like 'version' # note. See issue #25.
             value="${value%%#*}"  # Remove trailing comments
-            value="${value%% }"   # Trim trailing whitespace
+            # Trim all trailing whitespace (spaces and tabs). The prior %% pattern with
+            # a literal space only removed one character; multiple spaces or a tab before
+            # the comment (e.g., 'value'   # note or 'value'$'\t'# note) would leave
+            # stray whitespace that breaks the quote-strip below.
+            if [[ "$value" =~ ^(.*[^[:space:]])[[:space:]]*$ ]]; then
+                value="${BASH_REMATCH[1]}"
+            else
+                value=""
+            fi
 
             # Strip quotes if present (both single and double)
             value="${value#\'}"
@@ -513,21 +521,21 @@ parse_control_file() {
 
 discover_sql_files() {
     echo "Discovering SQL files for extension: $EXTENSION" >&2
-    debug 2 "discover_sql_files: Starting discovery for extension: $EXTENSION"
+    debug 30 "discover_sql_files: Starting discovery for extension: $EXTENSION"
 
     # Ensure default_version file exists and has content if base file exists
     # This handles the case where make all hasn't generated it yet, or it exists but is empty
     local default_version_file="sql/${EXTENSION}--${DEFAULT_VERSION}.sql"
     local base_file="sql/${EXTENSION}.sql"
     if [ -f "$base_file" ] && ([ ! -f "$default_version_file" ] || [ ! -s "$default_version_file" ]); then
-        debug 3 "discover_sql_files: Creating default_version file from base file"
+        debug 40 "discover_sql_files: Creating default_version file from base file"
         cp "$base_file" "$default_version_file"
     fi
 
     # Find versioned files: sql/{ext}--{version}.sql
     # Use find to get proper null-delimited output, then filter out upgrade scripts
     VERSION_FILES=()  # Reset array
-    debug 3 "discover_sql_files: Reset VERSION_FILES array"
+    debug 40 "discover_sql_files: Reset VERSION_FILES array"
     while IFS= read -r -d '' file; do
         local basename=$(basename "$file" .sql)
         local dash_count=$(echo "$basename" | grep -o -- "--" | wc -l | tr -d '[:space:]')
@@ -545,7 +553,7 @@ discover_sql_files() {
     # Find upgrade scripts: sql/{ext}--{ver1}--{ver2}.sql
     # These have TWO occurrences of "--" in the filename
     UPGRADE_FILES=()  # Reset array
-    debug 3 "discover_sql_files: Reset UPGRADE_FILES array"
+    debug 40 "discover_sql_files: Reset UPGRADE_FILES array"
     while IFS= read -r -d '' file; do
         # Empty upgrade files are allowed (no-op upgrades)
         local basename=$(basename "$file" .sql)
@@ -566,15 +574,15 @@ discover_sql_files() {
         echo "    - $f" >&2
     done
 
-    debug 3 "discover_sql_files: Checking UPGRADE_FILES array, count=${#UPGRADE_FILES[@]}"
+    debug 40 "discover_sql_files: Checking UPGRADE_FILES array, count=${#UPGRADE_FILES[@]}"
     if array_not_empty "${#UPGRADE_FILES[@]}"; then
         echo "  Found ${#UPGRADE_FILES[@]} upgrade script(s):" >&2
-        debug 2 "discover_sql_files: Iterating over ${#UPGRADE_FILES[@]} upgrade files"
+        debug 30 "discover_sql_files: Iterating over ${#UPGRADE_FILES[@]} upgrade files"
         for f in "${UPGRADE_FILES[@]}"; do
             echo "    - $f" >&2
         done
     else
-        debug 2 "discover_sql_files: No upgrade files found"
+        debug 30 "discover_sql_files: No upgrade files found"
     fi
 }
 
@@ -752,7 +760,7 @@ generate_install_update_path() {
 
 generate_pgtle_sql() {
     local pgtle_version="$1"
-    debug 2 "generate_pgtle_sql: Starting for version $pgtle_version, extension $EXTENSION"
+    debug 30 "generate_pgtle_sql: Starting for version $pgtle_version, extension $EXTENSION"
 
     # Get capability using function (compatible with bash < 4.0)
     local capability=$(get_pgtle_capability "$pgtle_version")
@@ -761,9 +769,9 @@ generate_pgtle_sql() {
 
     # Ensure arrays are initialized (defensive programming)
     # Arrays should already be initialized at top level, but ensure they exist
-    debug 3 "generate_pgtle_sql: Checking array initialization"
-    debug 2 "generate_pgtle_sql: VERSION_FILES is ${VERSION_FILES+set}, count=${#VERSION_FILES[@]}"
-    debug 2 "generate_pgtle_sql: UPGRADE_FILES is ${UPGRADE_FILES+set}, count=${#UPGRADE_FILES[@]}"
+    debug 40 "generate_pgtle_sql: Checking array initialization"
+    debug 30 "generate_pgtle_sql: VERSION_FILES is ${VERSION_FILES+set}, count=${#VERSION_FILES[@]}"
+    debug 30 "generate_pgtle_sql: UPGRADE_FILES is ${UPGRADE_FILES+set}, count=${#UPGRADE_FILES[@]}"
 
     if [ -z "${VERSION_FILES+set}" ]; then
         echo "WARNING: VERSION_FILES not set, initializing" >&2
@@ -822,17 +830,17 @@ EOF
 
         # Install all upgrade paths
         local upgrade_count=${#UPGRADE_FILES[@]}
-        debug 3 "generate_pgtle_sql: upgrade_count=$upgrade_count"
+        debug 40 "generate_pgtle_sql: upgrade_count=$upgrade_count"
         if [ "$upgrade_count" -gt 0 ]; then
-            debug 2 "generate_pgtle_sql: Processing $upgrade_count upgrade path(s)"
+            debug 30 "generate_pgtle_sql: Processing $upgrade_count upgrade path(s)"
             local i=0
             while [ "$i" -lt "$upgrade_count" ]; do
-                debug 4 "generate_pgtle_sql: Processing upgrade file $i: ${UPGRADE_FILES[$i]}"
+                debug 50 "generate_pgtle_sql: Processing upgrade file $i: ${UPGRADE_FILES[$i]}"
                 generate_install_update_path "${UPGRADE_FILES[$i]}"
                 i=$((i + 1))
             done
         else
-            debug 2 "generate_pgtle_sql: No upgrade paths to process"
+            debug 30 "generate_pgtle_sql: No upgrade paths to process"
         fi
 
         cat <<EOF