threads / patch / 17844

v2filter-branch -d: Export GIT_DIR earlier

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

## tl;dr

10 messages between Feb 17, 2009 and Feb 19, 2009. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Lars Noschinski· Feb 17, 2009, 08:31 UTC · lore

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(-)
Show changes to 2 files +15 −6

git-filter-branch.sh, t/t7003-filter-branch.sh

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
Lars Noschinski· Feb 17, 2009, 08:53 UTC · re: Lars Noschinski · lore

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

* Lars Noschinski <lars@public.noschinski.de> [09-02-17 09:31]:
Show 18 quoted lines
>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
Johannes Schindelin· Feb 17, 2009, 12:44 UTC · re: Lars Noschinski · lore

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

Hi,
On Tue, 17 Feb 2009, Lars Noschinski wrote:
Show 14 quoted lines
> 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).

Show 11 quoted lines
> 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

Lars Noschinski· Feb 17, 2009, 17:59 UTC · re: Johannes Schindelin · lore

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

* Johannes Schindelin <Johannes.Schindelin@gmx.de> [09-02-17 16:08]:
Show 20 quoted lines
>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'.

Show 5 quoted lines
>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.

Show 5 quoted lines
>We try to avoid cd'ing back, by using constructs like this:
>
>	(cd drepo &&
>	 ...
>	) &&
Ok, can do.
Show 7 quoted lines
>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?

Show 5 quoted lines
>So I _had_ to look who introduced make_commit:
>
>	$ git blame -L '/make_commit/,/}/' t/t7003*
>
>Making a fool out of yourself -- priceless.
:)
  - Lars.
Lars Noschinski· Feb 17, 2009, 18:05 UTC · re: Lars Noschinski · lore

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

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(-)
Show changes to 2 files +18 −6

git-filter-branch.sh, t/t7003-filter-branch.sh

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
Johannes Schindelin· Feb 17, 2009, 23:03 UTC · re: Lars Noschinski · lore

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

Hi,
On Tue, 17 Feb 2009, Lars Noschinski wrote:
Show 18 quoted lines
> 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

Lars Noschinski· Feb 18, 2009, 08:35 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +18 −6

git-filter-branch.sh, t/t7003-filter-branch.sh

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
Johannes Schindelin· Feb 18, 2009, 10:25 UTC · re: Lars Noschinski · lore

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

Hi,
On Wed, 18 Feb 2009, Lars Noschinski wrote:
Show 15 quoted lines
> 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

Johannes Schindelin· Feb 17, 2009, 23:01 UTC · re: Lars Noschinski · lore

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

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

← back to recent threads