[PATCH] Export GIT_DIR after setting it

[PATCH] Export GIT_DIR after setting it

[martin f. krafft]

git-sh-setup might set GIT_DIR, but not export it. When git-pull, for
instance, calls cd_to_toplevel, it changes the working directory, and later
calls git-ls-files, which does *not* inherit GIT_DIR since it's not imported.
It thus does the detection again, but in a different environment, since the
working directory changed. This breaks stuff subtly, especially when
core.worktree is set.

The patch simply exports GIT_DIR and makes it work such that git-ls-files
doesn't redo the work (wrongly).

Thanks to Björn Steinbrink for his help.

Signed-off-by: martin f. krafft <madduck@madduck.net>
---
 git-sh-setup.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index a44b1c7..de90f07 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -128,6 +128,7 @@ get_author_ident_from_commit () {
 if test -z "$NONGIT_OK"
 then
 	GIT_DIR=$(git rev-parse --git-dir) || exit
+	export GIT_DIR
 	if [ -z "$SUBDIRECTORY_OK" ]
 	then
 		test -z "$(git rev-parse --show-cdup)" || {
-- 
1.5.5.1

Re: [PATCH] Export GIT_DIR after setting it

[Junio C Hamano]

"martin f. krafft" <madduck@madduck.net> writes:

> git-sh-setup might set GIT_DIR, but not export it. When git-pull, for
> instance, calls cd_to_toplevel, it changes the working directory, and later
> calls git-ls-files, which does *not* inherit GIT_DIR since it's not imported.

"Not exporting GIT_DIR" was very much deliberately done when git-sh-setup
was introduced, and it is caller(includer)'s responsibility to export
GIT_DIR when necessary.  Depending on callers, some did not want to export
GIT_DIR, because exporting GIT_DIR means a bit more than that you are at
the toplevel of the tree (e.g. it tells the command not to do the usual
discovery of .git directory) and they have places in their codepath they
cannot cd up when running a git command internally, and/or they do not
want to cd up but they know what they run does GIT_DIR discovery on their
own.  I do not recall which exact callers they were, though.  Do people
recall the details?

Many scripted Porcelains were rewritten in C, and the need to be careful
and selective about when to export and when not to export might have been
removed already in which case it would be Ok to solve whatever you are
trying to solve like this patch does, but this change needs very careful
vetting to make sure that you did not break other scripts with this
change.

This arrangement predates separate work-tree by many months.  It could be
that what needs fixing is the separate work-tree code.  In any case, this
patch is a bit worrying.

Re: [PATCH] Export GIT_DIR after setting it

[martin f. krafft]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

Thank you, Junio, for taking the time to reply to this!

also sprach Junio C Hamano <gitster@pobox.com> [2008.05.15.0325 +0100]:
> trying to solve like this patch does, but this change needs very
> careful vetting to make sure that you did not break other scripts
> with this change.

Absolutely agreed. It occured to me as I lied down to sleep that
this fix could quite possibly have repercussions. And it's been in
my head all the walk to my work this morning. I ended up thinking
about it in this way:

If GIT_DIR is exported by git-sh-setup and we can assure that
git-sh-setup gets it right, then it's effectively the same as if the
user had set it explicitly, before calling the shell script: all
external commands called by the shell script will have GIT_DIR set
appropriately in all cases then.

The only problem I see now is when an external command (or the shell
script) can't properly deal with GIT_DIR being set, but then that's
a whole different bug.

I understand you're worried about this, but I can't really see
specifics, now having thought about this for a bit.

> This arrangement predates separate work-tree by many months.  It
> could be that what needs fixing is the separate work-tree code.

Oh yeah, and I've been meaning to look into that for a long time.
Sigh.

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
"she was rather too intelligent and competent-looking to be
 considered entirely beautiful, but all the more attractive because
 of it."
                           -- george spencer-brown, "a lion's teeth"
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Export GIT_DIR after setting it

[Junio C Hamano]

"martin f. krafft" <madduck@madduck.net> writes:

> The only problem I see now is when an external command (or the shell
> script) can't properly deal with GIT_DIR being set, but then that's
> a whole different bug.

One thing that we did not have to worry about when git-sh-setup was
invented is GIT_WORK_TREE and its cousin core.worktree.  When the user
uses GIT_DIR _but_ wants to work from a subdirectory of the checked out
work tree, the user _must_ tell git where the top of the work tree is; in
other words, setting and exporting only GIT_DIR is a misconfiguration.

I have a suspicion that "the whole different bug" is what bit you --
perhaps some places need to also set and export GIT_WORK_TREE as well when
the do GIT_DIR.

Re: [PATCH] Export GIT_DIR after setting it

[Björn Steinbrink]

On 2008.05.15 10:23:57 -0700, Junio C Hamano wrote:
> "martin f. krafft" <madduck@madduck.net> writes:
> 
> > The only problem I see now is when an external command (or the shell
> > script) can't properly deal with GIT_DIR being set, but then that's
> > a whole different bug.
> 
> One thing that we did not have to worry about when git-sh-setup was
> invented is GIT_WORK_TREE and its cousin core.worktree.  When the user
> uses GIT_DIR _but_ wants to work from a subdirectory of the checked out
> work tree, the user _must_ tell git where the top of the work tree is; in
> other words, setting and exporting only GIT_DIR is a misconfiguration.
> 
> I have a suspicion that "the whole different bug" is what bit you --
> perhaps some places need to also set and export GIT_WORK_TREE as well when
> the do GIT_DIR.

For completeness, here's an actual example of how it breaks:
doener@atjola:g $ git_fake_bare_checkout() {
>                 url="$1"
>                 repo="$2"
>                 worktree="$3"
>                 git clone --no-checkout "$url" "$repo"
>                 cd "$repo"
>                 mkdir -p "$worktree"
>                 git read-tree HEAD
>                 git checkout-index -a --prefix="$worktree" || true
>                 git config core.worktree "$worktree"
>                 mv .git/* .
>                 rmdir .git
>         }
doener@atjola:g $ git_fake_bare_checkout
git://git.madduck.net/etc/git.git git.git ../
Initialized empty Git repository in /home/doener/g/git.git/.git/
Receiving objects: 100% (6/6), done.
remote: Counting objects: 6, done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 6 (delta 0), reused 0 (delta 0)
doener@atjola:git.git (master) $ git fetch
doener@atjola:git.git (master) $ git pull
fatal: Not a git repository
fatal: Not a git repository
fatal: Not a git repository

So the git directory is not called .git but git.git, with core.worktree
set to "../". When "git fetch" is called directly, it correctly finds
that the git dir is "." Same for "git pull", but as GIT_DIR is neither
set in the environment, nor exported by git-pull, the commands that get
executed by git-pull do not find the git dir, because git-pull does
cd_to_toplevel first, and obviously the other commands won't look for
git.git, but just .git.

It kind of feels like a bug that git-pull does not export GIT_DIR there,
but you could probably also argue that it is wrong not to have GIT_DIR
set in the environment when using a non-standard name for the git dir.
Hm?

Björn

Re: [PATCH] Export GIT_DIR after setting it

[martin f. krafft]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]

also sprach Björn Steinbrink <B.Steinbrink@gmx.de> [2008.05.15.1855 +0100]:
> It kind of feels like a bug that git-pull does not export GIT_DIR there,
> but you could probably also argue that it is wrong not to have GIT_DIR
> set in the environment when using a non-standard name for the git dir.
> Hm?

Ah, but it is a standard name: .

If git does not find .git, it *does* seem to look into the current
directory too; that's why commands work inside bare repos...

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
click the start menu and select 'shut down.'
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Export GIT_DIR after setting it

[Björn Steinbrink]

On 2008.05.15 19:28:06 +0100, martin f. krafft wrote:
> also sprach Björn Steinbrink <B.Steinbrink@gmx.de> [2008.05.15.1855 +0100]:
> > It kind of feels like a bug that git-pull does not export GIT_DIR there,
> > but you could probably also argue that it is wrong not to have GIT_DIR
> > set in the environment when using a non-standard name for the git dir.
> > Hm?
> 
> Ah, but it is a standard name: .
> 
> If git does not find .git, it *does* seem to look into the current
> directory too; that's why commands work inside bare repos...

Yeah, but this is not a bare repo. As far as I understood Junio, it is
at least an error to use GIT_DIR without GIT_WORK_TREE (or
core.worktree), so I'm just thinking that it may also be an error to use
GIT_WORK_TREE (or core.worktree) without GIT_DIR.

Björn

Re: [PATCH] Export GIT_DIR after setting it

[martin f. krafft]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

also sprach Junio C Hamano <gitster@pobox.com> [2008.05.15.1823 +0100]:
> I have a suspicion that "the whole different bug" is what bit you
> -- perhaps some places need to also set and export GIT_WORK_TREE
> as well when the do GIT_DIR.

Probably also true, worktree support is still riddled with a lot of
small little bugs... but I don't see how this would actually solve
the problem that caused me to write this patch...

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
whatever you do will be insignificant,
but it is very important that you do it.
                                                     -- mahatma gandhi
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Export GIT_DIR after setting it

[martin f. krafft]

[-- Attachment #1: Type: text/plain, Size: 3294 bytes --]

also sprach Junio C Hamano <gitster@pobox.com> [2008.05.15.0325 +0100]:
> In any case, this patch is a bit worrying.

Your gut feeling is a good one!

See the following typescript. You'll notice that the new files
created in wc2 pushed and pulled get merged into the first wc at the
wrong location, and index and working dir get out of sync. This only
happens when I export GIT_DIR in git-sh-setup. Ouch.

Arguably, this is a bug in git-merge though!

% GIT_DIR=repo.git git --bare init
Initialized empty Git repository in repo.git/
% mkdir wc && cd wc && git init
Initialized empty Git repository in .git/
% git remote add origin `pwd`/../repo.git
% git config branch.master.remote origin
% git config branch.master.merge refs/heads/master
% touch a; git add a; git commit -m.
Created initial commit c80aa71: .
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 a
% git push origin master
Counting objects: 3, done.
Writing objects: 100% (3/3), 196 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
To /home/madduck/.tmp/cdt.SLg15374/wc/../repo.git
 * [new branch]      master -> master
% mkdir foo && touch foo/a && git add foo/a && git commit -m.
Created commit 8ccd80a: .
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 foo/a
% git push
Counting objects: 3, done.
Compressing objects: 100% (2/2), done.
Unpacking objects: 100% (2/2), done.
Writing objects: 100% (2/2), 247 bytes, done.
Total 2 (delta 0), reused 0 (delta 0)
To /home/madduck/.tmp/cdt.SLg15374/wc/../repo.git
   c80aa71..8ccd80a  master -> master
% cd ../
% git clone repo.git wc2
Initialized empty Git repository in /home/madduck/.tmp/cdt.SLg15374/wc2/.git/
% cd wc2
% cd foo && mkdir bar && touch bar/a && git add bar/a && git commit -m.
Created commit cba76e8: .
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 foo/bar/a
% git push
Counting objects: 5, done.
Compressing objects: 100% (3/3), done.
Unpacking objects: 100% (3/3), done.
Writing objects: 100% (3/3), 315 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
To /home/madduck/.tmp/cdt.SLg15374/repo.git
   8ccd80a..cba76e8  master -> master
% cd ../../wc/foo 
% ls
a
% git pull
remote: Counting objects: 5, done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
From /home/madduck/.tmp/cdt.SLg15374/wc/../repo
   8ccd80a..cba76e8  master     -> origin/master
Updating 8ccd80a..cba76e8
foo/a: needs update
Fast forward
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 foo/bar/a
% ls
a  foo
% git status
# On branch master
# Changed but not updated:
#   (use "git add/rm <file>..." to update what will be committed)
#
#       deleted:    bar/a
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       foo/
no changes added to commit (use "git add" and/or "git commit -a")
% git diff
diff --git a/foo/bar/a b/foo/bar/a
deleted file mode 100644
index e69de29..0000000

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
micro$oft windoze: proof that p. t. barnum was correct.
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 189 bytes --]