[PATCH v3] Add an explicit GIT_DIR to the list of excludes

[PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Pasha Bolokhov]

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

Signed-off-by: Pasha Bolokhov <pasha.bolokhov@gmail.com>
---
Slightly brush-up comments, and fix ">file" style in the test, together
with fixing the &&-chain.
One thing is, when you do "git --git-dir meta init", and then explicitly
say "add meta/", the message you get is
"The following paths are ignored by one of your .gitignore files:"
which is not exactly correct.

 Documentation/technical/api-directory-listing.txt |  4 +-
 dir.c                                             | 20 ++++++
 t/t2205-add-gitdir.sh                             | 84 +++++++++++++++++++++++
 3 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100755 t/t2205-add-gitdir.sh

diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
index 7f8e78d..fd4a178 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -90,8 +90,8 @@ marked. If you to exclude files, make sure you have loaded index first.
   `add_exclude()`.
 
 * To add patterns from a file (e.g. `.git/info/exclude`), call
-  `add_excludes_from_file()` , and/or set `dir.exclude_per_dir`.  A
-  short-hand function `setup_standard_excludes()` can be used to set
+  `add_excludes_from_file()` , and/or set `dir.exclude_per_dir`.  The
+  short-hand function `setup_standard_excludes()` must be used to set
   up the standard set of exclude settings.
 
 * Set options described in the Data Structure section above.
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) { /* "--git-dir" has been given */
+		char ngit[PATH_MAX + 1];
+
+		/*
+		 * 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 0000000..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
+#
+
+test_expect_success "setup" '
+	git --git-dir=meta init &&
+	for f in a b c d
+	do
+		echo "DATA" >"$f" || exit 1
+	done &&
+	mkdir subdir &&
+	for f in e f g h
+	do
+		echo "MORE DATA" >"subdir/$f" || exit 1
+	done &&
+	mkdir subdir/meta &&
+	echo "EVEN more Data" >subdir/meta/aa &&
+	mkdir subdir/ssubdir subdir/ssubdir/meta &&
+	echo "So much more data" >subdir/ssubdir/meta/aaa
+'
+
+test_expect_success "'git status' ignores the repository directory" '
+	git --git-dir=meta --work-tree=. status --porcelain >status.out &&
+	test_might_fail grep meta status.out >out &&
+	! test -s out
+'
+
+test_expect_success "'git add -A' ignores the repository directory" '
+	git --git-dir=meta --work-tree=. add -A &&
+	git --git-dir=meta --work-tree=. status --porcelain >status1.out &&
+	test_might_fail grep meta status1.out >out1 &&
+	! test -s out1
+'
+
+test_expect_success "'git status' acknowledges files 'meta' if repository is not within work tree" '
+	test_might_fail rm -rf meta/ &&
+	(
+		cd subdir &&
+		git --git-dir=../meta init &&
+		git --git-dir=../meta --work-tree=. status --porcelain >status2.out &&
+		test_might_fail grep meta status2.out >out2 &&
+		test -s out2
+	)
+'
+
+test_expect_success "'git add -A' adds 'meta' if the repository is outside the work tree" '
+	(
+		cd subdir &&
+		git --git-dir=../meta --work-tree=. add -A &&
+		git --git-dir=../meta --work-tree=. status --porcelain >status3.out &&
+		test_might_fail grep meta status3.out >out3 &&
+		test -s out3
+	)
+'
+
+test_done
-- 
1.9.1

Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Junio C Hamano]

Pasha Bolokhov <pasha.bolokhov@gmail.com> 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 0000000..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.

Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Pasha Bolokhov]

       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

Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Duy Nguyen]

On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov
<pasha.bolokhov@gmail.com> 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

Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Pasha Bolokhov]

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

Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov
> <pasha.bolokhov@gmail.com> 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.

Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Pasha Bolokhov]

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 <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov
>> <pasha.bolokhov@gmail.com> 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.

Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Jakub Narębski]

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

Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Pasha Bolokhov]

Agree, but "partial" here means... what? just a little doc?


On Wed, May 28, 2014 at 11:53 AM, Jakub Narębski <jnareb@gmail.com> 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
>
>

Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

[Jakub Narębski]

On Thu, May 29, 2014 at 4:33 AM, Pasha Bolokhov
<pasha.bolokhov@gmail.com> 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 <jnareb@gmail.com> 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