{"thread":{"id":"22863","subject":"[PATCH 3/3] builtin-add: fix exclude handling","startedAt":"2010-03-01T08:26:37Z","lastAt":"2010-03-09T22:48:15Z","messageCount":2,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"135930","messageId":"7vzl2s4fcy.fsf@alter.siamese.dyndns.org","threadId":"22863","inReplyTo":null,"subject":"[PATCH 3/3] builtin-add: fix exclude handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-01T08:26:37Z","receivedAt":"2010-03-01T08:26:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"After we finish walking the working tree to find paths to add, the\nprune_directory() function checks the pathspecs to find typoes in\nthem.  When a given pathspec did not produce any match (either in the\nuntracked files, or paths already in the index), there are two cases:\n\n - \"git add no-such-file\", i.e. the pathspec was misspelled; or\n\n - \"git add ignored-pattern.o\", i.e. the pathspec exactly matches but is\n   ignored by the traversal.\n\nFor the former, the function immediately errored out.  The latter were\nqueued in the dir structure and later used to give an error message with\n\"use -f if you really mean it\" advice.\n\ne96980e (builtin-add: simplify (and increase accuracy of) exclude\nhandling, 2007-06-12) somehow lost the latter.  This adds the logic back,\nbut with a bit of twist, as the code after e96980e uses collect_ignored\noption that does half the necessary work during the traversal.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-add.c |   23 ++++++++++++++++++++---\n dir.c         |   10 ++++------\n dir.h         |    2 ++\n 3 files changed, 26 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-add.c b/builtin-add.c\nindex cb6e590..c24c1bf 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -63,9 +63,26 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p\n \tfill_pathspec_matches(pathspec, seen, specs);\n \n \tfor (i = 0; i < specs; i++) {\n-\t\tif (!seen[i] && pathspec[i][0] && !file_exists(pathspec[i]))\n-\t\t\tdie(\"pathspec '%s' did not match any files\",\n-\t\t\t\t\tpathspec[i]);\n+\t\tconst char *match;\n+\t\tif (seen[i])\n+\t\t\tcontinue;\n+\t\tmatch = pathspec[i];\n+\t\tif (!match[0])\n+\t\t\tcontinue;\n+\n+\t\t/* Existing file?  We must have ignored it */\n+\t\tif (file_exists(match)) {\n+\t\t\tint len = strlen(match);\n+\t\t\tint i;\n+\t\t\tfor (i = 0; i < dir->ignored_nr; i++)\n+\t\t\t\tif (dir->ignored[i]->len == len &&\n+\t\t\t\t    !memcmp(dir->ignored[i]->name, match, len))\n+\t\t\t\t\tbreak;\n+\t\t\tif (dir->ignored_nr <= i)\n+\t\t\t\tdir_add_ignored(dir, match, strlen(match));\n+\t\t\tcontinue;\n+\t\t}\n+\t\tdie(\"pathspec '%s' did not match any files\", match);\n \t}\n         free(seen);\n }\ndiff --git a/dir.c b/dir.c\nindex 00d698d..11d8954 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -413,13 +413,10 @@ static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathna\n \treturn dir->entries[dir->nr++] = dir_entry_new(pathname, len);\n }\n \n-static struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len)\n+void dir_add_ignored(struct dir_struct *dir, const char *pathname, int len)\n {\n-\tif (!cache_name_is_other(pathname, len))\n-\t\treturn NULL;\n-\n \tALLOC_GROW(dir->ignored, dir->ignored_nr+1, dir->ignored_alloc);\n-\treturn dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len);\n+\tdir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len);\n }\n \n enum exist_status {\n@@ -638,7 +635,8 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n {\n \tint exclude = excluded(dir, path, &dtype);\n \tif (exclude && (dir->flags & DIR_COLLECT_IGNORED)\n-\t    && in_pathspec(path, *len, simplify))\n+\t    && in_pathspec(path, *len, simplify)\n+\t    && cache_name_is_other(path, *len))\n \t\tdir_add_ignored(dir, path, *len);\n \n \t/*\ndiff --git a/dir.h b/dir.h\nindex 320b6a2..9bc3337 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -96,4 +96,6 @@ extern int remove_dir_recursively(struct strbuf *path, int flag);\n /* tries to remove the path with empty directories along it, ignores ENOENT */\n extern int remove_path(const char *path);\n \n+extern void dir_add_ignored(struct dir_struct *, const char *, int);\n+\n #endif\n-- \n1.7.0.1.241.g6604f\n"},{"id":"136475","messageId":"20100309224815.GB25265@sigill.intra.peff.net","threadId":"22863","inReplyTo":"7vzl2s4fcy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] builtin-add: fix exclude handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-09T22:48:15Z","receivedAt":"2010-03-09T22:48:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 01, 2010 at 12:26:37AM -0800, Junio C Hamano wrote:\n\n>  - \"git add ignored-pattern.o\", i.e. the pathspec exactly matches but is\n>    ignored by the traversal.\n> \n> For the former, the function immediately errored out.  The latter were\n> queued in the dir structure and later used to give an error message with\n> \"use -f if you really mean it\" advice.\n> \n> e96980e (builtin-add: simplify (and increase accuracy of) exclude\n> handling, 2007-06-12) somehow lost the latter.  This adds the logic back,\n> but with a bit of twist, as the code after e96980e uses collect_ignored\n> option that does half the necessary work during the traversal.\n\nI'm not sure that logic is accurate. The \"increase accuracy\" part of\ne96980e was about the fact that COLLECT_IGNORED _knows_ something is\nignored, but builtin-add's prune_directory has to just _guess_ about it.\n\nAnd I think your new code may have similar problems.  Look at the\nfailing test in t0050 (the one you tweaked in patch 1/3).  Before this\npatch, it silently fails to add. Which is a bug, of course, but not this\none. But after this patch, we say \"Oh, CamelCase was excluded, use -f,\netc\". But that's not true. We are just making a guess after the fact\nabout why it wasn't included.\n\nNow I am slightly hesitant to use that as an example, because it clearly\nwas also broken _before_ your patch. So it may be that the behavior it\nexhibits (a pathspec not showing up as used, but the file exists and was\n_not_ ignored) shouldn't ever happen for other cases.  I'm not sure,\nwhich makes me a little nervous about the change. But I would be willing\nto accept \"it seems to work, so let's go with it for now and see if\nanybody complains\" if you want to try that.\n\nBut also:\n\n> +\t\t/* Existing file?  We must have ignored it */\n> +\t\tif (file_exists(match)) {\n> +\t\t\tint len = strlen(match);\n> +\t\t\tint i;\n> +\t\t\tfor (i = 0; i < dir->ignored_nr; i++)\n> +\t\t\t\tif (dir->ignored[i]->len == len &&\n> +\t\t\t\t    !memcmp(dir->ignored[i]->name, match, len))\n> +\t\t\t\t\tbreak;\n> +\t\t\tif (dir->ignored_nr <= i)\n> +\t\t\t\tdir_add_ignored(dir, match, strlen(match));\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tdie(\"pathspec '%s' did not match any files\", match);\n\nIs that memcmp right? If I have something like:\n\n  echo subdir >.gitignore\n  git add 'sub*'\n\nshouldn't it also say \"Sorry, subdir [or sub*] was ignored\"? But it\ndoesn't actually get added to the exclude list, and we get \"pathspec did\nnot match any files\", which is not quite true.\n\n-Peff\n"}]}