From: Johannes Sixt <j.sixt@viscovery.net>
Date: Wed, 11 Feb 2009 14:58:37 GMT
Subject: Re: [PATCH] git-filter-branch: Add more error-handling
Message-ID: <4992E79D.10208@viscovery.net>
In-Reply-To: <1234361365-63711-1-git-send-email-git@randomhacks.net>

Eric Kidd schrieb:
> 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?

I think it's OK to die if the commit filter fails.

But generally, I think it is not necessary to use 'die with error
message', a plain '|| exit' should be enough because an error will have
been reported already by the tool that failed.

> @@ -483,7 +486,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 || die "Unable to checkout rewritten tree"

Here you shouldn't die. But unlike elsewhere, this case warrants an
explanation for the user:

	git read-tree -u -m HEAD ||
		echo >&2 "WARNING: The working directory is not up-to-date!"

>  fi
>  
>  exit $ret

-- Hannes