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

Subsystems: the rest

DORMANTno replies

7 messages, 5 authors, 2016-06-15 · open the first message on its own page

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

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:43:19

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

	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

From: Jeff King <hidden>
Date: 2016-06-15 22:43:19

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

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:43:19

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

From: Josh Triplett <hidden>
Date: 2016-06-15 22:43:19

Jeff King wrote:

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

quoted

	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

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

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:43:19

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

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

From: Johannes Sixt <hidden>
Date: 2016-06-15 22:43:19

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

From: David Kastrup <hidden>
Date: 2016-06-15 22:43:19

Johannes Sixt [off-list ref] writes:

Johannes Schindelin wrote:

quoted

-       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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help