[PATCH 2/2] filter-branch: fail gracefully when a filter fails

[PATCH 2/2] filter-branch: fail gracefully when a filter fails

[Johannes Schindelin]

A common mistake is to provide a filter which fails unwantedly. For
example, this will stop in the middle:

	git filter-branch --env-filter '
		test $GIT_COMMITTER_EMAIL = xyz &&
		export GIT_COMMITTER_EMAIL = abc' rewritten

When $GIT_COMMITTER_EMAIL is not "xyz", the test fails, and consequently
the whole filter has a non-zero exit status. However, as demonstrated
in this example, filter-branch would just stop, and the user would be
none the wiser.

Also, a failing msg-filter would not have been caught, as was the
case with one of the tests.

This patch fixes both issues, by paying attention to the exit status
of msg-filter, and by saying what failed before exiting.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	It is slightly ugly that the output of msg-filter is written
	to a temporary file. But I do not know a better method to
	catch a failing msg-filter. Help?

 git-filter-branch.sh     |   39 +++++++++++++++++++++++++++++----------
 t/t7003-filter-branch.sh |    8 +++++++-
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 3bf5d88..f0a5070 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -20,6 +20,16 @@ map()
 	cat "$workdir/../map/$1"
 }
 
+# override die(): this version puts in an extra line break, so that
+# the progress is still visible
+
+die()
+{
+	echo >&2
+	echo "$*" >&2
+	exit 1
+}
+
 # When piped a commit, output a script to set the ident of either
 # "author" or "committer
 
@@ -173,23 +183,29 @@ while read commit parents; do
 	export GIT_COMMIT=$commit
 	git cat-file commit "$commit" >../commit
 
-	eval "$(set_ident AUTHOR <../commit)"
-	eval "$(set_ident COMMITTER <../commit)"
-	eval "$filter_env" < /dev/null
+	eval "$(set_ident AUTHOR <../commit)" ||
+		die "setting author failed for commit $commit"
+	eval "$(set_ident COMMITTER <../commit)" ||
+		die "setting committer failed for commit $commit"
+	eval "$filter_env" < /dev/null ||
+		die "env filter failed: $filter_env"
 
 	if [ "$filter_tree" ]; then
 		git checkout-index -f -u -a
 		# files that $commit removed are now still in the working tree;
 		# remove them, else they would be added again
 		git ls-files -z --others | xargs -0 rm -f
-		eval "$filter_tree" < /dev/null
+		eval "$filter_tree" < /dev/null ||
+			die "tree filter failed: $filter_tree"
+
 		git diff-index -r $commit | cut -f 2- | tr '\n' '\0' | \
 			xargs -0 git update-index --add --replace --remove
 		git ls-files -z --others | \
 			xargs -0 git update-index --add --replace --remove
 	fi
 
-	eval "$filter_index" < /dev/null
+	eval "$filter_index" < /dev/null ||
+		die "index filter failed: $filter_index"
 
 	parentstr=
 	for parent in $parents; do
@@ -198,13 +214,15 @@ while read commit parents; do
 		done
 	done
 	if [ "$filter_parent" ]; then
-		parentstr="$(echo "$parentstr" | eval "$filter_parent")"
+		parentstr="$(echo "$parentstr" | eval "$filter_parent")" ||
+				die "parent filter failed: $filter_parent"
 	fi
 
 	sed -e '1,/^$/d' <../commit | \
-		eval "$filter_msg" | \
-		sh -c "$filter_commit" "git commit-tree" $(git write-tree) \
-			$parentstr > ../map/$commit
+		eval "$filter_msg" > ../message ||
+			die "msg filter failed: $filter_msg"
+	sh -c "$filter_commit" "git commit-tree" \
+		$(git write-tree) $parentstr < ../message > ../map/$commit
 done <../revs
 
 src_head=$(tail -n 1 ../revs | sed -e 's/ .*//')
@@ -241,7 +259,8 @@ if [ "$filter_tag_name" ]; then
 		[ -f "../map/$sha1" ] || continue
 		new_sha1="$(cat "../map/$sha1")"
 		export GIT_COMMIT="$sha1"
-		new_ref="$(echo "$ref" | eval "$filter_tag_name")"
+		new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
+			die "tag name filter failed: $filter_tag_name"
 
 		echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
 
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 451ac86..4ddd656 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -107,13 +107,19 @@ test_expect_success 'use index-filter to move into a subdirectory' '
 		  mv \$GIT_INDEX_FILE.new \$GIT_INDEX_FILE" directorymoved &&
 	test -z "$(git diff HEAD directorymoved:newsubdir)"'
 
+test_expect_success 'stops when msg filter fails' '
+	! git-filter-branch --msg-filter false nonono &&
+	rm -rf .git-rewrite &&
+	! git rev-parse nonono
+'
+
 test_expect_success 'author information is preserved' '
 	: > i &&
 	git add i &&
 	test_tick &&
 	GIT_AUTHOR_NAME="B V Uips" git commit -m bvuips &&
 	git-filter-branch --msg-filter "cat; \
-			test \$GIT_COMMIT = $(git rev-parse master) && \
+			test \$GIT_COMMIT != $(git rev-parse master) || \
 			echo Hallo" \
 		preserved-author &&
 	test 1 = $(git rev-list --author="B V Uips" preserved-author | wc -l)
-- 
1.5.3.rc0.2646.g88600-dirty

Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails

[Jeff King]

On Wed, Jul 04, 2007 at 03:36:01PM +0100, Johannes Schindelin wrote:

> 	It is slightly ugly that the output of msg-filter is written
> 	to a temporary file. But I do not know a better method to
> 	catch a failing msg-filter. Help?

If you mean, in general, to catch the exit code of the first part of a
pipe, you have to do something like this:

status=`((cmd1; echo $? >&3) | cmd2) 3>&1`

which is pretty ugly in itself, and if you want the stdout of cmd2, then
you have to add even more redirection. I'm not sure it's worth it.

-Peff

Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails

[Johannes Schindelin]

Hi,

On Thu, 5 Jul 2007, Jeff King wrote:

> status=`((cmd1; echo $? >&3) | cmd2) 3>&1`

Cute.

This is the replacement patch, then (I guess there are still some nits to 
be had, so I did not redo the proper patch yet):

 git-filter-branch.sh |   43 ++++++++++++++++++++++++++++++++-----------
 1 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
old mode 100644
new mode 100755
index 6cf67df..f08288d
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -28,6 +28,16 @@ map()
 	fi
 }
 
+# override die(): this version puts in an extra line break, so that
+# the progress is still visible
+
+die()
+{
+	echo \ >&2
+	echo "$*" >&2
+	exit 1
+}
+
 # When piped a commit, output a script to set the ident of either
 # "author" or "committer
 
@@ -181,23 +191,29 @@ while read commit parents; do
 	export GIT_COMMIT=$commit
 	git cat-file commit "$commit" >../commit
 
-	eval "$(set_ident AUTHOR <../commit)"
-	eval "$(set_ident COMMITTER <../commit)"
-	eval "$filter_env" < /dev/null
+	eval "$(set_ident AUTHOR <../commit)" ||
+		die "setting author failed for commit $commit"
+	eval "$(set_ident COMMITTER <../commit)" ||
+		die "setting committer failed for commit $commit"
+	eval "$filter_env" < /dev/null ||
+		die "env filter failed: $filter_env"
 
 	if [ "$filter_tree" ]; then
 		git checkout-index -f -u -a
 		# files that $commit removed are now still in the working tree;
 		# remove them, else they would be added again
 		git ls-files -z --others | xargs -0 rm -f
-		eval "$filter_tree" < /dev/null
+		eval "$filter_tree" < /dev/null ||
+			die "tree filter failed: $filter_tree"
+
 		git diff-index -r $commit | cut -f 2- | tr '\n' '\0' | \
 			xargs -0 git update-index --add --replace --remove
 		git ls-files -z --others | \
 			xargs -0 git update-index --add --replace --remove
 	fi
 
-	eval "$filter_index" < /dev/null
+	eval "$filter_index" < /dev/null ||
+		die "index filter failed: $filter_index"
 
 	parentstr=
 	for parent in $parents; do
@@ -206,13 +222,17 @@ while read commit parents; do
 		done
 	done
 	if [ "$filter_parent" ]; then
-		parentstr="$(echo "$parentstr" | eval "$filter_parent")"
+		parentstr="$(echo "$parentstr" | eval "$filter_parent")" ||
+				die "parent filter failed: $filter_parent"
 	fi
 
-	sed -e '1,/^$/d' <../commit | \
-		eval "$filter_msg" | \
-		sh -c "$filter_commit" "git commit-tree" $(git write-tree) \
-			$parentstr > ../map/$commit
+	(sed -e '1,/^$/d' <../commit |
+		(eval "$filter_msg" ||
+		 die "msg filter failed: $filter_msg" 2>&3) |
+		(sh -c "$filter_commit" "git commit-tree" $(git write-tree) \
+			$parentstr > ../map/$commit ||
+		 die "commit filter failed: $filter_commit" 2>&3)) 3>&1 |
+	 grep . && die
 done <../revs
 
 src_head=$(tail -n 1 ../revs | sed -e 's/ .*//')
@@ -249,7 +269,8 @@ if [ "$filter_tag_name" ]; then
 		[ -f "../map/$sha1" ] || continue
 		new_sha1="$(cat "../map/$sha1")"
 		export GIT_COMMIT="$sha1"
-		new_ref="$(echo "$ref" | eval "$filter_tag_name")"
+		new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
+			die "tag name filter failed: $filter_tag_name"
 
 		echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
 

Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 5 Jul 2007, Jeff King wrote:
>
>> status=`((cmd1; echo $? >&3) | cmd2) 3>&1`
>
> Cute.
>
> This is the replacement patch, then (I guess there are still some nits to 
> be had, so I did not redo the proper patch yet):
> ...
> +	(sed -e '1,/^$/d' <../commit |
> +		(eval "$filter_msg" ||
> +		 die "msg filter failed: $filter_msg" 2>&3) |
> +		(sh -c "$filter_commit" "git commit-tree" $(git write-tree) \
> +			$parentstr > ../map/$commit ||
> +		 die "commit filter failed: $filter_commit" 2>&3)) 3>&1 |
> +	 grep . && die

You certainly meant "grep >&2 ." at the end of that pipeline;
while I welcome people to be aware that it is possible with
esoteric shell redirection games, I personally feel this is not
worth it.

Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails

[Johannes Sixt]

Johannes Schindelin wrote:
> -       sed -e '1,/^$/d' <../commit | \
> -               eval "$filter_msg" | \
> -               sh -c "$filter_commit" "git commit-tree" $(git write-tree) \
> -                       $parentstr > ../map/$commit
> +       (sed -e '1,/^$/d' <../commit |
> +               (eval "$filter_msg" ||
> +                die "msg filter failed: $filter_msg" 2>&3) |
> +               (sh -c "$filter_commit" "git commit-tree" $(git write-tree) \
> +                       $parentstr > ../map/$commit ||
> +                die "commit filter failed: $filter_commit" 2>&3)) 3>&1 |
> +        grep . && die

You introduce a handful of new forks and an exec. Isn't an intermediate
file much cheaper?

-- Hannes

Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails

[David Kastrup]

Johannes Sixt <J.Sixt@eudaptics.com> writes:

> Johannes Schindelin wrote:
>> -       sed -e '1,/^$/d' <../commit | \
>> -               eval "$filter_msg" | \
>> -               sh -c "$filter_commit" "git commit-tree" $(git write-tree) \
>> -                       $parentstr > ../map/$commit
>> +       (sed -e '1,/^$/d' <../commit |
>> +               (eval "$filter_msg" ||
>> +                die "msg filter failed: $filter_msg" 2>&3) |
>> +               (sh -c "$filter_commit" "git commit-tree" $(git write-tree) \
>> +                       $parentstr > ../map/$commit ||
>> +                die "commit filter failed: $filter_commit" 2>&3)) 3>&1 |
>> +        grep . && die
>
> You introduce a handful of new forks and an exec. Isn't an intermediate
> file much cheaper?

The number of forks can be reduced by using { ...; } instead of (
... ) here (though it is possible the shell optimizes them away).
grep . should likely redirect its output with >&2 so that it ends up
on stderr.  I'd probably prefer grep ^ or grep '' since that matches
empty lines as well.  When done that way, I don't see a "handful of
new forks".

Instead of "grep ." one could also do something like

if read line then
  while echo "$line" && read line; do :; done
  die
fi

which is fork-less.

-- 
David Kastrup

Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Instead of "grep ." one could also do something like
>
> if read line then
>   while echo "$line" && read line; do :; done
>   die
> fi
>
> which is fork-less.

I'd agree with you that "grep ^" would be preferable _if_ we
were to do this.  But in your fork-less example, you are
assuming that (1) "read" does not molest what is read, (2)
"echo" is built-in, and (3) "echo" does not munge the
parameter.

Since I am one of old fashioned shell people (I readily admit
that I used to have _fun_ with autoconf generated shell
scripts), I'd love to continue shell skill show-offs, but for
the purpose of updating this script, I would say a temporary
file is much better than any of the alternatives around 1>&3.
For one thing, it would make debugging the script while
developing and tweaking it, _and_ while using it, much more
pleasant.

Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails

[Jeff King]

On Fri, Jul 06, 2007 at 01:33:02AM -0700, Junio C Hamano wrote:

> the purpose of updating this script, I would say a temporary
> file is much better than any of the alternatives around 1>&3.

As the one who originally mentioned the redirection trick, let me say
that I agree. I brought it up more as "yuck, this is the way you have
to do it" than a real suggestion. I think the tempfile, while ugly, is
less ugly.

-Peff

Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

Jeff King wrote:
> On Wed, Jul 04, 2007 at 03:36:01PM +0100, Johannes Schindelin wrote:
> 
>> 	It is slightly ugly that the output of msg-filter is written
>> 	to a temporary file. But I do not know a better method to
>> 	catch a failing msg-filter. Help?
> 
> If you mean, in general, to catch the exit code of the first part of a
> pipe, you have to do something like this:
> 
> status=`((cmd1; echo $? >&3) | cmd2) 3>&1`
> 
> which is pretty ugly in itself, and if you want the stdout of cmd2, then
> you have to add even more redirection. I'm not sure it's worth it.

bash has "set -o pipefail", but that would require bash.  However, you could
try setting pipefail, and ignoring any failure to set it; that would give the
more friendly behavior with bash, while still allowing any /bin/sh in general.

- Josh Triplett



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH 2/2] filter-branch: fail gracefully when a filter fails

[Johannes Schindelin]

Hi,

On Thu, 5 Jul 2007, Josh Triplett wrote:

> bash has "set -o pipefail", but that would require bash.  However, you 
> could try setting pipefail, and ignoring any failure to set it; that 
> would give the more friendly behavior with bash, while still allowing 
> any /bin/sh in general.

I was aware of pipefail when I wrote that patch.  However, I have zero 
interest in a "solution" which works on bash, but fails on other shells.  

That is like allowing a precious few to overstep some serious line (and 
commuting them), but severely punish all others.  And that's wrong.  And 
to allow it to happen is wrong, too.

Ciao,
Dscho