Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes
On Thu, May 29, 2014 at 4:33 AM, Pasha Bolokhov wrote: > > Agree, but "partial" here means... what? just a little doc? I'm sorry, I tried to be cute and failed :$ This is not official name for documentation files that are meant for inclusion and not as standalone manpages. I think they are similar to 'partial templates' http://guides.rubyonrails.org/layouts_and_rendering.html#using-partials - usually just called "partials" - hence my use of this name. Example: date-formats.txt included in git-commit(1), git-commit-tree(1) and git-tag(1). > On Wed, May 28, 2014 at 11:53 AM, Jakub Narębski wrote: >> W dniu 2014-05-27 19:16, Pasha Bolokhov pisze: [...] >>> I suggest this. There appears to be a notion of "standard >>> excludes" both in the code (dir.c) and in the man pages (git-grep, >>> git-ls-files). However, it doesn't actually appear to be defined >>> strictly speaking. So my suggestion is to define those "standard >>> excludes" in one place (say "gitignore(5)"), and make other man pages >>> (git-config, git-grip, git-ls-files) have references to that place >>> instead of explaining every time in detail what is being excluded. >> >> >> Or define those "standard excludes" in standard-excludes.txt "partial", >> and include said file from git-grep(1), git-ls-files(1), and perhaps >> gitignore(5). -- Jakub Narębski -- 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 v3] Add an explicit GIT_DIR to the list of excludes
Agree, but "partial" here means... what? just a little doc? On Wed, May 28, 2014 at 11:53 AM, Jakub Narębski wrote: > W dniu 2014-05-27 19:16, Pasha Bolokhov pisze: > Add GIT_DIR to the list of excludes in setup_standard_excludes(), while checking that GIT_DIR is not just '.git', in which case it would be ignored by default, and that GIT_DIR is inside GIT_WORK_TREE >>> gives git-grep.txt and git-ls-files.txt. I don't know if we need to >>> add something about this extra exclude rule to those .txt. If it's so >>> obvious that this should be the expected behavior, then probably not. >> >> >> I suggest this. There appears to be a notion of "standard >> excludes" both in the code (dir.c) and in the man pages (git-grep, >> git-ls-files). However, it doesn't actually appear to be defined >> strictly speaking. So my suggestion is to define those "standard >> excludes" in one place (say "gitignore(5)"), and make other man pages >> (git-config, git-grip, git-ls-files) have references to that place >> instead of explaining every time in detail what is being excluded. > > > Or define those "standard excludes" in standard-excludes.txt "partial", > and include said file from git-grep(1), git-ls-files(1), and perhaps > gitignore(5). > > -- > Jakub Narębski > > -- 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 v3] Add an explicit GIT_DIR to the list of excludes
W dniu 2014-05-27 19:16, Pasha Bolokhov pisze: Add GIT_DIR to the list of excludes in setup_standard_excludes(), while checking that GIT_DIR is not just '.git', in which case it would be ignored by default, and that GIT_DIR is inside GIT_WORK_TREE gives git-grep.txt and git-ls-files.txt. I don't know if we need to add something about this extra exclude rule to those .txt. If it's so obvious that this should be the expected behavior, then probably not. I suggest this. There appears to be a notion of "standard excludes" both in the code (dir.c) and in the man pages (git-grep, git-ls-files). However, it doesn't actually appear to be defined strictly speaking. So my suggestion is to define those "standard excludes" in one place (say "gitignore(5)"), and make other man pages (git-config, git-grip, git-ls-files) have references to that place instead of explaining every time in detail what is being excluded. Or define those "standard excludes" in standard-excludes.txt "partial", and include said file from git-grep(1), git-ls-files(1), and perhaps gitignore(5). -- Jakub Narębski -- 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 v3] Add an explicit GIT_DIR to the list of excludes
I've sent out version [PATCH v4] with most of the things addressed. That one hasn't been reviewed by anyone yet On Tue, May 27, 2014 at 11:04 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov >> wrote: >>> When an explicit '--git-dir' option points to a directory inside >>> the work tree, git treats it as if it were any other directory. >>> In particular, 'git status' lists it as untracked, while 'git add -A' >>> stages the metadata directory entirely >>> >>> Add GIT_DIR to the list of excludes in setup_standard_excludes(), >>> while checking that GIT_DIR is not just '.git', in which case it >>> would be ignored by default, and that GIT_DIR is inside GIT_WORK_TREE >>> >>> Although an analogous comparison of any given path against '.git' >>> is done in treat_path(), this does not seem to be the right place >>> to compare against GIT_DIR. Instead, the excludes provide an >>> effective mechanism of ignoring a file/directory, and adding GIT_DIR >>> as an exclude is equivalent of putting it into '.gitignore'. Function >>> setup_standard_excludes() was chosen because that is the place where >>> the excludes are initialized by the commands that are concerned about >>> excludes >> >> I like this approach. A search of "exclude-standard" in Documentation/ >> gives git-grep.txt and git-ls-files.txt. I don't know if we need to >> add something about this extra exclude rule to those .txt. If it's so >> obvious that this should be the expected behavior, then probably not. > > OK, so is that an Acked/Reviewed-by? > >> >> The case of "git grep --exclude-standard" is interesting because it's >> intended to work without a repository. First reaction was would >> get_git_dir() return NULL in that case. But it should return ".git" so >> we're good. -- 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 v3] Add an explicit GIT_DIR to the list of excludes
Duy Nguyen writes: > On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov > wrote: >> When an explicit '--git-dir' option points to a directory inside >> the work tree, git treats it as if it were any other directory. >> In particular, 'git status' lists it as untracked, while 'git add -A' >> stages the metadata directory entirely >> >> Add GIT_DIR to the list of excludes in setup_standard_excludes(), >> while checking that GIT_DIR is not just '.git', in which case it >> would be ignored by default, and that GIT_DIR is inside GIT_WORK_TREE >> >> Although an analogous comparison of any given path against '.git' >> is done in treat_path(), this does not seem to be the right place >> to compare against GIT_DIR. Instead, the excludes provide an >> effective mechanism of ignoring a file/directory, and adding GIT_DIR >> as an exclude is equivalent of putting it into '.gitignore'. Function >> setup_standard_excludes() was chosen because that is the place where >> the excludes are initialized by the commands that are concerned about >> excludes > > I like this approach. A search of "exclude-standard" in Documentation/ > gives git-grep.txt and git-ls-files.txt. I don't know if we need to > add something about this extra exclude rule to those .txt. If it's so > obvious that this should be the expected behavior, then probably not. OK, so is that an Acked/Reviewed-by? > > The case of "git grep --exclude-standard" is interesting because it's > intended to work without a repository. First reaction was would > get_git_dir() return NULL in that case. But it should return ".git" so > we're good. -- 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 v3] Add an explicit GIT_DIR to the list of excludes
>> Add GIT_DIR to the list of excludes in setup_standard_excludes(), >> while checking that GIT_DIR is not just '.git', in which case it >> would be ignored by default, and that GIT_DIR is inside GIT_WORK_TREE >> > gives git-grep.txt and git-ls-files.txt. I don't know if we need to > add something about this extra exclude rule to those .txt. If it's so > obvious that this should be the expected behavior, then probably not. I suggest this. There appears to be a notion of "standard excludes" both in the code (dir.c) and in the man pages (git-grep, git-ls-files). However, it doesn't actually appear to be defined strictly speaking. So my suggestion is to define those "standard excludes" in one place (say "gitignore(5)"), and make other man pages (git-config, git-grip, git-ls-files) have references to that place instead of explaining every time in detail what is being excluded. Now, gitignore(5) actually does have this list of ignored items, we only need to call it "standard excludes". If done this way, then all that needs to be done regarding GIT_DIR is to insert it into that list in gitignore(5). Please let me know if that'd work Pasha -- 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 v3] Add an explicit GIT_DIR to the list of excludes
On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov wrote: > When an explicit '--git-dir' option points to a directory inside > the work tree, git treats it as if it were any other directory. > In particular, 'git status' lists it as untracked, while 'git add -A' > stages the metadata directory entirely > > Add GIT_DIR to the list of excludes in setup_standard_excludes(), > while checking that GIT_DIR is not just '.git', in which case it > would be ignored by default, and that GIT_DIR is inside GIT_WORK_TREE > > Although an analogous comparison of any given path against '.git' > is done in treat_path(), this does not seem to be the right place > to compare against GIT_DIR. Instead, the excludes provide an > effective mechanism of ignoring a file/directory, and adding GIT_DIR > as an exclude is equivalent of putting it into '.gitignore'. Function > setup_standard_excludes() was chosen because that is the place where > the excludes are initialized by the commands that are concerned about > excludes I like this approach. A search of "exclude-standard" in Documentation/ gives git-grep.txt and git-ls-files.txt. I don't know if we need to add something about this extra exclude rule to those .txt. If it's so obvious that this should be the expected behavior, then probably not. The case of "git grep --exclude-standard" is interesting because it's intended to work without a repository. First reaction was would get_git_dir() return NULL in that case. But it should return ".git" so we're good. -- 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 v3] Add an explicit GIT_DIR to the list of excludes
Hi,
>
>> /* "--git-dir" has been given */
>
> ... or it could have come from GIT_DIR environment, no?
Yes, it does not matter where it came from, but I'll correct the comment
>
> Does this "additional exclude" need to kick in if GIT_DIR is set to
> "/home/pasha/w/.git"? That is, when gitdir is ".git" or ends with
> "/.git"?
I don't think it needs to kick in in either of these cases, as
".git" is already handled by "treat_path()". Now, here ".git" is
excluded by "if (strcmp()) {", while the first case needs to be
addressed too. Agree.
>> +#
>> +# Create a tree:
>> +#
>> +#a b c d subdir/
>> +#
>> +# subdir:
>> +#e f g h meta/ ssubdir/
>> +#
>> +# subdir/meta:
>> +#aa
>> +#
>> +# subdir/ssubdir:
>> +#meta/
>> +#
>> +# subdir/ssubdir/meta:
>> +#aaa
>> +#
> It is not quite clear with this large blob of comment what are
> noises and what exactly are being tested. I think you have two
> directories called "meta", but which one is the repository? Or do
> you have yet another one next to {a,b,c,d,subdir} called "meta" that
> is not listed above?
>
> Given that the reason why people use --git-dir is so that they can
> put it completely outside the working tree (in which case, the usual
> "start from cwd and go upwards while trying to see if there is .git/
> that governs the working tree" logic would not work), readers would
> not expect to find the directory to be used as GIT_DIR in the
> hierarchy you are creating in the first place. Because of that, it
> is even more important to clearify which "meta" you mean to use as
> your GIT_DIR if you want to be understood by readers.
I guess I was too simplistic, need to clarify a bit. And indeed,
perhaps two distinct subtrees are needed to test the repository that
is outside the work-tree, that would just do a slightly cleaner job
Pasha
--
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 v3] Add an explicit GIT_DIR to the list of excludes
Pasha Bolokhov writes:
> diff --git a/dir.c b/dir.c
> index 98bb50f..76969a7 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1588,6 +1588,26 @@ void setup_standard_excludes(struct dir_struct *dir)
> {
> const char *path;
> char *xdg_path;
> + const char *gitdir = get_git_dir();
> +
> + /* Add git directory to the ignores first */
> + if (strcmp(gitdir, ".git") != 0) {
It is more idiomatic and common to say
if (strcmp(gitdir, ".git")) {
around here, I think.
> /* "--git-dir" has been given */
... or it could have come from GIT_DIR environment, no?
Does this "additional exclude" need to kick in if GIT_DIR is set to
"/home/pasha/w/.git"? That is, when gitdir is ".git" or ends with
"/.git"?
> + char ngit[PATH_MAX + 1];
Hmmm. As I recall that recently we had flurry of changes to
eradicate PATH_MAX from our codebase, I am not happy to see an
introduction of a new buffer that is fixed-size.
> + /*
> + * See if GIT_DIR is inside the work tree; need to normalize
> + * 'gitdir', whereas 'get_git_work_tree()' always appears
> + * absolute and normalized
> + */
> + normalize_path_copy(ngit, real_path(absolute_path(gitdir)));
> +
> + if (dir_inside_of(ngit, get_git_work_tree()) >= 0) {
> + struct exclude_list *el = add_exclude_list(dir,
> EXC_CMDL,
> + "--git-dir option");
> +
> + add_exclude(gitdir, "", 0, el, 0);
> + }
> + }
>
> dir->exclude_per_dir = ".gitignore";
> path = git_path("info/exclude");
> diff --git a/t/t2205-add-gitdir.sh b/t/t2205-add-gitdir.sh
> new file mode 100755
> index 000..0c99508
> --- /dev/null
> +++ b/t/t2205-add-gitdir.sh
> @@ -0,0 +1,84 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2014 Pasha Bolokhov
> +#
> +
> +test_description='alternative repository path specified by --git-dir is
> ignored by add and status'
> +
> +. ./test-lib.sh
> +
> +#
> +# Create a tree:
> +#
> +#a b c d subdir/
> +#
> +# subdir:
> +#e f g h meta/ ssubdir/
> +#
> +# subdir/meta:
> +#aa
> +#
> +# subdir/ssubdir:
> +#meta/
> +#
> +# subdir/ssubdir/meta:
> +#aaa
> +#
> +# Name the repository "meta" and see whether or not "git status" includes
> +# or ignores directories named "meta". The slightly deeper hierarchy is
> +# needed in order to be able to put the repository into "../meta", that is,
> +# outside the work tree and still have files called "meta" within the tree
> +#
It is not quite clear with this large blob of comment what are
noises and what exactly are being tested. I think you have two
directories called "meta", but which one is the repository? Or do
you have yet another one next to {a,b,c,d,subdir} called "meta" that
is not listed above?
Given that the reason why people use --git-dir is so that they can
put it completely outside the working tree (in which case, the usual
"start from cwd and go upwards while trying to see if there is .git/
that governs the working tree" logic would not work), readers would
not expect to find the directory to be used as GIT_DIR in the
hierarchy you are creating in the first place. Because of that, it
is even more important to clearify which "meta" you mean to use as
your GIT_DIR if you want to be understood by readers.
--
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
