Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
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 i...@ingy.net Signed-off-by: Dennis Kaarsemaker den...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
Karsten Blees karsten.bl...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
Am 03.12.2013 19:32, schrieb Junio C Hamano: Karsten Blees karsten.bl...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
Junio C Hamano wrote: Karsten Blees karsten.bl...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
On ma, 2013-12-02 at 07:38 +0700, Duy Nguyen wrote: On Mon, Dec 2, 2013 at 6:38 AM, Dennis Kaarsemaker den...@kaarsemaker.net wrote: On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote: Duy Nguyen pclo...@gmail.com writes: On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker den...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
On Mon, Dec 2, 2013 at 3:01 PM, Dennis Kaarsemaker den...@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 den...@kaarsemaker.net wrote: On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote: Duy Nguyen pclo...@gmail.com writes: On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker den...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
On ma, 2013-12-02 at 16:35 +0700, Duy Nguyen wrote: On Mon, Dec 2, 2013 at 3:01 PM, Dennis Kaarsemaker den...@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 den...@kaarsemaker.net wrote: On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote: Duy Nguyen pclo...@gmail.com writes: On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker den...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
On Mon, Dec 2, 2013 at 6:40 PM, Dennis Kaarsemaker den...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
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 i...@ingy.net
Signed-off-by: Dennis Kaarsemaker den...@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 den...@kaarsemaker.net
http://twitter.com/seveas
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker
den...@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 i...@ingy.net
Signed-off-by: Dennis Kaarsemaker den...@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 den...@kaarsemaker.net
http://twitter.com/seveas
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
Duy Nguyen pclo...@gmail.com writes: On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker den...@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 t...@thomasrast.ch -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote: Duy Nguyen pclo...@gmail.com writes: On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker den...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] path_treatment: also ignore $GIT_DIR if it's not .git
On Sun, Dec 1, 2013 at 2:04 PM, Dennis Kaarsemaker
den...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
