Smoke-test the bash and fish hooks, not just zsh

Two of the three shipped hooks had no test. hook-zsh.sh becomes
hook.sh <shell> and covers all three, checking both halves of the hook:
a session directory means preexec fired, its done marker means postexec
did.

fish needs a pty. It raises fish_preexec only for a command typed at a
terminal, so fed from a pipe it runs the command with no event and the
hook looks broken when nothing is wrong with it. script(1) gives it one.
The hook itself turned out to be fine, so the README no longer calls it
untested.

Installing the shells has to happen before make test, since each smoke
test skips itself when its shell is missing and would otherwise pass
while testing nothing.

Author: edaywalid

.github/workflows/ci.yml

@@ -23,12 +23,13 @@ jobs:
         run: go vet ./...
       - name: build
         run: make
+      # before the tests, not after: make test runs the hook smoke tests and
+      # each one skips itself when its shell is missing, so installing these
+      # late would have looked green while testing nothing
+      - name: shells for the hook tests
+        run: sudo apt-get update -qq && sudo apt-get install -y -qq zsh fish
       - name: unit and end-to-end tests
         run: make test
-      - name: zsh hook smoke test
-        run: |
-          sudo apt-get update -qq && sudo apt-get install -y -qq zsh
-          ./test/hook-zsh.sh
 
   musl:
     # glibc-isms in the shim would surface here; informational for now

CONTRIBUTING.md

@@ -9,8 +9,10 @@ make test     # unit tests + end-to-end suite
 
 The e2e suite (`test/e2e.sh`) arms the shim the same way the shell hooks
 do and exercises real deletions, overwrites, renames, undo, redo, and
-gc against a temp directory. `test/hook-zsh.sh` smoke-tests the zsh hook
-in an isolated interactive shell.
+gc against a temp directory. `test/hook.sh <shell>` smoke-tests a real
+hook in an isolated interactive shell, for zsh, bash and fish; it skips
+itself when that shell is not installed. fish is driven through a pty,
+since it raises `fish_preexec` only for a command typed at a terminal.
 
 Anything touching the shim (`shim/undo_shim.c`) should come with an e2e
 case. The shim runs inside every process a user launches: no output on

Makefile

@@ -17,6 +17,10 @@ bin/undo: $(GO_SRC) go.mod
 test: all
 	go test ./...
 	./test/e2e.sh
+	@# each skips itself when that shell is not installed
+	./test/hook.sh zsh
+	./test/hook.sh bash
+	./test/hook.sh fish
 
 install: all
 	install -Dm755 bin/undo $(PREFIX)/bin/undo

README.md

@@ -320,8 +320,9 @@ backups.
   Arch, openSUSE, ...). WSL2 counts, it is Linux.
 - **Alpine / musl**: build from source (`make install`, CI-tested); the
   prebuilt shim in releases targets glibc.
-- **Shells**: zsh, bash 5+, fish 3.4+ for the automatic hook. Any shell
-  works with `undo run`. The fish hook is currently untested.
+- **Shells**: zsh, bash 5+, fish 3.4+ for the automatic hook, all three
+  smoke-tested in CI against a real interactive shell. Any shell works
+  with `undo run`.
 - **macOS**: not supported. SIP blocks library injection into system
   binaries, so a port could not cover `rm` and friends.
 - **Windows**: use it inside WSL2.

test/hook-zsh.sh

@@ -0,0 +1,106 @@
+#!/usr/bin/env bash
+# Smoke test for a shell hook: source it in an isolated interactive shell,
+# delete a file, and check a session was recorded and is undoable.
+#
+#   test/hook.sh zsh|bash|fish
+#
+# This runs the hook the way a user gets it, through the shell's own
+# preexec/postexec machinery, which is the part e2e.sh cannot reach: it
+# arms the shim itself and never loads a hook at all.
+set -euo pipefail
+
+sh=${1:?usage: hook.sh <zsh|bash|fish>}
+ROOT=$(cd "$(dirname "$0")/.." && pwd)
+WORK=$(mktemp -d)
+trap 'rm -rf "$WORK"' EXIT
+
+mkdir -p "$WORK/store" "$WORK/play"
+echo "precious data" >"$WORK/play/file.txt"
+
+command -v "$sh" >/dev/null || { echo "$sh not installed, skipped"; exit 0; }
+
+# A developer running this already has undo in their own shell, and every
+# UNDO_* it exports would be inherited and tested instead of the repo.
+clean_env=(env -u LD_PRELOAD -u UNDO_SESSION -u UNDO_LIB -u UNDO_DATA_DIR
+    -u UNDO_HOOK -u UNDO_IGNORE -u UNDO_KEEP)
+
+# posix shells and fish disagree on everything about assignment, so the rc
+# is written per shell rather than shared
+case $sh in
+zsh | bash)
+    mkdir -p "$WORK/rcdir"
+    # zsh only reads $ZDOTDIR/.zshrc, bash takes any name via --rcfile
+    cat >"$WORK/rcdir/.zshrc" <<EOF
+export UNDO_DATA_DIR=$WORK/store
+export UNDO_LIB=$ROOT/build/libundo.so
+export PATH=$ROOT/bin:\$PATH
+source $ROOT/shell/undo.$sh
+EOF
+    ;;
+fish)
+    mkdir -p "$WORK/rcdir/fish"
+    cat >"$WORK/rcdir/fish/config.fish" <<EOF
+set -gx UNDO_DATA_DIR $WORK/store
+set -gx UNDO_LIB $ROOT/build/libundo.so
+set -gx PATH $ROOT/bin \$PATH
+source $ROOT/shell/undo.fish
+EOF
+    ;;
+*)
+    echo "unknown shell: $sh" >&2
+    exit 2
+    ;;
+esac
+
+cmds=$(printf 'rm %s/play/file.txt\nundo -y\ncat %s/play/file.txt\nexit\n' \
+    "$WORK" "$WORK")
+
+# The exit status is the last command's, not a verdict on the hook, and
+# fish hands back non-zero where the others do not. Judge the store and the
+# output below instead of dying here with nothing to show.
+case $sh in
+zsh)
+    out=$(printf '%s\n' "$cmds" |
+        "${clean_env[@]}" ZDOTDIR="$WORK/rcdir" zsh -i 2>&1 || true) ;;
+bash)
+    out=$(printf '%s\n' "$cmds" |
+        "${clean_env[@]}" bash --rcfile "$WORK/rcdir/.zshrc" -i 2>&1 || true) ;;
+fish)
+    # fish only raises fish_preexec for a command typed at a terminal. Fed
+    # from a pipe it runs the command and the event never fires, so the
+    # hook looks broken when it is the test that is. script(1) gives it a
+    # pty and forwards our stdin into it, which is close enough to typing.
+    command -v script >/dev/null ||
+        { echo "script(1) not installed, skipped"; exit 0; }
+    out=$(printf '%s\n' "$cmds" |
+        "${clean_env[@]}" XDG_CONFIG_HOME="$WORK/rcdir" \
+        script -qec "fish -i" /dev/null 2>&1 || true) ;;
+esac
+
+fail() {
+    echo "FAIL ($sh hook): $*" >&2
+    echo "--- shell output ---" >&2
+    echo "$out" >&2
+    exit 1
+}
+
+grep -q "precious data" <<<"$out" || fail "file not restored"
+
+# The store proves which half of the hook ran. A session directory means
+# preexec fired; the done marker means postexec did, which is what puts
+# LD_PRELOAD back. Checking $LD_PRELOAD from a command cannot see this:
+# preexec has already armed it again by the time the command runs.
+shopt -s nullglob
+sessions=("$WORK"/store/sessions/*/)
+((${#sessions[@]} > 0)) || fail "no session recorded, preexec never fired"
+
+# The session for the last command stays open by design: the shell exits
+# out from under it and postexec never gets to run. Any earlier one closing
+# is enough to show the second half of the hook works.
+closed=0
+for s in "${sessions[@]}"; do
+    [[ -f "$s/done" ]] && closed=$((closed + 1))
+done
+((closed > 0)) || fail "no session got a done marker, postexec never fired"
+
+echo "$sh hook smoke test passed ($closed/${#sessions[@]} sessions closed)"