patchgit-filter-branch.sh: Fix broken setting of GIT_DIR

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(-)

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(-)

David Kastrup <dak@gnu.org> writes:

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

... 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...

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

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.

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 <dak@gnu.org> writes:

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

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 <dak@gnu.org> writes:

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.

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?

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

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.

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

David Kastrup <dak@gnu.org> writes:

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.

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

Ok.

I can easily enough make it an echo.

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 <dak@gnu.org> writes:

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?

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

Ok, that's a tough one.

David Kastrup wrote:

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

Steven Grimm <koreth@midwinter.com> writes:

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?