Bug: git ls-files and ignored directories

Bug: git ls-files and ignored directories

[Christoph Buchner]

Hi!

I think I've been bitten by a bug in git ls-files - committed directories (and files therein) which are matched by a "dir/" gitignore rule are not shown by "git ls-files --exclude-standard -i", although they should be. Matched files are shown.

I've found a very brief discussion on this mailing list from March 2011 about this, including a repro procedure which still is valid today, at http://marc.info/?l=git&m=129926031808376&w=4

Is that a known/in progress bug? If yes, (when) can a fix be expected? How can I work around this in the meantime? 
If it's not a bug, how do you do this properly? My original problem was that  I want to list files which have been committed to git but match an existing gitignore pattern. Richard Hansen has written a very nice alias for that (http://stackoverflow.com/a/9370094/599884, with some discussion of this bug in the comments), but due to the present bug this does not work totally correctly. 

Having a built-in git feature for this would be great, btw ("git ls-files --committed-but-ignored"? :-)). Do I have to submit a separate feature request?

thanks, 
Christoph

p.s.: The repro procedure from the linked list discussion:
> $ mkdir test
> $ cd test
> $ git init
> $ mkdir testdir
> $ touch testdir/test
> $ git add testdir/test
> $ git commit -m "add test"
> $ echo "testdir/" > .gitignore
> $ git ls-files --exclude-standard -i

Bug: git ls-files and ignored directories

[Christoph Buchner]

This is the right place to report bugs, right?
Is there any protocol to follow I'm not aware of, to enable you to process this bug report?

greets,
Christoph

----- Original Message -----
From: bilderbuchi@phononoia.at
To: git@vger.kernel.org
Date: 30.05.2012 12:22:18
Subject: Bug: git ls-files and ignored directories


> Hi!
> 
> I think I've been bitten by a bug in git ls-files - committed directories (and files therein) which are matched by a "dir/" gitignore rule are not shown by "git ls-files --exclude-standard -i", although they should be. Matched files are shown.
> 
> I've found a very brief discussion on this mailing list from March 2011 about this, including a repro procedure which still is valid today, at http://marc.info/?l=git&m=129926031808376&w=4
> 
> Is that a known/in progress bug? If yes, (when) can a fix be expected? How can I work around this in the meantime? 
> If it's not a bug, how do you do this properly? My original problem was that  I want to list files which have been committed to git but match an existing gitignore pattern. Richard Hansen has written a very nice alias for that (http://stackoverflow.com/a/9370094/599884, with some discussion of this bug in the comments), but due to the present bug this does not work totally correctly. 
> 
> Having a built-in git feature for this would be great, btw ("git ls-files --committed-but-ignored"? :-)). Do I have to submit a separate feature request?
> 
> thanks, 
> Christoph
> 
> p.s.: The repro procedure from the linked list discussion:
>> $ mkdir test
>> $ cd test
>> $ git init
>> $ mkdir testdir
>> $ touch testdir/test
>> $ git add testdir/test
>> $ git commit -m "add test"
>> $ echo "testdir/" > .gitignore
>> $ git ls-files --exclude-standard -i
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: git ls-files and ignored directories

[Jeff King]

On Thu, May 31, 2012 at 12:14:51PM +0200, Christoph Buchner wrote:

> This is the right place to report bugs, right?

Yes.

> Is there any protocol to follow I'm not aware of, to enable you to
> process this bug report?

Not really. You send the report, and you may or may not get a response
(just as you may or may not have somebody respond to you in a bug
tracker).  Probably nobody has had a chance to look at it yet. If there
is a history to the bug (which there is in this case), it is sometimes
helpful to cc the previous participants to see if they are still
interested in it.

> > I think I've been bitten by a bug in git ls-files - committed
> > directories (and files therein) which are matched by a "dir/"
> > gitignore rule are not shown by "git ls-files --exclude-standard
> > -i", although they should be. Matched files are shown.
> > 
> > I've found a very brief discussion on this mailing list from March
> > 2011 about this, including a repro procedure which still is valid
> > today, at http://marc.info/?l=git&m=129926031808376&w=4
> > 
> > Is that a known/in progress bug? If yes, (when) can a fix be
> > expected? How can I work around this in the meantime?

No, I don't think anybody is working on it at the moment (at least I
do not see anything near the time of that old discussion, nor do I
recall it being discussed since).

+cc Clemens, in case he had any work-in-progress as a result that did
not end up getting published.

-Peff

Re: Bug: git ls-files and ignored directories

[Clemens Buchacher]

On Fri, Jun 01, 2012 at 05:37:57AM -0400, Jeff King wrote:
> 
> +cc Clemens, in case he had any work-in-progress as a result that did
> not end up getting published.

Unfortunately, I don't recall this bug at all. I probably don't have a
fix for it either.

Re: Bug: git ls-files and ignored directories

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> No, I don't think anybody is working on it at the moment (at least I
> do not see anything near the time of that old discussion, nor do I
> recall it being discussed since).
>
> +cc Clemens, in case he had any work-in-progress as a result that did
> not end up getting published.

I think the way the codepath for -i feeds path to excluded() is wrong.

The excluded() interface relies on the fact that the caller has
already checked foo/ before calling it for foo/bar; when asked to
see if "foo/bar" is excluded, it does not consider if "foo/" is
excluded---the caller should have already checked it.

The loop for cached/stage case in builtin/ls-files.c::show_files()
needs to learn to be more careful when DIR_SHOW_IGNORED is set in
dir->flags.  It needs to call a new helper function that notices
that the check is walking into a different directory from the path
it checked last, and feed leading directories to excluded(), or
something like that, instead of directly calling !excluded().

Re: Bug: git ls-files and ignored directories

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> No, I don't think anybody is working on it at the moment (at least I
>> do not see anything near the time of that old discussion, nor do I
>> recall it being discussed since).
>>
>> +cc Clemens, in case he had any work-in-progress as a result that did
>> not end up getting published.
>
> I think the way the codepath for -i feeds path to excluded() is wrong.
>
> The excluded() interface relies on the fact that the caller has
> already checked foo/ before calling it for foo/bar; when asked to
> see if "foo/bar" is excluded, it does not consider if "foo/" is
> excluded---the caller should have already checked it.
>
> The loop for cached/stage case in builtin/ls-files.c::show_files()
> needs to learn to be more careful when DIR_SHOW_IGNORED is set in
> dir->flags.  It needs to call a new helper function that notices
> that the check is walking into a different directory from the path
> it checked last, and feed leading directories to excluded(), or
> something like that, instead of directly calling !excluded().

A naive and unoptimized implementation may look like this patch.

The "path_exclude_check" structure can be enhanced to record the
leading directory it has last checked to be known to be excluded so
that path_excluded() can check if the ce->name[] is still inside
that directory and return true early, but I'll leave it as an
exercise for interested readers while I look at other topics for the
upcoming release.

-- >8 --
Subject: ls-files -i: pay attention to exclusion of leading paths

"git ls-files --exclude=t/ -i" does not show paths in directory t/
that have been added to the index, but it should.

The excluded() API was designed for callers who walk the tree from
the top, checking each level of the directory hierarchy as it
descends if it is excluded, and not even bothering to recurse into
an excluded directory.  This would allow us optimize for a common
case by not having to check if the exclude pattern "foo/" matches
when looking at "foo/bar", because the caller should have noticed
that "foo" is excluded and did not even bother to read "foo/bar"
out of opendir()/readdir() to call it.

The code for "ls-files -i" however walks the index linearly, feeding
paths without checking if the leading directory is already excluded.

Introduce a helper function path_excluded() to let this caller
properly call excluded() check for higher hierarchies as necessary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/ls-files.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 7cff175..40f8865 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -13,6 +13,7 @@
 #include "parse-options.h"
 #include "resolve-undo.h"
 #include "string-list.h"
+#include "strbuf.h"
 
 static int abbrev;
 static int show_deleted;
@@ -200,9 +201,50 @@ static void show_ru_info(void)
 	}
 }
 
+struct path_exclude_check {
+	struct dir_struct *dir;
+	struct strbuf path;
+};
+
+static void path_exclude_check_init(struct path_exclude_check *check,
+				    struct dir_struct *dir)
+{
+	check->dir = dir;
+	strbuf_init(&check->path, 256);
+}
+
+static void path_exclude_check_clear(struct path_exclude_check *check)
+{
+	strbuf_release(&check->path);
+}
+
+static int path_excluded(struct path_exclude_check *check, struct cache_entry *ce)
+{
+	int i, dtype;
+	struct strbuf *path = &check->path;
+	strbuf_setlen(path, 0);
+
+	for (i = 0; ce->name[i]; i++) {
+		int ch = ce->name[i];
+
+		if (ch == '/') {
+			dtype = DT_DIR;
+			if (excluded(check->dir, path->buf, &dtype))
+				return 1;
+		}
+		strbuf_addch(path, ch);
+	}
+	dtype = ce_to_dtype(ce);
+	return excluded(check->dir, ce->name, &dtype);
+}
+
 static void show_files(struct dir_struct *dir)
 {
 	int i;
+	struct path_exclude_check check;
+
+	if ((dir->flags & DIR_SHOW_IGNORED))
+		path_exclude_check_init(&check, dir);
 
 	/* For cached/deleted files we don't need to even do the readdir */
 	if (show_others || show_killed) {
@@ -215,9 +257,8 @@ static void show_files(struct dir_struct *dir)
 	if (show_cached | show_stage) {
 		for (i = 0; i < active_nr; i++) {
 			struct cache_entry *ce = active_cache[i];
-			int dtype = ce_to_dtype(ce);
-			if (dir->flags & DIR_SHOW_IGNORED &&
-			    !excluded(dir, ce->name, &dtype))
+			if ((dir->flags & DIR_SHOW_IGNORED) &&
+			    !path_excluded(&check, ce))
 				continue;
 			if (show_unmerged && !ce_stage(ce))
 				continue;
@@ -232,9 +273,8 @@ static void show_files(struct dir_struct *dir)
 			struct cache_entry *ce = active_cache[i];
 			struct stat st;
 			int err;
-			int dtype = ce_to_dtype(ce);
-			if (dir->flags & DIR_SHOW_IGNORED &&
-			    !excluded(dir, ce->name, &dtype))
+			if ((dir->flags & DIR_SHOW_IGNORED) &&
+			    !path_excluded(&check, ce))
 				continue;
 			if (ce->ce_flags & CE_UPDATE)
 				continue;
@@ -247,6 +287,9 @@ static void show_files(struct dir_struct *dir)
 				show_ce_entry(tag_modified, ce);
 		}
 	}
+
+	if ((dir->flags & DIR_SHOW_IGNORED))
+		path_exclude_check_clear(&check);
 }
 
 /*

Re: Bug: git ls-files and ignored directories

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> A naive and unoptimized implementation may look like this patch.
>
> The "path_exclude_check" structure can be enhanced to record the
> leading directory it has last checked to be known to be excluded so
> that path_excluded() can check if the ce->name[] is still inside
> that directory and return true early, but I'll leave it as an
> exercise for interested readers while I look at other topics for the
> upcoming release.

And an obvious and simple optimization would look like this.

We can keep the shallowest of the excluded directory (i.e. where a
traversing caller would have stopped recursing) in check->path, and
keep returning "Yes, it is excluded" as long as the path is inside
that directory.

 builtin/ls-files.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1385852..5beada0 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -218,11 +218,25 @@ static void path_exclude_check_clear(struct path_exclude_check *check)
 	strbuf_release(&check->path);
 }
 
+/*
+ * Is the ce->name excluded?  This is for a caller like show_files() that
+ * do not honor directory hierarchy and iterate through paths that are
+ * possibly in an ignored directory.
+ *
+ * A path to a directory known to be excluded is left in check->path to
+ * optimize for repeated checks for files in the same excluded directory.
+ */
 static int path_excluded(struct path_exclude_check *check, struct cache_entry *ce)
 {
 	int i, dtype;
 	struct strbuf *path = &check->path;
 
+	if (path->len &&
+	    path->len <= ce_namelen(ce) &&
+	    !memcmp(ce->name, path->buf, path->len) &&
+	    (!ce->name[path->len] || ce->name[path->len] == '/'))
+		return 1;
+
 	strbuf_setlen(path, 0);
 	for (i = 0; ce->name[i]; i++) {
 		int ch = ce->name[i];
@@ -234,6 +248,10 @@ static int path_excluded(struct path_exclude_check *check, struct cache_entry *c
 		}
 		strbuf_addch(path, ch);
 	}
+
+	/* An entry in the index; cannot be a directory with subentries */
+	strbuf_setlen(path, 0);
+
 	dtype = ce_to_dtype(ce);
 	return excluded(check->dir, ce->name, &dtype);
 }

Re: Bug: git ls-files and ignored directories

[Nguyen Thai Ngoc Duy]

On Fri, Jun 1, 2012 at 11:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> No, I don't think anybody is working on it at the moment (at least I
>> do not see anything near the time of that old discussion, nor do I
>> recall it being discussed since).
>>
>> +cc Clemens, in case he had any work-in-progress as a result that did
>> not end up getting published.
>
> I think the way the codepath for -i feeds path to excluded() is wrong.
>
> The excluded() interface relies on the fact that the caller has
> already checked foo/ before calling it for foo/bar; when asked to
> see if "foo/bar" is excluded, it does not consider if "foo/" is
> excluded---the caller should have already checked it.

Sparse checkout deals with the exact same problem and it reconstructs
a tree structure from cache list, before feeding to excluded(). I
thought of generalizing it (e.g. for this case), but with v5 in
progress, we'll probably have tree-based index soon. "ls-files -i"
could be fixed by then.
-- 
Duy

Bug: git ls-files and ignored directories

[Christoph Buchner]

@Jeff: Thank you for the clarification. I didn't want to appear pushy, I 
just wanted to make sure that I did everything right and that my mail 
won't get ignored/filtered because of some breach of protocol.

@Junio, Duy: Thanks for starting/proposing work on this!

thanks,
Christoph

On 06/02/2012 12:33 PM, Nguyen Thai Ngoc Duy wrote:
> On Fri, Jun 1, 2012 at 11:03 PM, Junio C Hamano<gitster@pobox.com>  wrote:
>> Jeff King<peff@peff.net>  writes:
>>
>>> No, I don't think anybody is working on it at the moment (at least I
>>> do not see anything near the time of that old discussion, nor do I
>>> recall it being discussed since).
>>>
>>> +cc Clemens, in case he had any work-in-progress as a result that did
>>> not end up getting published.
>> I think the way the codepath for -i feeds path to excluded() is wrong.
>>
>> The excluded() interface relies on the fact that the caller has
>> already checked foo/ before calling it for foo/bar; when asked to
>> see if "foo/bar" is excluded, it does not consider if "foo/" is
>> excluded---the caller should have already checked it.
> Sparse checkout deals with the exact same problem and it reconstructs
> a tree structure from cache list, before feeding to excluded(). I
> thought of generalizing it (e.g. for this case), but with v5 in
> progress, we'll probably have tree-based index soon. "ls-files -i"
> could be fixed by then.

Re: Bug: git ls-files and ignored directories

[Junio C Hamano]

Christoph Buchner <bilderbuchi@phononoia.at> writes:

> @Jeff: Thank you for the clarification. I didn't want to appear pushy,
> I just wanted to make sure that I did everything right and that my
> mail won't get ignored/filtered because of some breach of protocol.
>
> @Junio, Duy: Thanks for starting/proposing work on this!

The patches are queued to 'pu'.  Do they fix your issue?

Re: Bug: git ls-files and ignored directories

[Christoph Buchner]

Yes they do.
In my test repo, git compiled from current pu branch shows
git ls-files --exclude-standard -i
apps/devApps/projectGenerator/.svn/wrongly-committed-svn.txt
apps/devApps/projectGenerator/some-wrong-file.txt
while git 1.7.9.5 shows
apps/devApps/projectGenerator/some-wrong-file.txt

Thank you for fixing this! Which git release is planned to contain the 
fix (if that can be said yet)? I'm not familiar with your workflow..

thx,
christoph

(junio, sorry for sending the mail twice, I accidentally hit reply 
instead of reply all)

On 06/03/2012 12:56 AM, Junio C Hamano wrote:
> Christoph Buchner<bilderbuchi@phononoia.at>  writes:
>
>> @Jeff: Thank you for the clarification. I didn't want to appear pushy,
>> I just wanted to make sure that I did everything right and that my
>> mail won't get ignored/filtered because of some breach of protocol.
>>
>> @Junio, Duy: Thanks for starting/proposing work on this!
> The patches are queued to 'pu'.  Do they fix your issue?

Re: Bug: git ls-files and ignored directories

[Junio C Hamano]

Christoph Buchner <bilderbuchi@phononoia.at> writes:

[please do not top-post]

> On 06/03/2012 12:56 AM, Junio C Hamano wrote:
>
>> The patches are queued to 'pu'.  Do they fix your issue?
> 
> Yes they do.
> In my test repo, git compiled from current pu branch shows
> git ls-files --exclude-standard -i
> apps/devApps/projectGenerator/.svn/wrongly-committed-svn.txt
> apps/devApps/projectGenerator/some-wrong-file.txt
> while git 1.7.9.5 shows
> apps/devApps/projectGenerator/some-wrong-file.txt
>
> Thank you for fixing this! Which git release is planned to contain the
> fix (if that can be said yet)? I'm not familiar with your workflow..

As we are already in "regression fix only" phase, it won't be in the
upcoming 1.7.11 series, even though it may deserve to be in 1.7.11.2
maintenance release.

I see other uses of excluded() in builtin/add.c and unpack-trees.c;
these call sites need to be audited.

If they have the same problem, it probably is a good idea to convert
them to use path_excluded(), move the path_excluded() function and
its supporting infrastructure from builtin/ls-files.c to dir.c, and
make excluded() a file-static function to dir.c in order to avoid
similar problems in the future.  Or their use may be alright.

The work described in the previous paragraph preferrably should
happen before the change is shipped as part of any release.

Re: Bug: git ls-files and ignored directories

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> As we are already in "regression fix only" phase, it won't be in the
> upcoming 1.7.11 series, even though it may deserve to be in 1.7.11.2
> maintenance release.
>
> I see other uses of excluded() in builtin/add.c and unpack-trees.c;
> these call sites need to be audited.
>
> If they have the same problem, it probably is a good idea to convert
> them to use path_excluded(), move the path_excluded() function and
> its supporting infrastructure from builtin/ls-files.c to dir.c, and
> make excluded() a file-static function to dir.c in order to avoid
> similar problems in the future.  Or their use may be alright.
>
> The work described in the previous paragraph preferrably should
> happen before the change is shipped as part of any release.

I've moved the path_excluded() stuff to dir.[ch] and queued the
result to 'pu', but didn't look at other uses of excluded().

Just in case somebody is interested in working on it further.