[PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[David Kastrup]

If filter-branch is entered with an unset GIT_DIR, things are rather
fragile.  The GIT_DIR variable setting then points to something like
$(pwd)/../.. which is neither guaranteed to be a git directory
(depends on where filter-branch is started), nor will it continue to
work once the temporary directory (for which the pwd is output) ceases
to exist.

So we just call git-name-rev in order to get the correct setting here
for exporting.

Signed-off-by: David Kastrup <dak@gnu.org>
---
 git-filter-branch.sh |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index b5fa449..6cd489e 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -170,14 +170,10 @@ do
 	esac
 done < "$tempdir"/backup-refs
 
-case "$GIT_DIR" in
-/*)
-	;;
-*)
-	GIT_DIR="$(pwd)/../../$GIT_DIR"
-	;;
-esac
-export GIT_DIR GIT_WORK_TREE=.
+GIT_DIR=$(cd ../..;git-name-rev --git-dir)
+GIT_WORK_TREE=.
+
+export GIT_DIR GIT_WORK_TREE
 
 # These refs should be updated if their heads were rewritten
 
-- 
1.5.3.rc4.43.gaf14

[fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[David Kastrup]

If filter-branch is entered with an unset GIT_DIR, things are rather
fragile.  The GIT_DIR variable setting then points to something like
$(pwd)/../.. which is neither guaranteed to be a git directory
(depends on where filter-branch is started), nor will it continue to
work once the temporary directory (for which the pwd is output) ceases
to exist.

So we just call git-rev-parse in order to get the correct setting here
for exporting.

Signed-off-by: David Kastrup <dak@gnu.org>
---
 git-filter-branch.sh |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index b5fa449..9e9e8bf 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -170,14 +170,11 @@ do
 	esac
 done < "$tempdir"/backup-refs
 
-case "$GIT_DIR" in
-/*)
-	;;
-*)
-	GIT_DIR="$(pwd)/../../$GIT_DIR"
-	;;
-esac
-export GIT_DIR GIT_WORK_TREE=.
+GIT_DIR=$(cd ../..;cd "./$(git-rev-parse --git-dir)";pwd)
+
+GIT_WORK_TREE=.
+
+export GIT_DIR GIT_WORK_TREE
 
 # These refs should be updated if their heads were rewritten
 
-- 
1.5.3.rc4.74.g3739cb17

Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> If filter-branch is entered with an unset GIT_DIR, things are rather
> fragile.  The GIT_DIR variable setting then points to something like
> $(pwd)/../.. which is neither guaranteed to be a git directory

I think this comment refers to this part, ...

> -case "$GIT_DIR" in
> -/*)
> -	;;
> -*)
> -	GIT_DIR="$(pwd)/../../$GIT_DIR"
> -	;;
> -esac

... however, at the beginning of the script, it dot-includes
git-sh-setup, which sets (but not export) GIT_DIR for the rest
of the script to use (see the last if..then..else).

If you got an unset GIT_DIR when you reached that case statement
you are removing here, I suspect that there is something else
going on, but I do not see what it is...

Puzzled...

Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> If filter-branch is entered with an unset GIT_DIR, things are rather
>> fragile.  The GIT_DIR variable setting then points to something like
>> $(pwd)/../.. which is neither guaranteed to be a git directory
>
> I think this comment refers to this part, ...
>
>> -case "$GIT_DIR" in
>> -/*)
>> -	;;
>> -*)
>> -	GIT_DIR="$(pwd)/../../$GIT_DIR"
>> -	;;
>> -esac
>
> ... however, at the beginning of the script, it dot-includes
> git-sh-setup, which sets (but not export) GIT_DIR for the rest
> of the script to use (see the last if..then..else).

Ah, ok.  In that case my patch is overengineered.  However, one could
replace the whole case with

GITDIR=$(cd ../..;cd $GIT_DIR;pwd)

This would work with Windows absolute paths, too.

> If you got an unset GIT_DIR when you reached that case statement
> you are removing here, I suspect that there is something else
> going on, but I do not see what it is...
>
> Puzzled...

The problem I saw was that "$(pwd)/../../$GIT_DIR" does no longer exist
by the time the end of the script it reached, because what was
"$(pwd)" has already been removed.  So it is less onerous than I
thought, but still a nuisance when the script ends.

Anyway, if $GIT_DIR is a relative path, things would go wrong in some
lines earlier, where the path is already being changed.

If git-sh-setup sets GIT_DIR, perhaps it would be sanest if it also
made it absolute?

Otherwise any script that does "cd" will lose track of GIT_DIR, right?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[David Kastrup]

David Kastrup <dak@gnu.org> writes:

> Anyway, if $GIT_DIR is a relative path, things would go wrong in some
> lines earlier,

and if GIT_DIR were used or exported which it isn't, so we are lucky...

> where the path is already being changed.
>
> If git-sh-setup sets GIT_DIR, perhaps it would be sanest if it also
> made it absolute?
>
> Otherwise any script that does "cd" will lose track of GIT_DIR, right?

Actually, wouldn't by far the most straightforward thing be if
git-rev-parse --git-dir always returned an absolute path (even when
being passed a relative path in GIT_DIR)?

No need for postprocessing, no need to keep track of changed
directories.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[David Kastrup]

David Kastrup <dak@gnu.org> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> Anyway, if $GIT_DIR is a relative path, things would go wrong in some
>> lines earlier,
>
> and if GIT_DIR were used or exported which it isn't, so we are lucky...
>
>> where the path is already being changed.
>>
>> If git-sh-setup sets GIT_DIR, perhaps it would be sanest if it also
>> made it absolute?
>>
>> Otherwise any script that does "cd" will lose track of GIT_DIR, right?
>
> Actually, wouldn't by far the most straightforward thing be if
> git-rev-parse --git-dir always returned an absolute path (even when
> being passed a relative path in GIT_DIR)?
>
> No need for postprocessing, no need to keep track of changed
> directories.

To illustrate the potential for trouble:

git-commit.sh

has the lines

647:
    cd_to_toplevel

    git rerere

    if test "$ret" = 0
    then
            if test -x "$GIT_DIR"/hooks/post-commit
            then
                    "$GIT_DIR"/hooks/post-commit
            fi

Obviously, this won't work if GIT_DIR has a relative path and the
command has not been started from the top level already.

In a similar vein, git-checkout.sh has

263:
     GIT_DIR="$GIT_DIR" git symbolic-ref -m "checkout: moving from $old_branch_name to $branch" HEAD "refs/heads/$branch"

while we have previously cd_to_toplevel in line 154.


And so on.  Rather than fixing all those scripts, it would seem safer
to let git-rev-parse --git-dir (I don't understand the code in
builtin-rev-parse, by the way: why does it look at "prefix"?), or at
the very least git-sh-setup.sh return an absolute path.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[Junio C Hamano]

I think your other patch to always give full path to the shell
scripts from git-sh-setup makes sense.  Could you please make
test scripts to expose the problems you described in the message
I am replying to and add it as say t/t2050-git-dir-relative.sh?

Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> I think your other patch to always give full path to the shell
> scripts from git-sh-setup makes sense.  Could you please make
> test scripts to expose the problems you described in the message
> I am replying to and add it as say t/t2050-git-dir-relative.sh?

It is actually a bit harder to trigger than I expected: if GIT_DIR is
not a direct descendant of the current directory, git-rev-parse does
output an absolute path.  So at least for the test failure in
git-commit, I have to revert to a non-run-of-the-mill setup.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

[PATCH] Add a test for git-commit being confused by relative GIT_DIR

[David Kastrup]

Signed-off-by: David Kastrup <dak@gnu.org>
---
It takes an inordinarily large amount of time to create these tests, I
am afraid.  There is little sense in making a more extensive audit,
cranking out more cases which fail with relative paths, when the fix,
namely making GIT_DIR absolute, is so simple.

 t/t2050-git-dir-relative.sh |   54 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)
 create mode 100755 t/t2050-git-dir-relative.sh

diff --git a/t/t2050-git-dir-relative.sh b/t/t2050-git-dir-relative.sh
new file mode 100755
index 0000000..54a3512
--- /dev/null
+++ b/t/t2050-git-dir-relative.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='check problems with relative GIT_DIR
+
+This test creates a working tree state with a file and subdir:
+
+  top (committed several times)
+  subdir (a subdirectory)
+
+It creates a commit-hook and tests it, then moves .git
+into the subdir while keeping the worktree location, 
+and tries commits from the top and the subdir, checking
+that the commit-hook still gets called.'
+
+. ./test-lib.sh
+
+test_expect_success 'Setting up post-commit hook' '
+mkdir -p .git/hooks &&
+cat <<EOF >.git/hooks/post-commit &&
+#!/bin/sh
+touch $(pwd)/output
+echo "Post commit hook was called."
+EOF
+chmod +x .git/hooks/post-commit'
+
+test_expect_success 'post-commit hook used ordinarily' '
+echo initial >top &&
+git-add top
+git-commit -m initial &&
+test -r output
+'
+
+rm -rf output
+mkdir subdir
+mv .git subdir
+
+test_expect_success 'post-commit-hook created and used from top dir' '
+echo changed >top &&
+git --git-dir subdir/.git add top &&
+git --git-dir subdir/.git commit -m topcommit &&
+test -r output
+'
+
+rm -rf output
+
+test_expect_success 'post-commit-hook from sub dir' '
+echo changed again >top
+cd subdir &&
+git --git-dir .git --work-tree .. add ../top &&
+git --git-dir .git --work-tree .. commit -m subcommit &&
+test -r ../output
+'
+
+test_done
-- 
1.5.3.rc2.187.g9a1d2-dirty

Re: [PATCH] Add a test for git-commit being confused by relative GIT_DIR

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> +test_expect_success 'Setting up post-commit hook' '
> +mkdir -p .git/hooks &&
> +cat <<EOF >.git/hooks/post-commit &&
> +#!/bin/sh
> +touch $(pwd)/output
> +echo "Post commit hook was called."
> +EOF
> +chmod +x .git/hooks/post-commit'

We have avoided to use here text inside test_expect_success, as
there have been reports that some otherwise reasonably usable
shells do not grok it.  Although I prefer to do everything,
including the set-up part, inside test_expect_success, please
move this code outside.

Also I do not think you would want to say touch $(pwd)/output
there inside the here text that begins with <<EOF not <<\EOF.

Re: [PATCH] Add a test for git-commit being confused by relative GIT_DIR

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> +test_expect_success 'Setting up post-commit hook' '
>> +mkdir -p .git/hooks &&
>> +cat <<EOF >.git/hooks/post-commit &&
>> +#!/bin/sh
>> +touch $(pwd)/output
>> +echo "Post commit hook was called."
>> +EOF
>> +chmod +x .git/hooks/post-commit'
>
> We have avoided to use here text inside test_expect_success, as
> there have been reports that some otherwise reasonably usable
> shells do not grok it.

Ok.

> Although I prefer to do everything, including the set-up part,
> inside test_expect_success, please move this code outside.

I can easily enough make it an echo.

> Also I do not think you would want to say touch $(pwd)/output
> there inside the here text that begins with <<EOF not <<\EOF.

But I most certainly do!  The same file should be touched regardless
of what the cwd at the time of calling the hook is.  Otherwise, I
would not need the $(pwd) in the first place.  The whole point is that
it is expanded at the time of the hook creation.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Add a test for git-commit being confused by relative GIT_DIR

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

>> Also I do not think you would want to say touch $(pwd)/output
>> there inside the here text that begins with <<EOF not <<\EOF.
>
> But I most certainly do!

Then you would need to shell-quote $(pwd) so that you can have
your git.git checked out somewhere under "/My Documents/source"
directory, wouldn't you?

Re: [PATCH] Add a test for git-commit being confused by relative GIT_DIR

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>>> Also I do not think you would want to say touch $(pwd)/output
>>> there inside the here text that begins with <<EOF not <<\EOF.
>>
>> But I most certainly do!
>
> Then you would need to shell-quote $(pwd) so that you can have
> your git.git checked out somewhere under "/My Documents/source"
> directory, wouldn't you?

Ok, that's a tough one.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[Steven Grimm]

David Kastrup wrote:
> Actually, wouldn't by far the most straightforward thing be if
> git-rev-parse --git-dir always returned an absolute path (even when
> being passed a relative path in GIT_DIR)?
>   

That might also help get rid of an annoying failure mode where some git 
commands (the shell script ones) fail if you cd into a git repository 
via a symlink, while others (the builtins) work just fine.

-Steve

Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> David Kastrup wrote:
>> Actually, wouldn't by far the most straightforward thing be if
>> git-rev-parse --git-dir always returned an absolute path (even when
>> being passed a relative path in GIT_DIR)?
>>
>
> That might also help get rid of an annoying failure mode where some
> git commands (the shell script ones) fail if you cd into a git
> repository via a symlink, while others (the builtins) work just fine.

It might be the easiest workaround for this issue to unset PWD
in git-sh-setup.sh (and git-clone.sh) where we unset CDPATH.
Anybody wants to try to come up with additional test scripts to
expose the problem and a patch to these two files?