GIT_DIR not auto ignored

GIT_DIR not auto ignored

[Ingy dot Net]

Greetings,

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

Cheers, Ingy döt Net

Re: GIT_DIR not auto ignored

[Dennis Kaarsemaker]

On za, 2013-11-30 at 23:06 -0800, Ingy dot Net wrote:
> 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)
-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: GIT_DIR not auto ignored

[Dennis Kaarsemaker]

On zo, 2013-12-01 at 19:08 +0100, Dennis Kaarsemaker wrote:
> On za, 2013-11-30 at 23:06 -0800, Ingy dot Net wrote:
> > 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. 

This makes it go away:

diff --git a/dir.c b/dir.c
index 23b6de4..884b37d 100644
--- a/dir.c
+++ b/dir.c
@@ -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) || is_git_directory(path->buf))
                return path_none;
 
        dtype = DTYPE(de);

I'll add a test and submit a proper patch.

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

Comments in dir.c indicate that this is expected, so I didn't try to
"fix" that.

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

[PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[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

Reported-By: Ingy döt Net <ingy@ingy.net>
Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 dir.c             | 2 +-
 t/t7508-status.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index 23b6de4..884b37d 100644
--- a/dir.c
+++ b/dir.c
@@ -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) || is_git_directory(path->buf))
 		return path_none;
 
 	dtype = DTYPE(de);
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c987b5e..2bd7ef1 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -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 <dennis@kaarsemaker.net>
http://twitter.com/seveas

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Duy Nguyen]

On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> 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 <ingy@ingy.net>
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
>  dir.c             | 2 +-
>  t/t7508-status.sh | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/dir.c b/dir.c
> index 23b6de4..884b37d 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -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) || 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?


>
>         dtype = DTYPE(de);
> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
> index c987b5e..2bd7ef1 100755
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -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 <dennis@kaarsemaker.net>
> http://twitter.com/seveas
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Duy

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Thomas Rast]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker
> <dennis@kaarsemaker.net> wrote:
>> We always ignore anything named .git, but we should also ignore the git
>> directory if the user overrides it by setting $GIT_DIR
[...]
>> +       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?

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.

-- 
Thomas Rast
tr@thomasrast.ch

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Dennis Kaarsemaker]

On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker
> > <dennis@kaarsemaker.net> wrote:
> >> We always ignore anything named .git, but we should also ignore the git
> >> directory if the user overrides it by setting $GIT_DIR
> [...]
> >> +       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?
> 
> 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.

--- a/dir.c
+++ b/dir.c
@@ -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;
 
-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Duy Nguyen]

On Mon, Dec 2, 2013 at 6:38 AM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker
>> > <dennis@kaarsemaker.net> wrote:
>> >> We always ignore anything named .git, but we should also ignore the git
>> >> directory if the user overrides it by setting $GIT_DIR
>> [...]
>> >> +       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?
>>
>> 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.
>
> --- a/dir.c
> +++ b/dir.c
> @@ -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.
-- 
Duy

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Eric Sunshine]

On Sun, Dec 1, 2013 at 2:04 PM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
> index c987b5e..2bd7ef1 100755
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -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

If test_cmp returns a failure status, the following 'mv' command will
never be invoked, thus all subsequent tests in the script will fail
since the .git directory will be missing. Instead, use
test_when_finished to restore .git from .foo.

> +'
> +
>  test_expect_success 'status with gitignore' '
>         {
>                 echo ".gitignore" &&
> --
> 1.8.5-386-gb78cb96

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Dennis Kaarsemaker]

On ma, 2013-12-02 at 07:38 +0700, Duy Nguyen wrote:
> On Mon, Dec 2, 2013 at 6:38 AM, Dennis Kaarsemaker
> <dennis@kaarsemaker.net> wrote:
> > On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:
> >> Duy Nguyen <pclouds@gmail.com> writes:
> >>
> >> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker
> >> > <dennis@kaarsemaker.net> wrote:
> >> >> We always ignore anything named .git, but we should also ignore the git
> >> >> directory if the user overrides it by setting $GIT_DIR
> >> [...]
> >> >> +       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?
> >>
> >> 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.
> >
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -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.

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Duy Nguyen]

On Mon, Dec 2, 2013 at 3:01 PM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> On ma, 2013-12-02 at 07:38 +0700, Duy Nguyen wrote:
>> On Mon, Dec 2, 2013 at 6:38 AM, Dennis Kaarsemaker
>> <dennis@kaarsemaker.net> wrote:
>> > On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:
>> >> Duy Nguyen <pclouds@gmail.com> writes:
>> >>
>> >> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker
>> >> > <dennis@kaarsemaker.net> wrote:
>> >> >> We always ignore anything named .git, but we should also ignore the git
>> >> >> directory if the user overrides it by setting $GIT_DIR
>> >> [...]
>> >> >> +       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?
>> >>
>> >> 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.
>> >
>> > --- a/dir.c
>> > +++ b/dir.c
>> > @@ -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.

~/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.
-- 
Duy

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Dennis Kaarsemaker]

On ma, 2013-12-02 at 16:35 +0700, Duy Nguyen wrote:
> On Mon, Dec 2, 2013 at 3:01 PM, Dennis Kaarsemaker
> <dennis@kaarsemaker.net> wrote:
> > On ma, 2013-12-02 at 07:38 +0700, Duy Nguyen wrote:
> >> On Mon, Dec 2, 2013 at 6:38 AM, Dennis Kaarsemaker
> >> <dennis@kaarsemaker.net> wrote:
> >> > On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:
> >> >> Duy Nguyen <pclouds@gmail.com> writes:
> >> >>
> >> >> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker
> >> >> > <dennis@kaarsemaker.net> wrote:
> >> >> >> We always ignore anything named .git, but we should also ignore the git
> >> >> >> directory if the user overrides it by setting $GIT_DIR
> >> >> [...]
> >> >> >> +       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?
> >> >>
> >> >> 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.
> >> >
> >> > --- a/dir.c
> >> > +++ b/dir.c
> >> > @@ -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.
> 
> ~/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.

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

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Duy Nguyen]

On Mon, Dec 2, 2013 at 6:40 PM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
>> ~/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.
>
> 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

It looks like we try to convert git_dir relative to work_tree (in
setup_work_tree) so get_git_dir() probably always returns a path
relative to worktree if it's set. I don't know, it looks like so.
-- 
Duy

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Karsten Blees]

Am 01.12.2013 20:04, schrieb 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
> 
> Reported-By: Ingy döt Net <ingy@ingy.net>
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
>  dir.c             | 2 +-
>  t/t7508-status.sh | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/dir.c b/dir.c
> index 23b6de4..884b37d 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,

The special case for ".git" is hardcoded in many places in git, including the line immediately above this diff hunk. 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 want to support the rename case fully, I think there are a few more questions to answer (and a few more places to change), e.g.:
- What if GIT_DIR=.foo and someone upstream adds a ".foo" directory?
- Should it be possible to track ".git" as a normal file or directory if its not the GIT_DIR?
- What about other commands than status, e.g. does 'git clean -df' leave the GIT_DIR alone?

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.

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Junio C Hamano]

Karsten Blees <karsten.blees@gmail.com> writes:

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

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

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Karsten Blees]

Am 03.12.2013 19:32, schrieb Junio C Hamano:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> 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.
> 
>> 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".
> 

In setup_work_tree(), the result of remove_leading_path(git_dir, work_tree) must be absolute or start with ".." or ".git", otherwise warn?

Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git

[Jonathan Nieder]

Junio C Hamano wrote:
> Karsten Blees <karsten.blees@gmail.com> writes:

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

I think the idea was that *any* path under $(git rev-parse
--show-toplevel) would not be a valid GIT_DIR, unless its last path
component is ".git".

Alas, I don't think that would work smoothly.

 - Some people may already be using GIT_DIR=$HOME/dotfiles.git to
   track some files with a toplevel of $HOME.  That is error-prone and
   it would be cleaner to either use plain .git or keep the $GIT_DIR
   outside the worktree (for example by tucking dotfiles into a
   separate $HOME/dotfiles dir), true, but producing a noisy warning
   with no way out would not serve these people well.

 - There is no outside-the-worktree location when GIT_WORK_TREE=/.

So your suggestion of at least noticing when "git checkout" wants to
write files that overlap with the GIT_DIR seems simpler.