{"thread":{"id":"10883","subject":"Odd .gitignore behaviour","startedAt":"2007-11-15T12:49:21Z","lastAt":"2007-11-16T15:05:20Z","messageCount":8,"participants":["Bruce Stephens","Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"59978","messageId":"80ir431xny.fsf@tiny.isode.net","threadId":"10883","inReplyTo":null,"subject":"Odd .gitignore behaviour","fromName":"Bruce Stephens","fromEmail":"bruce.stephens@isode.com","sentAt":"2007-11-15T12:49:21Z","receivedAt":"2007-11-15T12:49:21Z","isPatch":false,"sender":{"key":"bruce.stephens@isode.com","avatar":null},"body":"Perhaps I'm misreading the manpage, but I think this is wrong:\n\n % mkdir base; cd base\n % git init\n % mkdir -p sub1/sub2\n % cd sub1\n % echo foo > .gitignore; echo '!sub2/foo' >> .gitignore\n % touch sub2/foo\n % git add sub2/foo\n The following paths are ignored by one of your .gitignore files:\n sub1/sub2/foo\n Use -f if you really want to add them.\n fatal: no files added\n\nSo sub1/sub2/foo matches the first pattern in sub1/.gitignore, but it\nalso matches the negated pattern '!sub2/foo' (in the same file, so\nprecedence isn't an issue). And the manpage says\n\n o An optional prefix ! which negates the pattern; any matching\n file excluded by a previous pattern will become included\n again. If a negated pattern matches, this will override\n lower precedence patterns sources.\n\nSo surely sub1/sub2/foo ought to be included again? Or is the first\nline in sub1/.gitignore not \"a previous pattern\" in this sense?\n\nIf I move the \"foo\" pattern up a level, creating a .gitignore in base\njust containing \"foo\", then sub1/sub2/foo is still regarded as\nignored, even though it surely matches the negating pattern\nsub1/.gitignore, and that should be of higher precedence than the\npattern in base/.gitignore?\n"},{"id":"60013","messageId":"alpine.LFD.0.9999.0711151044150.2786@woody.linux-foundation.org","threadId":"10883","inReplyTo":"80ir431xny.fsf@tiny.isode.net","subject":"Re: Odd .gitignore behaviour","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-11-15T18:56:27Z","receivedAt":"2007-11-15T18:56:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 15 Nov 2007, Bruce Stephens wrote:\n> \n> So surely sub1/sub2/foo ought to be included again? Or is the first\n> line in sub1/.gitignore not \"a previous pattern\" in this sense?\n\nYes.\n\nThe negated patterns have to show up *before* the patterns that they \noverride, because the first pattern that matches is the one that is used.\n\n> If I move the \"foo\" pattern up a level, creating a .gitignore in base\n> just containing \"foo\", then sub1/sub2/foo is still regarded as\n> ignored, even though it surely matches the negating pattern\n> sub1/.gitignore, and that should be of higher precedence than the\n> pattern in base/.gitignore?\n\nThe priority between nested .gitignore files should be that inner files \nhave higher priority than outer files (since the inner ones \"know more\", \nand the outer ones are \"generic\").\n\nSo what you describe sounds wrong.\n\nBut my quick test didn't actually support the behaviour you see. In this \nsituation:\n\n\t[torvalds@woody git-test]$ cat .gitignore\n\ta*\n\t[torvalds@woody git-test]$ cat subdir/.gitignore\n\t!a-ok\n\t[torvalds@woody git-test]$ find *\n\tall-files\n\tsubdir\n\tsubdir/a-ok\n\t[torvalds@woody git-test]$ git ls-files -o --exclude-per-directory=.gitignore\n\t.gitignore\n\tsubdir/.gitignore\n\tsubdir/a-ok\n\nie we *do* show \"showdir/a-ok\" (but we don't show \"all-files\") because \na-ok is explicitly marked to be not ignored by a higher-priority rule.\n\nSide note: \"git status\" uses the \"--directory\" flag, so it will not even \nrecurse into unknown directories, and will instead apply the gitignore \nrules to the directory names, so you get:\n\n\t[torvalds@woody git-test]$ git ls-files -o --exclude-per-directory=.gitignore --directory\n\t.gitignore\n\tsubdir/\n\nwhich doesn't show \"a-ok\", but that's for a totally unrelated reason and \nhas nothing to do with .gitignore!\n\n\t\tLinus\n"},{"id":"60018","messageId":"80ir43e04o.fsf@tiny.isode.net","threadId":"10883","inReplyTo":"alpine.LFD.0.9999.0711151044150.2786@woody.linux-foundation.org","subject":"Re: Odd .gitignore behaviour","fromName":"Bruce Stephens","fromEmail":"bruce.stephens@isode.com","sentAt":"2007-11-15T20:15:19Z","receivedAt":"2007-11-15T20:15:19Z","isPatch":false,"sender":{"key":"bruce.stephens@isode.com","avatar":null},"body":"Linus Torvalds writes:\n\n[...]\n\n> So what you describe sounds wrong.\n>\n> But my quick test didn't actually support the behaviour you see. In this \n> situation:\n>\n> \t[torvalds@woody git-test]$ cat .gitignore\n> \ta*\n> \t[torvalds@woody git-test]$ cat subdir/.gitignore\n> \t!a-ok\n> \t[torvalds@woody git-test]$ find *\n> \tall-files\n> \tsubdir\n> \tsubdir/a-ok\n> \t[torvalds@woody git-test]$ git ls-files -o --exclude-per-directory=.gitignore\n> \t.gitignore\n> \tsubdir/.gitignore\n> \tsubdir/a-ok\n>\n> ie we *do* show \"showdir/a-ok\" (but we don't show \"all-files\") because \n> a-ok is explicitly marked to be not ignored by a higher-priority rule.\n\nOK, and if I say \"git add subdir/a-ok\" I get no error, as expected.\n\nSo extend it just a tiny bit, moving a-ok down a directory and using\nthe negative pattern \"!subsubdir/a-ok\" to match:\n\n brs% cat .gitignore\n a*\n brs% cat subdir/.gitignore\n !subsubdir/a-ok\n brs% find *\n all-files\n subdir\n subdir/.gitignore\n subdir/subsubdir\n subdir/subsubdir/a-ok\n subdir/a-ok\n brs% git ls-files -o --exclude-per-directory=.gitignore\n .gitignore\n subdir/.gitignore\n subdir/subsubdir/a-ok\n brs% git add subdir/subsubdir/a-ok\n The following paths are ignored by one of your .gitignore files:\n subdir/subsubdir/a-ok\n Use -f if you really want to add them.\n fatal: no files added\n\nSo I think the output from git-ls-files is as expected (as I interpret\nthe manpage and your explanation). So is git-add just using some\ndifferent code?\n\n[...]\n"},{"id":"60027","messageId":"7vir4341ok.fsf@gitster.siamese.dyndns.org","threadId":"10883","inReplyTo":"80ir43e04o.fsf@tiny.isode.net","subject":"Re: Odd .gitignore behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-15T21:51:55Z","receivedAt":"2007-11-15T21:51:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bruce Stephens writes:\n\n> So I think the output from git-ls-files is as expected (as I interpret\n> the manpage and your explanation). So is git-add just using some\n> different code?\n\nNo, you found one of the longstanding bugs in dir.c:read_directory().\n\nThe funny thing is that I just sent out a message pointing out\nbogus handling of per-directory exclude files in ls-files last\nnight. Somehow people have a tendency to encounter the bugs in\nthe same vicinity independently.\n\nThe initial loop in read_directory() to push per-directory\nexclusion elements into the stack for directories above the\ngiven base forgets that push() does not make a copy of the path\ngiven as its parameter but stores the pointer to it instead, so\nmultiple calls to push() need to use separate path buffers.\n\nHere is a tentative patch. I do not think the patch is broken\nbut I call it tentative because:\n\n - It is ugly -- I never get this \"walking path delimited by\n slashes\" loop right;\n\n - It leaks the path buffer given to push(), but it is inherent\n in the design of \"push/pop exclude per-directory\" API. They\n were designed to be called from the recursive directory\n walking, and the path buffers are placed on the function call\n stack to be reclaimed automatically upon function return;\n\n---\n\n dir.c | 55 ++++++++++++++++++++++++++++++++++---------------------\n 1 files changed, 34 insertions(+), 21 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex fa9f902..d32f437 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -651,38 +651,51 @@ static void free_simplify(struct path_simplify *simplify)\n \t\tfree(simplify);\n }\n \n+static int push_excludes(struct dir_struct *dir, const char *base, int len)\n+{\n+\t/*\n+\t * base is like \"a/b/c/\" -- cause .gitignore, b/.gitignore and\n+\t * b/c/.gitignore to be read in this order, as if we recursed\n+\t * into it.\n+\t */\n+\tint stk = -1;\n+\tint partlen = 0;\n+\n+\tif (!(dir->exclude_per_dir && len))\n+\t\treturn stk;\n+\n+\twhile (1) {\n+\t\tchar *part = xmalloc(partlen + 1);\n+\t\tmemcpy(part, base, partlen);\n+\t\tpart[partlen] = '\\0';\n+\t\tstk = push_exclude_per_directory(dir, part, partlen);\n+\n+\t\tif (len <= partlen++)\n+\t\t\tbreak;\n+\n+\t\twhile (partlen < len && base[partlen] != '/')\n+\t\t\tpartlen++;\n+\t\tpartlen++; /* point at one past the found '/' */\n+\t}\n+\treturn stk;\n+}\n+\n int read_directory(struct dir_struct *dir, const char *path, const char *base, int baselen, const char **pathspec)\n {\n \tstruct path_simplify *simplify = create_simplify(pathspec);\n+\tint stk;\n \n \t/*\n \t * Make sure to do the per-directory exclude for all the\n \t * directories leading up to our base.\n \t */\n-\tif (baselen) {\n-\t\tif (dir->exclude_per_dir) {\n-\t\t\tchar *p, *pp = xmalloc(baselen+1);\n-\t\t\tmemcpy(pp, base, baselen+1);\n-\t\t\tp = pp;\n-\t\t\twhile (1) {\n-\t\t\t\tchar save = *p;\n-\t\t\t\t*p = 0;\n-\t\t\t\tpush_exclude_per_directory(dir, pp, p-pp);\n-\t\t\t\t*p++ = save;\n-\t\t\t\tif (!save)\n-\t\t\t\t\tbreak;\n-\t\t\t\tp = strchr(p, '/');\n-\t\t\t\tif (p)\n-\t\t\t\t\tp++;\n-\t\t\t\telse\n-\t\t\t\t\tp = pp + baselen;\n-\t\t\t}\n-\t\t\tfree(pp);\n-\t\t}\n-\t}\n+\tstk = push_excludes(dir, base, baselen);\n \n \tread_directory_recursive(dir, path, base, baselen, 0, simplify);\n \tfree_simplify(simplify);\n+\tif (0 <= stk)\n+\t\tpop_exclude_per_directory(dir, stk);\n+\n \tqsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);\n \tqsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), cmp_name);\n \treturn dir->nr;\n"},{"id":"60035","messageId":"7vabpf40ow.fsf@gitster.siamese.dyndns.org","threadId":"10883","inReplyTo":"7vir4341ok.fsf@gitster.siamese.dyndns.org","subject":"Re: Odd .gitignore behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-15T22:13:19Z","receivedAt":"2007-11-15T22:13:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Bruce Stephens writes:\n>\n>> So I think the output from git-ls-files is as expected (as I interpret\n>> the manpage and your explanation). So is git-add just using some\n>> different code?\n>\n> No, you found one of the longstanding bugs in dir.c:read_directory().\n>\n> The funny thing is that I just sent out a message pointing out\n> bogus handling of per-directory exclude files in ls-files last\n> night. Somehow people have a tendency to encounter the bugs in\n> the same vicinity independently.\n\nBy the way, about the problem I described briefly last night.\n\nI never understood the intended use of -i option to ls-files,\nbut in your test repository (the one that has subsubdir), you\ncan do:\n\n\t$ git ls-files -i --exclude='a*'\n\nto see \"What paths have I already _staged_ that would have been\nignored if the exclude pattern were 'a*'\". You can abuse this\nto list all the staged header files with:\n\n\t$ git ls-files -i --exclude='*.h'\n\nbut\n\n\t$ git ls-files -- '*.h'\n\nis much simpler for that ;-).\n\nIn any case, it appears to me that the codepath used for that\n\"feature\", and also the codepath used for -d (show deleted\nfiles) and -m (show modified files) makes calls to excluded()\nfunction to consult the exclusion mechanism without setting it\nup properly, and I do not think\n\n\t$ git ls-files -i --exclude-per-directory=.gitignore\n\ndoes what we would want.\n\nThe codepath for -o (show others) do use read_directory() which\nsets up the exclusion mechanism with push/pop per-directory\nexclude API, so that option should work. But I suspect even it\ndid not work from a subdirectory because of the problem the\nmessage I am responding to addresses.\n"},{"id":"60070","messageId":"7vodduzh36.fsf@gitster.siamese.dyndns.org","threadId":"10883","inReplyTo":"7vir4341ok.fsf@gitster.siamese.dyndns.org","subject":"Fix per-directory exclude handing for \"git add\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-16T09:15:41Z","receivedAt":"2007-11-16T09:15:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In \"dir_struct\", each exclusion element in the exclusion stack records a\nbase string (pointer to the beginning with length) so that we can tell\nwhere it came from, but this pointer is just pointing at the parameter\nthat is given by the caller to the push_exclude_per_directory()\nfunction.\n\nWhile read_directory_recursive() runs, calls to excluded() makes use\nthe data in the exclusion elements, including this base string. The\ncaller of read_directory_recursive() is not supposed to free the\nbuffer it gave to push_exclude_per_directory() earlier, until it\nreturns.\n\nThe test case Bruce Stephens gave in the mailing list discussion\nwas simplified and added to the t3700 test.\n\nSigned-off-by: Junio C Hamano \n---\n\n Junio C Hamano writes:\n\n > Here is a tentative patch. I do not think the patch is broken\n > but I call it tentative because:\n >\n > - It is ugly -- I never get this \"walking path delimited by\n > slashes\" loop right;\n >\n > - It leaks the path buffer given to push(), but it is inherent\n > in the design of \"push/pop exclude per-directory\" API.\n\n It turns out that a minimally invasive fix was a lot simpler\n than I thought.\n\n This still does not fix the other codepaths in ls-files that does not\n use read_directory() but walks the cache.\n\n dir.c | 6 ++++--\n t/t3700-add.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex fa9f902..225fdfb 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -654,6 +654,7 @@ static void free_simplify(struct path_simplify *simplify)\n int read_directory(struct dir_struct *dir, const char *path, const char *base, int baselen, const char **pathspec)\n {\n \tstruct path_simplify *simplify = create_simplify(pathspec);\n+\tchar *pp = NULL;\n \n \t/*\n \t * Make sure to do the per-directory exclude for all the\n@@ -661,7 +662,8 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i\n \t */\n \tif (baselen) {\n \t\tif (dir->exclude_per_dir) {\n-\t\t\tchar *p, *pp = xmalloc(baselen+1);\n+\t\t\tchar *p;\n+\t\t\tpp = xmalloc(baselen+1);\n \t\t\tmemcpy(pp, base, baselen+1);\n \t\t\tp = pp;\n \t\t\twhile (1) {\n@@ -677,12 +679,12 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i\n \t\t\t\telse\n \t\t\t\t\tp = pp + baselen;\n \t\t\t}\n-\t\t\tfree(pp);\n \t\t}\n \t}\n \n \tread_directory_recursive(dir, path, base, baselen, 0, simplify);\n \tfree_simplify(simplify);\n+\tfree(pp);\n \tqsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);\n \tqsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), cmp_name);\n \treturn dir->nr;\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex a328bf5..287e058 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -104,9 +104,33 @@ test_expect_success 'add ignored ones with -f' '\n \tgit ls-files --error-unmatch d.ig/d.if d.ig/d.ig\n '\n \n+test_expect_success 'add ignored ones with -f' '\n+\trm -f .git/index &&\n+\tgit add -f d.?? &&\n+\tgit ls-files --error-unmatch d.ig/d.if d.ig/d.ig\n+'\n+\n+test_expect_success '.gitignore with subdirectory' '\n+\n+\trm -f .git/index &&\n+\tmkdir -p sub/dir &&\n+\techo \"!dir/a.*\" >sub/.gitignore &&\n+\t>sub/a.ig &&\n+\t>sub/dir/a.ig &&\n+\tgit add sub/dir &&\n+\tgit ls-files --error-unmatch sub/dir/a.ig &&\n+\trm -f .git/index &&\n+\t(\n+\t\tcd sub/dir &&\n+\t\tgit add .\n+\t) &&\n+\tgit ls-files --error-unmatch sub/dir/a.ig\n+'\n+\n mkdir 1 1/2 1/3\n touch 1/2/a 1/3/b 1/2/c\n test_expect_success 'check correct prefix detection' '\n+\trm -f .git/index &&\n \tgit add 1/2/a 1/3/b 1/2/c\n '\n \n"},{"id":"60090","messageId":"80y7cy2tay.fsf@tiny.isode.net","threadId":"10883","inReplyTo":"7vodduzh36.fsf@gitster.siamese.dyndns.org","subject":"Re: Fix per-directory exclude handing for \"git add\"","fromName":"Bruce Stephens","fromEmail":"bruce.stephens@isode.com","sentAt":"2007-11-16T13:50:29Z","receivedAt":"2007-11-16T13:50:29Z","isPatch":false,"sender":{"key":"bruce.stephens@isode.com","avatar":null},"body":"Junio C Hamano writes:\n\n[...]\n\n> While read_directory_recursive() runs, calls to excluded() makes use\n> the data in the exclusion elements, including this base string. The\n> caller of read_directory_recursive() is not supposed to free the\n> buffer it gave to push_exclude_per_directory() earlier, until it\n> returns.\n\nCool, that fixes the \"git add\" issue I was seeing. (So Acked-by:\nBruce Stephens , for what it's worth.)\n\nI guess really the output of \"git status\" (or \"git runstatus\") is more\nsignificant since that's what we'd normally be running (that's\npresumably what \"git gui\" and similar tools run, though perhaps they\nuse \"git ls-files\"---probably the underlying code's the same, I\nguess).\n\nHowever, that doesn't mean the issue with git add shouldn't be\nresolved.\n\n[...]\n"},{"id":"60092","messageId":"80ve82z0wf.fsf@tiny.isode.net","threadId":"10883","inReplyTo":"80y7cy2tay.fsf@tiny.isode.net","subject":"Re: Fix per-directory exclude handing for \"git add\"","fromName":"Bruce Stephens","fromEmail":"bruce.stephens@isode.com","sentAt":"2007-11-16T15:05:20Z","receivedAt":"2007-11-16T15:05:20Z","isPatch":false,"sender":{"key":"bruce.stephens@isode.com","avatar":null},"body":"Bruce Stephens writes:\n\n[...]\n\n> I guess really the output of \"git status\" (or \"git runstatus\") is\n> more significant since that's what we'd normally be running (that's\n> presumably what \"git gui\" and similar tools run, though perhaps they\n> use \"git ls-files\"---probably the underlying code's the same, I\n> guess).\n\nBah. \"git status\" also works correctly, I think. I was just testing\nstupidly.\n\n[...]\n"}]}