[PATCH v2] filter-branch -d: Export GIT_DIR earlier

[PATCH v2] filter-branch -d: Export GIT_DIR earlier

[Lars Noschinski]

The improved error handling catches a bug in filter-branch when using
-d pointing to a path outside any git repository:

$ mkdir foo
$ cd foo
$ git init
$ touch bar
$ git add bar
$ git commit -m bar
$ cd ..
$ git clone --bare foo
$ cd foo.git
$ git filter-branch -d /tmp/filter master
fatal: Not a git repository (or any of the parent directories): .git

This error message comes from git for-each-ref in line 224. GIT_DIR is
set correctly by git-sh-setup (to the foo.git repository), but not
exported (yet).
---

The tests copies backup-ref into another directory and checks
that it contains a branch from the rewritten repository.

  git-filter-branch.sh     |   12 ++++++------
  t/t7003-filter-branch.sh |    9 +++++++++
  2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 27b57b8..9a09ba1 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -220,6 +220,12 @@ die ""
  # Remove tempdir on exit
  trap 'cd ../..; rm -rf "$tempdir"' 0
  
+ORIG_GIT_DIR="$GIT_DIR"
+ORIG_GIT_WORK_TREE="$GIT_WORK_TREE"
+ORIG_GIT_INDEX_FILE="$GIT_INDEX_FILE"
+GIT_WORK_TREE=.
+export GIT_DIR GIT_WORK_TREE
+
  # Make sure refs/original is empty
  git for-each-ref > "$tempdir"/backup-refs || exit
  while read sha1 type name
@@ -234,12 +240,6 @@ do
  	esac
  done < "$tempdir"/backup-refs
  
-ORIG_GIT_DIR="$GIT_DIR"
-ORIG_GIT_WORK_TREE="$GIT_WORK_TREE"
-ORIG_GIT_INDEX_FILE="$GIT_INDEX_FILE"
-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 "$@" > "$tempdir"/raw-heads || exit
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 56b5ecc..446700b 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -48,6 +48,15 @@ test_expect_success 'result is really identical' '
  	test $H = $(git rev-parse HEAD)
  '
  
+TRASHDIR=$(pwd)
+test_expect_success 'correct GIT_DIR while using -d' '
+        mkdir drepo && cd drepo && git init && make_commit drepo &&
+        git filter-branch -d "$TRASHDIR/dfoo" \
+            --index-filter "cp \"$TRASHDIR\"/dfoo/backup-refs \"$TRASHDIR\"" &&
+        cd .. &&
+        grep drepo "$TRASHDIR/backup-refs"
+'
+
  test_expect_success 'Fail if commit filter fails' '
  	test_must_fail git filter-branch -f --commit-filter "exit 1" HEAD
  '
-- 
1.6.1.3

Re: [PATCH v2] filter-branch -d: Export GIT_DIR earlier

[Lars Noschinski]

* Lars Noschinski <lars@public.noschinski.de> [09-02-17 09:31]:
>The improved error handling catches a bug in filter-branch when using
>-d pointing to a path outside any git repository:
>
>$ mkdir foo
>$ cd foo
>$ git init
>$ touch bar
>$ git add bar
>$ git commit -m bar
>$ cd ..
>$ git clone --bare foo
>$ cd foo.git
>$ git filter-branch -d /tmp/filter master
>fatal: Not a git repository (or any of the parent directories): .git
>
>This error message comes from git for-each-ref in line 224. GIT_DIR is
>set correctly by git-sh-setup (to the foo.git repository), but not
>exported (yet).

Ops, forgot the

Signed-off-by: Lars Noschinski <lars@public.noschinski.de>

Feel free to add it. I can also resend the patch.

   - Lars

Re: [PATCH v2] filter-branch -d: Export GIT_DIR earlier

[Johannes Schindelin]

Hi,

On Tue, 17 Feb 2009, Lars Noschinski wrote:

> The improved error handling catches a bug in filter-branch when using
> -d pointing to a path outside any git repository:
> 
> $ mkdir foo
> $ cd foo
> $ git init
> $ touch bar
> $ git add bar
> $ git commit -m bar
> $ cd ..
> $ git clone --bare foo
> $ cd foo.git
> $ git filter-branch -d /tmp/filter master
> fatal: Not a git repository (or any of the parent directories): .git

This could be written as

	$ cd .git
	$ git filter-branch -d /tmp/bla master

Right?

	
>  git-filter-branch.sh     |   12 ++++++------
>  t/t7003-filter-branch.sh |    9 +++++++++
>  2 files changed, 15 insertions(+), 6 deletions(-)

Funny, git am -3 reports:

	Did you hand edit your patch?
	It does not apply to blobs recorded in its index.
	Cannot fall back to three-way merge.

After realizing that the common lines were prefixed with a double space, 
and applying my l33t patch m0nkey ski77z, I could verify that it works as 
expected (in addition to looking at the patch and deeming it correct).

> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index 56b5ecc..446700b 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -48,6 +48,15 @@ test_expect_success 'result is really identical' '
>  	test $H = $(git rev-parse HEAD)
>  '
>  
> +TRASHDIR=$(pwd)
> +test_expect_success 'correct GIT_DIR while using -d' '
> +        mkdir drepo && cd drepo && git init && make_commit drepo &&

I usually prefer those to be on one line, each.

> +        git filter-branch -d "$TRASHDIR/dfoo" \
> +            --index-filter "cp \"$TRASHDIR\"/dfoo/backup-refs \"$TRASHDIR\""
> &&
> +        cd .. &&

We try to avoid cd'ing back, by using constructs like this:

	(cd drepo &&
	 ...
	) &&

After those two (maybe three) changes and your SOB: ACK.

BTW the reason I wanted to test this thing is that I suspected that you 
meant test_commit instead of make_commit.  But then, I realized that there 
exists a make_commit in t7003... which shares the shortcoming of our 
previous implementation of test_commit in that it adds ambiguities on 
case-insensitive filesystems.

So I _had_ to look who introduced make_commit:

	$ git blame -L '/make_commit/,/}/' t/t7003*

Making a fool out of yourself -- priceless.

Ciao,
Dscho

Re: [PATCH v2] filter-branch -d: Export GIT_DIR earlier

[Lars Noschinski]

* Johannes Schindelin <Johannes.Schindelin@gmx.de> [09-02-17 16:08]:
>On Tue, 17 Feb 2009, Lars Noschinski wrote:
>> The improved error handling catches a bug in filter-branch when using
>> -d pointing to a path outside any git repository:
>> 
>> $ mkdir foo
>> $ cd foo
>> $ git init
>> $ touch bar
>> $ git add bar
>> $ git commit -m bar
>> $ cd ..
>> $ git clone --bare foo
>> $ cd foo.git
>> $ git filter-branch -d /tmp/filter master
>> fatal: Not a git repository (or any of the parent directories): .git
>
>This could be written as
>
>	$ cd .git
>	$ git filter-branch -d /tmp/bla master

Does not work, as we get another (slightly misleading) error message:

/tmp/foo/.git$ git filter-branch -d /tmp/bar master
fatal: This operation must be run in a work tree
Cannot rewrite branch(es) with a dirty working directory.

But we do not need a bare repository at all to demonstrate this bug, so
we can skip even the 'cd .git'.

>Funny, git am -3 reports:
>
>	Did you hand edit your patch?
>	It does not apply to blobs recorded in its index.
>	Cannot fall back to three-way merge.

Hm, for some reason, format=flowed was enabled. I wonder, that it has
not bitten me earlier.

>We try to avoid cd'ing back, by using constructs like this:
>
>	(cd drepo &&
>	 ...
>	) &&

Ok, can do.

>After those two (maybe three) changes and your SOB: ACK.
>
>BTW the reason I wanted to test this thing is that I suspected that you 
>meant test_commit instead of make_commit.  But then, I realized that there 
>exists a make_commit in t7003... which shares the shortcoming of our 
>previous implementation of test_commit in that it adds ambiguities on 
>case-insensitive filesystems.

Yeah, I used make_commit to stay consistent with the rest of the file.
I'll change it to test_commit. I think as it does not bite us, it would
be unnecessary code churn to remove the remaining usage of make_commit?

>So I _had_ to look who introduced make_commit:
>
>	$ git blame -L '/make_commit/,/}/' t/t7003*
>
>Making a fool out of yourself -- priceless.

:)

  - Lars.

Re: [PATCH v3] filter-branch -d: Export GIT_DIR earlier

[Lars Noschinski]

The improved error handling catches a bug in filter-branch when using
-d pointing to a path outside any git repository:

$ mkdir foo
$ cd foo
$ git init
$ touch bar
$ git add bar
$ git commit -m bar
$ git filter-branch -d /tmp/filter master
fatal: Not a git repository (or any of the parent directories): .git

This error message comes from git for-each-ref in line 224. GIT_DIR is
set correctly by git-sh-setup (to the foo.git repository), but not
exported (yet).

Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
---
  git-filter-branch.sh     |   12 ++++++------
  t/t7003-filter-branch.sh |   12 ++++++++++++
  2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 27b57b8..9a09ba1 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -220,6 +220,12 @@ die ""
  # Remove tempdir on exit
  trap 'cd ../..; rm -rf "$tempdir"' 0
  
+ORIG_GIT_DIR="$GIT_DIR"
+ORIG_GIT_WORK_TREE="$GIT_WORK_TREE"
+ORIG_GIT_INDEX_FILE="$GIT_INDEX_FILE"
+GIT_WORK_TREE=.
+export GIT_DIR GIT_WORK_TREE
+
  # Make sure refs/original is empty
  git for-each-ref > "$tempdir"/backup-refs || exit
  while read sha1 type name
@@ -234,12 +240,6 @@ do
  	esac
  done < "$tempdir"/backup-refs
  
-ORIG_GIT_DIR="$GIT_DIR"
-ORIG_GIT_WORK_TREE="$GIT_WORK_TREE"
-ORIG_GIT_INDEX_FILE="$GIT_INDEX_FILE"
-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 "$@" > "$tempdir"/raw-heads || exit
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 56b5ecc..329c851 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -48,6 +48,18 @@ test_expect_success 'result is really identical' '
  	test $H = $(git rev-parse HEAD)
  '
  
+TRASHDIR=$(pwd)
+test_expect_success 'correct GIT_DIR while using -d' '
+	mkdir drepo &&
+	( cd drepo &&
+	git init &&
+	test_commit drepo &&
+	git filter-branch -d "$TRASHDIR/dfoo" \
+		--index-filter "cp \"$TRASHDIR\"/dfoo/backup-refs \"$TRASHDIR\"" \
+	) &&
+	grep drepo "$TRASHDIR/backup-refs"
+'
+
  test_expect_success 'Fail if commit filter fails' '
  	test_must_fail git filter-branch -f --commit-filter "exit 1" HEAD
  '
-- 
1.6.2.rc0.91.g0bb0.dirty

Re: [PATCH v3] filter-branch -d: Export GIT_DIR earlier

[Johannes Schindelin]

Hi,

On Tue, 17 Feb 2009, Lars Noschinski wrote:

> The improved error handling catches a bug in filter-branch when using
> -d pointing to a path outside any git repository:
> 
> $ mkdir foo
> $ cd foo
> $ git init
> $ touch bar
> $ git add bar
> $ git commit -m bar
> $ git filter-branch -d /tmp/filter master
> fatal: Not a git repository (or any of the parent directories): .git
> 
> This error message comes from git for-each-ref in line 224. GIT_DIR is
> set correctly by git-sh-setup (to the foo.git repository), but not
> exported (yet).
> 
> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
> ---

Let me quickly add my ACK before Junio can apply the patch without it.  
(Even I had the impression that you wanted to trim down the commit 
message, but it really does not matter to me all that much.)

Ciao,
Dscho

Re: [PATCH v4] filter-branch -d: Export GIT_DIR earlier

[Lars Noschinski]

The improved error handling catches a bug in filter-branch when using
-d pointing to a path outside any git repository:

$ git filter-branch -d /tmp/foo master
fatal: Not a git repository (or any of the parent directories): .git

This error message comes from git for-each-ref in line 224. GIT_DIR is
set correctly by git-sh-setup (to the foo.git repository), but not
exported (yet).

Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---

Last iteration :)

 git-filter-branch.sh     |   12 ++++++------
 t/t7003-filter-branch.sh |   12 ++++++++++++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 27b57b8..9a09ba1 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -220,6 +220,12 @@ die ""
 # Remove tempdir on exit
 trap 'cd ../..; rm -rf "$tempdir"' 0
 
+ORIG_GIT_DIR="$GIT_DIR"
+ORIG_GIT_WORK_TREE="$GIT_WORK_TREE"
+ORIG_GIT_INDEX_FILE="$GIT_INDEX_FILE"
+GIT_WORK_TREE=.
+export GIT_DIR GIT_WORK_TREE
+
 # Make sure refs/original is empty
 git for-each-ref > "$tempdir"/backup-refs || exit
 while read sha1 type name
@@ -234,12 +240,6 @@ do
 	esac
 done < "$tempdir"/backup-refs
 
-ORIG_GIT_DIR="$GIT_DIR"
-ORIG_GIT_WORK_TREE="$GIT_WORK_TREE"
-ORIG_GIT_INDEX_FILE="$GIT_INDEX_FILE"
-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 "$@" > "$tempdir"/raw-heads || exit
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 56b5ecc..329c851 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -48,6 +48,18 @@ test_expect_success 'result is really identical' '
 	test $H = $(git rev-parse HEAD)
 '
 
+TRASHDIR=$(pwd)
+test_expect_success 'correct GIT_DIR while using -d' '
+	mkdir drepo &&
+	( cd drepo &&
+	git init &&
+	test_commit drepo &&
+	git filter-branch -d "$TRASHDIR/dfoo" \
+		--index-filter "cp \"$TRASHDIR\"/dfoo/backup-refs \"$TRASHDIR\"" \
+	) &&
+	grep drepo "$TRASHDIR/backup-refs"
+'
+
 test_expect_success 'Fail if commit filter fails' '
 	test_must_fail git filter-branch -f --commit-filter "exit 1" HEAD
 '
-- 
1.6.1.3

Re: [PATCH v4] filter-branch -d: Export GIT_DIR earlier

[Johannes Schindelin]

Hi,

On Wed, 18 Feb 2009, Lars Noschinski wrote:

> The improved error handling catches a bug in filter-branch when using
> -d pointing to a path outside any git repository:
> 
> $ git filter-branch -d /tmp/foo master
> fatal: Not a git repository (or any of the parent directories): .git
> 
> This error message comes from git for-each-ref in line 224. GIT_DIR is
> set correctly by git-sh-setup (to the foo.git repository), but not
> exported (yet).
> 
> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
> Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---
> 
> Last iteration :)

Yep, I like it.

Thanks,
Dscho

Re: [PATCH v4] filter-branch -d: Export GIT_DIR earlier

[Junio C Hamano]

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

>> Last iteration :)
>
> Yep, I like it.

Thanks.

Re: [PATCH v2] filter-branch -d: Export GIT_DIR earlier

[Johannes Schindelin]

Hi,

On Tue, 17 Feb 2009, Lars Noschinski wrote:

> I'll change [make_commit] to test_commit.

Oops, sorry, I actually was not trying to suggest that, but relate a story 
that I found funny ;-)

> I think as it does not bite us, it would be unnecessary code churn to 
> remove the remaining usage of make_commit?

Probably.  I mean, every cleanup bears a very real possibility of 
introducing a regression, so it better be worth it.  As t7003 seems to 
work on (case) insensitive filesystems, let's leave it.

Ciao,
Dscho