[PATCH] git-filter-branch: Add more error-handling

[Eric Kidd <git@randomhacks.net>]

In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error
that had slipped by the test suites because of a missing check on 'git
read-tree -u -m HEAD'.

I mentioned to Johannes Schindelin that there were several bugs of this
type in git-filter-branch, and he suggested that I send a patch.

I've tested this patch using t/t7003-filter-branch.sh, and it passes all
the existing tests.  But it's entirely possible that this patch contains
errors, and I would love input from people who have more experience with
sh and who know more about git-filter-branch.

In particular, the following hunk may change the public UI to
git-filter-branch, although I'm not sure whether the change is for
better or for worse.  As I understand it, this hunk would allow
$filter_commit to abort the rewriting process by returning a non-0 exit
status:

 	@SHELL_PATH@ -c "$filter_commit" "git commit-tree" \
-		$(git write-tree) $parentstr < ../message > ../map/$commit
+		$(git write-tree) $parentstr < ../message > ../map/$commit ||
+			die "could not write rewritten commit"
 done <../revs

I'd be happy to add a test case for what happens when $filter_commit
returns a non-0 exit status.  Is the old behavior preferable?
---
 git-filter-branch.sh |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

 I'm trying to do the constructive thing, and send patches instead of bug
 reports. :-) -Eric

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86eef56..9d50978 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -221,7 +221,7 @@ die ""
 trap 'cd ../..; rm -rf "$tempdir"' 0
 
 # Make sure refs/original is empty
-git for-each-ref > "$tempdir"/backup-refs
+git for-each-ref > "$tempdir"/backup-refs || die "Can't back up refs"
 while read sha1 type name
 do
 	case "$force,$name" in
@@ -242,7 +242,7 @@ export GIT_DIR GIT_WORK_TREE
 
 # The refs should be updated if their heads were rewritten
 git rev-parse --no-flags --revs-only --symbolic-full-name --default HEAD "$@" |
-sed -e '/^^/d' >"$tempdir"/heads
+sed -e '/^^/d' >"$tempdir"/heads || die "Can't make list of original refs"
 
 test -s "$tempdir"/heads ||
 	die "Which ref do you want to rewrite?"
@@ -315,10 +315,11 @@ while read commit parents; do
 			die "tree filter failed: $filter_tree"
 
 		(
-			git diff-index -r --name-only $commit
+			git diff-index -r --name-only $commit &&
 			git ls-files --others
 		) |
-		git update-index --add --replace --remove --stdin
+		git update-index --add --replace --remove --stdin ||
+			die "unable to update index with results of tree filter"
 	fi
 
 	eval "$filter_index" < /dev/null ||
@@ -339,7 +340,8 @@ while read commit parents; do
 		eval "$filter_msg" > ../message ||
 			die "msg filter failed: $filter_msg"
 	@SHELL_PATH@ -c "$filter_commit" "git commit-tree" \
-		$(git write-tree) $parentstr < ../message > ../map/$commit
+		$(git write-tree) $parentstr < ../message > ../map/$commit ||
+			die "could not write rewritten commit"
 done <../revs
 
 # In case of a subdirectory filter, it is possible that a specified head
@@ -407,7 +409,8 @@ do
 			die "Could not rewrite $ref"
 	;;
 	esac
-	git update-ref -m "filter-branch: backup" "$orig_namespace$ref" $sha1
+	git update-ref -m "filter-branch: backup" "$orig_namespace$ref" $sha1 ||
+		 die "Could not back up branch ref"
 done < "$tempdir"/heads
 
 # TODO: This should possibly go, with the semantics that all positive given
@@ -483,7 +486,7 @@ test -z "$ORIG_GIT_INDEX_FILE" || {
 }
 
 if [ "$(is_bare_repository)" = false ]; then
-	git read-tree -u -m HEAD
+	git read-tree -u -m HEAD || die "Unable to checkout rewritten tree"
 fi
 
 exit $ret
-- 
1.6.0.4