patchette

errors: one helper fronts every fallible child call; repo paths relativize

commit 6f031e76588e173a4167ae4c2ed3c217b5d795a6 · parent efdd15f493 · browse this tree
Ximianth, 2026-10-19 15:40:13 UTC

Dogfooding feedback: error handling must be consistent across every
branch, not polished one reported case at a time.  So:

- try() is the single shape for running fallible child commands: stderr
  captured; on failure our message leads and the child's stderr indents
  beneath it; on success captured warnings still pass through rather
  than being eaten.  Applied to every child invocation that can
  plausibly fail -- replay (read-tree, apply, write-tree, hash-object),
  export (scratch init, cat-file, diff-tree), import (update-ref),
  verify (clone).
- export's commit walk moves out of a process substitution, where a
  failure was invisible: a failed rev-list read as an empty branch,
  and the empty series was swapped in over a good snapshot, exiting 0.
  The walk now goes through try() into a file before the loop reads it;
  the new fail-closed case failed before this change.
  The per-commit lookups after it (subject, parents) stay bare:
  the walk has just read those same objects.
- q() now also relativizes non-patch paths, against the directory the
  user invoked us from (captured up front: the commands cd around
  internally, so $PWD is no anchor at die time).  Paths outside both
  anchors stay absolute.

diff --git a/bin/patchette b/bin/patchette
index 5d8f7d0..b77334f 100755
--- a/bin/patchette
+++ b/bin/patchette
@@ -34,6 +34,8 @@ export GIT_ATTR_NOSYSTEM=1
 export LC_ALL=C.UTF-8
 
 PTMP=$(mktemp -d)
+# Where the user invoked us; q() relativizes message paths against this.
+START_PWD=$PWD
 # 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=''
@@ -63,16 +65,38 @@ die_detail() {
 	exit 1
 }
 
+## Run a child command with its stderr captured, so every failure presents
+## the same way: our message leads, the child's stderr indents beneath it.
+## On success, captured stderr still passes through -- warnings aren't ours to eat.
+##
+## - @param $1 message to lead with on failure
+## - @param $@ the command
+## - @stdout the command's stdout, untouched
+try() {
+	local msg=$1
+	shift
+	if ! "$@" 2>"$PTMP/child-err"; then
+		die_detail "$msg" "$PTMP/child-err"
+	fi
+	if [[ -s $PTMP/child-err ]]; then
+		>&2 cat "$PTMP/child-err"
+	fi
+}
+
 ## Format a user-originated string for inclusion in messages.
-## Paths are shown relative to the patches dir when one is in play;
+## Paths are shown relative where that reads better: patch-file paths relative
+## to the patches dir, other paths relative to where the user invoked us
+## (the commands cd around internally, so $PWD is no anchor at die time);
 ## 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
+	if [[ -n ${PATCHES_DIR_ABS:-} && $s == "$PATCHES_DIR_ABS"/* ]]; then
 		s=${s#"$PATCHES_DIR_ABS"/}
+	elif [[ $s == "$START_PWD"/* ]]; then
+		s=${s#"$START_PWD"/}
 	fi
 	printf "'%s'" "$s"
 }
@@ -281,15 +305,16 @@ replay_series() {
 		done
 
 		rm -f "$GIT_INDEX_FILE"
-		git read-tree "${parents[0]}"
+		try "$(q "$f"): could not read the first parent's tree" \
+			git read-tree "${parents[0]}"
 		if [[ -s $diffpart ]]; then # empty diff = empty commit; apply must be skipped, not fed nothing
 			# --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" 2>"$PTMP/apply-err" ||
-				die_detail "$(q "$f"): diff does not apply onto its parent" "$PTMP/apply-err"
+			try "$(q "$f"): diff does not apply onto its parent" \
+				git apply --cached --binary --whitespace=nowarn "$diffpart"
 		fi
-		tree=$(git write-tree)
+		tree=$(try "$(q "$f"): could not write the replayed tree" git write-tree)
 
 		sha=$({
 			printf 'tree %s\n' "$tree"
@@ -297,7 +322,7 @@ replay_series() {
 			printf '%s\n' "${headers[@]}"
 			printf '\n'
 			cat "$msgpart"
-		} | git hash-object -t commit -w --stdin)
+		} | try "$(q "$f"): could not store the replayed commit" git hash-object -t commit -w --stdin)
 
 		sha_of[$slug]=$sha
 		REPLAY_TIP=$sha
@@ -356,7 +381,8 @@ cmd_export() {
 	# of all of them, and its unborn HEAD keeps even committed attributes out.
 	# Only ref resolution touches the subject; from here on it's SHAs.
 	scratch="$PTMP/export.git"
-	git init -q --bare -b scratch --object-format="$(git -C "$repo" rev-parse --show-object-format)" "$scratch"
+	try "could not create the scratch repo for diff generation" \
+		git init -q --bare -b scratch --object-format="$(git -C "$repo" rev-parse --show-object-format)" "$scratch"
 	git -C "$repo" rev-parse --path-format=absolute --git-path objects >"$scratch/objects/info/alternates"
 	cd "$scratch"
 
@@ -387,6 +413,10 @@ cmd_export() {
 		declare -A taken=()   # slug -> 1
 		count=0
 
+		# Walked into a file first: a failure inside a process substitution
+		# would be invisible, and read as an empty branch.
+		try "could not list the commits of $(q "$branch")" \
+			git rev-list --reverse --topo-order "$base..$tip" >"$PTMP/revs"
 		while IFS= read -r sha; do
 			slug=$(slugify "$(git log -1 --format=%s "$sha")")
 			if [[ -n ${taken[$slug]:-} ]]; then
@@ -409,7 +439,8 @@ cmd_export() {
 			done
 			[[ ${#parents[@]} -gt 0 ]] || die "commit $sha has no parents (root commit in range); not supported"
 
-			git cat-file commit "$sha" >"$raw"
+			try "could not read commit $sha from $(q "$repo")" \
+				git cat-file commit "$sha" >"$raw"
 			sed -n '/^$/,$p' "$raw" | tail -n +2 >"$msg"
 
 			{
@@ -427,12 +458,13 @@ cmd_export() {
 					fi
 				done <"$msg"
 				printf -- '----\n\n'
-				git diff-tree -r -p --full-index --binary --no-renames --diff-algorithm=patience "$sha^" "$sha"
+				try "could not generate the diff for commit $sha" \
+					git diff-tree -r -p --full-index --binary --no-renames --diff-algorithm=patience "$sha^" "$sha"
 			} >"$EXPORT_STAGE/$slug.patch"
 
 			printf '%s\n' "$slug" >>"$EXPORT_STAGE/STACK"
 			count=$((count + 1))
-		done < <(git rev-list --reverse --topo-order "$base..$tip")
+		done <"$PTMP/revs"
 
 		rm -rf "$dir"
 		mv "$EXPORT_STAGE" "$dir"
@@ -459,7 +491,8 @@ cmd_import() {
 			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" ""
+		try "could not create $(q "$STACK_REF") (created concurrently?)" \
+			git update-ref "$STACK_REF" "$REPLAY_TIP" ""
 		echo "import: created $STACK_REF at $REPLAY_TIP"
 		if [[ -n $STACK_TIP && $REPLAY_TIP != "$STACK_TIP" ]]; then
 			echo "import: note: replayed tip differs from recorded tip $STACK_TIP (patches edited since the snapshot?)"
@@ -477,7 +510,8 @@ cmd_verify() {
 
 	# Replay into a throwaway shared bare clone: cheap even for huge repos,
 	# and zero pollution of the subject repo's object store.
-	git clone -q --shared --bare "$subject" "$PTMP/verify.git"
+	try "could not clone $(q "$subject") for verification" \
+		git clone -q --shared --bare "$subject" "$PTMP/verify.git"
 	cd "$PTMP/verify.git"
 	export GIT_INDEX_FILE="$PTMP/scratch-index"
 
diff --git a/tests/test-errors.sh b/tests/test-errors.sh
index 1e8933b..2564e32 100755
--- a/tests/test-errors.sh
+++ b/tests/test-errors.sh
@@ -30,4 +30,13 @@ order=disordered
 if [[ -n $ours && -n $detail && ${ours%%:*} -lt ${detail%%:*} ]]; then order=ordered; fi
 EXPECT equal "and in that order" -- "$order" "ordered"
 
+# Repo paths in messages relativize to where the user invoked us, even though
+# the commands cd around internally.
+git clone -q "$work/corpus/upstream" "$work/fresh"
+git -C "$work/fresh" branch topic/fancy main
+rc=0
+(cd "$work" && "$PATCHETTE" import fresh patches) >"$work/out2" 2>&1 || rc=$?
+EXPECT success "repo paths in messages are relative to the invocation dir" -- \
+	grep -q "already exists in 'fresh'" "$work/out2"
+
 exit "$RESULT"
diff --git a/tests/test-failclosed.sh b/tests/test-failclosed.sh
index 8c4b8f8..6972c63 100755
--- a/tests/test-failclosed.sh
+++ b/tests/test-failclosed.sh
@@ -145,6 +145,18 @@ EXPECT refusal "export into a fresh dir dies the same way" "out-of-range" -- \
 EXPECT equal "the failed fresh export left no debris behind" -- \
 	"$(ls -A "$work/freshout")" ""
 
+# A commit walk that fails must fail the export, not read as an empty branch:
+# an empty series swapped in would silently replace a good snapshot.
+# Trigger: a subject missing one mid-branch commit object.
+cp -a "$work/corpus/dev" "$work/holey-dev"
+mid=$(git -C "$work/holey-dev" rev-parse topic/fancy~3)
+rm "$work/holey-dev/.git/objects/${mid:0:2}/${mid:2}"
+clone_layout holey
+EXPECT refusal "export dies when the commit walk fails" "could not list the commits of 'topic/fancy'" -- \
+	"$PATCHETTE" export "$work/holey-dev" "$TAMPER" "$base" topic/fancy
+EXPECT success "the failed walk left the previous snapshot untouched" -- \
+	diff -r "$work/pristine" "$TAMPER"
+
 # Importing where the recorded base is absent tells the user what to do.
 git init -q -b main "$work/empty"
 EXPECT refusal "import without the recorded base points at fetching" "fetch from upstream first" -- \