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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 11, 2009, 19:03 UTC
Message-ID: <7vhc30eqy7.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1234372518-6924-1-git-send-email-git@randomhacks.net>

Eric Kidd <git@randomhacks.net> 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 <au.thor@example.xz>

Show 10 quoted lines

> 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

Why "exit 1", not "exit"?

Show 8 quoted lines

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

Likewise.

Show 12 quoted lines

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

Likewise.

Show 7 quoted lines

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

Show 7 quoted lines

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

Why "exit 1", not "exit"?

Show 6 quoted lines

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

Likewise

Show 12 quoted lines

> 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_must_fail git ..." would be better here than "! git ...".