{"thread":{"id":"7629","subject":"Teach git-update-index about gitlinks","startedAt":"2007-04-12T19:29:40Z","lastAt":"2007-04-13T08:14:24Z","messageCount":5,"participants":["Linus Torvalds","Junio C Hamano","Alex Riesen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"39227","messageId":"Pine.LNX.4.64.0704121218130.4061@woody.linux-foundation.org","threadId":"7629","inReplyTo":null,"subject":"Teach git-update-index about gitlinks","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-12T19:29:40Z","receivedAt":"2007-04-12T19:29:40Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nI finally got around to looking at Alex' patch to teach update-index about \ngitlinks too, so that \"git commit -a\" along with any other explicit \nupdate-index scripts can work.\n\nI don't think there was anything wrong with Alex' patch, but the code he \npatched I felt was just so ugly that the added cases just pushed it over \nthe edge. Especially as I don't think that patch necessarily did the right \nthing for a gitlink entry that already existed in the index, but that \nwasn't actually a real git repository in the working tree (just an empty \nsubdirectory or a non-git snapshot because it hadn't wanted to track that \nparticular subproject).\n\nSo I ended up deciding to clean up the git-update-index handling the same \nway I tackled the directory traversal used by git-add earlier: by \nsplitting the different cases up into multiple smaller functions, and just \nmaking the code easier to read (and adding more comments about the \ndifferent cases).\n\nSo this replaces the old \"process_file()\" with a new \"process_path()\" \nfunction that then just calls out to different helper functions depending \non what kind of path it is. Processing a nondirectory ends up being just \none of the simpler cases.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nAlex, I'd appreciate it if you gave this a look-over. I think the logic is \nfairly well-documented and the flow is fairly obvious (the patch is not \nquite as easy to read as the end result, but you can get the gist of it \nfrom the patch too if you're as used to unified diffs as I am..)\n\nJunio - if you prefer Alex' patch instead, I won't be upset. This is \ndefinitely a bigger re-org, and while I think it makes sense as a patch \neven *aside* from the gitlink support, it's probably largely a matter of \ntaste.\n\n builtin-update-index.c |  200 +++++++++++++++++++++++++++++++++---------------\n 1 files changed, 138 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 89aa0b0..022d5cc 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -9,6 +9,7 @@\n #include \"cache-tree.h\"\n #include \"tree-walk.h\"\n #include \"builtin.h\"\n+#include \"refs.h\"\n \n /*\n  * Default to not allowing changes to the list of files. The\n@@ -60,78 +61,153 @@ static int mark_valid(const char *path)\n \treturn -1;\n }\n \n-static int process_file(const char *path)\n+static int remove_one_path(const char *path)\n {\n-\tint size, namelen, option, status;\n-\tstruct cache_entry *ce;\n-\tstruct stat st;\n-\n-\tstatus = lstat(path, &st);\n-\n-\t/* We probably want to do this in remove_file_from_cache() and\n-\t * add_cache_entry() instead...\n-\t */\n-\tcache_tree_invalidate_path(active_cache_tree, path);\n+\tif (!allow_remove)\n+\t\treturn error(\"%s: does not exist and --remove not passed\", path);\n+\tif (remove_file_from_cache(path))\n+\t\treturn error(\"%s: cannot remove from the index\", path);\n+\treturn 0;\n+}\n \n-\tif (status < 0 || S_ISDIR(st.st_mode)) {\n-\t\t/* When we used to have \"path\" and now we want to add\n-\t\t * \"path/file\", we need a way to remove \"path\" before\n-\t\t * being able to add \"path/file\".  However,\n-\t\t * \"git-update-index --remove path\" would not work.\n-\t\t * --force-remove can be used but this is more user\n-\t\t * friendly, especially since we can do the opposite\n-\t\t * case just fine without --force-remove.\n-\t\t */\n-\t\tif (status == 0 || (errno == ENOENT || errno == ENOTDIR)) {\n-\t\t\tif (allow_remove) {\n-\t\t\t\tif (remove_file_from_cache(path))\n-\t\t\t\t\treturn error(\"%s: cannot remove from the index\",\n-\t\t\t\t\t             path);\n-\t\t\t\telse\n-\t\t\t\t\treturn 0;\n-\t\t\t} else if (status < 0) {\n-\t\t\t\treturn error(\"%s: does not exist and --remove not passed\",\n-\t\t\t\t             path);\n-\t\t\t}\n-\t\t}\n-\t\tif (0 == status)\n-\t\t\treturn error(\"%s: is a directory - add files inside instead\",\n-\t\t\t             path);\n-\t\telse\n-\t\t\treturn error(\"lstat(\\\"%s\\\"): %s\", path,\n-\t\t\t\t     strerror(errno));\n-\t}\n+/*\n+ * Handle a path that couldn't be lstat'ed. It's either:\n+ *  - missing file (ENOENT or ENOTDIR). That's ok if we're\n+ *    supposed to be removing it and the removal actually\n+ *    succeeds.\n+ *  - permission error. That's never ok.\n+ */\n+static int process_lstat_error(const char *path, int err)\n+{\n+\tif (err == ENOENT || err == ENOTDIR)\n+\t\treturn remove_one_path(path);\n+\treturn error(\"lstat(\\\"%s\\\"): %s\", path, strerror(errno));\n+}\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-\tif (trust_executable_bit && has_symlinks)\n-\t\tce->ce_mode = create_ce_mode(st.st_mode);\n-\telse {\n-\t\t/* If there is an existing entry, pick the mode bits and type\n-\t\t * from it, otherwise assume unexecutable regular file.\n-\t\t */\n-\t\tstruct cache_entry *ent;\n-\t\tint pos = cache_name_pos(path, namelen);\n+static int add_one_path(struct cache_entry *old, const char *path, int len, struct stat *st)\n+{\n+\tint option, size = cache_entry_size(len);\n+\tstruct cache_entry *ce = xcalloc(1, size);\n \n-\t\tent = (0 <= pos) ? active_cache[pos] : NULL;\n-\t\tce->ce_mode = ce_mode_from_stat(ent, st.st_mode);\n-\t}\n+\tmemcpy(ce->name, path, len);\n+\tce->ce_flags = htons(len);\n+\tfill_stat_cache_info(ce, st);\n+\tce->ce_mode = ce_mode_from_stat(old, st->st_mode);\n \n-\tif (index_path(ce->sha1, path, &st, !info_only))\n+\tif (index_path(ce->sha1, path, st, !info_only))\n \t\treturn -1;\n \toption = allow_add ? ADD_CACHE_OK_TO_ADD : 0;\n \toption |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;\n \tif (add_cache_entry(ce, option))\n-\t\treturn error(\"%s: cannot add to the index - missing --add option?\",\n-\t\t\t     path);\n+\t\treturn error(\"%s: cannot add to the index - missing --add option?\", path);\n \treturn 0;\n }\n \n+/*\n+ * Handle a path that was a directory. Four cases:\n+ *\n+ *  - it's already a gitlink in the index, and we keep it that\n+ *    way, and update it if we can (if we cannot find the HEAD,\n+ *    we're going to keep it unchanged in the index!)\n+ *\n+ *  - it's a *file* in the index, in which case it should be\n+ *    removed as a file if removal is allowed, since it doesn't\n+ *    exist as such any more. If removal isn't allowed, it's\n+ *    an error.\n+ *\n+ *    (NOTE! This is old and arguably fairly strange behaviour.\n+ *    We might want to make this an error unconditionally, and\n+ *    use \"--force-remove\" if you actually want to force removal).\n+ *\n+ *  - it used to exist as a subdirectory (ie multiple files with\n+ *    this particular prefix) in the index, in which case it's wrong\n+ *    to try to update it as a directory.\n+ *\n+ *  - it doesn't exist at all in the index, but it is a valid\n+ *    git directory, and it should be *added* as a gitlink.\n+ */\n+static int process_directory(const char *path, int len, struct stat *st)\n+{\n+\tunsigned char sha1[20];\n+\tint pos = cache_name_pos(path, len);\n+\n+\t/* Exact match: file or existing gitlink */\n+\tif (pos >= 0) {\n+\t\tstruct cache_entry *ce = active_cache[pos];\n+\t\tif (S_ISDIRLNK(ntohl(ce->ce_mode))) {\n+\n+\t\t\t/* Do nothing to the index if there is no HEAD! */\n+\t\t\tif (resolve_gitlink_ref(path, \"HEAD\", sha1) < 0)\n+\t\t\t\treturn 0;\n+\n+\t\t\treturn add_one_path(ce, path, len, st);\n+\t\t}\n+\t\t/* Should this be an unconditional error? */\n+\t\treturn remove_one_path(path);\n+\t}\n+\n+\t/* Inexact match: is there perhaps a subdirectory match? */\n+\tpos = -pos-1;\n+\twhile (pos < active_nr) {\n+\t\tstruct cache_entry *ce = active_cache[pos++];\n+\n+\t\tif (strncmp(ce->name, path, len))\n+\t\t\tbreak;\n+\t\tif (ce->name[len] > '/')\n+\t\t\tbreak;\n+\t\tif (ce->name[len] < '/')\n+\t\t\tcontinue;\n+\n+\t\t/* Subdirectory match - error out */\n+\t\treturn error(\"%s: is a directory - add individual files instead\", path);\n+\t}\n+\n+\t/* No match - should we add it as a gitlink? */\n+\tif (!resolve_gitlink_ref(path, \"HEAD\", sha1))\n+\t\treturn add_one_path(NULL, path, len, st);\n+\n+\t/* Error out. */\n+\treturn error(\"%s: is a directory - add files inside instead\", path);\n+}\n+\n+/*\n+ * Process a regular file\n+ */\n+static int process_file(const char *path, int len, struct stat *st)\n+{\n+\tint pos = cache_name_pos(path, len);\n+\tstruct cache_entry *ce = pos < 0 ? NULL : active_cache[pos];\n+\n+\tif (ce && S_ISDIRLNK(ntohl(ce->ce_mode)))\n+\t\treturn error(\"%s is already a gitlink, not replacing\", path);\n+\n+\treturn add_one_path(ce, path, len, st);\t\n+}\n+\n+static int process_path(const char *path)\n+{\n+\tint len;\n+\tstruct stat st;\n+\n+\t/* We probably want to do this in remove_file_from_cache() and\n+\t * add_cache_entry() instead...\n+\t */\n+\tcache_tree_invalidate_path(active_cache_tree, path);\n+\n+\t/*\n+\t * First things first: get the stat information, to decide\n+\t * what to do about the pathname!\n+\t */\n+\tif (lstat(path, &st) < 0)\n+\t\treturn process_lstat_error(path, errno);\n+\n+\tlen = strlen(path);\n+\tif (S_ISDIR(st.st_mode))\n+\t\treturn process_directory(path, len, &st);\n+\n+\treturn process_file(path, len, &st);\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@@ -210,8 +286,8 @@ static void update_one(const char *path, const char *prefix, int prefix_length)\n \t\treport(\"remove '%s'\", path);\n \t\tgoto free_return;\n \t}\n-\tif (process_file(p))\n-\t\tdie(\"Unable to process file %s\", path);\n+\tif (process_path(p))\n+\t\tdie(\"Unable to process path %s\", path);\n \treport(\"add '%s'\", path);\n  free_return:\n \tif (p < path || p > path + strlen(path))\n"},{"id":"39239","messageId":"7vtzvli53u.fsf@assigned-by-dhcp.cox.net","threadId":"7629","inReplyTo":"Pine.LNX.4.64.0704121218130.4061@woody.linux-foundation.org","subject":"Re: Teach git-update-index about gitlinks","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-12T22:45:57Z","receivedAt":"2007-04-12T22:45:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Junio - if you prefer Alex' patch instead, I won't be upset. This is \n> definitely a bigger re-org, and while I think it makes sense as a patch \n> even *aside* from the gitlink support, it's probably largely a matter of \n> taste.\n\nI find the result of applying this patch much easier to read\nthan the original.\n\n> +/*\n> + * Process a regular file\n> + */\n> +static int process_file(const char *path, int len, struct stat *st)\n> +{\n> +\tint pos = cache_name_pos(path, len);\n> +\tstruct cache_entry *ce = pos < 0 ? NULL : active_cache[pos];\n> +\n> +\tif (ce && S_ISDIRLNK(ntohl(ce->ce_mode)))\n> +\t\treturn error(\"%s is already a gitlink, not replacing\", path);\n> +\n> +\treturn add_one_path(ce, path, len, st);\t\n> +}\n\nI may be missing the obvious but if I have a subproject at\n\"path/S\" and I say \"update-index path/S/Makefile\", what should\nhappen?  There is ISDIRLNK entry at path/S and add_one_path()\nwould allow removal of \"path/S\" to make room for\npath/S/Makefile, when --replace is given.\n"},{"id":"39241","messageId":"Pine.LNX.4.64.0704121550540.4061@woody.linux-foundation.org","threadId":"7629","inReplyTo":"7vtzvli53u.fsf@assigned-by-dhcp.cox.net","subject":"Re: Teach git-update-index about gitlinks","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-12T22:59:41Z","receivedAt":"2007-04-12T22:59:41Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 12 Apr 2007, Junio C Hamano wrote:\n> \n> I find the result of applying this patch much easier to read\n> than the original.\n\nHey, that makes two of us then ;)\n\n> > +/*\n> > + * Process a regular file\n> > + */\n> > +static int process_file(const char *path, int len, struct stat *st)\n> > +{\n> > +\tint pos = cache_name_pos(path, len);\n> > +\tstruct cache_entry *ce = pos < 0 ? NULL : active_cache[pos];\n> > +\n> > +\tif (ce && S_ISDIRLNK(ntohl(ce->ce_mode)))\n> > +\t\treturn error(\"%s is already a gitlink, not replacing\", path);\n> > +\n> > +\treturn add_one_path(ce, path, len, st);\t\n> > +}\n> \n> I may be missing the obvious but if I have a subproject at\n> \"path/S\" and I say \"update-index path/S/Makefile\", what should\n> happen?  There is ISDIRLNK entry at path/S and add_one_path()\n> would allow removal of \"path/S\" to make room for\n> path/S/Makefile, when --replace is given.\n\nGood catch. I think we should quite possibly make that be a check in \n\"add_cache_entry()\", along with the check_file_directory_conflict() stuff.\n\nI don't think anything protects against it as-is.\n\nOf course, we could do it inside builtin-update-index, but we've generally \nhad the rule that \"add_cache_entry()\" is the thing that *enforces* any \nindex consistency rules, and the callers are just doing their own sanity \nchecks.\n\n\t\t\tLinus\n"},{"id":"39270","messageId":"81b0412b0704130042k1f5be3bay30cbbb6b70259f6@mail.gmail.com","threadId":"7629","inReplyTo":"Pine.LNX.4.64.0704121218130.4061@woody.linux-foundation.org","subject":"Re: Teach git-update-index about gitlinks","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-13T07:42:47Z","receivedAt":"2007-04-13T07:42:47Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/12/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:\n> +static int process_lstat_error(const char *path, int err)\n> +{\n> +       if (err == ENOENT || err == ENOTDIR)\n> +               return remove_one_path(path);\n> +       return error(\"lstat(\\\"%s\\\"): %s\", path, strerror(errno));\n\nYou forgot to change errno to err in strerror.\n\n> +static int add_one_path(struct cache_entry *old, const char *path, int len, struct stat *st)\n...\n>         option = allow_add ? ADD_CACHE_OK_TO_ADD : 0;\n>         option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;\n>         if (add_cache_entry(ce, option))\n> -               return error(\"%s: cannot add to the index - missing --add option?\",\n> -                            path);\n> +               return error(\"%s: cannot add to the index - missing --add option?\", path);\n\nreturn error(\"%s: cannot add to the index%s\", path, \"\\0 - missing\n--add option?\" + !allow_add);\nis be nicer. We really do know whether \"--add\" was passed or not.\n\n> + *    (NOTE! This is old and arguably fairly strange behaviour.\n> + *    We might want to make this an error unconditionally, and\n> + *    use \"--force-remove\" if you actually want to force removal).\n...\n> +               /* Should this be an unconditional error? */\n> +               return remove_one_path(path);\n\nMakes me uncomfortable too. Is it be possible anyone is depending\non it? Maybe it still can be changed?\n"},{"id":"39272","messageId":"81b0412b0704130114u2a7217cep6cd77b902e92bbd8@mail.gmail.com","threadId":"7629","inReplyTo":"81b0412b0704130042k1f5be3bay30cbbb6b70259f6@mail.gmail.com","subject":"Re: Teach git-update-index about gitlinks","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-13T08:14:24Z","receivedAt":"2007-04-13T08:14:24Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/13/07, Alex Riesen <raa.lkml@gmail.com> wrote:\n> > + *    (NOTE! This is old and arguably fairly strange behaviour.\n> > + *    We might want to make this an error unconditionally, and\n> > + *    use \"--force-remove\" if you actually want to force removal).\n> ...\n> > +               /* Should this be an unconditional error? */\n> > +               return remove_one_path(path);\n>\n> Makes me uncomfortable too. Is it be possible anyone is depending\n> on it? Maybe it still can be changed?\n>\n\nMaybe not. It actually makes sense: the old file (now a directory)\nis actually removed, so a plain --remove in its old meaning still\n_is_ correct: remove the file from index if it is removed from\nworking directory.\nAnd it breaks t1001.\n"}]}