threads / patch / 13522

patchExport GIT_DIR after setting it

Subject: [PATCH] Export GIT_DIR after setting it

## tl;dr

9 messages between May 14, 2008 and May 20, 2008. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

martin f. krafft· May 14, 2008, 23:23 UTC · lore

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(-)
Show changes to git-sh-setup.sh +1 −0
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
Junio C Hamano· May 15, 2008, 02:25 UTC · re: martin f. krafft · lore

Re: [PATCH] Export GIT_DIR after setting it

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

martin f. krafft· May 15, 2008, 10:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] Export GIT_DIR after setting it

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
Junio C Hamano· May 15, 2008, 17:23 UTC · re: martin f. krafft · lore

Re: [PATCH] Export GIT_DIR after setting it

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

Björn Steinbrink· May 15, 2008, 17:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] Export GIT_DIR after setting it

On 2008.05.15 10:23:57 -0700, Junio C Hamano wrote:
Show 15 quoted lines
> "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() {

Show 12 quoted lines
>                 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
martin f. krafft· May 15, 2008, 18:28 UTC · re: Björn Steinbrink · lore

Re: [PATCH] Export GIT_DIR after setting it

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
Björn Steinbrink· May 15, 2008, 18:44 UTC · re: martin f. krafft · lore

Re: [PATCH] Export GIT_DIR after setting it

On 2008.05.15 19:28:06 +0100, martin f. krafft wrote:
Show 10 quoted lines
> 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
martin f. krafft· May 16, 2008, 21:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] Export GIT_DIR after setting it

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
martin f. krafft· May 20, 2008, 16:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] Export GIT_DIR after setting it

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
Show changes to foo/bar/a +0 −0
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

← back to recent threads