{"thread":{"id":"34927","subject":"[PATCH 0/3] stop storing trailing slash in dir-hash","startedAt":"2013-09-13T11:15:40Z","lastAt":"2013-09-13T22:20:17Z","messageCount":9,"participants":["Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"227623","messageId":"1379070943-36595-1-git-send-email-sunshine@sunshineco.com","threadId":"34927","inReplyTo":null,"subject":"[PATCH 0/3] stop storing trailing slash in dir-hash","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-13T11:15:40Z","receivedAt":"2013-09-13T11:15:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This series alters name-hash so that it no longer stores the (redundant)\ntrailing slash with index_state.dir_hash entries. As an intentional\nside-effect, the series fixes [1] in a cleaner way (suggested by Junio\n[2]) than either [3] (680be044 in master) or [4].\n\nAs noted by Peff [5], this change is at a fairly fundamental level, so\ncare has been taken to ensure that all tests still pass (thus it at\nleast does not break anything covered by the tests).\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/232727\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/232727/focus=232813\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/232796\n[4]: http://thread.gmane.org/gmane.comp.version-control.git/232833\n[5]: http://thread.gmane.org/gmane.comp.version-control.git/232727/focus=232822\n\nEric Sunshine (3):\n  name-hash: refactor polymorphic index_name_exists()\n  name-hash: stop storing trailing '/' on paths in index_state.dir_hash\n  dir: revert work-around for retired dangerous behavior\n\n cache.h      |  2 ++\n dir.c        | 25 +++++++------------------\n name-hash.c  | 54 ++++++++++++++++++++++++++++--------------------------\n read-cache.c |  2 +-\n 4 files changed, 38 insertions(+), 45 deletions(-)\n\n-- \n1.8.4.457.g424cb08\n"},{"id":"227625","messageId":"1379070943-36595-2-git-send-email-sunshine@sunshineco.com","threadId":"34927","inReplyTo":"1379070943-36595-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 1/3] name-hash: refactor polymorphic index_name_exists()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-13T11:15:41Z","receivedAt":"2013-09-13T11:15:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Depending upon the absence or presence of a trailing '/' on the incoming\npathname, index_name_exists() checks either if a file is present in the\nindex or if a directory is represented within the index.  Each caller\nexplicitly chooses the mode of operation by adding or removing a\ntrailing '/' before invoking index_name_exists().\n\nSince these two modes of operations are disjoint and have no code in\ncommon (one searches index_state.name_hash; the other dir_hash), they\ncan be represented more naturally as distinct functions: one to search\nfor a file, and one for a directory.\n\nSplitting index searching into two functions relieves callers of the\nartificial burden of having to add or remove a slash to select the mode\nof operation; instead they just call the desired function. A subsequent\npatch will take advantage of this benefit in order to eliminate the\nrequirement that the incoming pathname for a directory search must have\na trailing slash.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n cache.h      |  2 ++\n dir.c        |  7 ++++---\n name-hash.c  | 47 ++++++++++++++++++++++++-----------------------\n read-cache.c |  2 +-\n 4 files changed, 31 insertions(+), 27 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 9ef778a..03ec24c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -314,6 +314,7 @@ extern void free_name_hash(struct index_state *istate);\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n+#define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))\n #define cache_name_exists(name, namelen, igncase) index_name_exists(&the_index, (name), (namelen), (igncase))\n #define cache_name_is_other(name, namelen) index_name_is_other(&the_index, (name), (namelen))\n #define resolve_undo_clear() resolve_undo_clear_index(&the_index)\n@@ -463,6 +464,7 @@ extern int write_index(struct index_state *, int newfd);\n extern int discard_index(struct index_state *);\n extern int unmerged_index(const struct index_state *);\n extern int verify_path(const char *path);\n+extern struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen);\n extern struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int igncase);\n extern int index_name_pos(const struct index_state *, const char *name, int namelen);\n #define ADD_CACHE_OK_TO_ADD 1\t\t/* Ok to add */\ndiff --git a/dir.c b/dir.c\nindex b439ff0..0080673 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -860,7 +860,8 @@ static struct dir_entry *dir_entry_new(const char *pathname, int len)\n \n static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len)\n {\n-\tif (cache_name_exists(pathname, len, ignore_case))\n+\tif ((len == 0 || pathname[len - 1] != '/') &&\n+\t    cache_name_exists(pathname, len, ignore_case))\n \t\treturn NULL;\n \n \tALLOC_GROW(dir->entries, dir->nr+1, dir->alloc);\n@@ -885,11 +886,11 @@ enum exist_status {\n /*\n  * Do not use the alphabetically sorted index to look up\n  * the directory name; instead, use the case insensitive\n- * name hash.\n+ * directory hash.\n  */\n static enum exist_status directory_exists_in_index_icase(const char *dirname, int len)\n {\n-\tconst struct cache_entry *ce = cache_name_exists(dirname, len + 1, ignore_case);\n+\tconst struct cache_entry *ce = cache_dir_exists(dirname, len + 1);\n \tunsigned char endchar;\n \n \tif (!ce)\ndiff --git a/name-hash.c b/name-hash.c\nindex 617c86c..5b01554 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -222,11 +222,35 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen\n \treturn slow_same_name(name, namelen, ce->name, len);\n }\n \n+struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen)\n+{\n+\tstruct cache_entry *ce;\n+\tstruct dir_entry *dir;\n+\n+\tlazy_init_name_hash(istate);\n+\tdir = find_dir_entry(istate, name, namelen);\n+\tif (dir && dir->nr)\n+\t\treturn dir->ce;\n+\n+\t/*\n+\t * It might be a submodule. Unlike plain directories, which are stored\n+\t * in the dir-hash, submodules are stored in the name-hash, so check\n+\t * there, as well.\n+\t */\n+\tce = index_name_exists(istate, name, namelen - 1, 1);\n+\tif (ce && S_ISGITLINK(ce->ce_mode))\n+\t\treturn ce;\n+\n+\treturn NULL;\n+}\n+\n struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int icase)\n {\n \tunsigned int hash = hash_name(name, namelen);\n \tstruct cache_entry *ce;\n \n+\tassert(namelen == 0 || name[namelen - 1] != '/');\n+\n \tlazy_init_name_hash(istate);\n \tce = lookup_hash(hash, &istate->name_hash);\n \n@@ -237,29 +261,6 @@ struct cache_entry *index_name_exists(struct index_state *istate, const char *na\n \t\t}\n \t\tce = ce->next;\n \t}\n-\n-\t/*\n-\t * When looking for a directory (trailing '/'), it might be a\n-\t * submodule or a directory. Despite submodules being directories,\n-\t * they are stored in the name hash without a closing slash.\n-\t * When ignore_case is 1, directories are stored in a separate hash\n-\t * table *with* their closing slash.\n-\t *\n-\t * The side effect of this storage technique is we have need to\n-\t * lookup the directory in a separate hash table, and if not found\n-\t * remove the slash from name and perform the lookup again without\n-\t * the slash.  If a match is made, S_ISGITLINK(ce->mode) will be\n-\t * true.\n-\t */\n-\tif (icase && name[namelen - 1] == '/') {\n-\t\tstruct dir_entry *dir = find_dir_entry(istate, name, namelen);\n-\t\tif (dir && dir->nr)\n-\t\t\treturn dir->ce;\n-\n-\t\tce = index_name_exists(istate, name, namelen - 1, icase);\n-\t\tif (ce && S_ISGITLINK(ce->ce_mode))\n-\t\t\treturn ce;\n-\t}\n \treturn NULL;\n }\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 885943a..a59644a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -643,7 +643,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \t\t\tif (*ptr == '/') {\n \t\t\t\tstruct cache_entry *foundce;\n \t\t\t\t++ptr;\n-\t\t\t\tfoundce = index_name_exists(istate, ce->name, ptr - ce->name, ignore_case);\n+\t\t\t\tfoundce = index_dir_exists(istate, ce->name, ptr - ce->name);\n \t\t\t\tif (foundce) {\n \t\t\t\t\tmemcpy((void *)startPtr, foundce->name + (startPtr - ce->name), ptr - startPtr);\n \t\t\t\t\tstartPtr = ptr;\n-- \n1.8.4.457.g424cb08\n"},{"id":"227624","messageId":"1379070943-36595-3-git-send-email-sunshine@sunshineco.com","threadId":"34927","inReplyTo":"1379070943-36595-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 2/3] name-hash: stop storing trailing '/' on paths in index_state.dir_hash","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-13T11:15:42Z","receivedAt":"2013-09-13T11:15:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When 5102c617 (Add case insensitivity support for directories when using\ngit status, 2010-10-03) added directories to the name-hash there was\nonly a single hash table in which both real cache entries and leading\ndirectory prefixes were registered. To distinguish between the two types\nof entries, directories were stored with a trailing '/'.\n\n2092678c (name-hash.c: fix endless loop with core.ignorecase=true,\n2013-02-28), however, moved directories to a separate hash table\n(index_state.dir_hash) but retained the now-redundant trailing '/', thus\ncallers still bear the burden of ensuring its presence before searching\nvia index_dir_exists(). Eliminate this redundancy by storing paths in\nthe dir-hash without the trailing '/'.\n\nAn important benefit of this change is that it eliminates undocumented\nand dangerous behavior of dir.c:directory_exists_in_index_icase() in\nwhich it assumes not only that it can validly access one character\nbeyond the end of its incoming directory argument, but also that that\ncharacter will unconditionally be a '/'. This perilous behavior was\n\"tolerated\" because the string passed in by its lone caller always had a\n'/' in that position, however, things broke [1] when 2eac2a4c (ls-files\n-k: a directory only can be killed if the index has a non-directory,\n2013-08-15) added a new caller which failed to respect the undocumented\nassumption.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/232727\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n dir.c        | 2 +-\n name-hash.c  | 9 +++++----\n read-cache.c | 2 +-\n 3 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 0080673..69d04a0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -890,7 +890,7 @@ enum exist_status {\n  */\n static enum exist_status directory_exists_in_index_icase(const char *dirname, int len)\n {\n-\tconst struct cache_entry *ce = cache_dir_exists(dirname, len + 1);\n+\tconst struct cache_entry *ce = cache_dir_exists(dirname, len);\n \tunsigned char endchar;\n \n \tif (!ce)\ndiff --git a/name-hash.c b/name-hash.c\nindex 5b01554..2bae75d 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -58,9 +58,9 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,\n {\n \t/*\n \t * Throw each directory component in the hash for quick lookup\n-\t * during a git status. Directory components are stored with their\n+\t * during a git status. Directory components are stored without their\n \t * closing slash.  Despite submodules being a directory, they never\n-\t * reach this point, because they are stored without a closing slash\n+\t * reach this point, because they are stored\n \t * in index_state.name_hash (as ordinary cache_entries).\n \t *\n \t * Note that the cache_entry stored with the dir_entry merely\n@@ -78,6 +78,7 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,\n \t\tnamelen--;\n \tif (namelen <= 0)\n \t\treturn NULL;\n+\tnamelen--;\n \n \t/* lookup existing entry for that directory */\n \tdir = find_dir_entry(istate, ce->name, namelen);\n@@ -97,7 +98,7 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,\n \t\t}\n \n \t\t/* recursively add missing parent directories */\n-\t\tdir->parent = hash_dir_entry(istate, ce, namelen - 1);\n+\t\tdir->parent = hash_dir_entry(istate, ce, namelen);\n \t}\n \treturn dir;\n }\n@@ -237,7 +238,7 @@ struct cache_entry *index_dir_exists(struct index_state *istate, const char *nam\n \t * in the dir-hash, submodules are stored in the name-hash, so check\n \t * there, as well.\n \t */\n-\tce = index_name_exists(istate, name, namelen - 1, 1);\n+\tce = index_name_exists(istate, name, namelen, 1);\n \tif (ce && S_ISGITLINK(ce->ce_mode))\n \t\treturn ce;\n \ndiff --git a/read-cache.c b/read-cache.c\nindex a59644a..8990b61 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -643,7 +643,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \t\t\tif (*ptr == '/') {\n \t\t\t\tstruct cache_entry *foundce;\n \t\t\t\t++ptr;\n-\t\t\t\tfoundce = index_dir_exists(istate, ce->name, ptr - ce->name);\n+\t\t\t\tfoundce = index_dir_exists(istate, ce->name, ptr - ce->name - 1);\n \t\t\t\tif (foundce) {\n \t\t\t\t\tmemcpy((void *)startPtr, foundce->name + (startPtr - ce->name), ptr - startPtr);\n \t\t\t\t\tstartPtr = ptr;\n-- \n1.8.4.457.g424cb08\n"},{"id":"227626","messageId":"1379070943-36595-4-git-send-email-sunshine@sunshineco.com","threadId":"34927","inReplyTo":"1379070943-36595-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 3/3] dir: revert work-around for retired dangerous behavior","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-13T11:15:43Z","receivedAt":"2013-09-13T11:15:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"directory_exists_in_index_icase() dangerously assumed that it could\naccess one character beyond the end of its directory argument, and that\nthat character would unconditionally be '/'.  2eac2a4c (ls-files -k: a\ndirectory only can be killed if the index has a non-directory,\n2013-08-15) added a caller which did not respect this undocumented\nassumption, and 680be044 (dir.c::test_one_path(): work around\ndirectory_exists_in_index_icase() breakage, 2013-08-23) added a\nwork-around which temporarily appends a '/' before invoking\ndirectory_exists_in_index_icase().\n\nSince the dangerous behavior of directory_exists_in_index_icase() has\nbeen eliminated, the work-around is now redundant, so retire it (but not\nthe tests added by the same commit).\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n dir.c | 18 +++---------------\n 1 file changed, 3 insertions(+), 15 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 69d04a0..ef95160 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1161,21 +1161,9 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \t */\n \tif ((dir->flags & DIR_COLLECT_KILLED_ONLY) &&\n \t    (dtype == DT_DIR) &&\n-\t    !has_path_in_index) {\n-\t\t/*\n-\t\t * NEEDSWORK: directory_exists_in_index_icase()\n-\t\t * assumes that one byte past the given path is\n-\t\t * readable and has '/', which needs to be fixed, but\n-\t\t * until then, work it around in the caller.\n-\t\t */\n-\t\tstrbuf_addch(path, '/');\n-\t\tif (directory_exists_in_index(path->buf, path->len - 1) ==\n-\t\t    index_nonexistent) {\n-\t\t\tstrbuf_setlen(path, path->len - 1);\n-\t\t\treturn path_none;\n-\t\t}\n-\t\tstrbuf_setlen(path, path->len - 1);\n-\t}\n+\t    !has_path_in_index &&\n+\t    (directory_exists_in_index(path->buf, path->len) == index_nonexistent))\n+\t\treturn path_none;\n \n \texclude = is_excluded(dir, path->buf, &dtype);\n \n-- \n1.8.4.457.g424cb08\n"},{"id":"227641","messageId":"xmqq38p8k1kf.fsf@gitster.dls.corp.google.com","threadId":"34927","inReplyTo":"1379070943-36595-2-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH 1/3] name-hash: refactor polymorphic index_name_exists()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T18:40:16Z","receivedAt":"2013-09-13T18:40:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Depending upon the absence or presence of a trailing '/' on the incoming\n> pathname, index_name_exists() checks either if a file is present in the\n> index or if a directory is represented within the index.  Each caller\n> explicitly chooses the mode of operation by adding or removing a\n> trailing '/' before invoking index_name_exists().\n>\n> Since these two modes of operations are disjoint and have no code in\n> common (one searches index_state.name_hash; the other dir_hash), they\n> can be represented more naturally as distinct functions: one to search\n> for a file, and one for a directory.\n>\n> Splitting index searching into two functions relieves callers of the\n> artificial burden of having to add or remove a slash to select the mode\n> of operation; instead they just call the desired function. A subsequent\n> patch will take advantage of this benefit in order to eliminate the\n> requirement that the incoming pathname for a directory search must have\n> a trailing slash.\n\nGood thinking.  Thanks for working on this; I agree with the general\ndirection this series is going.\n\nI however wonder if it would be a good idea to rename the other one\nto {cache|index}_file_exists(), and even keep the *_name_exists()\nvariant that switches between the two based on the trailing slash,\nto avoid breaking other topics in flight that may have added new\ncallsites to *_name_exists().  Otherwise the new callsites will feed\na path with a trailing slash to *_name_exists() and get a false\nresult, no?\n\n\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>  cache.h      |  2 ++\n>  dir.c        |  7 ++++---\n>  name-hash.c  | 47 ++++++++++++++++++++++++-----------------------\n>  read-cache.c |  2 +-\n>  4 files changed, 31 insertions(+), 27 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 9ef778a..03ec24c 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -314,6 +314,7 @@ extern void free_name_hash(struct index_state *istate);\n>  #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n>  #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n>  #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n> +#define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))\n>  #define cache_name_exists(name, namelen, igncase) index_name_exists(&the_index, (name), (namelen), (igncase))\n>  #define cache_name_is_other(name, namelen) index_name_is_other(&the_index, (name), (namelen))\n>  #define resolve_undo_clear() resolve_undo_clear_index(&the_index)\n> @@ -463,6 +464,7 @@ extern int write_index(struct index_state *, int newfd);\n>  extern int discard_index(struct index_state *);\n>  extern int unmerged_index(const struct index_state *);\n>  extern int verify_path(const char *path);\n> +extern struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen);\n>  extern struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int igncase);\n>  extern int index_name_pos(const struct index_state *, const char *name, int namelen);\n>  #define ADD_CACHE_OK_TO_ADD 1\t\t/* Ok to add */\n> diff --git a/dir.c b/dir.c\n> index b439ff0..0080673 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -860,7 +860,8 @@ static struct dir_entry *dir_entry_new(const char *pathname, int len)\n>  \n>  static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len)\n>  {\n> -\tif (cache_name_exists(pathname, len, ignore_case))\n> +\tif ((len == 0 || pathname[len - 1] != '/') &&\n> +\t    cache_name_exists(pathname, len, ignore_case))\n>  \t\treturn NULL;\n>  \n>  \tALLOC_GROW(dir->entries, dir->nr+1, dir->alloc);\n> @@ -885,11 +886,11 @@ enum exist_status {\n>  /*\n>   * Do not use the alphabetically sorted index to look up\n>   * the directory name; instead, use the case insensitive\n> - * name hash.\n> + * directory hash.\n>   */\n>  static enum exist_status directory_exists_in_index_icase(const char *dirname, int len)\n>  {\n> -\tconst struct cache_entry *ce = cache_name_exists(dirname, len + 1, ignore_case);\n> +\tconst struct cache_entry *ce = cache_dir_exists(dirname, len + 1);\n>  \tunsigned char endchar;\n>  \n>  \tif (!ce)\n> diff --git a/name-hash.c b/name-hash.c\n> index 617c86c..5b01554 100644\n> --- a/name-hash.c\n> +++ b/name-hash.c\n> @@ -222,11 +222,35 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen\n>  \treturn slow_same_name(name, namelen, ce->name, len);\n>  }\n>  \n> +struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen)\n> +{\n> +\tstruct cache_entry *ce;\n> +\tstruct dir_entry *dir;\n> +\n> +\tlazy_init_name_hash(istate);\n> +\tdir = find_dir_entry(istate, name, namelen);\n> +\tif (dir && dir->nr)\n> +\t\treturn dir->ce;\n> +\n> +\t/*\n> +\t * It might be a submodule. Unlike plain directories, which are stored\n> +\t * in the dir-hash, submodules are stored in the name-hash, so check\n> +\t * there, as well.\n> +\t */\n> +\tce = index_name_exists(istate, name, namelen - 1, 1);\n> +\tif (ce && S_ISGITLINK(ce->ce_mode))\n> +\t\treturn ce;\n> +\n> +\treturn NULL;\n> +}\n> +\n>  struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int icase)\n>  {\n>  \tunsigned int hash = hash_name(name, namelen);\n>  \tstruct cache_entry *ce;\n>  \n> +\tassert(namelen == 0 || name[namelen - 1] != '/');\n> +\n>  \tlazy_init_name_hash(istate);\n>  \tce = lookup_hash(hash, &istate->name_hash);\n>  \n> @@ -237,29 +261,6 @@ struct cache_entry *index_name_exists(struct index_state *istate, const char *na\n>  \t\t}\n>  \t\tce = ce->next;\n>  \t}\n> -\n> -\t/*\n> -\t * When looking for a directory (trailing '/'), it might be a\n> -\t * submodule or a directory. Despite submodules being directories,\n> -\t * they are stored in the name hash without a closing slash.\n> -\t * When ignore_case is 1, directories are stored in a separate hash\n> -\t * table *with* their closing slash.\n> -\t *\n> -\t * The side effect of this storage technique is we have need to\n> -\t * lookup the directory in a separate hash table, and if not found\n> -\t * remove the slash from name and perform the lookup again without\n> -\t * the slash.  If a match is made, S_ISGITLINK(ce->mode) will be\n> -\t * true.\n> -\t */\n> -\tif (icase && name[namelen - 1] == '/') {\n> -\t\tstruct dir_entry *dir = find_dir_entry(istate, name, namelen);\n> -\t\tif (dir && dir->nr)\n> -\t\t\treturn dir->ce;\n> -\n> -\t\tce = index_name_exists(istate, name, namelen - 1, icase);\n> -\t\tif (ce && S_ISGITLINK(ce->ce_mode))\n> -\t\t\treturn ce;\n> -\t}\n>  \treturn NULL;\n>  }\n>  \n> diff --git a/read-cache.c b/read-cache.c\n> index 885943a..a59644a 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -643,7 +643,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n>  \t\t\tif (*ptr == '/') {\n>  \t\t\t\tstruct cache_entry *foundce;\n>  \t\t\t\t++ptr;\n> -\t\t\t\tfoundce = index_name_exists(istate, ce->name, ptr - ce->name, ignore_case);\n> +\t\t\t\tfoundce = index_dir_exists(istate, ce->name, ptr - ce->name);\n>  \t\t\t\tif (foundce) {\n>  \t\t\t\t\tmemcpy((void *)startPtr, foundce->name + (startPtr - ce->name), ptr - startPtr);\n>  \t\t\t\t\tstartPtr = ptr;\n"},{"id":"227642","messageId":"CAPig+cQxvHPbxy9YE7qYTVEXy1JCXkmBi48QrkL=bvhKRgryuQ@mail.gmail.com","threadId":"34927","inReplyTo":"xmqq38p8k1kf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] name-hash: refactor polymorphic index_name_exists()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-13T19:29:20Z","receivedAt":"2013-09-13T19:29:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 13, 2013 at 2:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n>> Depending upon the absence or presence of a trailing '/' on the incoming\n>> pathname, index_name_exists() checks either if a file is present in the\n>> index or if a directory is represented within the index.  Each caller\n>> explicitly chooses the mode of operation by adding or removing a\n>> trailing '/' before invoking index_name_exists().\n>>\n>> Since these two modes of operations are disjoint and have no code in\n>> common (one searches index_state.name_hash; the other dir_hash), they\n>> can be represented more naturally as distinct functions: one to search\n>> for a file, and one for a directory.\n>>\n>> Splitting index searching into two functions relieves callers of the\n>> artificial burden of having to add or remove a slash to select the mode\n>> of operation; instead they just call the desired function. A subsequent\n>> patch will take advantage of this benefit in order to eliminate the\n>> requirement that the incoming pathname for a directory search must have\n>> a trailing slash.\n>\n> Good thinking.  Thanks for working on this; I agree with the general\n> direction this series is going.\n>\n> I however wonder if it would be a good idea to rename the other one\n> to {cache|index}_file_exists(), and even keep the *_name_exists()\n> variant that switches between the two based on the trailing slash,\n> to avoid breaking other topics in flight that may have added new\n> callsites to *_name_exists().  Otherwise the new callsites will feed\n> a path with a trailing slash to *_name_exists() and get a false\n> result, no?\n\nAn assert(\"no trailing /\") was added to index_name_exists() precisely\nfor the purpose of catching cases of code incorrectly calling\nindex_name_exists() to search for a directory. No existing code in\n'master' trips the assertion (at least according to the tests),\nhowever, the assertion may be annoying for topics in flight which do.\n\nOther than plopping these patches atop 'pu' and seeing if anything\nbreaks, what would be the \"git way\" of detecting in-flight topics\nwhich add callers of index_name_exists()? (Excuse my git ignorance.)\n"},{"id":"227655","messageId":"CAPig+cS4x1h3v2=0T95+g2_08_7qZj7fUsSiLgDtFyRSbFE0bA@mail.gmail.com","threadId":"34927","inReplyTo":"CAPc5daVtDByrA6yakk_1fq9g5Hv3naNDzEho5G4Ghxc6jzpawg@mail.gmail.com","subject":"Re: [PATCH 1/3] name-hash: refactor polymorphic index_name_exists()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-13T21:44:43Z","receivedAt":"2013-09-13T21:44:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 13, 2013 at 3:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> On Fri, Sep 13, 2013 at 12:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Fri, Sep 13, 2013 at 2:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Eric Sunshine <sunshine@sunshineco.com> writes:\n>>>\n>>>> Since these two modes of operations are disjoint and have no code in\n>>>> common (one searches index_state.name_hash; the other dir_hash), they\n>>>> can be represented more naturally as distinct functions: one to search\n>>>> for a file, and one for a directory.\n>>>\n>>> Good thinking.  Thanks for working on this; I agree with the general\n>>> direction this series is going.\n>>>\n>>> I however wonder if it would be a good idea to rename the other one\n>>> to {cache|index}_file_exists(), and even keep the *_name_exists()\n>>> variant that switches between the two based on the trailing slash,\n>>> to avoid breaking other topics in flight that may have added new\n>>> callsites to *_name_exists().  Otherwise the new callsites will feed\n>>> a path with a trailing slash to *_name_exists() and get a false\n>>> result, no?\n>>\n>> An assert(\"no trailing /\") was added to index_name_exists() precisely\n>> for the purpose of catching cases of code incorrectly calling\n>> index_name_exists() to search for a directory. No existing code in\n>> 'master' trips the assertion (at least according to the tests),\n>> however, the assertion may be annoying for topics in flight which do.\n>>\n>> Other than plopping these patches atop 'pu' and seeing if anything\n>> breaks, what would be the \"git way\" of detecting in-flight topics\n>> which add callers of index_name_exists()? (Excuse my git ignorance.)\n>\n> I would do a quick\n>\n> $ git diff master..pu | grep '^+.*_name_exists'\n>\n> which would be more conservative (it would also show an invocation\n> moved from one place to another).\n\nThere appear to be no topics in flight which add new\nindex_name_exists() callers (which is not to say that no new callers\nwill be introduced before this topic lands in 'master').\n\nI also plopped the patches atop 'pu' and there are no new tests\nfailures on Linux or Mac OS X, so the series does not seem to break\nanything in flight. (There are a few test failures on 'pu' on Mac OS\nX, but they are not related to this series.)\n\nGiven the above. How should I proceed? Do you still feel that it is\nadvisable to keep an index_name_exists() around for compatibility\nreasons in case any new callers are introduced? Regardless of that\nanswer, do you want index_name_exists() renamed to\nindex_file_exists()?\n"},{"id":"227657","messageId":"xmqqfvt8iczh.fsf@gitster.dls.corp.google.com","threadId":"34927","inReplyTo":"CAPig+cS4x1h3v2=0T95+g2_08_7qZj7fUsSiLgDtFyRSbFE0bA@mail.gmail.com","subject":"Re: [PATCH 1/3] name-hash: refactor polymorphic index_name_exists()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T22:16:34Z","receivedAt":"2013-09-13T22:16:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Given the above. How should I proceed? Do you still feel that it is\n> advisable to keep an index_name_exists() around for compatibility\n> reasons in case any new callers are introduced? Regardless of that\n> answer, do you want index_name_exists() renamed to\n> index_file_exists()?\n\nRenaming *_name_exists() to *_file_exists() without keeping a\ncompatibility one will force new topics to be rebased on this\nseries.  Alternatively we could merge them to 'pu' (and later 'next'\nand 'master') with evil merges to adjust the change in the semantics\nof the called function.  That increases the risk of accidental\nbreakages, I think.\n\nIt is safer to keep index_name_exists() around with the older\nsemantics, if we can, and rename your \"file only\" one to a different\nname.  That way, even if a new topic still uses index_name_exists()\nexpecting the traditional behaviour, it will not break immediately\nand we do not need to risk evil merges making mistakes.\n\nLater, we can \"git grep _name_exists\" to spot them and convert such\nold-style calls to either \"directory only\" or \"file only\" variants\nafter this series and these follow-on topics hit 'master' (and we do\nnot know at this point in what order that happens).\n\nHmm?\n"},{"id":"227659","messageId":"CAPig+cR8oYcp1ERUtW7wUvjFN0j0921iBOUjYKor59gTOb+Zcw@mail.gmail.com","threadId":"34927","inReplyTo":"xmqqfvt8iczh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] name-hash: refactor polymorphic index_name_exists()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-13T22:20:17Z","receivedAt":"2013-09-13T22:20:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 13, 2013 at 6:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n>> Given the above. How should I proceed? Do you still feel that it is\n>> advisable to keep an index_name_exists() around for compatibility\n>> reasons in case any new callers are introduced? Regardless of that\n>> answer, do you want index_name_exists() renamed to\n>> index_file_exists()?\n>\n> Renaming *_name_exists() to *_file_exists() without keeping a\n> compatibility one will force new topics to be rebased on this\n> series.  Alternatively we could merge them to 'pu' (and later 'next'\n> and 'master') with evil merges to adjust the change in the semantics\n> of the called function.  That increases the risk of accidental\n> breakages, I think.\n>\n> It is safer to keep index_name_exists() around with the older\n> semantics, if we can, and rename your \"file only\" one to a different\n> name.  That way, even if a new topic still uses index_name_exists()\n> expecting the traditional behaviour, it will not break immediately\n> and we do not need to risk evil merges making mistakes.\n>\n> Later, we can \"git grep _name_exists\" to spot them and convert such\n> old-style calls to either \"directory only\" or \"file only\" variants\n> after this series and these follow-on topics hit 'master' (and we do\n> not know at this point in what order that happens).\n\nThanks. That's what I needed to know. I'll re-roll with the suggested changes.\n\n(And, I'm looking into the Mac-only test breakages not related to this\npatch series.)\n"}]}