{"thread":{"id":"4185","subject":"Do \"git add\" as a builtin","startedAt":"2006-05-17T16:33:32Z","lastAt":"2006-05-19T06:28:34Z","messageCount":13,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"20155","messageId":"Pine.LNX.4.64.0605170927050.10823@g5.osdl.org","threadId":"4185","inReplyTo":null,"subject":"Do \"git add\" as a builtin","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-17T16:33:32Z","receivedAt":"2006-05-17T16:33:32Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFirst try. Let's see how well this works.\n\nIn many ways, the hard parts of \"git commit\" are not so different from\nthis, and a builtin commit would share a lot of the code, I think.\n\nSigned-off-by: Linus Torvalds <torvalds@osdl.org>\n---\n\nIt passes the tests for me. I dropped the strange \"--error-unmatch\" test, \nbecause it was really ugly and I didn't see the point. It seems to do all \nthe right things, but hey, a mistake here is obviously a bad thing.\n\nI suspect that if I get around to \"git commit\", we'd be getting to the \npoint where most of the core/easy/often-used/whatever commands would be \nall built-in.\n\ndiff --git a/Makefile b/Makefile\nindex f43ac63..e6f7794 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -218,7 +218,7 @@ LIB_OBJS = \\\n \n BUILTIN_OBJS = \\\n \tbuiltin-log.o builtin-help.o builtin-count.o builtin-diff.o builtin-push.o \\\n-\tbuiltin-grep.o\n+\tbuiltin-grep.o builtin-add.o\n \n GITLIBS = $(LIB_FILE) $(XDIFF_LIB)\n LIBS = $(GITLIBS) -lz\ndiff --git a/builtin-add.c b/builtin-add.c\nnew file mode 100644\nindex 0000000..e815b3d\n--- /dev/null\n+++ b/builtin-add.c\n@@ -0,0 +1,228 @@\n+/*\n+ * \"git add\" builtin command\n+ *\n+ * Copyright (C) 2006 Linus Torvalds\n+ */\n+#include <fnmatch.h>\n+\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"dir.h\"\n+\n+static const char builtin_add_usage[] =\n+\"git-add [-n] [-v] <filepattern>...\";\n+\n+static int common_prefix(const char **pathspec)\n+{\n+\tconst char *path, *slash, *next;\n+\tint prefix;\n+\n+\tif (!pathspec)\n+\t\treturn 0;\n+\n+\tpath = *pathspec;\n+\tslash = strrchr(path, '/');\n+\tif (!slash)\n+\t\treturn 0;\n+\n+\tprefix = slash - path + 1;\n+\twhile ((next = *++pathspec) != NULL) {\n+\t\tint len = strlen(next);\n+\t\tif (len >= prefix && !memcmp(path, next, len))\n+\t\t\tcontinue;\n+\t\tfor (;;) {\n+\t\t\tif (!len)\n+\t\t\t\treturn 0;\n+\t\t\tif (next[--len] != '/') \n+\t\t\t\tcontinue;\n+\t\t\tif (memcmp(path, next, len+1))\n+\t\t\t\tcontinue;\n+\t\t\tprefix = len + 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\treturn prefix;\n+}\n+\n+static int match(const char **pathspec, const char *name, int namelen, int prefix)\n+{\n+\tconst char *match;\n+\n+\tname += prefix;\n+\tnamelen -= prefix;\n+\n+\twhile ((match = *pathspec++) != NULL) {\n+\t\tint matchlen;\n+\n+\t\tmatch += prefix;\n+\t\tmatchlen = strlen(match);\n+\t\tif (!matchlen)\n+\t\t\treturn 1;\n+\t\tif (!strncmp(match, name, matchlen)) {\n+\t\t\tif (match[matchlen-1] == '/')\n+\t\t\t\treturn 1;\n+\t\t\tswitch (name[matchlen]) {\n+\t\t\tcase '/': case '\\0':\n+\t\t\t\treturn 1;\n+\t\t\t}\n+\t\t}\n+\t\tif (!fnmatch(match, name, 0))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)\n+{\n+\tint i;\n+\tstruct dir_entry **src, **dst;\n+\n+\tsrc = dst = dir->entries;\n+\ti = dir->nr;\n+\twhile (--i >= 0) {\n+\t\tstruct dir_entry *entry = *src++;\n+\t\tif (!match(pathspec, entry->name, entry->len, prefix)) {\n+\t\t\tfree(entry);\n+\t\t\tcontinue;\n+\t\t}\n+\t\t*dst++ = entry;\n+\t}\n+\tdir->nr = dst - dir->entries;\n+}\n+\n+static void fill_directory(struct dir_struct *dir, const char **pathspec)\n+{\n+\tconst char *path, *base;\n+\tint baselen;\n+\n+\t/* Set up the default git porcelain excludes */\n+\tmemset(dir, 0, sizeof(*dir));\n+\tdir->exclude_per_dir = \".gitignore\";\n+\tpath = git_path(\"info/exclude\");\n+\tif (!access(path, R_OK))\n+\t\tadd_excludes_from_file(dir, path);\n+\n+\t/*\n+\t * Calculate common prefix for the pathspec, and\n+\t * use that to optimize the directory walk\n+\t */\n+\tbaselen = common_prefix(pathspec);\n+\tpath = \".\";\n+\tbase = \"\";\n+\tif (baselen) {\n+\t\tchar *common = xmalloc(baselen + 1);\n+\t\tcommon = xmalloc(baselen + 1);\n+\t\tmemcpy(common, *pathspec, baselen);\n+\t\tcommon[baselen] = 0;\n+\t\tpath = base = common;\n+\t}\n+\n+\t/* Read the directory and prune it */\n+\tread_directory(dir, path, base, baselen);\n+\tif (pathspec)\n+\t\tprune_directory(dir, pathspec, baselen);\n+}\n+\n+static int add_file_to_index(const char *path, int verbose)\n+{\n+\tint size, namelen;\n+\tstruct stat st;\n+\tstruct cache_entry *ce;\n+\n+\tif (lstat(path, &st))\n+\t\tdie(\"%s: unable to stat (%s)\", path, strerror(errno));\n+\n+\tif (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode))\n+\t\tdie(\"%s: can only add regular files or symbolic links\", path);\n+\n+\tnamelen = strlen(path);\n+\tsize = cache_entry_size(namelen);\n+\tce = xcalloc(1, size);\n+\tmemcpy(ce->name, path, namelen);\n+\tce->ce_flags = htons(namelen);\n+\tfill_stat_cache_info(ce, &st);\n+\n+\tce->ce_mode = create_ce_mode(st.st_mode);\n+\tif (!trust_executable_bit) {\n+\t\t/* If there is an existing entry, pick the mode bits\n+\t\t * from it.\n+\t\t */\n+\t\tint pos = cache_name_pos(path, namelen);\n+\t\tif (pos >= 0)\n+\t\t\tce->ce_mode = active_cache[pos]->ce_mode;\n+\t}\n+\n+\tif (index_path(ce->sha1, path, &st, 1))\n+\t\tdie(\"unable to index file %s\", path);\n+\tif (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD))\n+\t\tdie(\"unable to add %s to index\",path);\n+\tif (verbose)\n+\t\tprintf(\"add '%s'\\n\", path);\n+\treturn 0;\n+}\n+\n+static struct cache_file cache_file;\n+\n+int cmd_add(int argc, const char **argv, char **envp)\n+{\n+\tint i, newfd;\n+\tint verbose = 0, show_only = 0;\n+\tconst char *prefix = setup_git_directory();\n+\tconst char **pathspec;\n+\tstruct dir_struct dir;\n+\n+\tgit_config(git_default_config);\n+\n+\tnewfd = hold_index_file_for_update(&cache_file, get_index_file());\n+\tif (newfd < 0)\n+\t\tdie(\"unable to create new cachefile\");\n+\n+\tif (read_cache() < 0)\n+\t\tdie(\"index file corrupt\");\n+\n+\tfor (i = 1; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\n+\t\tif (arg[0] != '-')\n+\t\t\tbreak;\n+\t\tif (!strcmp(arg, \"--\")) {\n+\t\t\ti++;\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!strcmp(arg, \"-n\")) {\n+\t\t\tshow_only = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(arg, \"-v\")) {\n+\t\t\tverbose = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tdie(builtin_add_usage);\n+\t}\n+\tgit_config(git_default_config);\n+\tpathspec = get_pathspec(prefix, argv + i);\n+\n+\tfill_directory(&dir, pathspec);\n+\n+\tif (show_only) {\n+\t\tconst char *sep = \"\", *eof = \"\";\n+\t\tfor (i = 0; i < dir.nr; i++) {\n+\t\t\tprintf(\"%s%s\", sep, dir.entries[i]->name);\n+\t\t\tsep = \" \";\n+\t\t\teof = \"\\n\";\n+\t\t}\n+\t\tfputs(eof, stdout);\n+\t\treturn 0;\n+\t}\n+\n+\tfor (i = 0; i < dir.nr; i++)\n+\t\tadd_file_to_index(dir.entries[i]->name, verbose);\n+\n+\tif (active_cache_changed) {\n+\t\tif (write_cache(newfd, active_cache, active_nr) ||\n+\t\t    commit_index_file(&cache_file))\n+\t\t\tdie(\"Unable to write new index file\");\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/builtin.h b/builtin.h\nindex 7744f7d..1b77f4b 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -24,5 +24,6 @@ extern int cmd_count_objects(int argc, c\n \n extern int cmd_push(int argc, const char **argv, char **envp);\n extern int cmd_grep(int argc, const char **argv, char **envp);\n+extern int cmd_add(int argc, const char **argv, char **envp);\n \n #endif\ndiff --git a/git.c b/git.c\nindex a94d9ee..fac46af 100644\n--- a/git.c\n+++ b/git.c\n@@ -50,6 +50,7 @@ static void handle_internal_command(int \n \t\t{ \"count-objects\", cmd_count_objects },\n \t\t{ \"diff\", cmd_diff },\n \t\t{ \"grep\", cmd_grep },\n+\t\t{ \"add\", cmd_add },\n \t};\n \tint i;\n \n"},{"id":"20168","messageId":"7vhd3ocvyy.fsf@assigned-by-dhcp.cox.net","threadId":"4185","inReplyTo":"Pine.LNX.4.64.0605170927050.10823@g5.osdl.org","subject":"Re: Do \"git add\" as a builtin","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-17T19:06:13Z","receivedAt":"2006-05-17T19:06:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> ... I dropped the strange \"--error-unmatch\" test, because it\n> was really ugly and I didn't see the point.\n\nBy \"not seeing the point\", do you mean you do not agree with\nwhat bba319b5 and 45e48120 tried to do to help users?\n\n        $ git add no-such-path-in-the-tree\n        $ git add 'this-pattern-matches-nobody*'\n\nMore realistically:\n\n\t$ git add Documentaiton\t\t;# misspelled\n\n\n[References]\n\n(bba319b5)\nWhen you say \"git commit Documentaiton\" to make partial commit\nfor the files only in that directory, we did not detect that as\na misspelled pathname and attempted to commit index without\nchange.  If nothing matched, there is no harm done, but if the\nindex gets modified otherwise by having another valid pathspec\nor after an explicit update-index, a user will not notice\nwithout paying attention to the \"git status\" preview.\n\nThis introduces --error-unmatch option to ls-files, and uses it\nto detect this common user error.\n\n(45e48120)\nThis is in the same spirit as an earlier patch for git-commit.\nIt does an extra ls-files to avoid complaining when a fully\ntracked directory name is given on the command line (otherwise\n--others restriction would say the pathspec does not match).\n"},{"id":"20182","messageId":"Pine.LNX.4.64.0605171321020.10823@g5.osdl.org","threadId":"4185","inReplyTo":"7vhd3ocvyy.fsf@assigned-by-dhcp.cox.net","subject":"Re: Do \"git add\" as a builtin","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-17T20:23:19Z","receivedAt":"2006-05-17T20:23:19Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 17 May 2006, Junio C Hamano wrote:\n> \n> By \"not seeing the point\", do you mean you do not agree with\n> what bba319b5 and 45e48120 tried to do to help users?\n\nNaah, I just didn't see why, and didn't bother to go exploring.\n\nHow about this patch on top of the previous one?\n\n\t\tLinus\n\n----\ndiff --git a/builtin-add.c b/builtin-add.c\nindex e815b3d..82e8f44 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -44,50 +44,89 @@ static int common_prefix(const char **pa\n \treturn prefix;\n }\n \n-static int match(const char **pathspec, const char *name, int namelen, int prefix)\n+static int match_one(const char *match, const char *name, int namelen)\n {\n+\tint matchlen;\n+\n+\t/* If the match was just the prefix, we matched */\n+\tmatchlen = strlen(match);\n+\tif (!matchlen)\n+\t\treturn 1;\n+\n+\t/*\n+\t * If we don't match the matchstring exactly,\n+\t * we need to match by fnmatch\n+\t */\n+\tif (strncmp(match, name, matchlen))\n+\t\treturn !fnmatch(match, name, 0);\n+\n+\t/*\n+\t * If we did match the string exactly, we still\n+\t * need to make sure that it happened on a path\n+\t * component boundary (ie either the last character\n+\t * of the match was '/', or the next character of\n+\t * the name was '/' or the terminating NUL.\n+\t */\n+\treturn\tmatch[matchlen-1] == '/' ||\n+\t\tname[matchlen] == '/' ||\n+\t\t!name[matchlen];\n+}\n+\n+static int match(const char **pathspec, const char *name, int namelen, int prefix, char *seen)\n+{\n+\tint retval;\n \tconst char *match;\n \n \tname += prefix;\n \tnamelen -= prefix;\n \n-\twhile ((match = *pathspec++) != NULL) {\n-\t\tint matchlen;\n-\n+\tfor (retval = 0; (match = *pathspec++) != NULL; seen++) {\n+\t\tif (retval & *seen)\n+\t\t\tcontinue;\n \t\tmatch += prefix;\n-\t\tmatchlen = strlen(match);\n-\t\tif (!matchlen)\n-\t\t\treturn 1;\n-\t\tif (!strncmp(match, name, matchlen)) {\n-\t\t\tif (match[matchlen-1] == '/')\n-\t\t\t\treturn 1;\n-\t\t\tswitch (name[matchlen]) {\n-\t\t\tcase '/': case '\\0':\n-\t\t\t\treturn 1;\n-\t\t\t}\n+\t\tif (match_one(match, name, namelen)) {\n+\t\t\tretval = 1;\n+\t\t\t*seen = 1;\n \t\t}\n-\t\tif (!fnmatch(match, name, 0))\n-\t\t\treturn 1;\n \t}\n-\treturn 0;\n+\treturn retval;\n }\n \n static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)\n {\n-\tint i;\n+\tchar *seen;\n+\tint i, specs;\n \tstruct dir_entry **src, **dst;\n \n+\tfor (specs = 0; pathspec[specs];  specs++)\n+\t\t/* nothing */;\n+\tseen = xmalloc(specs);\n+\tmemset(seen, 0, specs);\n+\n \tsrc = dst = dir->entries;\n \ti = dir->nr;\n \twhile (--i >= 0) {\n \t\tstruct dir_entry *entry = *src++;\n-\t\tif (!match(pathspec, entry->name, entry->len, prefix)) {\n+\t\tif (!match(pathspec, entry->name, entry->len, prefix, seen)) {\n \t\t\tfree(entry);\n \t\t\tcontinue;\n \t\t}\n \t\t*dst++ = entry;\n \t}\n \tdir->nr = dst - dir->entries;\n+\n+\tfor (i = 0; i < specs; i++) {\n+\t\tstruct stat st;\n+\t\tconst char *match;\n+\t\tif (seen[i])\n+\t\t\tcontinue;\n+\n+\t\t/* Existing file? We must have ignored it */\n+\t\tmatch = pathspec[i];\n+\t\tif (!lstat(match, &st))\n+\t\t\tcontinue;\n+\t\tdie(\"pathspec '%s' did not match any files\", match);\n+\t}\n }\n \n static void fill_directory(struct dir_struct *dir, const char **pathspec)\n"},{"id":"20216","messageId":"7v64k3698l.fsf@assigned-by-dhcp.cox.net","threadId":"4185","inReplyTo":"Pine.LNX.4.64.0605171321020.10823@g5.osdl.org","subject":"Re: Do \"git add\" as a builtin","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-18T08:13:46Z","receivedAt":"2006-05-18T08:13:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> On Wed, 17 May 2006, Junio C Hamano wrote:\n>> \n>> By \"not seeing the point\", do you mean you do not agree with\n>> what bba319b5 and 45e48120 tried to do to help users?\n>\n> Naah, I just didn't see why, and didn't bother to go exploring.\n>\n> How about this patch on top of the previous one?\n\nWell, not good as-is.  This makes it barf on this sequence:\n\n\t$ rm -f junk\n        $ cd junk\n        $ git init-db\n        $ date >frotz\n        $ mkdir nitfol\n        $ date >nitfol/rezrov\n\t$ git add .\t\t;# OK up to this point - added everything.\n\n\t$ git add .\t\t;# This is bogus because...\n        fatal: pathspec '' did not match any files\n\t$ git add nitfol\t;# ...this does not barf.\n\nI admit I did not spot it when I read the code, but this part\ngets an empty string for 'match' when pathspec is '.'.\n\n> +\t\t/* Existing file? We must have ignored it */\n> +\t\tmatch = pathspec[i];\n> +\t\tif (!lstat(match, &st))\n> +\t\t\tcontinue;\n> +\t\tdie(\"pathspec '%s' did not match any files\", match);\n\nThat's why '.' barfs but nitfol doesn't.\n"},{"id":"20218","messageId":"7vwtcj4tp6.fsf@assigned-by-dhcp.cox.net","threadId":"4185","inReplyTo":"7v64k3698l.fsf@assigned-by-dhcp.cox.net","subject":"Re: Do \"git add\" as a builtin","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-18T08:34:45Z","receivedAt":"2006-05-18T08:34:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> Linus Torvalds <torvalds@osdl.org> writes:\n>\n>> On Wed, 17 May 2006, Junio C Hamano wrote:\n>>> \n>>> By \"not seeing the point\", do you mean you do not agree with\n>>> what bba319b5 and 45e48120 tried to do to help users?\n>>\n>> Naah, I just didn't see why, and didn't bother to go exploring.\n>>\n>> How about this patch on top of the previous one?\n>\n> Well, not good as-is.  This makes it barf on this sequence:\n>...\n\nOuch, things are worse than I thought...\n\n\t$ mkdir foo\n        $ date >bar\n        $ git-add foo/../bar\n\t$ git ls-files\n        foo/../bar\n\nHuh?\n"},{"id":"20219","messageId":"7vsln74sv7.fsf_-_@assigned-by-dhcp.cox.net","threadId":"4185","inReplyTo":"7vwtcj4tp6.fsf@assigned-by-dhcp.cox.net","subject":"[WARNING] Please be careful when using \"git add\" from \"next\" branch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-18T08:52:44Z","receivedAt":"2006-05-18T08:52:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There is still a small breakage in the built-in \"git add\" on the\n\"next\" branch that ends up creating nonsense tree objects.\n\n        $ mkdir foo\n        $ date >bar\n        $ git-add foo/../bar\n        $ git ls-files\n        foo/../bar\n        $ git ls-tree -r -t $(git-write-tree)\n        040000 tree ef5562cd3a9bf66d41a8d4f42f159e8c694ce7e3\tfoo\n        040000 tree 6e1612248e8da43fc5f91592e559da1ad5f9a852\tfoo/..\n        100644 blob 53ab446c3f4e42ce9bb728a0ccb283a101be4979\tfoo/../bar\n\nIf you do not do funky things like foo/../bar, I do not think\nyou have to worry, but scripted use might break.  It used to\nwarn and ignore such bogus input, but now it happily accepts it\nand produces bogus index which results in bogus trees.\n\n\"git update-index --add\" is not affected by this breakage.\n"},{"id":"20230","messageId":"Pine.LNX.4.64.0605180800370.10823@g5.osdl.org","threadId":"4185","inReplyTo":"7v64k3698l.fsf@assigned-by-dhcp.cox.net","subject":"Re: Do \"git add\" as a builtin","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-18T15:06:41Z","receivedAt":"2006-05-18T15:06:41Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 May 2006, Junio C Hamano wrote:\n> \n> Well, not good as-is.  This makes it barf on this sequence:\n> \n> \t$ rm -f junk\n>         $ cd junk\n>         $ git init-db\n>         $ date >frotz\n>         $ mkdir nitfol\n>         $ date >nitfol/rezrov\n> \t$ git add .\t\t;# OK up to this point - added everything.\n> \n> \t$ git add .\t\t;# This is bogus because...\n>         fatal: pathspec '' did not match any files\n\nAhh. The empty pathspec is special.\n\nIt's special for a totally stupid reason: it's not a valid pathname, but \nwe obviously _turn_ it into a valid pathname when we read the directory \ntree (ie from a filesystem standpoint, it means \".\").\n\nSo then, when we do the \"lstat()\", we really _should_ have done the same.\n\nIf course, since the lstat() is there just to test for existence, and \nsince \".\" always exists, it's easier to just pass an empty match entirely \nthan to turn it into \".\" and then do an lstat that we know is going to \nsucceed.\n\nSo the patch would be something trivial like this..\n\n\t\tLinus\n\n---\ndiff --git a/builtin-add.c b/builtin-add.c\nindex 7083820..ac9ed2d 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -124,7 +124,7 @@ static void prune_directory(struct dir_s\n \n \t\t/* Existing file? We must have ignored it */\n \t\tmatch = pathspec[i];\n-\t\tif (!lstat(match, &st))\n+\t\tif (!*match || !lstat(match, &st))\n \t\t\tcontinue;\n \t\tdie(\"pathspec '%s' did not match any files\", match);\n \t}\n"},{"id":"20231","messageId":"Pine.LNX.4.64.0605180807060.10823@g5.osdl.org","threadId":"4185","inReplyTo":"7vwtcj4tp6.fsf@assigned-by-dhcp.cox.net","subject":"Re: Do \"git add\" as a builtin","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-18T15:26:38Z","receivedAt":"2006-05-18T15:26:38Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 May 2006, Junio C Hamano wrote:\n> \n> Ouch, things are worse than I thought...\n> \n> \t$ mkdir foo\n>         $ date >bar\n>         $ git-add foo/../bar\n> \t$ git ls-files\n>         foo/../bar\n> \n> Huh?\n\n\"git-update-index\" does a \"verify_path()\" which I missed, so the new \nbuiltin add doesn't do it.\n\nWe could do it either in the \"add_cache_entry()\" function (which would be \nsafest, because then by _definition_ you couldn't make this mistake \nagain), or we could do it in the caller.\n\nThis patch does it in the caller, just to match the old \"git add\" (which \nwould just say \"Ignoring path ...\" and not do anything).\n\nBut if people are ok with changing it from a \"print a warning and ignore\" \ninto an _error_, we could just move it into \"add_cache_entry()\".\n\nThe real _meat_ of this patch is really just that first hunk to \n\"builtin-add.c\" - all the rest is just making that \"verify_path()\" \nfunction available in the git library.\n\n\t\tLinus\n---\ndiff --git a/builtin-add.c b/builtin-add.c\nindex 7083820..0346bb5 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -172,6 +172,11 @@ static int add_file_to_index(const char \n \tif (lstat(path, &st))\n \t\tdie(\"%s: unable to stat (%s)\", path, strerror(errno));\n \n+\tif (!verify_path(path)) {\n+\t\tfprintf(stderr, \"Ignoring path %s\\n\", path);\n+\t\treturn -1;\n+\t}\n+\t\t\n \tif (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode))\n \t\tdie(\"%s: can only add regular files or symbolic links\", path);\n \ndiff --git a/cache.h b/cache.h\nindex b1300cd..f9b7049 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -143,6 +143,7 @@ #define alloc_nr(x) (((x)+16)*3/2)\n /* Initialize and use the cache information */\n extern int read_cache(void);\n extern int write_cache(int newfd, struct cache_entry **cache, int entries);\n+extern int verify_path(const char *path);\n extern int cache_name_pos(const char *name, int namelen);\n #define ADD_CACHE_OK_TO_ADD 1\t\t/* Ok to add */\n #define ADD_CACHE_OK_TO_REPLACE 2\t/* Ok to replace file/directory */\ndiff --git a/read-cache.c b/read-cache.c\nindex ed0da38..6b323dd 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -347,6 +347,70 @@ int ce_path_match(const struct cache_ent\n }\n \n /*\n+ * We fundamentally don't like some paths: we don't want\n+ * dot or dot-dot anywhere, and for obvious reasons don't\n+ * want to recurse into \".git\" either.\n+ *\n+ * Also, we don't want double slashes or slashes at the\n+ * end that can make pathnames ambiguous.\n+ */\n+static int verify_dotfile(const char *rest)\n+{\n+\t/*\n+\t * The first character was '.', but that\n+\t * has already been discarded, we now test\n+\t * the rest.\n+\t */\n+\tswitch (*rest) {\n+\t/* \".\" is not allowed */\n+\tcase '\\0': case '/':\n+\t\treturn 0;\n+\n+\t/*\n+\t * \".git\" followed by  NUL or slash is bad. This\n+\t * shares the path end test with the \"..\" case.\n+\t */\n+\tcase 'g':\n+\t\tif (rest[1] != 'i')\n+\t\t\tbreak;\n+\t\tif (rest[2] != 't')\n+\t\t\tbreak;\n+\t\trest += 2;\n+\t/* fallthrough */\n+\tcase '.':\n+\t\tif (rest[1] == '\\0' || rest[1] == '/')\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n+int verify_path(const char *path)\n+{\n+\tchar c;\n+\n+\tgoto inside;\n+\tfor (;;) {\n+\t\tif (!c)\n+\t\t\treturn 1;\n+\t\tif (c == '/') {\n+inside:\n+\t\t\tc = *path++;\n+\t\t\tswitch (c) {\n+\t\t\tdefault:\n+\t\t\t\tcontinue;\n+\t\t\tcase '/': case '\\0':\n+\t\t\t\tbreak;\n+\t\t\tcase '.':\n+\t\t\t\tif (verify_dotfile(path))\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\treturn 0;\n+\t\t}\n+\t\tc = *path++;\n+\t}\n+}\n+\n+/*\n  * Do we have another file that has the beginning components being a\n  * proper superset of the name we're trying to add?\n  */\ndiff --git a/update-index.c b/update-index.c\nindex f6b09a4..69b9a71 100644\n--- a/update-index.c\n+++ b/update-index.c\n@@ -8,6 +8,7 @@ #include \"strbuf.h\"\n #include \"quote.h\"\n #include \"cache-tree.h\"\n #include \"tree-walk.h\"\n+#include \"dir.h\"\n \n /*\n  * Default to not allowing changes to the list of files. The\n@@ -245,70 +246,6 @@ static int refresh_cache(int really)\n \treturn has_errors;\n }\n \n-/*\n- * We fundamentally don't like some paths: we don't want\n- * dot or dot-dot anywhere, and for obvious reasons don't\n- * want to recurse into \".git\" either.\n- *\n- * Also, we don't want double slashes or slashes at the\n- * end that can make pathnames ambiguous.\n- */\n-static int verify_dotfile(const char *rest)\n-{\n-\t/*\n-\t * The first character was '.', but that\n-\t * has already been discarded, we now test\n-\t * the rest.\n-\t */\n-\tswitch (*rest) {\n-\t/* \".\" is not allowed */\n-\tcase '\\0': case '/':\n-\t\treturn 0;\n-\n-\t/*\n-\t * \".git\" followed by  NUL or slash is bad. This\n-\t * shares the path end test with the \"..\" case.\n-\t */\n-\tcase 'g':\n-\t\tif (rest[1] != 'i')\n-\t\t\tbreak;\n-\t\tif (rest[2] != 't')\n-\t\t\tbreak;\n-\t\trest += 2;\n-\t/* fallthrough */\n-\tcase '.':\n-\t\tif (rest[1] == '\\0' || rest[1] == '/')\n-\t\t\treturn 0;\n-\t}\n-\treturn 1;\n-}\n-\n-static int verify_path(const char *path)\n-{\n-\tchar c;\n-\n-\tgoto inside;\n-\tfor (;;) {\n-\t\tif (!c)\n-\t\t\treturn 1;\n-\t\tif (c == '/') {\n-inside:\n-\t\t\tc = *path++;\n-\t\t\tswitch (c) {\n-\t\t\tdefault:\n-\t\t\t\tcontinue;\n-\t\t\tcase '/': case '\\0':\n-\t\t\t\tbreak;\n-\t\t\tcase '.':\n-\t\t\t\tif (verify_dotfile(path))\n-\t\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\treturn 0;\n-\t\t}\n-\t\tc = *path++;\n-\t}\n-}\n-\n static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \t\t\t const char *path, int stage)\n {\n"},{"id":"20233","messageId":"Pine.LNX.4.64.0605180917060.10823@g5.osdl.org","threadId":"4185","inReplyTo":"Pine.LNX.4.64.0605180807060.10823@g5.osdl.org","subject":"Re: Do \"git add\" as a builtin","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-18T16:25:31Z","receivedAt":"2006-05-18T16:25:31Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 May 2006, Linus Torvalds wrote:\n> \n> But if people are ok with changing it from a \"print a warning and ignore\" \n> into an _error_, we could just move it into \"add_cache_entry()\".\n\nThis is the incremental patch to do that instead, if you prefer it.\n\nPutting it into add_cache_entry() definitely has advantages, in that it \nshould make it much harder for this bug to happen again - all users will \nnow be verified.\n\nWith this one, it's now a fatal error to try to add a pathname that cannot \nbe added, ie\n\n\t[torvalds@g5 git]$ git add .git/config \n\tfatal: unable to add .git/config to index\n\nand\n\n\t[torvalds@g5 git]$ git add foo/../bar \n\tfatal: unable to add foo/../bar to index\n\ninstead of the old \"Ignoring path xyz\" warning that would end up silently \nsucceeding on any other paths.\n\n\t\tLinus\n\n---\ndiff --git a/builtin-add.c b/builtin-add.c\nindex 0346bb5..ac9ed2d 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -172,11 +172,6 @@ static int add_file_to_index(const char \n \tif (lstat(path, &st))\n \t\tdie(\"%s: unable to stat (%s)\", path, strerror(errno));\n \n-\tif (!verify_path(path)) {\n-\t\tfprintf(stderr, \"Ignoring path %s\\n\", path);\n-\t\treturn -1;\n-\t}\n-\t\t\n \tif (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode))\n \t\tdie(\"%s: can only add regular files or symbolic links\", path);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 6b323dd..9a272f8 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -551,6 +551,8 @@ int add_cache_entry(struct cache_entry *\n \n \tif (!ok_to_add)\n \t\treturn -1;\n+\tif (!verify_path(ce->name))\n+\t\treturn -1;\n \n \tif (!skip_df_check &&\n \t    check_file_directory_conflict(ce, pos, ok_to_replace)) {\n"},{"id":"20234","messageId":"Pine.LNX.4.64.0605180929450.10823@g5.osdl.org","threadId":"4185","inReplyTo":"Pine.LNX.4.64.0605180917060.10823@g5.osdl.org","subject":"Re: Do \"git add\" as a builtin","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-18T16:35:58Z","receivedAt":"2006-05-18T16:35:58Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 May 2006, Linus Torvalds wrote:\n> > \n> > But if people are ok with changing it from a \"print a warning and ignore\" \n> > into an _error_, we could just move it into \"add_cache_entry()\".\n> \n> This is the incremental patch to do that instead, if you prefer it.\n\nThinking some more about it, I think I personally much prefer this.\n\nEspecially as a quick look-through seems to say that there's actually a \npath through git-update-index too that allows a unverified filename to get \ninto the index (the new \"--unresolve\" thing also misses the need to verify \nthe path).\n\nMaking it a fatal error makes it more obvious that the user did something \nfundamentally wrong. And the safety in doing it in add_cache_entry() just \nmakes be happier, particularly in light of the above problem with \n--unresolve.\n\nThat still leaves the question of whether we should also drop all the \n\"verify_path()\" calls in update-index.c, and make it fatal there too. I \nthink we probably should.\n\n(At that point we could turn verify_path() back into a static function, \nthis time local entirely to read-cache.c, rather than update-index.c)\n\n\t\t\tLinus\n"},{"id":"20235","messageId":"7vzmhfnupz.fsf@assigned-by-dhcp.cox.net","threadId":"4185","inReplyTo":"Pine.LNX.4.64.0605180929450.10823@g5.osdl.org","subject":"Re: Do \"git add\" as a builtin","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-18T16:50:00Z","receivedAt":"2006-05-18T16:50:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> Thinking some more about it, I think I personally much prefer this.\n\nI like this much better than the old (pre-builtin) behaviour.\n\n> Especially as a quick look-through seems to say that there's actually a \n> path through git-update-index too that allows a unverified filename to get \n> into the index (the new \"--unresolve\" thing also misses the need to verify \n> the path).\n\nPerhaps, but unresolve splits an entry that is available from\nthe heads being merged, so it would use unverified filename to\ntry finding the ents from trees, but get_tree_entry() would not\nfind one, so I think we are safe already.  Nevertheless, I think\nyour change makes things more strict and safer.\n\nI doubt this would break people's scripts.  If they were relying\non the old behaviour, that means they threw real paths and\ngarbage at update-index and relied on it to sift them apart,\nwhich indicates they were buggy to begin with anyway.\n"},{"id":"20236","messageId":"Pine.LNX.4.64.0605181020570.10823@g5.osdl.org","threadId":"4185","inReplyTo":"7vzmhfnupz.fsf@assigned-by-dhcp.cox.net","subject":"Re: Do \"git add\" as a builtin","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-18T17:22:03Z","receivedAt":"2006-05-18T17:22:03Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 May 2006, Junio C Hamano wrote:\n> \n> Perhaps, but unresolve splits an entry that is available from\n> the heads being merged, so it would use unverified filename to\n> try finding the ents from trees, but get_tree_entry() would not\n> find one, so I think we are safe already.\n\nYes, I agree, that looks safe.\n\n> I doubt this would break people's scripts.  If they were relying\n> on the old behaviour, that means they threw real paths and\n> garbage at update-index and relied on it to sift them apart,\n> which indicates they were buggy to begin with anyway.\n\nAgreed. I don't think the semantic change can really break anything, and \nif anything will probably help expose any really strange/broken stuff.\n\n\t\tLinus\n"},{"id":"20270","messageId":"7vy7wyil4d.fsf_-_@assigned-by-dhcp.cox.net","threadId":"4185","inReplyTo":"7vsln74sv7.fsf_-_@assigned-by-dhcp.cox.net","subject":"FIXED: [WARNING] Please be careful when using \"git add\" from \"next\" branch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-19T06:28:34Z","receivedAt":"2006-05-19T06:28:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> There is still a small breakage in the built-in \"git add\" on the\n> \"next\" branch that ends up creating nonsense tree objects.\n\nThis was fixed this morning, in case I scared people.\n \n"}]}