{"thread":{"id":"40033","subject":"[PATCH v2] untracked-cache: fix subdirectory handling","startedAt":"2015-08-07T17:17:53Z","lastAt":"2015-08-15T07:32:19Z","messageCount":2,"participants":["David Turner","Duy Nguyen"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"267619","messageId":"1438967873-792-1-git-send-email-dturner@twopensource.com","threadId":"40033","inReplyTo":null,"subject":"[PATCH v2] untracked-cache: fix subdirectory handling","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-08-07T17:17:53Z","receivedAt":"2015-08-07T17:17:53Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Previously, some calls lookup_untracked would pass a full path.  But\nlookup_untracked assumes that the portion of the path up to and\nincluding to the untracked_cache_dir has been removed.  So\nlookup_untracked would be looking in the untracked_cache for 'foo' for\n'foo/bar' (instead of just looking for 'bar').  This would cause\nuntracked cache corruption.\n\nTo fix this, untracked_cache_dir learns to track its depth.  Callers\nto lookup_untracked which have a full path now first trim off a\nsufficient number of path prefixes.  A new helper function,\nlookup_untracked_recursive, helps untracked_cache_invalidate_path to\nperform this operation.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\nThis version removes debugging cruft.  Oops!\n\n---\n dir.c                             | 50 ++++++++++++++++++++-------\n dir.h                             |  1 +\n t/t7063-status-untracked-cache.sh | 72 ++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 110 insertions(+), 13 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex e7b89fe..314080b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -631,6 +631,7 @@ static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,\n \tmemset(d, 0, sizeof(*d));\n \tmemcpy(d->name, name, len);\n \td->name[len] = '\\0';\n+\td->depth = dir->depth + 1;\n \n \tALLOC_GROW(dir->dirs, dir->dirs_nr + 1, dir->dirs_alloc);\n \tmemmove(dir->dirs + first + 1, dir->dirs + first,\n@@ -1324,7 +1325,19 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \tif (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))\n \t\treturn exclude ? path_excluded : path_untracked;\n \n-\tuntracked = lookup_untracked(dir->untracked, untracked, dirname, len);\n+\tif (untracked) {\n+\t\tconst char *cur = dirname;\n+\t\tint i;\n+\n+\t\tfor (i = 0; i < untracked->depth; i++) {\n+\t\t\tcur = strchr(cur, '/');\n+\t\t\tassert(cur);\n+\t\t\tcur++;\n+\t\t}\n+\t\tuntracked = lookup_untracked(dir->untracked, untracked,\n+\t\t\t\t\t     cur,\n+\t\t\t\t\t     len - (cur - dirname));\n+\t}\n \treturn read_directory_recursive(dir, dirname, len,\n \t\t\t\t\tuntracked, 1, simplify);\n }\n@@ -2431,7 +2444,7 @@ static void stat_data_from_disk(struct stat_data *to, const struct stat_data *fr\n }\n \n static int read_one_dir(struct untracked_cache_dir **untracked_,\n-\t\t\tstruct read_data *rd)\n+\t\t\tstruct read_data *rd, int depth)\n {\n \tstruct untracked_cache_dir ud, *untracked;\n \tconst unsigned char *next, *data = rd->data, *end = rd->end;\n@@ -2444,6 +2457,7 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,\n \tvalue = decode_varint(&next);\n \tif (next > end)\n \t\treturn -1;\n+\tud.depth = depth;\n \tud.recurse\t   = 1;\n \tud.untracked_alloc = value;\n \tud.untracked_nr\t   = value;\n@@ -2480,7 +2494,7 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,\n \trd->data = data;\n \n \tfor (i = 0; i < untracked->dirs_nr; i++) {\n-\t\tlen = read_one_dir(untracked->dirs + i, rd);\n+\t\tlen = read_one_dir(untracked->dirs + i, rd, depth + 1);\n \t\tif (len < 0)\n \t\t\treturn -1;\n \t}\n@@ -2577,7 +2591,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long\n \trd.index      = 0;\n \trd.ucd        = xmalloc(sizeof(*rd.ucd) * len);\n \n-\tif (read_one_dir(&uc->root, &rd) || rd.index != len)\n+\tif (read_one_dir(&uc->root, &rd, 0) || rd.index != len)\n \t\tgoto done;\n \n \tnext = rd.data;\n@@ -2614,20 +2628,32 @@ done2:\n \treturn uc;\n }\n \n+static struct untracked_cache_dir *lookup_untracked_recursive(\n+\tstruct untracked_cache *uc, struct untracked_cache_dir *dir,\n+\tconst char *path, int len)\n+{\n+\tconst char *rest = strchr(path, '/');\n+\n+\tif (rest) {\n+\t\tint component_len = rest - path;\n+\t\tstruct untracked_cache_dir *d =\n+\t\t\tlookup_untracked(uc, dir, path, component_len);\n+\t\treturn lookup_untracked_recursive(uc, d, rest + 1,\n+\t\t\t\t\t\t  len - (component_len + 1));\n+\t} else {\n+\t\treturn dir;\n+\t}\n+}\n+\n void untracked_cache_invalidate_path(struct index_state *istate,\n \t\t\t\t     const char *path)\n {\n-\tconst char *sep;\n \tstruct untracked_cache_dir *d;\n \tif (!istate->untracked || !istate->untracked->root)\n \t\treturn;\n-\tsep = strrchr(path, '/');\n-\tif (sep)\n-\t\td = lookup_untracked(istate->untracked,\n-\t\t\t\t     istate->untracked->root,\n-\t\t\t\t     path, sep - path);\n-\telse\n-\t\td = istate->untracked->root;\n+\td = lookup_untracked_recursive(istate->untracked,\n+\t\t\t\t       istate->untracked->root,\n+\t\t\t\t       path, strlen(path));\n \tistate->untracked->dir_invalidated++;\n \td->valid = 0;\n \td->untracked_nr = 0;\ndiff --git a/dir.h b/dir.h\nindex 7b5855d..885ed43 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -109,6 +109,7 @@ struct sha1_stat {\n  *  excludes_file_sha1[])\n  */\n struct untracked_cache_dir {\n+\tunsigned int depth;\n \tstruct untracked_cache_dir **dirs;\n \tchar **untracked;\n \tstruct stat_data stat_data;\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex ff23f4e..ca8dc3a 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -408,7 +408,8 @@ test_expect_success 'set up sparse checkout' '\n \ttest_path_is_file done/one\n '\n \n-test_expect_success 'create files, some of which are gitignored' '\n+test_expect_success 'create/modify files, some of which are gitignored' '\n+\techo two bis >done/two &&\n \techo three >done/three && # three is gitignored\n \techo four >done/four && # four is gitignored at a higher level\n \techo five >done/five # five is not gitignored\n@@ -420,6 +421,7 @@ test_expect_success 'test sparse status with untracked cache' '\n \tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace\" \\\n \tgit status --porcelain >../status.actual &&\n \tcat >../status.expect <<EOF &&\n+ M done/two\n ?? .gitignore\n ?? done/five\n ?? dtwo/\n@@ -459,6 +461,7 @@ test_expect_success 'test sparse status again with untracked cache' '\n \tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace\" \\\n \tgit status --porcelain >../status.actual &&\n \tcat >../status.expect <<EOF &&\n+ M done/two\n ?? .gitignore\n ?? done/five\n ?? dtwo/\n@@ -473,4 +476,71 @@ EOF\n \ttest_cmp ../trace.expect ../trace\n '\n \n+test_expect_success 'set up for test of subdir and sparse checkouts' '\n+\tmkdir done/sub &&\n+\tmkdir done/sub/sub &&\n+\techo \"sub\" > done/sub/sub/file\n+'\n+\n+test_expect_success 'test sparse status with untracked cache and subdir' '\n+\tavoid_racy &&\n+\t: >../trace &&\n+\tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace\" \\\n+\tgit status --porcelain >../status.actual &&\n+\tcat >../status.expect <<EOF &&\n+ M done/two\n+?? .gitignore\n+?? done/five\n+?? done/sub/\n+?? dtwo/\n+EOF\n+\ttest_cmp ../status.expect ../status.actual &&\n+\tcat >../trace.expect <<EOF &&\n+node creation: 2\n+gitignore invalidation: 0\n+directory invalidation: 1\n+opendir: 3\n+EOF\n+\ttest_cmp ../trace.expect ../trace\n+'\n+\n+test_expect_success 'verify untracked cache dump (sparse/subdirs)' '\n+\ttest-dump-untracked-cache >../actual &&\n+\tcat >../expect <<EOF &&\n+info/exclude 13263c0978fb9fad16b2d580fb800b6d811c3ff0\n+core.excludesfile 0000000000000000000000000000000000000000\n+exclude_per_dir .gitignore\n+flags 00000006\n+/ e6fcc8f2ee31bae321d66afd183fcb7237afae6e recurse valid\n+.gitignore\n+dtwo/\n+/done/ 1946f0437f90c5005533cbe1736a6451ca301714 recurse valid\n+five\n+sub/\n+/done/sub/ 0000000000000000000000000000000000000000 recurse check_only valid\n+sub/\n+/done/sub/sub/ 0000000000000000000000000000000000000000 recurse check_only valid\n+file\n+/dthree/ 0000000000000000000000000000000000000000 recurse check_only valid\n+/dtwo/ 0000000000000000000000000000000000000000 recurse check_only valid\n+two\n+EOF\n+\ttest_cmp ../expect ../actual\n+'\n+\n+test_expect_success 'test sparse status again with untracked cache and subdir' '\n+\tavoid_racy &&\n+\t: >../trace &&\n+\tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace\" \\\n+\tgit status --porcelain >../status.actual &&\n+\ttest_cmp ../status.expect ../status.actual &&\n+\tcat >../trace.expect <<EOF &&\n+node creation: 0\n+gitignore invalidation: 0\n+directory invalidation: 0\n+opendir: 0\n+EOF\n+\ttest_cmp ../trace.expect ../trace\n+'\n+\n test_done\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"268112","messageId":"20150815073219.GA4496@lanh","threadId":"40033","inReplyTo":"1438967873-792-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v2] untracked-cache: fix subdirectory handling","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-15T07:32:19Z","receivedAt":"2015-08-15T07:32:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"First of all, sorry for my long silence. I'm going through my git\ninbox now.\n\n> diff --git a/dir.c b/dir.c\n> index e7b89fe..314080b 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -631,6 +631,7 @@ static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,\n>  \tmemset(d, 0, sizeof(*d));\n>  \tmemcpy(d->name, name, len);\n>  \td->name[len] = '\\0';\n> +\td->depth = dir->depth + 1;\n>  \n>  \tALLOC_GROW(dir->dirs, dir->dirs_nr + 1, dir->dirs_alloc);\n>  \tmemmove(dir->dirs + first + 1, dir->dirs + first,\n> @@ -1324,7 +1325,19 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n>  \tif (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))\n>  \t\treturn exclude ? path_excluded : path_untracked;\n>  \n> -\tuntracked = lookup_untracked(dir->untracked, untracked, dirname, len);\n> +\tif (untracked) {\n> +\t\tconst char *cur = dirname;\n> +\t\tint i;\n> +\n> +\t\tfor (i = 0; i < untracked->depth; i++) {\n> +\t\t\tcur = strchr(cur, '/');\n> +\t\t\tassert(cur);\n> +\t\t\tcur++;\n> +\t\t}\n> +\t\tuntracked = lookup_untracked(dir->untracked, untracked,\n> +\t\t\t\t\t     cur,\n> +\t\t\t\t\t     len - (cur - dirname));\n> +\t}\n\nIf I read it correctly, the call chain is, treat_path() reconstructs\nfull path, then it calls treat_one_path -> treat_directory ->\nlookup_untracked.\n\nIn the same treat_path(), we may also call treat_fast_path ->\nread_directory_recursive -> lookup_untracked. In this call chain, we\nretain the \"baselen\" and we can exclude the base path before passing\nit to lookup_untracked().\n\nSo instead of adding \"depth\" (and spending some more cycles cutting\npath components), perhaps we can do the same for the first call chain,\npassing baselen to treat_one_path and treat_directory?  Something like\nthis passes your new tests\n\n-- 8< --\ndiff --git a/dir.c b/dir.c\nindex 07a6642..a4c52bf 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1297,7 +1297,7 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)\n  */\n static enum path_treatment treat_directory(struct dir_struct *dir,\n \tstruct untracked_cache_dir *untracked,\n-\tconst char *dirname, int len, int exclude,\n+\tconst char *dirname, int len, int baselen, int exclude,\n \tconst struct path_simplify *simplify)\n {\n \t/* The \"len-1\" is to strip the final '/' */\n@@ -1324,7 +1324,8 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \tif (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))\n \t\treturn exclude ? path_excluded : path_untracked;\n \n-\tuntracked = lookup_untracked(dir->untracked, untracked, dirname, len);\n+\tuntracked = lookup_untracked(dir->untracked, untracked,\n+\t\t\t\t     dirname + baselen, len - baselen);\n \treturn read_directory_recursive(dir, dirname, len,\n \t\t\t\t\tuntracked, 1, simplify);\n }\n@@ -1444,6 +1445,7 @@ static int get_dtype(struct dirent *de, const char *path, int len)\n static enum path_treatment treat_one_path(struct dir_struct *dir,\n \t\t\t\t\t  struct untracked_cache_dir *untracked,\n \t\t\t\t\t  struct strbuf *path,\n+\t\t\t\t\t  int baselen,\n \t\t\t\t\t  const struct path_simplify *simplify,\n \t\t\t\t\t  int dtype, struct dirent *de)\n {\n@@ -1495,8 +1497,8 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \t\treturn path_none;\n \tcase DT_DIR:\n \t\tstrbuf_addch(path, '/');\n-\t\treturn treat_directory(dir, untracked, path->buf, path->len, exclude,\n-\t\t\tsimplify);\n+\t\treturn treat_directory(dir, untracked, path->buf, path->len,\n+\t\t\t\t       baselen, exclude, simplify);\n \tcase DT_REG:\n \tcase DT_LNK:\n \t\treturn exclude ? path_excluded : path_untracked;\n@@ -1557,7 +1559,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\treturn path_none;\n \n \tdtype = DTYPE(de);\n-\treturn treat_one_path(dir, untracked, path, simplify, dtype, de);\n+\treturn treat_one_path(dir, untracked, path, baselen, simplify, dtype, de);\n }\n \n static void add_untracked(struct untracked_cache_dir *dir, const char *name)\n@@ -1827,7 +1829,7 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\t\tbreak;\n \t\tif (simplify_away(sb.buf, sb.len, simplify))\n \t\t\tbreak;\n-\t\tif (treat_one_path(dir, NULL, &sb, simplify,\n+\t\tif (treat_one_path(dir, NULL, &sb, baselen, simplify,\n \t\t\t\t   DT_DIR, NULL) == path_none)\n \t\t\tbreak; /* do not recurse into it */\n \t\tif (len <= baselen) {\n-- 8< --\n\n> +static struct untracked_cache_dir *lookup_untracked_recursive(\n> +\tstruct untracked_cache *uc, struct untracked_cache_dir *dir,\n> +\tconst char *path, int len)\n> +{\n> +\tconst char *rest = strchr(path, '/');\n> +\n> +\tif (rest) {\n> +\t\tint component_len = rest - path;\n> +\t\tstruct untracked_cache_dir *d =\n> +\t\t\tlookup_untracked(uc, dir, path, component_len);\n> +\t\treturn lookup_untracked_recursive(uc, d, rest + 1,\n> +\t\t\t\t\t\t  len - (component_len + 1));\n> +\t} else {\n> +\t\treturn dir;\n> +\t}\n> +}\n> +\n>  void untracked_cache_invalidate_path(struct index_state *istate,\n>  \t\t\t\t     const char *path)\n>  {\n> -\tconst char *sep;\n>  \tstruct untracked_cache_dir *d;\n>  \tif (!istate->untracked || !istate->untracked->root)\n>  \t\treturn;\n> -\tsep = strrchr(path, '/');\n> -\tif (sep)\n> -\t\td = lookup_untracked(istate->untracked,\n> -\t\t\t\t     istate->untracked->root,\n> -\t\t\t\t     path, sep - path);\n> -\telse\n> -\t\td = istate->untracked->root;\n> +\td = lookup_untracked_recursive(istate->untracked,\n> +\t\t\t\t       istate->untracked->root,\n> +\t\t\t\t       path, strlen(path));\n\nThis is totally my fault. It's so obvious, how could I miss\nit.. Thanks for fixing.\n\nBy the way, while running the tests, I noticed that the test \"set up\nsparse checkout\" in t7063 performs the time-consuming \"Testing\n...... OK\" again. At this point in the test suite, you are already\ncertain the underlying filesystem supports untracked-cached, maybe do\nthis to reduce test time for people who do not run tests in parallel?\n\ns/update-index --untracked-cache/update-index --force-untracked-cache/\n\n--\nDuy\n"}]}