{"thread":{"id":"35445","subject":"GIT_DIR not auto ignored","startedAt":"2013-12-01T07:06:11Z","lastAt":"2013-12-03T19:07:15Z","messageCount":17,"participants":["Ingy dot Net","Dennis Kaarsemaker","Duy Nguyen","Thomas Rast","Eric Sunshine","Karsten Blees","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"231321","messageId":"CAHJtQJ77drefyhjrs_C8bEq14ZiSNf6Boztqx+JYx51dRtrd-w@mail.gmail.com","threadId":"35445","inReplyTo":null,"subject":"GIT_DIR not auto ignored","fromName":"Ingy dot Net","fromEmail":"ingy@ingy.net","sentAt":"2013-12-01T07:06:11Z","receivedAt":"2013-12-01T07:06:11Z","isPatch":false,"sender":{"key":"ingy@ingy.net","avatar":null},"body":"Greetings,\n\nI found this probable bug:\nhttps://gist.github.com/anonymous/01979fd9e6e285df41a2\n\nCheers, Ingy döt Net\n"},{"id":"231333","messageId":"1385921319.3240.3.camel@localhost","threadId":"35445","inReplyTo":"CAHJtQJ77drefyhjrs_C8bEq14ZiSNf6Boztqx+JYx51dRtrd-w@mail.gmail.com","subject":"Re: GIT_DIR not auto ignored","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-12-01T18:08:39Z","receivedAt":"2013-12-01T18:08:39Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On za, 2013-11-30 at 23:06 -0800, Ingy dot Net wrote:\n> Greetings,\n> \n> I found this probable bug:\n> https://gist.github.com/anonymous/01979fd9e6e285df41a2\n\nSummary:\n\n$ mv .git .foo\n$ export GIT_DIR=$PWD/.foo\n$ git status\n# On branch master\n#\n# Initial commit\n#\n# Untracked files:\n# .foo/\nnothing added to commit but untracked files present\n\n\nI checked with 1.8.5 and this still happens. And this also happens:\n\n$ mv .git .foo \n$ export GIT_DIR=.foo\ndennis@lightning:~/code/git$ touch .git\ndennis@lightning:~/code/git$ git status\nOn branch master\nUntracked files:\n  (use \"git add <file>...\" to include in what will be committed)\n\n\t.foo/\n\nnothing added to commit but untracked files present (use \"git add\" to\ntrack)\n\n(Note the absence of .git there)\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"231334","messageId":"1385922611.3240.6.camel@localhost","threadId":"35445","inReplyTo":"1385921319.3240.3.camel@localhost","subject":"Re: GIT_DIR not auto ignored","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-12-01T18:30:11Z","receivedAt":"2013-12-01T18:30:11Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On zo, 2013-12-01 at 19:08 +0100, Dennis Kaarsemaker wrote:\n> On za, 2013-11-30 at 23:06 -0800, Ingy dot Net wrote:\n> > Greetings,\n> > \n> > I found this probable bug:\n> > https://gist.github.com/anonymous/01979fd9e6e285df41a2\n> \n> Summary:\n> \n> $ mv .git .foo\n> $ export GIT_DIR=$PWD/.foo\n> $ git status\n> # On branch master\n> #\n> # Initial commit\n> #\n> # Untracked files:\n> # .foo/\n> nothing added to commit but untracked files present\n> \n> \n> I checked with 1.8.5 and this still happens. \n\nThis makes it go away:\n\ndiff --git a/dir.c b/dir.c\nindex 23b6de4..884b37d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n                return path_none;\n        strbuf_setlen(path, baselen);\n        strbuf_addstr(path, de->d_name);\n-       if (simplify_away(path->buf, path->len, simplify))\n+       if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))\n                return path_none;\n \n        dtype = DTYPE(de);\n\nI'll add a test and submit a proper patch.\n\n> And this also happens:\n> \n> $ mv .git .foo \n> $ export GIT_DIR=.foo\n> dennis@lightning:~/code/git$ touch .git\n> dennis@lightning:~/code/git$ git status\n> On branch master\n> Untracked files:\n>   (use \"git add <file>...\" to include in what will be committed)\n> \n> \t.foo/\n> \n> nothing added to commit but untracked files present (use \"git add\" to\n> track)\n> \n> (Note the absence of .git there)\n\nComments in dir.c indicate that this is expected, so I didn't try to\n\"fix\" that.\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"231335","messageId":"20131201190447.GA31367@kaarsemaker.net","threadId":"35445","inReplyTo":"1385922611.3240.6.camel@localhost","subject":"[PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-12-01T19:04:50Z","receivedAt":"2013-12-01T19:04:50Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"We always ignore anything named .git, but we should also ignore the git\ndirectory if the user overrides it by setting $GIT_DIR\n\nReported-By: Ingy döt Net <ingy@ingy.net>\nSigned-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n---\n dir.c             | 2 +-\n t/t7508-status.sh | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 23b6de4..884b37d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\treturn path_none;\n \tstrbuf_setlen(path, baselen);\n \tstrbuf_addstr(path, de->d_name);\n-\tif (simplify_away(path->buf, path->len, simplify))\n+\tif (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))\n \t\treturn path_none;\n \n \tdtype = DTYPE(de);\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex c987b5e..2bd7ef1 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -198,6 +198,13 @@ test_expect_success 'status -s' '\n \n '\n \n+test_expect_success 'status -s with non-standard $GIT_DIR' '\n+\tmv .git .foo &&\n+\tGIT_DIR=.foo git status -s >output &&\n+\ttest_cmp expect output &&\n+\tmv .foo .git\n+'\n+\n test_expect_success 'status with gitignore' '\n \t{\n \t\techo \".gitignore\" &&\n-- \n1.8.5-386-gb78cb96\n\n\n-- \nDennis Kaarsemaker <dennis@kaarsemaker.net>\nhttp://twitter.com/seveas\n"},{"id":"231343","messageId":"CACsJy8CxR+wj-P+fxF37DU=Tzk=su+V=UudbO7NkqTMS8qTn_w@mail.gmail.com","threadId":"35445","inReplyTo":"20131201190447.GA31367@kaarsemaker.net","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-01T23:02:32Z","receivedAt":"2013-12-01T23:02:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker\n<dennis@kaarsemaker.net> wrote:\n> We always ignore anything named .git, but we should also ignore the git\n> directory if the user overrides it by setting $GIT_DIR\n>\n> Reported-By: Ingy döt Net <ingy@ingy.net>\n> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n> ---\n>  dir.c             | 2 +-\n>  t/t7508-status.sh | 7 +++++++\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 23b6de4..884b37d 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n>                 return path_none;\n>         strbuf_setlen(path, baselen);\n>         strbuf_addstr(path, de->d_name);\n> -       if (simplify_away(path->buf, path->len, simplify))\n> +       if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))\n>                 return path_none;\n\nthis adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path\nwe check. Is it worth the cost?\n\n\n>\n>         dtype = DTYPE(de);\n> diff --git a/t/t7508-status.sh b/t/t7508-status.sh\n> index c987b5e..2bd7ef1 100755\n> --- a/t/t7508-status.sh\n> +++ b/t/t7508-status.sh\n> @@ -198,6 +198,13 @@ test_expect_success 'status -s' '\n>\n>  '\n>\n> +test_expect_success 'status -s with non-standard $GIT_DIR' '\n> +       mv .git .foo &&\n> +       GIT_DIR=.foo git status -s >output &&\n> +       test_cmp expect output &&\n> +       mv .foo .git\n> +'\n> +\n>  test_expect_success 'status with gitignore' '\n>         {\n>                 echo \".gitignore\" &&\n> --\n> 1.8.5-386-gb78cb96\n>\n>\n> --\n> Dennis Kaarsemaker <dennis@kaarsemaker.net>\n> http://twitter.com/seveas\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nDuy\n"},{"id":"231344","messageId":"877gbop3so.fsf@linux-1gf2.Speedport_W723_V_Typ_A_1_00_098","threadId":"35445","inReplyTo":"CACsJy8CxR+wj-P+fxF37DU=Tzk=su+V=UudbO7NkqTMS8qTn_w@mail.gmail.com","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-12-01T23:08:55Z","receivedAt":"2013-12-01T23:08:55Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker\n> <dennis@kaarsemaker.net> wrote:\n>> We always ignore anything named .git, but we should also ignore the git\n>> directory if the user overrides it by setting $GIT_DIR\n[...]\n>> +       if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))\n>>                 return path_none;\n>\n> this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path\n> we check. Is it worth the cost?\n\nMoreover it is a much more inclusive check than what the commit message\nclaims: it will ignore anything that looks like a .git directory,\nregardless of the name.  In particular GIT_DIR doesn't have anything to\ndo with it.\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"231345","messageId":"1385941093.3240.10.camel@localhost","threadId":"35445","inReplyTo":"877gbop3so.fsf@linux-1gf2.Speedport_W723_V_Typ_A_1_00_098","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-12-01T23:38:13Z","receivedAt":"2013-12-01T23:38:13Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n> \n> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker\n> > <dennis@kaarsemaker.net> wrote:\n> >> We always ignore anything named .git, but we should also ignore the git\n> >> directory if the user overrides it by setting $GIT_DIR\n> [...]\n> >> +       if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))\n> >>                 return path_none;\n> >\n> > this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path\n> > we check. Is it worth the cost?\n> \n> Moreover it is a much more inclusive check than what the commit message\n> claims: it will ignore anything that looks like a .git directory,\n> regardless of the name.  In particular GIT_DIR doesn't have anything to\n> do with it.\n\nAh, yes thanks, that's rather incorrect indeed. How about the following\ninstead? Passes all tests, including the new one.\n\n--- a/dir.c\n+++ b/dir.c\n@@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n                return path_none;\n        strbuf_setlen(path, baselen);\n        strbuf_addstr(path, de->d_name);\n-       if (simplify_away(path->buf, path->len, simplify))\n+       if (simplify_away(path->buf, path->len, simplify) || !strncmp(get_git_dir(), path->buf, path->len))\n                return path_none;\n \n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"231348","messageId":"CACsJy8CSQ2RfZub6As9TJc2Vd-wmp75ZVnjQ4nr1QY4mZ4f_TA@mail.gmail.com","threadId":"35445","inReplyTo":"1385941093.3240.10.camel@localhost","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-02T00:38:20Z","receivedAt":"2013-12-02T00:38:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 2, 2013 at 6:38 AM, Dennis Kaarsemaker\n<dennis@kaarsemaker.net> wrote:\n> On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:\n>> Duy Nguyen <pclouds@gmail.com> writes:\n>>\n>> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker\n>> > <dennis@kaarsemaker.net> wrote:\n>> >> We always ignore anything named .git, but we should also ignore the git\n>> >> directory if the user overrides it by setting $GIT_DIR\n>> [...]\n>> >> +       if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))\n>> >>                 return path_none;\n>> >\n>> > this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path\n>> > we check. Is it worth the cost?\n>>\n>> Moreover it is a much more inclusive check than what the commit message\n>> claims: it will ignore anything that looks like a .git directory,\n>> regardless of the name.  In particular GIT_DIR doesn't have anything to\n>> do with it.\n>\n> Ah, yes thanks, that's rather incorrect indeed. How about the following\n> instead? Passes all tests, including the new one.\n>\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n>                 return path_none;\n>         strbuf_setlen(path, baselen);\n>         strbuf_addstr(path, de->d_name);\n> -       if (simplify_away(path->buf, path->len, simplify))\n> +       if (simplify_away(path->buf, path->len, simplify) || !strncmp(get_git_dir(), path->buf, path->len))\n>                 return path_none;\n\nget_git_dir() may return a relative or absolute path, depending on\nGIT_DIR/GIT_WORK_TREE. path->buf is always relative. You'll pass one\ncase with this (relative vs relative) and fail another. It might be\nsimpler to just add get_git_dir(), after converting to relative path\nand check if it's in worktree, to the exclude list and let the current\nexclude mechanism handle it.\n-- \nDuy\n"},{"id":"231349","messageId":"CAPig+cRVYLsVrmFzfKHE8VdYo90enuOpsir2HT8YueQsWs58ng@mail.gmail.com","threadId":"35445","inReplyTo":"20131201190447.GA31367@kaarsemaker.net","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-12-02T01:21:53Z","receivedAt":"2013-12-02T01:21:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Dec 1, 2013 at 2:04 PM, Dennis Kaarsemaker\n<dennis@kaarsemaker.net> wrote:\n> diff --git a/t/t7508-status.sh b/t/t7508-status.sh\n> index c987b5e..2bd7ef1 100755\n> --- a/t/t7508-status.sh\n> +++ b/t/t7508-status.sh\n> @@ -198,6 +198,13 @@ test_expect_success 'status -s' '\n>\n>  '\n>\n> +test_expect_success 'status -s with non-standard $GIT_DIR' '\n> +       mv .git .foo &&\n> +       GIT_DIR=.foo git status -s >output &&\n> +       test_cmp expect output &&\n> +       mv .foo .git\n\nIf test_cmp returns a failure status, the following 'mv' command will\nnever be invoked, thus all subsequent tests in the script will fail\nsince the .git directory will be missing. Instead, use\ntest_when_finished to restore .git from .foo.\n\n> +'\n> +\n>  test_expect_success 'status with gitignore' '\n>         {\n>                 echo \".gitignore\" &&\n> --\n> 1.8.5-386-gb78cb96\n"},{"id":"231354","messageId":"1385971274.3240.14.camel@localhost","threadId":"35445","inReplyTo":"CACsJy8CSQ2RfZub6As9TJc2Vd-wmp75ZVnjQ4nr1QY4mZ4f_TA@mail.gmail.com","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-12-02T08:01:14Z","receivedAt":"2013-12-02T08:01:14Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On ma, 2013-12-02 at 07:38 +0700, Duy Nguyen wrote:\n> On Mon, Dec 2, 2013 at 6:38 AM, Dennis Kaarsemaker\n> <dennis@kaarsemaker.net> wrote:\n> > On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:\n> >> Duy Nguyen <pclouds@gmail.com> writes:\n> >>\n> >> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker\n> >> > <dennis@kaarsemaker.net> wrote:\n> >> >> We always ignore anything named .git, but we should also ignore the git\n> >> >> directory if the user overrides it by setting $GIT_DIR\n> >> [...]\n> >> >> +       if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))\n> >> >>                 return path_none;\n> >> >\n> >> > this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path\n> >> > we check. Is it worth the cost?\n> >>\n> >> Moreover it is a much more inclusive check than what the commit message\n> >> claims: it will ignore anything that looks like a .git directory,\n> >> regardless of the name.  In particular GIT_DIR doesn't have anything to\n> >> do with it.\n> >\n> > Ah, yes thanks, that's rather incorrect indeed. How about the following\n> > instead? Passes all tests, including the new one.\n> >\n> > --- a/dir.c\n> > +++ b/dir.c\n> > @@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n> >                 return path_none;\n> >         strbuf_setlen(path, baselen);\n> >         strbuf_addstr(path, de->d_name);\n> > -       if (simplify_away(path->buf, path->len, simplify))\n> > +       if (simplify_away(path->buf, path->len, simplify) || !strncmp(get_git_dir(), path->buf, path->len))\n> >                 return path_none;\n> \n> get_git_dir() may return a relative or absolute path, depending on\n> GIT_DIR/GIT_WORK_TREE. path->buf is always relative. You'll pass one\n> case with this (relative vs relative) and fail another. It might be\n> simpler to just add get_git_dir(), after converting to relative path\n> and check if it's in worktree, to the exclude list and let the current\n> exclude mechanism handle it.\n\nThis type of invocation really only works from the root of the workdir\nanyway and both a relative and absolute path work just fine:\n\ndennis@lightning:~/code/git$ GIT_DIR=$(pwd)/.foo ./git status\nOn branch master\nnothing to commit, working directory clean\ndennis@lightning:~/code/git$ GIT_DIR=./.foo ./git status\nOn branch master\nnothing to commit, working directory clean\n\nWell, unless you set GIT_WORK_TREE as well, but then it still works:\n\ndennis@lightning:~/code/git/t$ GIT_DIR=$(pwd)/../.foo GIT_WORK_TREE=.. ../git status\nOn branch master\nnothing to commit, working directory clean\ndennis@lightning:~/code/git/t$ GIT_DIR=../.foo GIT_WORK_TREE=.. ../git status\nOn branch master\nnothing to commit, working directory clean\n\nSo I'm wondering when you think this will fail. Because then I can add a\ntest for that case too.\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"231357","messageId":"CACsJy8AuSej7Pwm5Tbo5r_FNaND1-E62Btk=7dZ74YD8K6UJDg@mail.gmail.com","threadId":"35445","inReplyTo":"1385971274.3240.14.camel@localhost","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-02T09:35:33Z","receivedAt":"2013-12-02T09:35:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 2, 2013 at 3:01 PM, Dennis Kaarsemaker\n<dennis@kaarsemaker.net> wrote:\n> On ma, 2013-12-02 at 07:38 +0700, Duy Nguyen wrote:\n>> On Mon, Dec 2, 2013 at 6:38 AM, Dennis Kaarsemaker\n>> <dennis@kaarsemaker.net> wrote:\n>> > On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:\n>> >> Duy Nguyen <pclouds@gmail.com> writes:\n>> >>\n>> >> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker\n>> >> > <dennis@kaarsemaker.net> wrote:\n>> >> >> We always ignore anything named .git, but we should also ignore the git\n>> >> >> directory if the user overrides it by setting $GIT_DIR\n>> >> [...]\n>> >> >> +       if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))\n>> >> >>                 return path_none;\n>> >> >\n>> >> > this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path\n>> >> > we check. Is it worth the cost?\n>> >>\n>> >> Moreover it is a much more inclusive check than what the commit message\n>> >> claims: it will ignore anything that looks like a .git directory,\n>> >> regardless of the name.  In particular GIT_DIR doesn't have anything to\n>> >> do with it.\n>> >\n>> > Ah, yes thanks, that's rather incorrect indeed. How about the following\n>> > instead? Passes all tests, including the new one.\n>> >\n>> > --- a/dir.c\n>> > +++ b/dir.c\n>> > @@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n>> >                 return path_none;\n>> >         strbuf_setlen(path, baselen);\n>> >         strbuf_addstr(path, de->d_name);\n>> > -       if (simplify_away(path->buf, path->len, simplify))\n>> > +       if (simplify_away(path->buf, path->len, simplify) || !strncmp(get_git_dir(), path->buf, path->len))\n>> >                 return path_none;\n>>\n>> get_git_dir() may return a relative or absolute path, depending on\n>> GIT_DIR/GIT_WORK_TREE. path->buf is always relative. You'll pass one\n>> case with this (relative vs relative) and fail another. It might be\n>> simpler to just add get_git_dir(), after converting to relative path\n>> and check if it's in worktree, to the exclude list and let the current\n>> exclude mechanism handle it.\n>\n> This type of invocation really only works from the root of the workdir\n> anyway and both a relative and absolute path work just fine:\n>\n> dennis@lightning:~/code/git$ GIT_DIR=$(pwd)/.foo ./git status\n> On branch master\n> nothing to commit, working directory clean\n> dennis@lightning:~/code/git$ GIT_DIR=./.foo ./git status\n> On branch master\n> nothing to commit, working directory clean\n>\n> Well, unless you set GIT_WORK_TREE as well, but then it still works:\n>\n> dennis@lightning:~/code/git/t$ GIT_DIR=$(pwd)/../.foo GIT_WORK_TREE=.. ../git status\n> On branch master\n> nothing to commit, working directory clean\n> dennis@lightning:~/code/git/t$ GIT_DIR=../.foo GIT_WORK_TREE=.. ../git status\n> On branch master\n> nothing to commit, working directory clean\n>\n> So I'm wondering when you think this will fail. Because then I can add a\n> test for that case too.\n\n~/w/git $ cd t\n~/w/git/t $ GIT_TRACE_SETUP=1 ../git --git-dir=../.git --work-tree=..\n--no-pager status\nsetup: git_dir: /home/pclouds/w/git/.git\nsetup: worktree: /home/pclouds/w/git\nsetup: cwd: /home/pclouds/w/git\nsetup: prefix: t/\nOn branch exclude-pathspec\nYour branch and 'origin/master' have diverged,\nand have 2 and 5 different commits each, respectively.\n\nI can't say this is the only case though. One has to audit to all\npossible setup cases in setup_git_directory() to make that claim.\n-- \nDuy\n"},{"id":"231363","messageId":"1385984412.3240.17.camel@localhost","threadId":"35445","inReplyTo":"CACsJy8AuSej7Pwm5Tbo5r_FNaND1-E62Btk=7dZ74YD8K6UJDg@mail.gmail.com","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-12-02T11:40:12Z","receivedAt":"2013-12-02T11:40:12Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On ma, 2013-12-02 at 16:35 +0700, Duy Nguyen wrote:\n> On Mon, Dec 2, 2013 at 3:01 PM, Dennis Kaarsemaker\n> <dennis@kaarsemaker.net> wrote:\n> > On ma, 2013-12-02 at 07:38 +0700, Duy Nguyen wrote:\n> >> On Mon, Dec 2, 2013 at 6:38 AM, Dennis Kaarsemaker\n> >> <dennis@kaarsemaker.net> wrote:\n> >> > On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote:\n> >> >> Duy Nguyen <pclouds@gmail.com> writes:\n> >> >>\n> >> >> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker\n> >> >> > <dennis@kaarsemaker.net> wrote:\n> >> >> >> We always ignore anything named .git, but we should also ignore the git\n> >> >> >> directory if the user overrides it by setting $GIT_DIR\n> >> >> [...]\n> >> >> >> +       if (simplify_away(path->buf, path->len, simplify) || is_git_directory(path->buf))\n> >> >> >>                 return path_none;\n> >> >> >\n> >> >> > this adds 2 access, 1 lstat, 1 open, 1 read, 1 close to _every_ path\n> >> >> > we check. Is it worth the cost?\n> >> >>\n> >> >> Moreover it is a much more inclusive check than what the commit message\n> >> >> claims: it will ignore anything that looks like a .git directory,\n> >> >> regardless of the name.  In particular GIT_DIR doesn't have anything to\n> >> >> do with it.\n> >> >\n> >> > Ah, yes thanks, that's rather incorrect indeed. How about the following\n> >> > instead? Passes all tests, including the new one.\n> >> >\n> >> > --- a/dir.c\n> >> > +++ b/dir.c\n> >> > @@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n> >> >                 return path_none;\n> >> >         strbuf_setlen(path, baselen);\n> >> >         strbuf_addstr(path, de->d_name);\n> >> > -       if (simplify_away(path->buf, path->len, simplify))\n> >> > +       if (simplify_away(path->buf, path->len, simplify) || !strncmp(get_git_dir(), path->buf, path->len))\n> >> >                 return path_none;\n> >>\n> >> get_git_dir() may return a relative or absolute path, depending on\n> >> GIT_DIR/GIT_WORK_TREE. path->buf is always relative. You'll pass one\n> >> case with this (relative vs relative) and fail another. It might be\n> >> simpler to just add get_git_dir(), after converting to relative path\n> >> and check if it's in worktree, to the exclude list and let the current\n> >> exclude mechanism handle it.\n> >\n> > This type of invocation really only works from the root of the workdir\n> > anyway and both a relative and absolute path work just fine:\n> >\n> > dennis@lightning:~/code/git$ GIT_DIR=$(pwd)/.foo ./git status\n> > On branch master\n> > nothing to commit, working directory clean\n> > dennis@lightning:~/code/git$ GIT_DIR=./.foo ./git status\n> > On branch master\n> > nothing to commit, working directory clean\n> >\n> > Well, unless you set GIT_WORK_TREE as well, but then it still works:\n> >\n> > dennis@lightning:~/code/git/t$ GIT_DIR=$(pwd)/../.foo GIT_WORK_TREE=.. ../git status\n> > On branch master\n> > nothing to commit, working directory clean\n> > dennis@lightning:~/code/git/t$ GIT_DIR=../.foo GIT_WORK_TREE=.. ../git status\n> > On branch master\n> > nothing to commit, working directory clean\n> >\n> > So I'm wondering when you think this will fail. Because then I can add a\n> > test for that case too.\n> \n> ~/w/git $ cd t\n> ~/w/git/t $ GIT_TRACE_SETUP=1 ../git --git-dir=../.git --work-tree=..\n> --no-pager status\n> setup: git_dir: /home/pclouds/w/git/.git\n> setup: worktree: /home/pclouds/w/git\n> setup: cwd: /home/pclouds/w/git\n> setup: prefix: t/\n> On branch exclude-pathspec\n> Your branch and 'origin/master' have diverged,\n> and have 2 and 5 different commits each, respectively.\n> \n> I can't say this is the only case though. One has to audit to all\n> possible setup cases in setup_git_directory() to make that claim.\n\nI'm probably missing something, but that's the same as my second\nexample, and works. I also tried running it from completely outside the\nrepo:\n\ndennis@lightning:~$ code/git/git --git-dir=code/git/.foo --work-tree=code/git status\nOn branch master\nnothing to commit, working directory clean\ndennis@lightning:~$ code/git/git --git-dir=/home/dennis/code/git/.foo --work-tree=code/git status\nOn branch master\nnothing to commit, working directory clean\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"231364","messageId":"CACsJy8Afhr1syRW8UetKKHss2r9e2pN2dCmc99Aj9418SxAUMg@mail.gmail.com","threadId":"35445","inReplyTo":"1385984412.3240.17.camel@localhost","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-02T12:01:13Z","receivedAt":"2013-12-02T12:01:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 2, 2013 at 6:40 PM, Dennis Kaarsemaker\n<dennis@kaarsemaker.net> wrote:\n>> ~/w/git $ cd t\n>> ~/w/git/t $ GIT_TRACE_SETUP=1 ../git --git-dir=../.git --work-tree=..\n>> --no-pager status\n>> setup: git_dir: /home/pclouds/w/git/.git\n>> setup: worktree: /home/pclouds/w/git\n>> setup: cwd: /home/pclouds/w/git\n>> setup: prefix: t/\n>> On branch exclude-pathspec\n>> Your branch and 'origin/master' have diverged,\n>> and have 2 and 5 different commits each, respectively.\n>>\n>> I can't say this is the only case though. One has to audit to all\n>> possible setup cases in setup_git_directory() to make that claim.\n>\n> I'm probably missing something, but that's the same as my second\n> example, and works. I also tried running it from completely outside the\n> repo:\n>\n> dennis@lightning:~$ code/git/git --git-dir=code/git/.foo --work-tree=code/git status\n> On branch master\n> nothing to commit, working directory clean\n> dennis@lightning:~$ code/git/git --git-dir=/home/dennis/code/git/.foo --work-tree=code/git status\n> On branch master\n> nothing to commit, working directory clean\n\nIt looks like we try to convert git_dir relative to work_tree (in\nsetup_work_tree) so get_git_dir() probably always returns a path\nrelative to worktree if it's set. I don't know, it looks like so.\n-- \nDuy\n"},{"id":"231434","messageId":"529DF64A.70801@gmail.com","threadId":"35445","inReplyTo":"20131201190447.GA31367@kaarsemaker.net","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2013-12-03T15:18:34Z","receivedAt":"2013-12-03T15:18:34Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 01.12.2013 20:04, schrieb Dennis Kaarsemaker:\n> We always ignore anything named .git, but we should also ignore the git\n> directory if the user overrides it by setting $GIT_DIR\n> \n> Reported-By: Ingy döt Net <ingy@ingy.net>\n> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n> ---\n>  dir.c             | 2 +-\n>  t/t7508-status.sh | 7 +++++++\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git a/dir.c b/dir.c\n> index 23b6de4..884b37d 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1198,7 +1198,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n\nThe 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).\n\nIf 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.:\n- What if GIT_DIR=.foo and someone upstream adds a \".foo\" directory?\n- Should it be possible to track \".git\" as a normal file or directory if its not the GIT_DIR?\n- What about other commands than status, e.g. does 'git clean -df' leave the GIT_DIR alone?\n\nIf 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.\n"},{"id":"231444","messageId":"xmqqtxepokej.fsf@gitster.dls.corp.google.com","threadId":"35445","inReplyTo":"529DF64A.70801@gmail.com","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-03T18:32:20Z","receivedAt":"2013-12-03T18:32:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karsten Blees <karsten.blees@gmail.com> writes:\n\n>So I figure that GIT_DIR is not meant to _rename_ the \".git\" dir,\n>but to point somewhere _outside_ the worktree (or somewhere within\n>the .git dir).\n\nCorrect.\n\n> If we don't want to support this, though, I think it would be more\n> approrpiate to issue a warning if GIT_DIR points to a worktree\n> location.\n\nBut how do tell what is and isn't a \"worktree location\"?  Having the\npath in the index would be one, but you may find it out only after\nissuing \"git checkout $antient_commit\".\n"},{"id":"231450","messageId":"529E2A33.7000804@gmail.com","threadId":"35445","inReplyTo":"xmqqtxepokej.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2013-12-03T19:00:03Z","receivedAt":"2013-12-03T19:00:03Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 03.12.2013 19:32, schrieb Junio C Hamano:\n> Karsten Blees <karsten.blees@gmail.com> writes:\n> \n>> So I figure that GIT_DIR is not meant to _rename_ the \".git\" dir,\n>> but to point somewhere _outside_ the worktree (or somewhere within\n>> the .git dir).\n> \n> Correct.\n> \n>> If we don't want to support this, though, I think it would be more\n>> approrpiate to issue a warning if GIT_DIR points to a worktree\n>> location.\n> \n> But how do tell what is and isn't a \"worktree location\"?  Having the\n> path in the index would be one, but you may find it out only after\n> issuing \"git checkout $antient_commit\".\n> \n\nIn setup_work_tree(), the result of remove_leading_path(git_dir, work_tree) must be absolute or start with \"..\" or \".git\", otherwise warn?\n"},{"id":"231452","messageId":"20131203190715.GC29959@google.com","threadId":"35445","inReplyTo":"xmqqtxepokej.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-03T19:07:15Z","receivedAt":"2013-12-03T19:07:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Karsten Blees <karsten.blees@gmail.com> writes:\n\n>> If we don't want to support this, though, I think it would be more\n>> approrpiate to issue a warning if GIT_DIR points to a worktree\n>> location.\n>\n> But how do tell what is and isn't a \"worktree location\"?  Having the\n> path in the index would be one, but you may find it out only after\n> issuing \"git checkout $antient_commit\".\n\nI think the idea was that *any* path under $(git rev-parse\n--show-toplevel) would not be a valid GIT_DIR, unless its last path\ncomponent is \".git\".\n\nAlas, I don't think that would work smoothly.\n\n - Some people may already be using GIT_DIR=$HOME/dotfiles.git to\n   track some files with a toplevel of $HOME.  That is error-prone and\n   it would be cleaner to either use plain .git or keep the $GIT_DIR\n   outside the worktree (for example by tucking dotfiles into a\n   separate $HOME/dotfiles dir), true, but producing a noisy warning\n   with no way out would not serve these people well.\n\n - There is no outside-the-worktree location when GIT_WORK_TREE=/.\n\nSo your suggestion of at least noticing when \"git checkout\" wants to\nwrite files that overlap with the GIT_DIR seems simpler.\n"}]}