GIT_DIR not auto ignored

Greetings,

I found this probable bug:
https://gist.github.com/anonymous/01979fd9e6e285df41a2

Quoted 4 lines · Ingy dot Net

Greetings,

I found this probable bug:
https://gist.github.com/anonymous/01979fd9e6e285df41a2

Summary:

$ mv .git .foo
$ export GIT_DIR=$PWD/.foo
$ git status
# On branch master
#
# Initial commit
#
# Untracked files:
# .foo/
nothing added to commit but untracked files present

I checked with 1.8.5 and this still happens.

And this also happens:

$ mv .git .foo 
$ export GIT_DIR=.foo
dennis@lightning:~/code/git$ touch .git
dennis@lightning:~/code/git$ git status
On branch master
Untracked files:
  (use "git add <file>..." to include in what will be committed)

	.foo/

nothing added to commit but untracked files present (use "git add" to track)

(Note the absence of .git there)

We always ignore anything named .git, but we should also ignore the git directory if the user overrides it by setting $GIT_DIR

Reported-By Ingy döt Net <[email protected]>

Signed-off-by Dennis Kaarsemaker <[email protected]>

---
 dir.c             | 2 +-
 t/t7508-status.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

dir.c +0 -0

index 23b6de4..884b37d 100644

dir.c +0 -0

        dtype = DTYPE(de);

t/t7508-status.sh +7 -1

index c987b5e..2bd7ef1 100755
@@ -198,6 +198,13 @@ test_expect_success 'status -s' '
 
 '
 
+test_expect_success 'status -s with non-standard $GIT_DIR' '
+       mv .git .foo &&
+       GIT_DIR=.foo git status -s >output &&
+       test_cmp expect output &&
+       mv .foo .git
+'
+
 test_expect_success 'status with gitignore' '
        {
                echo ".gitignore" &&
--

1.8.5-386-gb78cb96

--
Dennis Kaarsemaker <[email protected]>
http://twitter.com/seveas
--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines · Dennis Kaarsemaker

We always ignore anything named .git, but we should also ignore the git directory if the user overrides it by setting $GIT_DIR

Quoted 2 lines

+ if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))

                return path_none;

this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path we check. Is it worth the cost?

Quoted 4 lines · Duy Nguyen

Quoted 2 lines · Dennis Kaarsemaker

We always ignore anything named .git, but we should also ignore the git directory if the user overrides it by setting $GIT_DIR

[...]

Quoted 3 lines

Quoted 2 lines

+ if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))

                return path_none;

Quoted 2 lines

this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path we check. Is it worth the cost?

Moreover it is a much more inclusive check than what the commit message
claims: it will ignore anything that looks like a .git directory, regardless of the name. In particular GIT_DIR doesn't have anything to do with it.

Quoted 10 lines · Thomas Rast

Quoted 4 lines · Duy Nguyen

Quoted 2 lines · Dennis Kaarsemaker

We always ignore anything named .git, but we should also ignore the git directory if the user overrides it by setting $GIT_DIR

[...]

Quoted 3 lines

Quoted 2 lines

+ if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))

                return path_none;

Quoted 7 lines

Quoted 2 lines

this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path we check. Is it worth the cost?

Moreover it is a much more inclusive check than what the commit message
claims: it will ignore anything that looks like a .git directory, regardless of the name. In particular GIT_DIR doesn't have anything to do with it.

Ah, yes thanks, that's rather incorrect indeed. How about the following instead? Passes all tests, including the new one.

Quoted 22 lines · Dennis Kaarsemaker

Quoted 10 lines · Thomas Rast

Quoted 4 lines · Duy Nguyen

Quoted 2 lines · Dennis Kaarsemaker

We always ignore anything named .git, but we should also ignore the git directory if the user overrides it by setting $GIT_DIR

[...]

Quoted 3 lines

Quoted 2 lines

+ if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))

                return path_none;

Quoted 7 lines

Quoted 2 lines

this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path we check. Is it worth the cost?

Moreover it is a much more inclusive check than what the commit message
claims: it will ignore anything that looks like a .git directory, regardless of the name. In particular GIT_DIR doesn't have anything to do with it.

Ah, yes thanks, that's rather incorrect indeed. How about the following instead? Passes all tests, including the new one.

dir.c static enum path_treatment treat_path(struct dir_struct *dir, · line 1202

Quoted 9 lines

dir.c +1 -1

@@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
                return path_none;
        strbuf_setlen(path, baselen);
        strbuf_addstr(path, de->d_name);
-       if (simplify_away(path->buf, path->len, simplify))
+       if (simplify_away(path->buf, path->len, simplify) || !strncmp(get_git_dir(), path->buf, path->len))
                return path_none;

get_git_dir() may return a relative or absolute path, depending on GIT_DIR/GIT_WORK_TREE. path->buf is always relative. You'll pass one case with this (relative vs relative) and fail another. It might be simpler to just add get_git_dir(), after converting to relative path and check if it's in worktree, to the exclude list and let the current exclude mechanism handle it.

Quoted 24 lines · Duy Nguyen

Quoted 22 lines · Dennis Kaarsemaker

Quoted 10 lines · Thomas Rast

Quoted 4 lines · Duy Nguyen

Quoted 2 lines · Dennis Kaarsemaker

We always ignore anything named .git, but we should also ignore the git directory if the user overrides it by setting $GIT_DIR

[...]

Quoted 3 lines

Quoted 2 lines

+ if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))

                return path_none;

Quoted 7 lines

Quoted 2 lines

this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path we check. Is it worth the cost?

Moreover it is a much more inclusive check than what the commit message
claims: it will ignore anything that looks like a .git directory, regardless of the name. In particular GIT_DIR doesn't have anything to do with it.

Ah, yes thanks, that's rather incorrect indeed. How about the following instead? Passes all tests, including the new one.

Quoted 16 lines

dir.c static enum path_treatment treat_path(struct dir_struct *dir, · line 1202

Quoted 9 lines

dir.c +1 -1

@@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
                return path_none;
        strbuf_setlen(path, baselen);
        strbuf_addstr(path, de->d_name);
-       if (simplify_away(path->buf, path->len, simplify))
+       if (simplify_away(path->buf, path->len, simplify) || !strncmp(get_git_dir(), path->buf, path->len))
                return path_none;

get_git_dir() may return a relative or absolute path, depending on GIT_DIR/GIT_WORK_TREE. path->buf is always relative. You'll pass one case with this (relative vs relative) and fail another. It might be simpler to just add get_git_dir(), after converting to relative path and check if it's in worktree, to the exclude list and let the current exclude mechanism handle it.

This type of invocation really only works from the root of the workdir anyway and both a relative and absolute path work just fine:

dennis@lightning:~/code/git$ GIT_DIR=$(pwd)/.foo ./git status
On branch master
nothing to commit, working directory clean
dennis@lightning:~/code/git$ GIT_DIR=./.foo ./git status
On branch master
nothing to commit, working directory clean

Well, unless you set GIT_WORK_TREE as well, but then it still works:

dennis@lightning:~/code/git/t$ GIT_DIR=$(pwd)/../.foo GIT_WORK_TREE=.. ../git status
On branch master
nothing to commit, working directory clean
dennis@lightning:~/code/git/t$ GIT_DIR=../.foo GIT_WORK_TREE=.. ../git status
On branch master
nothing to commit, working directory clean

So I'm wondering when you think this will fail. Because then I can add a test for that case too.

Quoted 61 lines · Dennis Kaarsemaker

Quoted 24 lines · Duy Nguyen

Quoted 22 lines · Dennis Kaarsemaker

Quoted 10 lines · Thomas Rast

Quoted 4 lines · Duy Nguyen

Quoted 2 lines · Dennis Kaarsemaker

We always ignore anything named .git, but we should also ignore the git directory if the user overrides it by setting $GIT_DIR

[...]

Quoted 3 lines

Quoted 2 lines

+ if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))

                return path_none;

Quoted 7 lines

Quoted 2 lines

this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path we check. Is it worth the cost?

Moreover it is a much more inclusive check than what the commit message
claims: it will ignore anything that looks like a .git directory, regardless of the name. In particular GIT_DIR doesn't have anything to do with it.

Ah, yes thanks, that's rather incorrect indeed. How about the following instead? Passes all tests, including the new one.

Quoted 16 lines

dir.c static enum path_treatment treat_path(struct dir_struct *dir, · line 1202

Quoted 9 lines

dir.c +1 -1

@@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
                return path_none;
        strbuf_setlen(path, baselen);
        strbuf_addstr(path, de->d_name);
-       if (simplify_away(path->buf, path->len, simplify))
+       if (simplify_away(path->buf, path->len, simplify) || !strncmp(get_git_dir(), path->buf, path->len))
                return path_none;

get_git_dir() may return a relative or absolute path, depending on GIT_DIR/GIT_WORK_TREE. path->buf is always relative. You'll pass one case with this (relative vs relative) and fail another. It might be simpler to just add get_git_dir(), after converting to relative path and check if it's in worktree, to the exclude list and let the current exclude mechanism handle it.

This type of invocation really only works from the root of the workdir anyway and both a relative and absolute path work just fine:

dennis@lightning:~/code/git$ GIT_DIR=$(pwd)/.foo ./git status
On branch master
nothing to commit, working directory clean
dennis@lightning:~/code/git$ GIT_DIR=./.foo ./git status
On branch master
nothing to commit, working directory clean

Well, unless you set GIT_WORK_TREE as well, but then it still works:

dennis@lightning:~/code/git/t$ GIT_DIR=$(pwd)/../.foo GIT_WORK_TREE=.. ../git status
On branch master
nothing to commit, working directory clean
dennis@lightning:~/code/git/t$ GIT_DIR=../.foo GIT_WORK_TREE=.. ../git status
On branch master
nothing to commit, working directory clean

Quoted 2 lines

So I'm wondering when you think this will fail. Because then I can add a test for that case too.

~/w/git $ cd t
~/w/git/t $ GIT_TRACE_SETUP=1 ../git --git-dir=../.git --work-tree=.. --no-pager status
setup: git_dir: /home/pclouds/w/git/.git
setup: worktree: /home/pclouds/w/git
setup: cwd: /home/pclouds/w/git
setup: prefix: t/
On branch exclude-pathspec
Your branch and 'origin/master' have diverged,
and have 2 and 5 different commits each, respectively.

I can't say this is the only case though. One has to audit to all possible setup cases in setup_git_directory() to make that claim.

Quoted 11 lines

~/w/git $ cd t
~/w/git/t $ GIT_TRACE_SETUP=1 ../git --git-dir=../.git --work-tree=.. --no-pager status
setup: git_dir: /home/pclouds/w/git/.git
setup: worktree: /home/pclouds/w/git
setup: cwd: /home/pclouds/w/git
setup: prefix: t/
On branch exclude-pathspec
Your branch and 'origin/master' have diverged,
and have 2 and 5 different commits each, respectively.

Quoted 2 lines

I can't say this is the only case though. One has to audit to all possible setup cases in setup_git_directory() to make that claim.

I'm probably missing something, but that's the same as my second example, and works. I also tried running it from completely outside the repo:

dennis@lightning:~$ code/git/git --git-dir=code/git/.foo --work-tree=code/git status
On branch master
nothing to commit, working directory clean
dennis@lightning:~$ code/git/git --git-dir=/home/dennis/code/git/.foo --work-tree=code/git status
On branch master
nothing to commit, working directory clean

t/t7508-status.sh +2 -1

+'
+
 test_expect_success 'status with gitignore' '
        {
                echo ".gitignore" &&
--

1.8.5-386-gb78cb96

So I figure that GIT_DIR is not meant to _rename_ the ".git" dir, but to point somewhere _outside_ the worktree (or somewhere within the .git dir).

If we don't want to support this, though, I think it would be more approrpiate to issue a warning if GIT_DIR points to a worktree location.

Quoted 3 lines · Karsten Blees

If we don't want to support this, though, I think it would be more approrpiate to issue a warning if GIT_DIR points to a worktree location.

But how do tell what is and isn't a "worktree location"? Having the path in the index would be one, but you may find it out only after issuing "git checkout $antient_commit".

Quoted 3 lines · Karsten Blees

So I figure that GIT_DIR is not meant to _rename_ the ".git" dir, but to point somewhere _outside_ the worktree (or somewhere within the .git dir).

Correct.

Quoted 3 lines

If we don't want to support this, though, I think it would be more approrpiate to issue a warning if GIT_DIR points to a worktree location.

But how do tell what is and isn't a "worktree location"? Having the path in the index would be one, but you may find it out only after issuing "git checkout $antient_commit".