patchette

errors: our message leads, git detail indents beneath, UGC gets quoted

commit b6ebe86871513a13f501f7de82272cf7a6ba597d · parent 2df663fec9 · browse this tree
Ximianth, 2026-10-19 11:53:50 UTC

Dogfooding showed an apply failure printing git's stderr ahead of our
own message, with a full absolute path in ours.  Three changes:

- Child-git stderr on apply failure is captured and printed indented
  beneath our message (die_detail), not discarded: in a multi-file
  patch those lines are the only source of which file and hunk failed.
- Patch-file paths in messages display relative to the patches dir.
- Every user-originated string in a die message -- paths, branch names,
  refs, slugs, parsed line content -- is quoted, via one helper (q)
  that also does the relativizing.  Swept all die call sites.

The new test asserts the full shape on an apply failure: our line first
with the relative quoted path, git's 'error: patch failed' indented
after it.

diff --git a/bin/patchette b/bin/patchette
index 46e9d49..5d8f7d0 100755
--- a/bin/patchette
+++ b/bin/patchette
@@ -37,6 +37,8 @@ PTMP=$(mktemp -d)
 # EXPORT_STAGE is a half-built branch folder; it lives next to its final
 # destination (not in PTMP), so it also needs cleanup if we die mid-build.
 EXPORT_STAGE=''
+# PATCHES_DIR_ABS makes patch-file paths in messages display relative (see q()).
+PATCHES_DIR_ABS=''
 trap 'rm -rf "$PTMP" ${EXPORT_STAGE:+"$EXPORT_STAGE"}' EXIT
 
 ## Report a refusal and quit.
@@ -48,6 +50,33 @@ die() {
 	exit 1
 }
 
+## Report a refusal with captured detail beneath it, and quit.
+## Our message leads; the detail (e.g. child-git stderr, which in a multi-file
+## patch is the only source of which file or hunk failed) follows, indented.
+##
+## - @param $1 the message
+## - @param $2 file holding the detail lines
+## - @stderr the message, then the detail indented
+die_detail() {
+	>&2 echo "patchette: $1"
+	>&2 sed 's/^/    /' "$2"
+	exit 1
+}
+
+## Format a user-originated string for inclusion in messages.
+## Paths are shown relative to the patches dir when one is in play;
+## everything gets quoted (these strings are effectively user-generated content).
+##
+## - @param $1 the string (a path, branch name, ref, slug, ...)
+## - @stdout the quoted, relativized string
+q() {
+	local s=$1
+	if [[ -n ${PATCHES_DIR_ABS:-} ]]; then
+		s=${s#"$PATCHES_DIR_ABS"/}
+	fi
+	printf "'%s'" "$s"
+}
+
 usage() {
 	cat <<-'EOF'
 		patchette: manage branch snapshots as patch files.
@@ -87,7 +116,7 @@ slugify() {
 ## - @param $1 path
 ## - @stdout the absolute path
 absdir() {
-	cd "$1" 2>/dev/null && pwd || die "no such directory: $1"
+	cd "$1" 2>/dev/null && pwd || die "no such directory: $(q "$1")"
 }
 
 ## Parse a STACK control file.
@@ -101,34 +130,34 @@ parse_stack() {
 	STACK_REF='' STACK_BASE='' STACK_TIP='' STACK_SERIES=()
 	while IFS= read -r line; do
 		if [[ $mode == series ]]; then
-			[[ $line =~ ^[a-z0-9-]+$ ]] || die "$f: malformed series entry: $line"
-			[[ -z ${seen[$line]:-} ]] || die "$f: duplicate series entry: $line"
+			[[ $line =~ ^[a-z0-9-]+$ ]] || die "$(q "$f"): malformed series entry: $(q "$line")"
+			[[ -z ${seen[$line]:-} ]] || die "$(q "$f"): duplicate series entry: $(q "$line")"
 			seen[$line]=1
 			STACK_SERIES+=("$line")
 			continue
 		fi
 		key=${line%% *}
-		[[ -z ${keys_seen[$key]:-} ]] || die "$f: duplicate $key line"
+		[[ -z ${keys_seen[$key]:-} ]] || die "$(q "$f"): duplicate $key line"
 		keys_seen[$key]=1
 		case $line in
 		"ref "*) STACK_REF=${line#ref } ;;
 		"base "*)
 			STACK_BASE=${line#base }
-			[[ $STACK_BASE =~ $oid ]] || die "$f: base is not a full commit ID: $STACK_BASE"
+			[[ $STACK_BASE =~ $oid ]] || die "$(q "$f"): base is not a full commit ID: $(q "$STACK_BASE")"
 			;;
 		"tip "*)
 			STACK_TIP=${line#tip }
-			[[ $STACK_TIP =~ $oid ]] || die "$f: tip is not a full commit ID: $STACK_TIP"
+			[[ $STACK_TIP =~ $oid ]] || die "$(q "$f"): tip is not a full commit ID: $(q "$STACK_TIP")"
 			;;
 		series) mode=series ;;
-		*) die "$f: unrecognized line: $line" ;;
+		*) die "$(q "$f"): unrecognized line: $(q "$line")" ;;
 		esac
 	done <"$f"
 	# tip is optional: a hand-made STACK has no expectable value for it
 	# until the branch has been built.
 	[[ -n $STACK_REF && -n $STACK_BASE && $mode == series ]] ||
-		die "$f: missing ref, base, or series section"
-	[[ $STACK_REF == refs/heads/* ]] || die "$f: ref is not under refs/heads/: $STACK_REF"
+		die "$(q "$f"): missing ref, base, or series section"
+	[[ $STACK_REF == refs/heads/* ]] || die "$(q "$f"): ref is not under refs/heads/: $(q "$STACK_REF")"
 }
 
 ## Check folder-level concordance between a STACK's series and the *.patch
@@ -145,13 +174,13 @@ validate_branch_dir() {
 	declare -A listed=()
 	for slug in "${STACK_SERIES[@]}"; do
 		listed[$slug]=1
-		[[ -f $dir/$slug.patch ]] || die "$dir/STACK: series lists $slug, but $slug.patch is missing"
+		[[ -f $dir/$slug.patch ]] || die "$(q "$dir/STACK"): series lists $(q "$slug"), but $(q "$slug.patch") is missing"
 	done
 	for f in "$dir"/*.patch; do
 		[[ -f $f ]] || continue # unmatched glob
 		slug=${f##*/}
 		slug=${slug%.patch}
-		[[ -n ${listed[$slug]:-} ]] || die "$f: orphan patch file, not listed in the STACK series"
+		[[ -n ${listed[$slug]:-} ]] || die "$(q "$f"): orphan patch file, not listed in the STACK series"
 	done
 }
 
@@ -182,7 +211,7 @@ replay_series() {
 			lineno=$((lineno + 1))
 			case $state in
 			start)
-				[[ $line == parents\ * ]] || die "$f: line 1 is not a parents line"
+				[[ $line == parents\ * ]] || die "$(q "$f"): line 1 is not a parents line"
 				pline=$line
 				state=headers
 				;;
@@ -192,13 +221,13 @@ replay_series() {
 				elif [[ $line != *' '* ]]; then
 					# Headers are `key value`, and continuation lines begin with
 					# a space, so every header line contains one.
-					die "$f: malformed header line (no space): $line"
+					die "$(q "$f"): malformed header line (no space): $(q "$line")"
 				else
 					headers+=("$line")
 				fi
 				;;
 			msgopen)
-				[[ $line == '----' ]] || die "$f: expected opening ---- flank, got: $line"
+				[[ $line == '----' ]] || die "$(q "$f"): expected opening ---- flank, got: $(q "$line")"
 				state=directives
 				;;
 			directives)
@@ -211,7 +240,7 @@ replay_series() {
 					continue
 				fi
 				if [[ -n $line && $line != '----' && $line != '>' && $line != '> '* && $line != $'\t'* ]]; then
-					die "$f: unrecognized directive line: $line"
+					die "$(q "$f"): unrecognized directive line: $(q "$line")"
 				fi
 				state=message
 				;&
@@ -226,28 +255,28 @@ replay_series() {
 				elif [[ $line == $'\t'* ]]; then
 					printf '%s\n' "${line:1}" >>"$msgpart"
 				else
-					die "$f: malformed message line: $line"
+					die "$(q "$f"): malformed message line: $(q "$line")"
 				fi
 				;;
 			postmsg)
-				[[ -z $line ]] || die "$f: expected blank line after closing flank, got: $line"
+				[[ -z $line ]] || die "$(q "$f"): expected blank line after closing flank, got: $(q "$line")"
 				state=diff
 				break
 				;;
 			esac
 		done <"$f"
-		[[ $state == diff ]] || die "$f: truncated (ended in state $state)"
-		[[ ${#headers[@]} -gt 0 ]] || die "$f: no commit headers"
+		[[ $state == diff ]] || die "$(q "$f"): truncated (ended in state $state)"
+		[[ ${#headers[@]} -gt 0 ]] || die "$(q "$f"): no commit headers"
 		tail -n +$((lineno + 1)) "$f" >"$diffpart"
 		if [[ -n $nofinal ]]; then
 			head -c -1 "$msgpart" >"$msgpart.t" && mv "$msgpart.t" "$msgpart"
 		fi
 
 		read -ra prefs <<<"${pline#parents }"
-		[[ ${#prefs[@]} -gt 0 ]] || die "$f: parents line lists no parents"
+		[[ ${#prefs[@]} -gt 0 ]] || die "$(q "$f"): parents line lists no parents"
 		parents=()
 		for pref in "${prefs[@]}"; do
-			[[ -n ${sha_of[$pref]:-} ]] || die "$f: parent '$pref' is not BASE or an earlier slug in the series"
+			[[ -n ${sha_of[$pref]:-} ]] || die "$(q "$f"): parent $(q "$pref") is not BASE or an earlier slug in the series"
 			parents+=("${sha_of[$pref]}")
 		done
 
@@ -257,7 +286,8 @@ replay_series() {
 			# --whitespace=nowarn: the target repo's own apply.whitespace must not
 			# alter (=fix) or refuse (=error) the recorded bytes, and warnings about
 			# whitespace that's faithfully preserved are just noise.
-			git apply --cached --binary --whitespace=nowarn "$diffpart" || die "$f: diff does not apply onto its parent"
+			git apply --cached --binary --whitespace=nowarn "$diffpart" 2>"$PTMP/apply-err" ||
+				die_detail "$(q "$f"): diff does not apply onto its parent" "$PTMP/apply-err"
 		fi
 		tree=$(git write-tree)
 
@@ -288,12 +318,12 @@ branch_dirs() {
 	for dir in "$patches"/*/; do
 		dir=${dir%/}
 		[[ -d $dir ]] || continue # unmatched glob
-		[[ -f $dir/STACK ]] || die "$dir has no STACK file; not a patchette branch folder"
+		[[ -f $dir/STACK ]] || die "$(q "$dir") has no STACK file; not a patchette branch folder"
 		parse_stack "$dir/STACK"
 		validate_branch_dir "$dir"
 		BRANCH_DIRS+=("$dir")
 	done
-	[[ ${#BRANCH_DIRS[@]} -gt 0 ]] || die "no branch folders found in $patches"
+	[[ ${#BRANCH_DIRS[@]} -gt 0 ]] || die "no branch folders found in $(q "$patches")"
 }
 
 cmd_export() {
@@ -301,7 +331,7 @@ cmd_export() {
 	while [[ $# -gt 0 && $1 == --* ]]; do
 		case $1 in
 		--sigil=quote | --sigil=tab) sigil=${1#--sigil=} ;;
-		*) die "unknown flag: $1" ;;
+		*) die "unknown flag: $(q "$1")" ;;
 		esac
 		shift
 	done
@@ -311,11 +341,12 @@ cmd_export() {
 	repo=$(absdir "$1")
 	mkdir -p "$2"
 	out=$(absdir "$2")
+	PATCHES_DIR_ABS=$out
 	baserev=$3
 	shift 3
 
 	base=$(git -C "$repo" rev-parse --verify -q "$baserev^{commit}") ||
-		die "base revision does not resolve to a commit: $baserev"
+		die "base revision does not resolve to a commit: $(q "$baserev")"
 
 	# Patch bytes must depend only on the commits, so the diffs are generated in
 	# a scratch bare repo that borrows the subject's objects via alternates.
@@ -331,12 +362,12 @@ cmd_export() {
 
 	for branch in "$@"; do
 		tip=$(git -C "$repo" rev-parse --verify -q "refs/heads/$branch") ||
-			die "no such branch: $branch"
+			die "no such branch: $(q "$branch")"
 		dir="$out/${branch//\//$'\u00a6'}"
 		if [[ -e $dir ]]; then
 			# Re-exporting a tracked branch replaces its folder; that's the
 			# normal snapshot flow.  But never delete things we didn't write.
-			[[ -f $dir/STACK ]] || die "$dir exists but has no STACK file; not a patchette branch folder, refusing to replace it"
+			[[ -f $dir/STACK ]] || die "$(q "$dir") exists but has no STACK file; not a patchette branch folder, refusing to replace it"
 		fi
 		# Build in a stage folder beside the destination, and swap in only a
 		# finished snapshot: a death mid-export must leave the previous
@@ -373,7 +404,7 @@ cmd_export() {
 				elif [[ -n ${slug_of[$p]:-} ]]; then
 					parents+=("${slug_of[$p]}")
 				else
-					die "commit $sha has parent $p outside $baserev..$branch; out-of-range parents are not supported"
+					die "commit $sha has parent $p outside $(q "$baserev")..$(q "$branch"); out-of-range parents are not supported"
 				fi
 			done
 			[[ ${#parents[@]} -gt 0 ]] || die "commit $sha has no parents (root commit in range); not supported"
@@ -415,6 +446,7 @@ cmd_import() {
 	local target patches dir
 	target=$(absdir "$1")
 	patches=$(absdir "$2")
+	PATCHES_DIR_ABS=$patches
 	branch_dirs "$patches"
 	cd "$target"
 	export GIT_INDEX_FILE="$PTMP/scratch-index"
@@ -422,9 +454,9 @@ cmd_import() {
 	for dir in "${BRANCH_DIRS[@]}"; do
 		parse_stack "$dir/STACK"
 		git show-ref --verify --quiet "$STACK_REF" &&
-			die "$STACK_REF already exists in $target; refusing to touch it"
+			die "$(q "$STACK_REF") already exists in $(q "$target"); refusing to touch it"
 		git cat-file -e "$STACK_BASE^{commit}" 2>/dev/null ||
-			die "recorded base $STACK_BASE is not present in $target; fetch from upstream first"
+			die "recorded base $STACK_BASE is not present in $(q "$target"); fetch from upstream first"
 		replay_series "$dir"
 		# Empty old-value makes must-not-exist atomic: no clobbering, even racily.
 		git update-ref "$STACK_REF" "$REPLAY_TIP" ""
@@ -440,6 +472,7 @@ cmd_verify() {
 	local subject patches dir bad=0
 	subject=$(absdir "$1")
 	patches=$(absdir "$2")
+	PATCHES_DIR_ABS=$patches
 	branch_dirs "$patches"
 
 	# Replay into a throwaway shared bare clone: cheap even for huge repos,
@@ -451,7 +484,7 @@ cmd_verify() {
 	for dir in "${BRANCH_DIRS[@]}"; do
 		parse_stack "$dir/STACK"
 		git cat-file -e "$STACK_BASE^{commit}" 2>/dev/null ||
-			die "recorded base $STACK_BASE is not present in $subject"
+			die "recorded base $STACK_BASE is not present in $(q "$subject")"
 		replay_series "$dir"
 		if [[ -z $STACK_TIP ]]; then
 			echo "verify: note: $STACK_REF replays cleanly to $REPLAY_TIP, but records no tip to cross-check against"
@@ -481,6 +514,6 @@ export | import | verify)
 	exit 1
 	;;
 *)
-	die "unknown command: $1 (try: patchette help)"
+	die "unknown command: $(q "$1") (try: patchette help)"
 	;;
 esac
diff --git a/tests/test-errors.sh b/tests/test-errors.sh
new file mode 100755
index 0000000..493eb8f
--- /dev/null
+++ b/tests/test-errors.sh
@@ -0,0 +1,32 @@
+#!/bin/bash
+## Error presentation: when child git fails, our message leads and git's
+## stderr follows indented beneath it (in a multi-file patch it's the only
+## source of which file or hunk failed); patch paths in our messages are
+## relative to the patches dir and quoted.
+set -euo pipefail
+cd "$(dirname "$0")"
+source ./lib.sh
+
+work=$(mktemp -d)
+trap 'rm -rf "$work"' EXIT
+
+make_corpus "$work/corpus"
+source "$work/corpus/FACTS"
+
+"$PATCHETTE" export "$work/corpus/dev" "$work/patches" "$base" topic/fancy >/dev/null
+# Corrupt a context line in the diff so git apply itself fails.
+sed -i 's/^ gear 4 turns smoothly$/ gear 4 turns wrongly/' "$work/patches/topic¦fancy/teach-gizmo-to-hum.patch"
+
+rc=0
+"$PATCHETTE" verify "$work/corpus/dev" "$work/patches" >"$work/out" 2>&1 || rc=$?
+EXPECT equal "an unapplicable diff still fails verify" -- "$rc" "1"
+EXPECT success "our message leads, with the patch path relative and quoted" -- \
+	grep -q "^patchette: 'topic¦fancy/teach-gizmo-to-hum.patch': diff does not apply onto its parent$" "$work/out"
+EXPECT success "git's own stderr follows, indented as detail" -- \
+	grep -q "^    error: patch failed" "$work/out"
+ours=$(grep -nm1 '^patchette:' "$work/out") && ours=${ours%%:*}
+detail=$(grep -nm1 '^    error:' "$work/out") && detail=${detail%%:*}
+EXPECT success "and in that order" -- \
+	test "$ours" -lt "$detail"
+
+exit "$RESULT"
diff --git a/tests/test-failclosed.sh b/tests/test-failclosed.sh
index 31a0164..8c4b8f8 100755
--- a/tests/test-failclosed.sh
+++ b/tests/test-failclosed.sh
@@ -113,9 +113,9 @@ EXPECT equal "the refused import created no refs" -- \
 
 clone_layout missingfile
 rm "$TAMPER/$fancy/note-to-self.patch"
-EXPECT refusal "a series entry naming no file is refused by verify" "note-to-self.patch is missing" -- \
+EXPECT refusal "a series entry naming no file is refused by verify" "'note-to-self.patch' is missing" -- \
 	"$PATCHETTE" verify "$work/corpus/dev" "$TAMPER"
-EXPECT refusal "a series entry naming no file is refused by import" "note-to-self.patch is missing" -- \
+EXPECT refusal "a series entry naming no file is refused by import" "'note-to-self.patch' is missing" -- \
 	"$PATCHETTE" import "$work/orphantarget" "$TAMPER"
 
 # Export never deletes a folder it didn't write.