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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 11, 2009, 15:24 UTC
Message-ID: <alpine.DEB.1.00.0902111621490.13279@intel-tinevez-2-302>
In-Reply-To: <1234361365-63711-1-git-send-email-git@randomhacks.net>

Hi,

On Wed, 11 Feb 2009, Eric Kidd wrote:

Show 27 quoted lines

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

Show 10 quoted lines

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

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

Show 11 quoted lines

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

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