git-filter-branch exits early

git-filter-branch exits early

[Alex Riesen]

I have a Debian system where git-filter-branch exits immediately after
"unset CDPATH" in git-sh-setup (the command exits with 1, as CDPATH is
not defined). The system still has bash-2.05a.

git-filter-branch has "set -e", which is why the script finishes
prematurely. If this is not really needed, maybe it can be removed?

I'll see if the system can be upgraded, but I suspect someone can get
a similar problem.

Re: git-filter-branch exits early

[Johannes Schindelin]

Hi,

On Tue, 10 Jul 2007, Alex Riesen wrote:

> I have a Debian system where git-filter-branch exits immediately after
> "unset CDPATH" in git-sh-setup (the command exits with 1, as CDPATH is
> not defined). The system still has bash-2.05a.
> 
> git-filter-branch has "set -e", which is why the script finishes
> prematurely. If this is not really needed, maybe it can be removed?
> 
> I'll see if the system can be upgraded, but I suspect someone can get
> a similar problem.

I do not really understand why "unset CDPATH" should trigger an error.  I 
guess that this is one of the nice braindamages in dash, right?

Anyway, "set -e" was one thing I wanted to fix.  But I'm not sure I want 
to work on filter-branch now, what with skimo (possibly? hopefully?) 
working on my wishlist for rewrite-commits.  If rewrite-commits gets the 
features I wished for, IMHO filter-branch is obsolete.

Using rewrite-commits would have a couple of advantages:

- the name is much better,

- since we have reflogs enabled by default now, there is really no good 
  reason why you should have to copy the rewritten branches back to their 
  original name (we would need a way to redirect that, though, for 
  example for subdirectory filters),

- it is faster,

- since it is a C program, it should be more stable, eventually, than a 
  shell script, where you have to work around limitations all the time,

- with the trick I described in the mail to skimo, you can have 
  convenience functions (think "map") in the commit filter, too,

- and it would be less work for me.

Your case is a really good illustration for why C is a better language 
than shell in the long run.

Oh, BTW, if the index filter gets the current (original) commit SHA1 as an 
environment variable, like it does in filter-branch, it can act as a 
the first half of a subdirectory filter:

	--index-filter "git read-tree $COMMIT_SHA1:sub/directory/"

The second half of it is to add "-- sub/directory/" to the command line.

Ciao,
Dscho

Re: git-filter-branch exits early

[Alex Riesen]

On 7/10/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Tue, 10 Jul 2007, Alex Riesen wrote:
>
> > I have a Debian system where git-filter-branch exits immediately after
> > "unset CDPATH" in git-sh-setup (the command exits with 1, as CDPATH is
> > not defined). The system still has bash-2.05a.
> >
> > git-filter-branch has "set -e", which is why the script finishes
> > prematurely. If this is not really needed, maybe it can be removed?
> >
> > I'll see if the system can be upgraded, but I suspect someone can get
> > a similar problem.
>
> I do not really understand why "unset CDPATH" should trigger an error.  I
> guess that this is one of the nice braindamages in dash, right?

in bash. Bash-2.05.a

> Anyway, "set -e" was one thing I wanted to fix. ...

BTW, what can break if I just go on and remove it?

> - since it is a C program, it should be more stable, eventually, than a
>   shell script, where you have to work around limitations all the time,

right :)

Re: git-filter-branch exits early

[Sven Verdoolaege]

On Tue, Jul 10, 2007 at 10:57:44PM +0100, Johannes Schindelin wrote:
> Anyway, "set -e" was one thing I wanted to fix.  But I'm not sure I want 
> to work on filter-branch now, what with skimo (possibly? hopefully?) 
> working on my wishlist for rewrite-commits.

I'm working on it, but only evenings (and weekends).

skimo

[PATCH] filter-branch: get rid of "set -e"

[Johannes Schindelin]

It was reported by Alex Riesen that "set -e" can break something as 
trivial as "unset CDPATH" in bash.

So get rid of "set -e".

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

	On Tue, 10 Jul 2007, Alex Riesen wrote:

	> I have a Debian system where git-filter-branch exits immediately 
	> after "unset CDPATH" in git-sh-setup (the command exits with 1, 
	> as CDPATH is not defined). The system still has bash-2.05a.
	> 
	> git-filter-branch has "set -e", which is why the script finishes
	> prematurely. If this is not really needed, maybe it can be 
	> removed?

	I hope I got all of the interesting places equipped by the 
	appropriate conditional die() calls.

 git-filter-branch.sh |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5401970..0d000ed 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -8,8 +8,6 @@
 # a new branch. You can specify a number of filters to modify the commits,
 # files and trees.
 
-set -e
-
 USAGE="git-filter-branch [-d TEMPDIR] [FILTERS] DESTBRANCH [REV-RANGE]"
 . git-sh-setup
 
@@ -141,9 +139,10 @@ git show-ref "refs/heads/$dstbranch" 2> /dev/null &&
 	die "branch $dstbranch already exists"
 
 test ! -e "$tempdir" || die "$tempdir already exists, please remove it"
-mkdir -p "$tempdir/t"
-cd "$tempdir/t"
-workdir="$(pwd)"
+mkdir -p "$tempdir/t" &&
+cd "$tempdir/t" &&
+workdir="$(pwd)" ||
+die ""
 
 case "$GIT_DIR" in
 /*)
@@ -155,12 +154,12 @@ esac
 export GIT_DIR GIT_WORK_TREE=.
 
 export GIT_INDEX_FILE="$(pwd)/../index"
-git read-tree # seed the index file
+git read-tree || die "Could not seed the index"
 
 ret=0
 
-
-mkdir ../map # map old->new commit ids for rewriting parents
+# map old->new commit ids for rewriting parents
+mkdir ../map || die "Could not create map/ directory"
 
 case "$filter_subdir" in
 "")
@@ -170,7 +169,7 @@ case "$filter_subdir" in
 *)
 	git rev-list --reverse --topo-order --default HEAD \
 		--parents --full-history "$@" -- "$filter_subdir"
-esac > ../revs
+esac > ../revs || die "Could not get the commits"
 commits=$(wc -l <../revs | tr -d " ")
 
 test $commits -eq 0 && die "Found nothing to rewrite"
@@ -186,10 +185,11 @@ while read commit parents; do
 		;;
 	*)
 		git read-tree -i -m $commit:"$filter_subdir"
-	esac
+	esac || die "Could not initialize the index"
 
 	export GIT_COMMIT=$commit
-	git cat-file commit "$commit" >../commit
+	git cat-file commit "$commit" >../commit ||
+		die "Cannot read commit $commit"
 
 	eval "$(set_ident AUTHOR <../commit)" ||
 		die "setting author failed for commit $commit"
@@ -199,7 +199,8 @@ while read commit parents; do
 		die "env filter failed: $filter_env"
 
 	if [ "$filter_tree" ]; then
-		git checkout-index -f -u -a
+		git checkout-index -f -u -a ||
+			die "Could not checkout the index"
 		# 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
@@ -240,7 +241,8 @@ case "$target_head" in
 	echo Nothing rewritten
 	;;
 *)
-	git update-ref refs/heads/"$dstbranch" $target_head
+	git update-ref refs/heads/"$dstbranch" $target_head ||
+		die "Could not update $dstbranch with $target_head"
 	if [ $(wc -l <../map/$src_head) -gt 1 ]; then
 		echo "WARNING: Your commit filter caused the head commit to expand to several rewritten commits. Only the first such commit was recorded as the current $dstbranch head but you will need to resolve the situation now (probably by manually merging the other commits). These are all the commits:" >&2
 		sed 's/^/	/' ../map/$src_head >&2
@@ -277,7 +279,8 @@ if [ "$filter_tag_name" ]; then
 			warn "unreferencing tag object $sha1t"
 		fi
 
-		git update-ref "refs/tags/$new_ref" "$new_sha1"
+		git update-ref "refs/tags/$new_ref" "$new_sha1" ||
+			die "Could not write tag $new_ref"
 	done
 fi
 
-- 
1.5.3.rc1.16.g9d6f-dirty

Re: [PATCH] filter-branch: get rid of "set -e"

[David Kastrup]

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

> It was reported by Alex Riesen that "set -e" can break something as 
> trivial as "unset CDPATH" in bash.

Not in bash.  Just tried it.  But there are Bourne shells that do get
annoyed.

> So get rid of "set -e".

I'd rather write

[ "X$CDPATH" = "X" ] || unset CDPATH


With regard to shells, there is an interesting document
(info "(autoconf) Portable Shell")
which is rather disillusioning.

A real horror show is
(info "(autoconf) Limitations of Builtins")
where one learns that using if without else can lead to bad exit
status, too.

-- 
David Kastrup

Re: [PATCH] filter-branch: get rid of "set -e"

[Johannes Schindelin]

Hi,

[please Cc me]

On Wed, 18 Jul 2007, David Kastrup wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > It was reported by Alex Riesen that "set -e" can break something as 
> > trivial as "unset CDPATH" in bash.
> 
> Not in bash.  Just tried it.  But there are Bourne shells that do get
> annoyed.

The mail I was replying to _explicitely_ stated that it _was_ with bash.  
Not your version of it, but some version.

> > So get rid of "set -e".
> 
> I'd rather write
> 
> [ "X$CDPATH" = "X" ] || unset CDPATH

We would write that as "test", not "[".

But it is much cleaner to get rid of "set -e" anyway.

Ciao,
Dscho