Re: [PATCH v2] filter-branch: Add more error-handling

Eric Kidd [off-list ref] writes:

In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error
...
Thank you to charon on #git for pointing me in the right direction.

The commit message is not a reception speech at Emmy Awards.  If you want
to do the speech, do so after the three-dash lines.

Please just stick to what problem it tries to solve, how it does so, and
what the outcome is.  

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

That's a very good start for the description of the solution, which would
be for the second paragraph.  The problem description is missing.

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

Giving credits to others like this with a short sentence at the end is
fine.

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

This goes after three-dashes; people who read "git log" output wouldn't
know nor care what was in v1.

    Subject: Fix X under condition Z

    X should do Y if condition Z holds, but it does not.  This can result
    in broken results such as W and V.

    This patch fixes X by changing A, B and C.

    Thanks for M, N and O for reviewing and suggesting improvements.

    Signed-off-by: A U Thor [off-list ref]

Why "exit 1", not "exit"?

Likewise.

Likewise.

Hmm, wouldn't commit-tree have issued its own error message already?  If
redirect failed, then the shell would have.

Why "exit 1", not "exit"?

Likewise

"test_must_fail git ..." would be better here than "! git ...".

On Wed, Feb 11, 2009 at 2:03 PM, Junio C Hamano [off-list ref] wrote:

quoted

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

Hmm, wouldn't commit-tree have issued its own error message already?  If
redirect failed, then the shell would have.

If a custom $filter_commit calls 'exit', there won't be an error
message from git. Of course, there may be an error message from the
custom $filter_commit, but I decided not to rely on that. Would you
prefer me to remove the error message?

I'll have another patch ready shortly, incorporating your suggestions.
My apologies for giving credit in the wrong part of the commit
message, and thank you for your feedback!

Cheers,
Eric

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'.

This patch attemps to add all the missing error checks to
git-filter-branch, and removes an existing $ret variable that did
nothing.  I've tested this patch using t/t7003-filter-branch.sh, and it
passes all the existing tests.

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

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

Feedback on this patch was provided by Johannes Sixt, Johannes Schindelin
and Junio C Hamano.  charon on #git helped with pipeline error handling.

Signed-off-by: Eric Kidd <redacted>

---
 git-filter-branch.sh     |   26 ++++++++++++++------------
 t/t7003-filter-branch.sh |    4 ++++
 2 files changed, 18 insertions(+), 12 deletions(-)

v3:
  Replaced 'exit 1' with 'exit' to use exit status of last command
  Use test_must_fail for unit test expecting failure

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

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86eef56..27b57b8 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
 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
+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
+		git update-index --add --replace --remove --stdin \
+			< "$tempdir"/tree-state || exit
 	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
 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
 fi
 
-exit $ret
+exit 0

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index cb04743..11ed4c4 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_must_fail '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

Hi,

On Wed, 11 Feb 2009, Eric Kidd wrote:

charon on #git helped with pipeline error handling.

JFYI charon is the nick of Thomas Rast.

I haven't checked, but is "$tempdir" not the working directory?  If so, 
this would lead to funny interaction with --tree-filter.  Rather, I'd 
write the file into "$GIT_DIR".  Likewise the other files.

That's not how it is supposed to be used.  Rather,

	test_expect_success $LABEL '
		test_must_fail git filter-branc $OPTIONS
	'

Thanks,
Dscho

On Wed, Feb 11, 2009 at 3:48 PM, Johannes Schindelin
[off-list ref] wrote:

I haven't checked, but is "$tempdir" not the working directory?  If so,
this would lead to funny interaction with --tree-filter.  Rather, I'd
write the file into "$GIT_DIR".  Likewise the other files.

The working directory actually lives one level down:

 tempdir=.git-rewrite
 workdir="$tempdir/t"

At the end of the script, git-filter-branch cleans up all of its
temporary files by deleting $tempdir. There's actually a fair bit of
stuff in there already, and none of it interferes with --tree-filter.

That's not how it is supposed to be used.  Rather,

       test_expect_success $LABEL '
               test_must_fail git filter-branc $OPTIONS
       '

Will fix. Thank you.

I really appreciate all this feedback from the git team. Thank you for
taking the time to help me get this right!

Cheers,
Eric

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'.

This patch attemps to add all the missing error checks to
git-filter-branch, and removes an existing $ret variable that did
nothing.  I've tested this patch using t/t7003-filter-branch.sh, and it
passes all the existing tests.

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

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

Feedback on this patch was provided by Johannes Sixt, Johannes Schindelin
and Junio C Hamano.  Thomas Rast helped with pipeline error handling.

Signed-off-by: Eric Kidd <redacted>

---
 git-filter-branch.sh     |   26 ++++++++++++++------------
 t/t7003-filter-branch.sh |    4 ++++
 2 files changed, 18 insertions(+), 12 deletions(-)

v4:
  Call test_must_fail from inside test_expect_success

v3:
  Replaced 'exit 1' with 'exit' to use exit status of last command
  Use test_must_fail for unit test expecting failure

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

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86eef56..27b57b8 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
 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
+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
+		git update-index --add --replace --remove --stdin \
+			< "$tempdir"/tree-state || exit
 	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
 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
 fi
 
-exit $ret
+exit 0

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index cb04743..56b5ecc 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' '
+	test_must_fail 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

Quoting Junio C Hamano [off-list ref]:

This goes after three-dashes; people who read "git log" output wouldn't
know nor care what was in v1.

    Subject: Fix X under condition Z

    X should do Y if condition Z holds, but it does not.  This can result
    in broken results such as W and V.

    This patch fixes X by changing A, B and C.

    Thanks for M, N and O for reviewing and suggesting improvements.

    Signed-off-by: A U Thor [off-list ref]

I think you meant this as a sample to follow.  Can we add it to Documentation/SubmittingPatches?

-- 
Nanako Shiraishi, an unofficial project secretary
http://ivory.ap.teacup.com/nanako3/