# Bug: git ls-files and ignored directories 13 messages from 2012-05-30 to 2012-06-04. Participants: Christoph Buchner, Jeff King, Clemens Buchacher, Junio C Hamano, Nguyen Thai Ngoc Duy. Thread: https://gitlist.dev/t/30653 ## Christoph Buchner, 2012-05-30 10:22 Subject: Bug: git ls-files and ignored directories Message-ID: <20120530102218.0625CFC006A@dd24126.kasserver.com> URL: https://gitlist.dev/e/20120530102218.0625CFC006A%40dd24126.kasserver.com ``` 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 ``` ## Christoph Buchner, 2012-05-31 10:14 Subject: Bug: git ls-files and ignored directories Message-ID: <20120531101451.C35C5B4C00D@dd24126.kasserver.com> URL: https://gitlist.dev/e/20120531101451.C35C5B4C00D%40dd24126.kasserver.com In-Reply-To: <20120530102218.0625CFC006A@dd24126.kasserver.com> ``` 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 ``` ## Jeff King, 2012-06-01 09:37 Subject: Re: Bug: git ls-files and ignored directories Message-ID: <20120601093757.GE32340@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120601093757.GE32340%40sigill.intra.peff.net In-Reply-To: <20120531101451.C35C5B4C00D@dd24126.kasserver.com> ``` 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 ``` ## Clemens Buchacher, 2012-06-01 15:30 Subject: Re: Bug: git ls-files and ignored directories Message-ID: <20120601153002.GA13233@ecki> URL: https://gitlist.dev/e/20120601153002.GA13233%40ecki In-Reply-To: <20120601093757.GE32340@sigill.intra.peff.net> ``` 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. ``` ## Junio C Hamano, 2012-06-01 16:03 Subject: Re: Bug: git ls-files and ignored directories Message-ID: <7vr4tz3tpw.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr4tz3tpw.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120601093757.GE32340@sigill.intra.peff.net> ``` Jeff King 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(). ``` ## Junio C Hamano, 2012-06-01 18:28 Subject: Re: Bug: git ls-files and ignored directories Message-ID: <7vipfa3n0v.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vipfa3n0v.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vr4tz3tpw.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Jeff King 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 --- 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); } /* ``` ## Junio C Hamano, 2012-06-01 19:22 Subject: Re: Bug: git ls-files and ignored directories Message-ID: <7vvcja25ye.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvcja25ye.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vipfa3n0v.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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); } ``` ## Nguyen Thai Ngoc Duy, 2012-06-02 10:33 Subject: Re: Bug: git ls-files and ignored directories Message-ID: URL: https://gitlist.dev/e/CACsJy8CqMudg%3DuKuDmRHoYAXhUoYhOp3V2ZLuKM8W5wWWE460w%40mail.gmail.com In-Reply-To: <7vr4tz3tpw.fsf@alter.siamese.dyndns.org> ``` On Fri, Jun 1, 2012 at 11:03 PM, Junio C Hamano wrote: > Jeff King 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 ``` ## Christoph Buchner, 2012-06-02 11:00 Subject: Bug: git ls-files and ignored directories Message-ID: <4FC9F24D.7000306@phononoia.at> URL: https://gitlist.dev/e/4FC9F24D.7000306%40phononoia.at In-Reply-To: ``` @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 wrote: >> Jeff King 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. ``` ## Junio C Hamano, 2012-06-02 22:56 Subject: Re: Bug: git ls-files and ignored directories Message-ID: <7vr4txz5j9.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr4txz5j9.fsf%40alter.siamese.dyndns.org In-Reply-To: <4FC9F24D.7000306@phononoia.at> ``` Christoph Buchner 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? ``` ## Christoph Buchner, 2012-06-03 11:44 Subject: Re: Bug: git ls-files and ignored directories Message-ID: <4FCB4E1E.2010907@phononoia.at> URL: https://gitlist.dev/e/4FCB4E1E.2010907%40phononoia.at In-Reply-To: <7vr4txz5j9.fsf@alter.siamese.dyndns.org> ``` 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 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? ``` ## Junio C Hamano, 2012-06-03 22:24 Subject: Re: Bug: git ls-files and ignored directories Message-ID: <7vwr3ovxsp.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwr3ovxsp.fsf%40alter.siamese.dyndns.org In-Reply-To: <4FCB4E1E.2010907@phononoia.at> ``` Christoph Buchner 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. ``` ## Junio C Hamano, 2012-06-04 02:12 Subject: Re: Bug: git ls-files and ignored directories Message-ID: <7vmx4jvn8j.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmx4jvn8j.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vwr3ovxsp.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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. ```