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

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

Eric Kidd schrieb:

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?

I think it's OK to die if the commit filter fails.

But generally, I think it is not necessary to use 'die with error
message', a plain '|| exit' should be enough because an error will have
been reported already by the tool that failed.

Here you shouldn't die. But unlike elsewhere, this case warrants an
explanation for the user:

	git read-tree -u -m HEAD ||
		echo >&2 "WARNING: The working directory is not up-to-date!"

 fi
 
 exit $ret

-- Hannes

Hi,

On Wed, 11 Feb 2009, Eric Kidd wrote:

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?
---

Thanks.  Although it lacks a Sign-off, and part of the commit message is 
actually a personal comment that belongs after the three dashes.

This will catch errors in the sed invocation, but not in rev-parse.

This will catch errors in the update-index call, but neither in diff-index 
nor ls-files.

Otherwise, the patch looks good to me.

Ciao,
Dscho

Johannes Sixt schrieb:

quoted

@@ -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"

Here you shouldn't die.

I take this back. I was distracted by the 'exit $ret' that is visible in
the context:

quoted

 fi
 
 exit $ret

But this exit statement is pointless: ret is initialized to zero and never
changed.

-- Hannes

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.

In two places, I've had to break apart pipelines in order to check the
error code for the first stage of the pipeline, as discussed here:

  http://kerneltrap.org/mailarchive/git/2009/1/28/4835614

Thank you to charon on #git for pointing me in the right direction.

This patch causes 'git filter-branch' to fail if the --commit-filter
argument returns an error.  A test case for this behavior is included.

Feedback on the original version of this patch was provided by Johannes
Sixt and Johannes Schindelin.

v2:
  Remove useless $ret variable
  Correctly check the first command in a pipeline, not the second
  Replace verbose 'die' messages with 'exit 1' in most cases

Signed-off-by: Eric Kidd <redacted>
---
 git-filter-branch.sh     |   26 ++++++++++++++------------
 t/t7003-filter-branch.sh |    4 ++++
 2 files changed, 18 insertions(+), 12 deletions(-)

 Thank you for the feedback!  I've tried to incorporate everybody's
 suggestions.  Please let me know if some of those 'exit 1' statements
 should be changed back to 'die'. -Eric

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86eef56..fff07c8 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 || exit 1
 while read sha1 type name
 do
 	case "$force,$name" in

@@ -241,8 +241,9 @@ GIT_WORK_TREE=.
 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
+git rev-parse --no-flags --revs-only --symbolic-full-name \
+	--default HEAD "$@" > "$tempdir"/raw-heads || exit 1
+sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
 
 test -s "$tempdir"/heads ||
 	die "Which ref do you want to rewrite?"

@@ -251,8 +252,6 @@ GIT_INDEX_FILE="$(pwd)/../index"
 export GIT_INDEX_FILE
 git read-tree || die "Could not seed the index"
 
-ret=0
-
 # map old->new commit ids for rewriting parents
 mkdir ../map || die "Could not create map/ directory"
 

@@ -315,10 +314,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
+		) > "$tempdir"/tree-state || exit 1
+		git update-index --add --replace --remove --stdin \
+			< "$tempdir"/tree-state || exit 1
 	fi
 
 	eval "$filter_index" < /dev/null ||

@@ -339,7 +339,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 +408,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 ||
+		 exit 1
 done < "$tempdir"/heads
 
 # TODO: This should possibly go, with the semantics that all positive given

@@ -483,7 +485,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 || exit 1
 fi
 
-exit $ret
+exit 0

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index cb04743..39affd9 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh

@@ -48,6 +48,10 @@ test_expect_success 'result is really identical' '
 	test $H = $(git rev-parse HEAD)
 '
 
+test_expect_success 'Fail if commit filter fails' '
+	! git filter-branch -f --commit-filter "exit 1" HEAD
+'
+
 test_expect_success 'rewrite, renaming a specific file' '
 	git filter-branch -f --tree-filter "mv d doh || :" HEAD
 '

-- 
1.6.0.4