On Mon, Dec 2, 2013 at 3:01 PM, Dennis Kaarsemaker <dennis@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote: >> > On ma, 2013-12-02 at 00:08 +0100, Thomas Rast wrote: >> >> Duy Nguyen <pclouds@xxxxxxxxx> writes: >> >> >> >> > On Mon, Dec 2, 2013 at 2:04 AM, Dennis Kaarsemaker >> >> > <dennis@xxxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html