{"thread":{"id":"41090","subject":"[PATCH v4 00/10] Untracked cache improvements","startedAt":"2015-12-29T07:09:24Z","lastAt":"2016-01-08T19:43:24Z","messageCount":24,"participants":["Christian Couder","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":4,"patchTotal":10},"messages":[{"id":"275082","messageId":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":null,"subject":"[PATCH v4 00/10] Untracked cache improvements","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:24Z","receivedAt":"2015-12-29T07:09:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Here is a new version of a patch series to improve the untracked cache\nfeature.\n\nThis v4 implements core.untrackedCache as a tristate config\nvariable. When it's `true`, Git commands, especially `git status`,\nshould always add the untracked cache and use it, and when `false`,\nGit commands should remove it. The default though is now `keep` in\nwhich case the untracked cache is neither removed nor added, and used\nif it is there.\n\nPatch 1/10 is a small bugfix that has not changed since v3.\n\nPatch 2/10 to 4/10 add some small features that are missing. The only\nchqnge there since v3 is that we are now using `report()` to display\nverbose information, thanks to Duy.\n\nPatchs 5/10 to 7/10 are some refactoring to prepare for the following\npatchs. Among them 6/10 is the result of merging two patchs from v3,\nthanks to Eric.\n\nPatch 8/10 deals with the \"ident\" field in \"struct untracked_cache\"\nand is mostly the same as in v3. The difference is just a small bug\nfix to prevent a crash.\n\nPatch 9/10 adds core.untrackedCache. It has been changed compared to\nv3 in the following ways:\n - the config variable is now a tristate, thanks to Junio,\n - we use `switch` to deal with different values, thanks to Torsten,\n - documentation for --test-untracked-cache is improved, thanks to\n David.\n\nPatch 10/10, which contains tests, has been changed to reflect changes\nin 9/10 and to add a few tests.\n\nSo the changes compared to v3 are mostly small updates, and patchs\n6/10, 9/10 and 10/10.\n\nThe patch series is also available there:\n\nhttps://github.com/chriscool/git/tree/uc-notifs40\n\nThanks to the reviewers and helpers.\n\nChristian Couder (10):\n dir: free untracked cache when removing it\n update-index: use enum for untracked cache options\n update-index: add --test-untracked-cache\n update-index: add untracked cache notifications\n update-index: move 'uc' var declaration\n dir: add {new,add}_untracked_cache()\n dir: add remove_untracked_cache()\n dir: simplify untracked cache \"ident\" field\n config: add core.untrackedCache\n t7063: add tests for core.untrackedCache\n\n Documentation/config.txt | 7 +++\n Documentation/git-update-index.txt | 65 +++++++++++++++++++++++-----\n builtin/update-index.c | 62 ++++++++++++++++----------\n cache.h | 1 +\n config.c | 11 +++++\n contrib/completion/git-completion.bash | 1 +\n dir.c | 70 ++++++++++++++++++++++++------\n dir.h | 2 +\n environment.c | 1 +\n t/t7063-status-untracked-cache.sh | 79 +++++++++++++++++++++++++++++++---\n wt-status.c | 13 ++++++\n 11 files changed, 260 insertions(+), 52 deletions(-)\n\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275083","messageId":"1451372974-16266-2-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 01/10] dir: free untracked cache when removing it","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:25Z","receivedAt":"2015-12-29T07:09:25Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n builtin/update-index.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 7431938..a6fff87 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1123,6 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tadd_untracked_ident(the_index.untracked);\n \t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n \t} else if (!untracked_cache && the_index.untracked) {\n+\t\tfree_untracked_cache(the_index.untracked);\n \t\tthe_index.untracked = NULL;\n \t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n \t}\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275084","messageId":"1451372974-16266-3-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 02/10] update-index: use enum for untracked cache options","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:26Z","receivedAt":"2015-12-29T07:09:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n builtin/update-index.c | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex a6fff87..1e546a3 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -35,6 +35,14 @@ static int mark_skip_worktree_only;\n #define UNMARK_FLAG 2\n static struct strbuf mtime_dir = STRBUF_INIT;\n \n+/* Untracked cache mode */\n+enum uc_mode {\n+\tUC_UNSPECIFIED = -1,\n+\tUC_DISABLE = 0,\n+\tUC_ENABLE,\n+\tUC_FORCE\n+};\n+\n __attribute__((format (printf, 1, 2)))\n static void report(const char *fmt, ...)\n {\n@@ -902,7 +910,7 @@ static int reupdate_callback(struct parse_opt_ctx_t *ctx,\n int cmd_update_index(int argc, const char **argv, const char *prefix)\n {\n \tint newfd, entries, has_errors = 0, line_termination = '\\n';\n-\tint untracked_cache = -1;\n+\tenum uc_mode untracked_cache = UC_UNSPECIFIED;\n \tint read_from_stdin = 0;\n \tint prefix_length = prefix ? strlen(prefix) : 0;\n \tint preferred_index_format = 0;\n@@ -997,7 +1005,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"untracked-cache\", &untracked_cache,\n \t\t\tN_(\"enable/disable untracked cache\")),\n \t\tOPT_SET_INT(0, \"force-untracked-cache\", &untracked_cache,\n-\t\t\t N_(\"enable untracked cache without testing the filesystem\"), 2),\n+\t\t\t N_(\"enable untracked cache without testing the filesystem\"), UC_FORCE),\n \t\tOPT_END()\n \t};\n \n@@ -1104,10 +1112,10 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tthe_index.split_index = NULL;\n \t\tthe_index.cache_changed |= SOMETHING_CHANGED;\n \t}\n-\tif (untracked_cache > 0) {\n+\tif (untracked_cache > UC_DISABLE) {\n \t\tstruct untracked_cache *uc;\n \n-\t\tif (untracked_cache < 2) {\n+\t\tif (untracked_cache < UC_FORCE) {\n \t\t\tsetup_work_tree();\n \t\t\tif (!test_if_untracked_cache_is_supported())\n \t\t\t\treturn 1;\n@@ -1122,7 +1130,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tadd_untracked_ident(the_index.untracked);\n \t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n-\t} else if (!untracked_cache && the_index.untracked) {\n+\t} else if (untracked_cache == UC_DISABLE && the_index.untracked) {\n \t\tfree_untracked_cache(the_index.untracked);\n \t\tthe_index.untracked = NULL;\n \t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275087","messageId":"1451372974-16266-4-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 03/10] update-index: add --test-untracked-cache","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:27Z","receivedAt":"2015-12-29T07:09:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"It is nice to just be able to test if untracked cache is\nsupported without enabling it.\n\nSigned-off-by: Christian Couder \n---\n Documentation/git-update-index.txt | 12 +++++++++++-\n builtin/update-index.c | 5 +++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\nindex f4e5a85..a0afe17 100644\n--- a/Documentation/git-update-index.txt\n+++ b/Documentation/git-update-index.txt\n@@ -18,7 +18,7 @@ SYNOPSIS\n \t [--[no-]skip-worktree]\n \t [--ignore-submodules]\n \t [--[no-]split-index]\n-\t [--[no-|force-]untracked-cache]\n+\t [--[no-|test-|force-]untracked-cache]\n \t [--really-refresh] [--unresolve] [--again | -g]\n \t [--info-only] [--index-info]\n \t [-z] [--stdin] [--index-version ]\n@@ -180,6 +180,16 @@ may not support it yet.\n \tsystem must change `st_mtime` field of a directory if files\n \tare added or deleted in that directory.\n \n+--test-untracked-cache::\n+\tOnly perform tests on the working directory to make sure\n+\tuntracked cache can be used. You have to manually enable\n+\tuntracked cache using `--force-untracked-cache` (or\n+\t`--untracked-cache` but this will run the tests again)\n+\tafterwards if you really want to use it. If a test fails\n+\tthe exit code is 1 and a message explains what is not\n+\tworking as needed, otherwise the exit code is 0 and OK is\n+\tprinted.\n+\n --force-untracked-cache::\n \tFor safety, `--untracked-cache` performs tests on the working\n \tdirectory to make sure untracked cache can be used. These\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 1e546a3..62222dd 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -40,6 +40,7 @@ enum uc_mode {\n \tUC_UNSPECIFIED = -1,\n \tUC_DISABLE = 0,\n \tUC_ENABLE,\n+\tUC_TEST,\n \tUC_FORCE\n };\n \n@@ -1004,6 +1005,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"enable or disable split index\")),\n \t\tOPT_BOOL(0, \"untracked-cache\", &untracked_cache,\n \t\t\tN_(\"enable/disable untracked cache\")),\n+\t\tOPT_SET_INT(0, \"test-untracked-cache\", &untracked_cache,\n+\t\t\t N_(\"test if the filesystem supports untracked cache\"), UC_TEST),\n \t\tOPT_SET_INT(0, \"force-untracked-cache\", &untracked_cache,\n \t\t\t N_(\"enable untracked cache without testing the filesystem\"), UC_FORCE),\n \t\tOPT_END()\n@@ -1119,6 +1122,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tsetup_work_tree();\n \t\t\tif (!test_if_untracked_cache_is_supported())\n \t\t\t\treturn 1;\n+\t\t\tif (untracked_cache == UC_TEST)\n+\t\t\t\treturn 0;\n \t\t}\n \t\tif (!the_index.untracked) {\n \t\t\tuc = xcalloc(1, sizeof(*uc));\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275086","messageId":"1451372974-16266-5-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 04/10] update-index: add untracked cache notifications","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:28Z","receivedAt":"2015-12-29T07:09:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Attempting to flip the untracked-cache feature on for a random index\nfile with\n\n cd /random/unrelated/place\n git --git-dir=/somewhere/else/.git update-index --untracked-cache\n\nwould not work as you might expect. Because flipping the feature on\nin the index also records the location of the corresponding working\ntree (/random/unrelated/place in the above example), when the index\nis subsequently used to keep track of files in the working tree in\n/somewhere/else, the feature is disabled.\n\nWith this patch \"git update-index --[test-]untracked-cache\" tells the\nuser in which directory tests are performed. This makes it easy to\nspot any problem.\n\nAlso in verbose mode, let's tell the user when the cache is enabled\nor disabled.\n\nSigned-off-by: Christian Couder \n---\n builtin/update-index.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 62222dd..369c207 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -130,7 +130,7 @@ static int test_if_untracked_cache_is_supported(void)\n \tif (!mkdtemp(mtime_dir.buf))\n \t\tdie_errno(\"Could not make temporary directory\");\n \n-\tfprintf(stderr, _(\"Testing \"));\n+\tfprintf(stderr, _(\"Testing mtime in '%s' \"), xgetcwd());\n \tatexit(remove_test_directory);\n \txstat_mtime_dir(&st);\n \tfill_stat_data(&base, &st);\n@@ -1135,10 +1135,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tadd_untracked_ident(the_index.untracked);\n \t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n-\t} else if (untracked_cache == UC_DISABLE && the_index.untracked) {\n-\t\tfree_untracked_cache(the_index.untracked);\n-\t\tthe_index.untracked = NULL;\n-\t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n+\t\treport(_(\"Untracked cache enabled for '%s'\"), get_git_work_tree());\n+\t} else if (untracked_cache == UC_DISABLE) {\n+\t\tif (the_index.untracked) {\n+\t\t\tfree_untracked_cache(the_index.untracked);\n+\t\t\tthe_index.untracked = NULL;\n+\t\t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n+\t\t}\n+\t\treport(_(\"Untracked cache disabled\"));\n \t}\n \n \tif (active_cache_changed) {\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275088","messageId":"1451372974-16266-6-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 05/10] update-index: move 'uc' var declaration","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:29Z","receivedAt":"2015-12-29T07:09:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n builtin/update-index.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 369c207..fe7aaa3 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1116,8 +1116,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tthe_index.cache_changed |= SOMETHING_CHANGED;\n \t}\n \tif (untracked_cache > UC_DISABLE) {\n-\t\tstruct untracked_cache *uc;\n-\n \t\tif (untracked_cache < UC_FORCE) {\n \t\t\tsetup_work_tree();\n \t\t\tif (!test_if_untracked_cache_is_supported())\n@@ -1126,7 +1124,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\t\treturn 0;\n \t\t}\n \t\tif (!the_index.untracked) {\n-\t\t\tuc = xcalloc(1, sizeof(*uc));\n+\t\t\tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \t\t\tstrbuf_init(&uc->ident, 100);\n \t\t\tuc->exclude_per_dir = \".gitignore\";\n \t\t\t/* should be the same flags used by git-status */\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275085","messageId":"1451372974-16266-7-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 06/10] dir: add {new,add}_untracked_cache()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:30Z","receivedAt":"2015-12-29T07:09:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Factor out code into new_untracked_cache() and\nadd_untracked_cache(), which will be used\nin later commits.\n\nSigned-off-by: Christian Couder \n---\n builtin/update-index.c | 11 +----------\n dir.c | 18 ++++++++++++++++++\n dir.h | 1 +\n 3 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex fe7aaa3..74a6f8d 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1123,16 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tif (untracked_cache == UC_TEST)\n \t\t\t\treturn 0;\n \t\t}\n-\t\tif (!the_index.untracked) {\n-\t\t\tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n-\t\t\tstrbuf_init(&uc->ident, 100);\n-\t\t\tuc->exclude_per_dir = \".gitignore\";\n-\t\t\t/* should be the same flags used by git-status */\n-\t\t\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n-\t\t\tthe_index.untracked = uc;\n-\t\t}\n-\t\tadd_untracked_ident(the_index.untracked);\n-\t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n+\t\tadd_untracked_cache();\n \t\treport(_(\"Untracked cache enabled for '%s'\"), get_git_work_tree());\n \t} else if (untracked_cache == UC_DISABLE) {\n \t\tif (the_index.untracked) {\ndiff --git a/dir.c b/dir.c\nindex d2a8f06..4227ba6 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1938,6 +1938,24 @@ void add_untracked_ident(struct untracked_cache *uc)\n \tstrbuf_addch(&uc->ident, 0);\n }\n \n+static void new_untracked_cache(void)\n+{\n+\tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n+\tstrbuf_init(&uc->ident, 100);\n+\tuc->exclude_per_dir = \".gitignore\";\n+\t/* should be the same flags used by git-status */\n+\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\tthe_index.untracked = uc;\n+}\n+\n+void add_untracked_cache(void)\n+{\n+\tif (!the_index.untracked)\n+\t\tnew_untracked_cache();\n+\tadd_untracked_ident(the_index.untracked);\n+\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n+}\n+\n static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,\n \t\t\t\t\t\t int base_len,\n \t\t\t\t\t\t const struct pathspec *pathspec)\ndiff --git a/dir.h b/dir.h\nindex 7b5855d..ee94c76 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -308,4 +308,5 @@ void free_untracked_cache(struct untracked_cache *);\n struct untracked_cache *read_untracked_extension(const void *data, unsigned long sz);\n void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);\n void add_untracked_ident(struct untracked_cache *);\n+void add_untracked_cache(void);\n #endif\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275090","messageId":"1451372974-16266-8-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 07/10] dir: add remove_untracked_cache()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:31Z","receivedAt":"2015-12-29T07:09:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Factor out code into remove_untracked_cache(), which will be used\nin a later commit.\n\nSigned-off-by: Christian Couder \n---\n builtin/update-index.c | 6 +-----\n dir.c | 9 +++++++++\n dir.h | 1 +\n 3 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 74a6f8d..7844991 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1126,11 +1126,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tadd_untracked_cache();\n \t\treport(_(\"Untracked cache enabled for '%s'\"), get_git_work_tree());\n \t} else if (untracked_cache == UC_DISABLE) {\n-\t\tif (the_index.untracked) {\n-\t\t\tfree_untracked_cache(the_index.untracked);\n-\t\t\tthe_index.untracked = NULL;\n-\t\t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n-\t\t}\n+\t\tremove_untracked_cache();\n \t\treport(_(\"Untracked cache disabled\"));\n \t}\n \ndiff --git a/dir.c b/dir.c\nindex 4227ba6..dba1ad0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1956,6 +1956,15 @@ void add_untracked_cache(void)\n \tthe_index.cache_changed |= UNTRACKED_CHANGED;\n }\n \n+void remove_untracked_cache(void)\n+{\n+\tif (the_index.untracked) {\n+\t\tfree_untracked_cache(the_index.untracked);\n+\t\tthe_index.untracked = NULL;\n+\t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n+\t}\n+}\n+\n static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,\n \t\t\t\t\t\t int base_len,\n \t\t\t\t\t\t const struct pathspec *pathspec)\ndiff --git a/dir.h b/dir.h\nindex ee94c76..3e5114d 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -309,4 +309,5 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long\n void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);\n void add_untracked_ident(struct untracked_cache *);\n void add_untracked_cache(void);\n+void remove_untracked_cache(void);\n #endif\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275089","messageId":"1451372974-16266-9-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 08/10] dir: simplify untracked cache \"ident\" field","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:32Z","receivedAt":"2015-12-29T07:09:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"It is not a good idea to compare kernel versions and disable\nthe untracked cache if it changes as people may upgrade and\nstill want the untracked cache to work. So let's just\ncompare work tree locations to decide if we should disable\nit.\n\nAlso it's not useful to store many locations in the ident\nfield and compare to any of them. It can even be dangerous\nif GIT_WORK_TREE is used with different values. So let's\njust store one location, the location of the current work\ntree.\n\nIf this location changed and we still want an untracked\ncache, let's delete the cache and recreate it.\n\nSigned-off-by: Christian Couder \n---\n dir.c | 49 ++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 34 insertions(+), 15 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex dba1ad0..1a8b5a2 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1918,23 +1918,36 @@ static const char *get_ident_string(void)\n \treturn sb.buf;\n }\n \n-static int ident_in_untracked(const struct untracked_cache *uc)\n+static int ident_current_location_in_untracked(const struct untracked_cache *uc)\n {\n-\tconst char *end = uc->ident.buf + uc->ident.len;\n-\tconst char *p = uc->ident.buf;\n+\tstruct strbuf cur_loc = STRBUF_INIT;\n+\tint res = 0;\n \n-\tfor (p = uc->ident.buf; p < end; p += strlen(p) + 1)\n-\t\tif (!strcmp(p, get_ident_string()))\n-\t\t\treturn 1;\n-\treturn 0;\n+\t/*\n+\t * Previous git versions may have saved many strings in the\n+\t * \"ident\" field, but it is insane to manage many locations,\n+\t * so just take care of the first one.\n+\t */\n+\n+\tstrbuf_addf(&cur_loc, \"Location %s, system \", get_git_work_tree());\n+\n+\tif (starts_with(uc->ident.buf, cur_loc.buf))\n+\t\tres = 1;\n+\n+\tstrbuf_release(&cur_loc);\n+\n+\treturn res;\n }\n \n-void add_untracked_ident(struct untracked_cache *uc)\n+static void set_untracked_ident(struct untracked_cache *uc)\n {\n-\tif (ident_in_untracked(uc))\n-\t\treturn;\n+\tstrbuf_reset(&uc->ident);\n \tstrbuf_addstr(&uc->ident, get_ident_string());\n-\t/* this strbuf contains a list of strings, save NUL too */\n+\n+\t/*\n+\t * This strbuf used to contain a list of NUL separated\n+\t * strings, so save NUL too for backward compatibility.\n+\t */\n \tstrbuf_addch(&uc->ident, 0);\n }\n \n@@ -1945,15 +1958,21 @@ static void new_untracked_cache(void)\n \tuc->exclude_per_dir = \".gitignore\";\n \t/* should be the same flags used by git-status */\n \tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\tset_untracked_ident(uc);\n \tthe_index.untracked = uc;\n+\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n }\n \n void add_untracked_cache(void)\n {\n-\tif (!the_index.untracked)\n+\tif (!the_index.untracked) {\n \t\tnew_untracked_cache();\n-\tadd_untracked_ident(the_index.untracked);\n-\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n+\t} else {\n+\t\tif (!ident_current_location_in_untracked(the_index.untracked)) {\n+\t\t\tfree_untracked_cache(the_index.untracked);\n+\t\t\tnew_untracked_cache();\n+\t\t}\n+\t}\n }\n \n void remove_untracked_cache(void)\n@@ -2021,7 +2040,7 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \tif (dir->exclude_list_group[EXC_CMDL].nr)\n \t\treturn NULL;\n \n-\tif (!ident_in_untracked(dir->untracked)) {\n+\tif (!ident_current_location_in_untracked(dir->untracked)) {\n \t\twarning(_(\"Untracked cache is disabled on this system.\"));\n \t\treturn NULL;\n \t}\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275092","messageId":"1451372974-16266-10-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 09/10] config: add core.untrackedCache","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:33Z","receivedAt":"2015-12-29T07:09:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When we know that mtime is fully supported by the environment, we\nmight want the untracked cache to be always used by default without\nany mtime test or kernel version check being performed.\n\nAlso when we know that mtime is not supported by the environment,\nfor example because the repo is shared over a network file system,\nthen we might want 'git update-index --untracked-cache' to fail\nimmediately instead of performing tests (because it might work on\nsome systems using the repo over the network file system but not\nothers).\n\nThe normal way to achieve the above in Git is to use a config\nvariable. That's why this patch introduces \"core.untrackedCache\".\n\nThis variable is a tristate, `keep`, `false` and `true`, which\ndefault to `keep`.\n\nWhen \"git status\" is run, it now adds or removes the untracked\ncache in the index to respect the value of this variable. So it\ndoes nothing if the value is `keep` or if the variable is unset;\nit adds the untracked cache if the value is `true`; and it\nremoves the cache if the value is `false`.\n\n`git update-index --[no-|force-]untracked-cache` still adds or\nremoves the untracked cache from the index, but this now fails\nif it goes against the value of core.untrackedCache.\n\nAlso `--untracked-cache` used to check that the underlying\noperating system and file system change `st_mtime` field of a\ndirectory if files are added or deleted in that directory. But\nthose tests take a long time and there is now\n`--test-untracked-cache` to perform them.\n\nThat's why, to be more consistent with other git commands, this\npatch prevents `--untracked-cache` to perform tests, so that\nafter this patch there is no difference any more between\n`--untracked-cache` and `--force-untracked-cache`.\n\nThis last change is backward incompatible and should be\nmentioned in the release notes.\n\nSigned-off-by: Christian Couder \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 7 ++++\n Documentation/git-update-index.txt | 65 ++++++++++++++++++++++++++--------\n builtin/update-index.c | 35 ++++++++++++------\n cache.h | 1 +\n config.c | 11 ++++++\n contrib/completion/git-completion.bash | 1 +\n environment.c | 1 +\n t/t7063-status-untracked-cache.sh | 4 +--\n wt-status.c | 13 +++++++\n 9 files changed, 109 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f617886..d892127 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -308,6 +308,13 @@ core.trustctime::\n \tcrawlers and some backup systems).\n \tSee linkgit:git-update-index[1]. True by default.\n \n+core.untrackedCache::\n+\tDetermines if untracked cache will be automatically enabled or\n+\tdisabled. It can be `keep`, `true` or `false`. Before setting\n+\tit to `true`, you should check that mtime is working properly\n+\ton your system.\n+\tSee linkgit:git-update-index[1]. `keep` by default.\n+\n core.checkStat::\n \tDetermines which stat fields to match between the index\n \tand work tree. The user can set this to 'default' or\ndiff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\nindex a0afe17..813f6cc 100644\n--- a/Documentation/git-update-index.txt\n+++ b/Documentation/git-update-index.txt\n@@ -174,27 +174,31 @@ may not support it yet.\n \n --untracked-cache::\n --no-untracked-cache::\n-\tEnable or disable untracked cache extension. This could speed\n-\tup for commands that involve determining untracked files such\n-\tas `git status`. The underlying operating system and file\n-\tsystem must change `st_mtime` field of a directory if files\n-\tare added or deleted in that directory.\n+\tEnable or disable untracked cache extension. Please use\n+\t`--test-untracked-cache` before enabling it.\n++\n+These options cannot override the `core.untrackedCache` configuration\n+variable when it is set to `true` or `false` (see\n+linkgit:git-config[1]). They can override it only when it is unset or\n+set to `keep`. If you want untracked cache to persist, it is better\n+anyway to just set this configuration variable to `true` or `false`\n+directly.\n \n --test-untracked-cache::\n \tOnly perform tests on the working directory to make sure\n \tuntracked cache can be used. You have to manually enable\n-\tuntracked cache using `--force-untracked-cache` (or\n-\t`--untracked-cache` but this will run the tests again)\n-\tafterwards if you really want to use it. If a test fails\n-\tthe exit code is 1 and a message explains what is not\n-\tworking as needed, otherwise the exit code is 0 and OK is\n-\tprinted.\n+\tuntracked cache using `--untracked-cache` or\n+\t`--force-untracked-cache` or the `core.untrackedCache`\n+\tconfiguration variable afterwards if you really want to use\n+\tit. If a test fails the exit code is 1 and a message\n+\texplains what is not working as needed, otherwise the exit\n+\tcode is 0 and OK is printed.\n \n --force-untracked-cache::\n-\tFor safety, `--untracked-cache` performs tests on the working\n-\tdirectory to make sure untracked cache can be used. These\n-\ttests can take a few seconds. `--force-untracked-cache` can be\n-\tused to skip the tests.\n+\tSame as `--untracked-cache`. Provided for backwards\n+\tcompatibility with older versions of Git where\n+\t`--untracked-cache` used to imply `--test-untracked-cache` but\n+\tthis option would enable the extension unconditionally.\n \n \\--::\n \tDo not interpret any more arguments as options.\n@@ -385,6 +389,34 @@ Although this bit looks similar to assume-unchanged bit, its goal is\n different from assume-unchanged bit's. Skip-worktree also takes\n precedence over assume-unchanged bit when both are set.\n \n+Untracked cache\n+---------------\n+\n+This cache could speed up commands that involve determining untracked\n+files such as `git status`.\n+\n+This feature works by recording the mtime of the working tree\n+directories and then omitting reading directories and stat calls\n+against files in those directories whose mtime hasn't changed. For\n+this to work the underlying operating system and file system must\n+change the `st_mtime` field of directories if files in the directory\n+are added, modified or deleted.\n+\n+You can test whether the filesystem supports that with the\n+`--test-untracked-cache` option. The `--untracked-cache` option used\n+to implicitly perform that test in older versions of Git, but that's\n+no longer the case.\n+\n+It is recommended to use the `core.untrackedCache` configuration\n+variable (see linkgit:git-config[1]) to enable or disable this feature\n+instead of using the `--[no-|force-]untracked-cache`, as it is more\n+persistant and visible with a configuration variable.\n+\n+When the `core.untrackedCache` configuration variable is changed, the\n+untracked cache is added to or removed from the index the next time\n+\"git status\" is run; while when `--[no-|force-]untracked-cache` are\n+used, the untracked cache is immediately added to or removed from the\n+index.\n \n Configuration\n -------------\n@@ -410,6 +442,9 @@ It can be useful when the inode change time is regularly modified by\n something outside Git (file system crawlers and backup systems use\n ctime for marking files processed) (see linkgit:git-config[1]).\n \n+The untracked cache extension can be enabled by the\n+`core.untrackedCache` configuration variable (see\n+linkgit:git-config[1]).\n \n SEE ALSO\n --------\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 7844991..497f904 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1115,19 +1115,32 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tthe_index.split_index = NULL;\n \t\tthe_index.cache_changed |= SOMETHING_CHANGED;\n \t}\n-\tif (untracked_cache > UC_DISABLE) {\n-\t\tif (untracked_cache < UC_FORCE) {\n-\t\t\tsetup_work_tree();\n-\t\t\tif (!test_if_untracked_cache_is_supported())\n-\t\t\t\treturn 1;\n-\t\t\tif (untracked_cache == UC_TEST)\n-\t\t\t\treturn 0;\n-\t\t}\n-\t\tadd_untracked_cache();\n-\t\treport(_(\"Untracked cache enabled for '%s'\"), get_git_work_tree());\n-\t} else if (untracked_cache == UC_DISABLE) {\n+\n+\tswitch (untracked_cache) {\n+\tcase UC_UNSPECIFIED:\n+\t\tbreak;\n+\tcase UC_DISABLE:\n+\t\tif (use_untracked_cache == 1)\n+\t\t\tdie(\"core.untrackedCache is set to true; \"\n+\t\t\t \"remove or change it, if you really want to \"\n+\t\t\t \"disable the untracked cache\");\n \t\tremove_untracked_cache();\n \t\treport(_(\"Untracked cache disabled\"));\n+\t\tbreak;\n+\tcase UC_TEST:\n+\t\tsetup_work_tree();\n+\t\treturn !test_if_untracked_cache_is_supported();\n+\tcase UC_ENABLE:\n+\tcase UC_FORCE:\n+\t\tif (use_untracked_cache == 0)\n+\t\t\tdie(\"core.untrackedCache is set to false; \"\n+\t\t\t \"remove or change it, if you really want to \"\n+\t\t\t \"enable the untracked cache\");\n+\t\tadd_untracked_cache();\n+\t\treport(_(\"Untracked cache enabled for '%s'\"), get_git_work_tree());\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"Bug: bad untracked_cache value: %d\", untracked_cache);\n \t}\n \n \tif (active_cache_changed) {\ndiff --git a/cache.h b/cache.h\nindex c63fcc1..8d09858 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -619,6 +619,7 @@ extern void set_alternate_index_output(const char *);\n /* Environment bits from configuration mechanism */\n extern int trust_executable_bit;\n extern int trust_ctime;\n+extern int use_untracked_cache;\n extern int check_stat;\n extern int quote_path_fully;\n extern int has_symlinks;\ndiff --git a/config.c b/config.c\nindex 86a5eb2..b7d7b25 100644\n--- a/config.c\n+++ b/config.c\n@@ -691,6 +691,17 @@ static int git_default_core_config(const char *var, const char *value)\n \t\ttrust_ctime = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"core.untrackedcache\")) {\n+\t\tif (!strcasecmp(value, \"keep\"))\n+\t\t\tuse_untracked_cache = -1;\n+\t\telse {\n+\t\t\tuse_untracked_cache = git_config_maybe_bool(var, value);\n+\t\t\tif (use_untracked_cache == -1)\n+\t\t\t\terror(\"unknown core.untrackedCache value '%s'; \"\n+\t\t\t\t \"using 'keep' default value\", value);\n+\t\t}\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"core.checkstat\")) {\n \t\tif (!strcasecmp(value, \"default\"))\n \t\t\tcheck_stat = 1;\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 6956807..84bcec3 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2055,6 +2055,7 @@ _git_config ()\n \t\tcore.sparseCheckout\n \t\tcore.symlinks\n \t\tcore.trustctime\n+\t\tcore.untrackedCache\n \t\tcore.warnAmbiguousRefs\n \t\tcore.whitespace\n \t\tcore.worktree\ndiff --git a/environment.c b/environment.c\nindex 2da7fe2..394b15c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -14,6 +14,7 @@\n \n int trust_executable_bit = 1;\n int trust_ctime = 1;\n+int use_untracked_cache = -1; /* keep */\n int check_stat = 1;\n int has_symlinks = 1;\n int minimum_abbrev = 4, default_abbrev = 7;\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 0e8d0d4..253160a 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -8,10 +8,8 @@ avoid_racy() {\n \tsleep 1\n }\n \n-# It's fine if git update-index returns an error code other than one,\n-# it'll be caught in the first test.\n test_lazy_prereq UNTRACKED_CACHE '\n-\t{ git update-index --untracked-cache; ret=$?; } &&\n+\t{ git update-index --test-untracked-cache; ret=$?; } &&\n \ttest $ret -ne 1\n '\n \ndiff --git a/wt-status.c b/wt-status.c\nindex bba2596..75fafe5 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -578,6 +578,19 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \tif (!s->show_untracked_files)\n \t\treturn;\n \n+\tswitch (use_untracked_cache) {\n+\tcase -1: /* keep: do nothing */\n+\t\tbreak;\n+\tcase 0: /* false */\n+\t\tremove_untracked_cache();\n+\t\tbreak;\n+\tcase 1: /* true */\n+\t\tadd_untracked_cache();\n+\t\tbreak;\n+\tdefault: /* unknown value: do nothing */\n+\t\tbreak;\n+\t}\n+\n \tmemset(&dir, 0, sizeof(dir));\n \tif (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)\n \t\tdir.flags |=\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275091","messageId":"1451372974-16266-11-git-send-email-chriscool@tuxfamily.org","threadId":"41090","inReplyTo":"1451372974-16266-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH v4 10/10] t7063: add tests for core.untrackedCache","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-29T07:09:34Z","receivedAt":"2015-12-29T07:09:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n t/t7063-status-untracked-cache.sh | 75 ++++++++++++++++++++++++++++++++++++---\n 1 file changed, 71 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 253160a..67e049e 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -18,6 +18,10 @@ if ! test_have_prereq UNTRACKED_CACHE; then\n \ttest_done\n fi\n \n+test_expect_success 'core.untrackedCache is unset' '\n+\ttest_must_fail git config --get core.untrackedCache\n+'\n+\n test_expect_success 'setup' '\n \tgit init worktree &&\n \tcd worktree &&\n@@ -30,13 +34,13 @@ test_expect_success 'setup' '\n \n test_expect_success 'untracked cache is empty' '\n \ttest-dump-untracked-cache >../actual &&\n-\tcat >../expect <../expect-empty <../status.expect <../actual &&\n-\tcat >../expect <../expect-from-test-dump <../actual &&\n+\techo \"no untracked cache\" >../expect &&\n+\ttest_cmp ../expect ../actual\n+'\n+\n+test_expect_success 'git status does not change anything' '\n+\tgit status &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect ../actual\n+'\n+\n+test_expect_success 'setting core.untrackedCache to true and using git status creates the cache' '\n+\tgit config core.untrackedCache true &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect ../actual &&\n+\tgit status &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect-from-test-dump ../actual\n+'\n+\n+test_expect_success 'using --no-untracked-cache fails when core.untrackedCache is true' '\n+\ttest_must_fail git update-index --no-untracked-cache &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect-from-test-dump ../actual\n+'\n+\n+test_expect_success 'setting core.untrackedCache to false and using git status removes the cache' '\n+\tgit config core.untrackedCache false &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect-from-test-dump ../actual &&\n+\tgit status &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect ../actual\n+'\n+\n+test_expect_success 'using --untracked-cache fails when core.untrackedCache is false' '\n+\ttest_must_fail git update-index --untracked-cache &&\n+\ttest_must_fail git update-index --force-untracked-cache &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect ../actual\n+'\n+\n+test_expect_success 'setting core.untrackedCache to keep' '\n+\tgit config core.untrackedCache keep &&\n+\tgit update-index --untracked-cache &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect-empty ../actual &&\n+\tgit status &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect-from-test-dump ../actual &&\n+\tgit update-index --no-untracked-cache &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect ../actual &&\n+\tgit update-index --force-untracked-cache &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect-empty ../actual &&\n+\tgit status &&\n+\ttest-dump-untracked-cache >../actual &&\n+\ttest_cmp ../expect-from-test-dump ../actual\n+'\n+\n test_done\n-- \n2.7.0.rc2.10.g544ad6b\n"},{"id":"275154","messageId":"xmqq37ukhowt.fsf@gitster.mtv.corp.google.com","threadId":"41090","inReplyTo":"1451372974-16266-2-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH v4 01/10] dir: free untracked cache when removing it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-29T22:26:42Z","receivedAt":"2015-12-29T22:26:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Signed-off-by: Christian Couder \n> ---\n> builtin/update-index.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index 7431938..a6fff87 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -1123,6 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n> \t\tadd_untracked_ident(the_index.untracked);\n> \t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n> \t} else if (!untracked_cache && the_index.untracked) {\n> +\t\tfree_untracked_cache(the_index.untracked);\n> \t\tthe_index.untracked = NULL;\n> \t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n> \t}\n\nLooks good.\n"},{"id":"275155","messageId":"xmqqy4ccga9k.fsf@gitster.mtv.corp.google.com","threadId":"41090","inReplyTo":"1451372974-16266-4-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH v4 03/10] update-index: add --test-untracked-cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-29T22:28:23Z","receivedAt":"2015-12-29T22:28:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> It is nice to just be able to test if untracked cache is\n> supported without enabling it.\n>\n> Signed-off-by: Christian Couder \n> ---\n> Documentation/git-update-index.txt | 12 +++++++++++-\n> builtin/update-index.c | 5 +++++\n> 2 files changed, 16 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\n> index f4e5a85..a0afe17 100644\n> --- a/Documentation/git-update-index.txt\n> +++ b/Documentation/git-update-index.txt\n> @@ -18,7 +18,7 @@ SYNOPSIS\n> \t [--[no-]skip-worktree]\n> \t [--ignore-submodules]\n> \t [--[no-]split-index]\n> -\t [--[no-|force-]untracked-cache]\n> +\t [--[no-|test-|force-]untracked-cache]\n> \t [--really-refresh] [--unresolve] [--again | -g]\n> \t [--info-only] [--index-info]\n> \t [-z] [--stdin] [--index-version ]\n> @@ -180,6 +180,16 @@ may not support it yet.\n> \tsystem must change `st_mtime` field of a directory if files\n> \tare added or deleted in that directory.\n> \n> +--test-untracked-cache::\n> +\tOnly perform tests on the working directory to make sure\n> +\tuntracked cache can be used. You have to manually enable\n> +\tuntracked cache using `--force-untracked-cache` (or\n> +\t`--untracked-cache` but this will run the tests again)\n> +\tafterwards if you really want to use it. If a test fails\n> +\tthe exit code is 1 and a message explains what is not\n> +\tworking as needed, otherwise the exit code is 0 and OK is\n> +\tprinted.\n> +\n\nLooks good.\n\n> --force-untracked-cache::\n> \tFor safety, `--untracked-cache` performs tests on the working\n> \tdirectory to make sure untracked cache can be used. These\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index 1e546a3..62222dd 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -40,6 +40,7 @@ enum uc_mode {\n> \tUC_UNSPECIFIED = -1,\n> \tUC_DISABLE = 0,\n> \tUC_ENABLE,\n> +\tUC_TEST,\n> \tUC_FORCE\n> };\n> \n> @@ -1004,6 +1005,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n> \t\t\tN_(\"enable or disable split index\")),\n> \t\tOPT_BOOL(0, \"untracked-cache\", &untracked_cache,\n> \t\t\tN_(\"enable/disable untracked cache\")),\n> +\t\tOPT_SET_INT(0, \"test-untracked-cache\", &untracked_cache,\n> +\t\t\t N_(\"test if the filesystem supports untracked cache\"), UC_TEST),\n> \t\tOPT_SET_INT(0, \"force-untracked-cache\", &untracked_cache,\n> \t\t\t N_(\"enable untracked cache without testing the filesystem\"), UC_FORCE),\n> \t\tOPT_END()\n> @@ -1119,6 +1122,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n> \t\t\tsetup_work_tree();\n> \t\t\tif (!test_if_untracked_cache_is_supported())\n> \t\t\t\treturn 1;\n> +\t\t\tif (untracked_cache == UC_TEST)\n> +\t\t\t\treturn 0;\n> \t\t}\n> \t\tif (!the_index.untracked) {\n> \t\t\tuc = xcalloc(1, sizeof(*uc));\n"},{"id":"275156","messageId":"xmqqsi2kga1x.fsf@gitster.mtv.corp.google.com","threadId":"41090","inReplyTo":"1451372974-16266-9-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH v4 08/10] dir: simplify untracked cache \"ident\" field","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-29T22:32:58Z","receivedAt":"2015-12-29T22:32:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> -static int ident_in_untracked(const struct untracked_cache *uc)\n> +static int ident_current_location_in_untracked(const struct untracked_cache *uc)\n> {\n> -\tconst char *end = uc->ident.buf + uc->ident.len;\n> -\tconst char *p = uc->ident.buf;\n> +\tstruct strbuf cur_loc = STRBUF_INIT;\n> +\tint res = 0;\n> \n> -\tfor (p = uc->ident.buf; p < end; p += strlen(p) + 1)\n> -\t\tif (!strcmp(p, get_ident_string()))\n> -\t\t\treturn 1;\n> -\treturn 0;\n> +\t/*\n> +\t * Previous git versions may have saved many strings in the\n> +\t * \"ident\" field, but it is insane to manage many locations,\n> +\t * so just take care of the first one.\n> +\t */\n> +\n> +\tstrbuf_addf(&cur_loc, \"Location %s, system \", get_git_work_tree());\n> +\n> +\tif (starts_with(uc->ident.buf, cur_loc.buf))\n> +\t\tres = 1;\n> +\n> +\tstrbuf_release(&cur_loc);\n> +\n> +\treturn res;\n> }\n\nThe \", system \" part looks funny. I think I know what you are\ntrying to do, though.\n\nIf the path to the working tree has \", system \" as a substring,\nI wonder if funny things may happen with this starts_with()?\n"},{"id":"275157","messageId":"xmqqlh8cg9y2.fsf@gitster.mtv.corp.google.com","threadId":"41090","inReplyTo":"1451372974-16266-10-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH v4 09/10] config: add core.untrackedCache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-29T22:35:17Z","receivedAt":"2015-12-29T22:35:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> +core.untrackedCache::\n> +\tDetermines if untracked cache will be automatically enabled or\n> +\tdisabled. It can be `keep`, `true` or `false`. Before setting\n> +\tit to `true`, you should check that mtime is working properly\n> +\ton your system.\n> +\tSee linkgit:git-update-index[1]. `keep` by default.\n> +\n\nBefore \"Before setting it to `true`\", the reader needs to be told\nwhat would happen when this is set to each of these three values (as\nwell as what happens when this is not set at all).\n\n> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\n> index a0afe17..813f6cc 100644\n> --- a/Documentation/git-update-index.txt\n> +++ b/Documentation/git-update-index.txt\n> @@ -174,27 +174,31 @@ may not support it yet.\n> \n> --untracked-cache::\n> --no-untracked-cache::\n> -\tEnable or disable untracked cache extension. This could speed\n> -\tup for commands that involve determining untracked files such\n> -\tas `git status`. The underlying operating system and file\n> -\tsystem must change `st_mtime` field of a directory if files\n> -\tare added or deleted in that directory.\n> +\tEnable or disable untracked cache extension. Please use\n> +\t`--test-untracked-cache` before enabling it.\n\n\"extension\" is a term that is fairly close to the machinery. In\nother parts of the documentation (like the added text below in this\npatch), it is called \"untracked cache FEATURE\", which probably is a\nbetter word to use here as well.\n\n> ++\n> +These options cannot override the `core.untrackedCache` configuration\n> +variable when it is set to `true` or `false` (see\n> +linkgit:git-config[1]). They can override it only when it is unset or\n> +set to `keep`. If you want untracked cache to persist, it is better\n> +anyway to just set this configuration variable to `true` or `false`\n> +directly.\n\nWhile the above is not wrong per-se, from the point of those who\nlooked for these options (that is, those who wanted to do a one-shot\nenabling or disabling of the feature, perhaps to try it out to see\nhow well it helps on their system), I think the explanation of the\ninteraction between the option and the config is backwards. For\ntheir purpose, setting it to `true` or `false` will be hinderance,\nbecause the options are made no-op by such a setting. IOW, the\nadvertisement \"once you decided that you want to use the feature,\nconfiguration is a better place to set it\" does not belong here.\n\nIf I were writing this documentation, I'd probably rephrase the\nsecond paragraph like so:\n\n\tThese options take effect only when the\n\t`core.untrackedCache` configuration variable (see\n\tlinkgit:git-config[1]) is set to `keep` (or it is left\n\tunset). When the configuration variable is set to `true`,\n\tthe untracked cache feature is always enabled (and when it\n\tis set to `false`, it is always disabled), making these\n\toptions no-op.\n\nperhaps.\n\n> @@ -385,6 +389,34 @@ Although this bit looks similar to assume-unchanged bit, its goal is\n> different from assume-unchanged bit's. Skip-worktree also takes\n> precedence over assume-unchanged bit when both are set.\n> \n> +Untracked cache\n> +---------------\n> +\n> +This cache could speed up commands that involve determining untracked\n> +...\n> +It is recommended to use the `core.untrackedCache` configuration\n> +variable (see linkgit:git-config[1]) to enable or disable this feature\n> +instead of using the `--[no-|force-]untracked-cache`, as it is more\n> +persistant and visible with a configuration variable.\n\ns/persistant/persistent/; but more importantly, I do not think\npersistence has much to do with the choice between the option and\nconfiguration. Once it is set with `--untracked-cache`, it should\npersist until you explicitly use `--no-untracked-cache` to disable\nit, no? Otherwise you have a bug that needs to be fixed.\n\nThe reason you'd want to use a configuration is because the effect\nof using the `--untracked-cache` option is per repository (rather,\nper index-file).\n\n If you want to enable (or disable) this feature, it is easier to\n use the `core.untrackedCache` configuration variable than using\n the `--untracked-cache` option to `git update-index` in each\n repository, especially if you want to do so across all\n repositories you use, because you can set the configuration\n variable to `true` (or `false`) in your `$HOME/.gitconfig` just\n once and have it affect all repositories you touch.\n\nor something, perhaps.\n\n> +When the `core.untrackedCache` configuration variable is changed, the\n> +untracked cache is added to or removed from the index the next time\n> +\"git status\" is run; while when `--[no-|force-]untracked-cache` are\n> +used, the untracked cache is immediately added to or removed from the\n> +index.\n\nIs it only \"git status\", or anything that writes/updates the index\nfile? The above makes it sound as if you are saying \"If you change\nthe variable, you must run `git status` for the change to take\neffect\", and if that is indeed the case, perhaps you should say so\nmore explicitly. My cursory reading of the code suggests that the\nuser must run `git status` in a mode that shows untracked files\n(i.e. \"git status -uno\" does not fix)?\n\nI somehow thought, per the previous discussion with Duy, that the\ncheck and addition of an empty one (if missing in the index and\nconfiguration is set to `true`) or removal of an existing one (if\nconfiguration is set to `false`) were to be done when the index is\nread by read_index_from(), though. If you have to say \"After\nsetting the configuration, you must run this command\", that does not\nsound like a huge improvement over \"If you want to enable this\nfeature, you must run this command\".\n\n> +\tswitch (untracked_cache) {\n> +\tcase UC_UNSPECIFIED:\n> +\t\tbreak;\n> +\tcase UC_DISABLE:\n> +\t\tif (use_untracked_cache == 1)\n> +\t\t\tdie(\"core.untrackedCache is set to true; \"\n> +\t\t\t \"remove or change it, if you really want to \"\n> +\t\t\t \"disable the untracked cache\");\n> \t\tremove_untracked_cache();\n> \t\treport(_(\"Untracked cache disabled\"));\n> +\t\tbreak;\n> +\tcase UC_TEST:\n> +\t\tsetup_work_tree();\n> +\t\treturn !test_if_untracked_cache_is_supported();\n> +\tcase UC_ENABLE:\n> +\tcase UC_FORCE:\n> +\t\tif (use_untracked_cache == 0)\n> +\t\t\tdie(\"core.untrackedCache is set to false; \"\n> +\t\t\t \"remove or change it, if you really want to \"\n> +\t\t\t \"enable the untracked cache\");\n> +\t\tadd_untracked_cache();\n> +\t\treport(_(\"Untracked cache enabled for '%s'\"), get_git_work_tree());\n> +\t\tbreak;\n\nI do buy the decision to make these no-op when the configuration\nsays `true` or `false`, but I am not sure if these deserve die().\n\nExiting with 0 (= success) after issuing a warning() might be more\nappropriate. I'd especially anticipate that some newbies will\ncomplain that they got \"fatal:\" when they used the \"--force-\"\nvariant, saying \"I know what I am doing, that is why I said 'force',\nwhy does stupid Git refuse?\".\n"},{"id":"275179","messageId":"5683C463.9010003@web.de","threadId":"41090","inReplyTo":"1451372974-16266-9-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH v4 08/10] dir: simplify untracked cache \"ident\" field","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-12-30T11:47:47Z","receivedAt":"2015-12-30T11:47:47Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-12-29 08.09, Christian Couder wrote:\n> It is not a good idea to compare kernel versions and disable\n> the untracked cache if it changes as people may upgrade and\n> still want the untracked cache to work. So let's just\n> compare work tree locations to decide if we should disable\n> it.\nOK with that.\n> \n> Also it's not useful to store many locations in the ident\n> field and compare to any of them. It can even be dangerous\n> if GIT_WORK_TREE is used with different values. So let's\n> just store one location, the location of the current work\n> tree.\nI don't think I fully agree in all details.\nThe list is there to distinguish between different clients when sharing\na Git workspace over a network to different clients:\n\nA file system is exported from Linux to Linux via NFS,\nincluding a Git workspace)\nThe same Workspace is exported via SAMBA to Windows and, in an extreme case,\nAFS to Mac OS.\n\nOther combinations could be\nLinux - SAMBA - Linux\nLinux - AFP - Linux\n\nMac OS - NFS - Linux\nMac OS - AFP - Mac OS\nMac OS - SMB - Linux, Mac OS, Windows\nThe list is incomplete (BSD and other Unix is missing),\nthere are other combinations of network protocols,\nthere are NAS boxes which mostly Linux under the hood, but\nmay have vendor specific tweaks.\n\nNow we want to know, which of the combinations work.\nThe different clients need to test separately.\nE.g. for a given server:\n\nNFS - Linux\nSMB - Linux\nSMB Windows.\n\nAt this point I would agree that the old code from dir.c:\n\nstatic const char *get_ident_string(void)\n{\n\tstatic struct strbuf sb = STRBUF_INIT;\n\tstruct utsname uts;\n\n\tif (sb.len)\n\t\treturn sb.buf;\n\tif (uname(&uts) < 0)\n\t\tdie_errno(_(\"failed to get kernel name and information\"));\n\tstrbuf_addf(&sb, \"Location %s, system %s %s %s\", get_git_work_tree(),\n\t\t uts.sysname, uts.release, uts.version);\n\treturn sb.buf;\n}\n------------------\nis unneccessary picky, and the sysname should be enough:\n\nstrbuf_addf(&sb, \"Location %s, system %s\", get_git_work_tree(),\nuts.sysname);\n\n\nThe old code did not find out, which network protocol that we used,\n(you need to call \"mount\", and grep for the directory, and try to get\nthe FS information, which may be ext4, btrfs, smbfs, nfs....)\nThis is unnecessary complicated, so we endet up in using the path.\n\nIf I was a system administrator, (I sometimes am), I would set up a\nbunch of Linux boxes in a similar way, so that the repo is under\n/nfs/server1/projects/myproject\n(and the same path is used for all Linux boxes)\n\nThe Windows machines may use something like\nN:/projects/myproject\nor\n//server1/projects/myproject\n\nand Mac OS may use\n/Volumes/projects/myproject\n(If mounted from the finder)\nor\n/nfs/projects/myproject\n(When autofs is used)\n\n\nI may have missed the discussion somewhat, could you explain why having\ndifferent uname/location combinations are not usable at your site ?\n\nHow much burden is actually put on your system, and is there a way to keep a\nlist of different clients ?\n\n\nWhat I understand is that a kernel update of a Linux machine shouldn't matter,\nbut if a machine runs Windows or Linux should matter.\n"},{"id":"275190","messageId":"CAP8UFD1LdPODnfsesLO+HzGeTg0WvzcvSeq6Hy4-Eb+yLdBqnA@mail.gmail.com","threadId":"41090","inReplyTo":"5683C463.9010003@web.de","subject":"Re: [PATCH v4 08/10] dir: simplify untracked cache \"ident\" field","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-30T17:20:25Z","receivedAt":"2015-12-30T17:20:25Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Dec 30, 2015 at 12:47 PM, Torsten Bögershausen wrote:\n> On 2015-12-29 08.09, Christian Couder wrote:\n>> It is not a good idea to compare kernel versions and disable\n>> the untracked cache if it changes as people may upgrade and\n>> still want the untracked cache to work. So let's just\n>> compare work tree locations to decide if we should disable\n>> it.\n> OK with that.\n>>\n>> Also it's not useful to store many locations in the ident\n>> field and compare to any of them. It can even be dangerous\n>> if GIT_WORK_TREE is used with different values. So let's\n>> just store one location, the location of the current work\n>> tree.\n> I don't think I fully agree in all details.\n\nSuppose I use \"git update-index --untracked-cache\" first with\nGIT_WORK_TREE=/foo and then with GIT_WORK_TREE=/bar. It will store\nboth /foo and /bar as valid locations for the worktree in the \"ident\"\nfield.\n\nThen I work a bit with GIT_WORK_TREE=/foo and then a bit with\nGIT_WORK_TREE=/bar. Now does the untracked cache info in the index\nreflect the state of /foo or the state of /bar?\n\nWith a config variable, the problem is worse, because, as we\nautomatically add the untracked cache when git status is used, we are\neven less likely to pay attention to GIT_WORK_TREE when the cache is\nadded.\n\n> The list is there to distinguish between different clients when sharing\n> a Git workspace over a network to different clients:\n>\n> A file system is exported from Linux to Linux via NFS,\n> including a Git workspace)\n> The same Workspace is exported via SAMBA to Windows and, in an extreme case,\n> AFS to Mac OS.\n>\n> Other combinations could be\n> Linux - SAMBA - Linux\n> Linux - AFP - Linux\n>\n> Mac OS - NFS - Linux\n> Mac OS - AFP - Mac OS\n> Mac OS - SMB - Linux, Mac OS, Windows\n> The list is incomplete (BSD and other Unix is missing),\n> there are other combinations of network protocols,\n> there are NAS boxes which mostly Linux under the hood, but\n> may have vendor specific tweaks.\n>\n> Now we want to know, which of the combinations work.\n\nIn my opinion at this point it is a bit insane to want untracked cache\nto work when it is accessed by different clients over the network. It\nis much safer to just disable it (with a config option) in this case.\nBut I am ok to try to improve things for your use case to work if it\ndoesn't complicate things too much.\n\n> The different clients need to test separately.\n> E.g. for a given server:\n>\n> NFS - Linux\n> SMB - Linux\n> SMB Windows.\n>\n> At this point I would agree that the old code from dir.c:\n>\n> static const char *get_ident_string(void)\n> {\n> static struct strbuf sb = STRBUF_INIT;\n> struct utsname uts;\n>\n> if (sb.len)\n> return sb.buf;\n> if (uname(&uts) < 0)\n> die_errno(_(\"failed to get kernel name and information\"));\n> strbuf_addf(&sb, \"Location %s, system %s %s %s\", get_git_work_tree(),\n> uts.sysname, uts.release, uts.version);\n> return sb.buf;\n> }\n> ------------------\n> is unneccessary picky, and the sysname should be enough:\n>\n> strbuf_addf(&sb, \"Location %s, system %s\", get_git_work_tree(),\n> uts.sysname);\n>\n> The old code did not find out, which network protocol that we used,\n> (you need to call \"mount\", and grep for the directory, and try to get\n> the FS information, which may be ext4, btrfs, smbfs, nfs....)\n> This is unnecessary complicated, so we endet up in using the path.\n\nI am ok with the change to compare only the location and the system\nname. I wonder a bit though if this change is completely backward\ncompatible.\n\n> If I was a system administrator, (I sometimes am), I would set up a\n> bunch of Linux boxes in a similar way, so that the repo is under\n> /nfs/server1/projects/myproject\n> (and the same path is used for all Linux boxes)\n>\n> The Windows machines may use something like\n> N:/projects/myproject\n> or\n> //server1/projects/myproject\n>\n> and Mac OS may use\n> /Volumes/projects/myproject\n> (If mounted from the finder)\n> or\n> /nfs/projects/myproject\n> (When autofs is used)\n>\n> I may have missed the discussion somewhat, could you explain why having\n> different uname/location combinations are not usable at your site ?\n\nI tried to explain above why using a config option to set the\nuntracked cache implies that you have to be careful at what happens\nwhen people use GIT_WORK_TREE.\n\nBut it should be usable if indeed we put only the system name as you\nsuggest. So I think I will do that.\n\n> How much burden is actually put on your system, and is there a way to keep a\n> list of different clients ?\n\nAbout a list of different client, instead of just one, I don't think\nit is sane given the GIT_WORK_TREE problem.\n\nIf you really have many clients that should use the untracked cache\nwith different paths, then what you might want is a set of config\noptions like this:\n\n[untrackedCache \"windows7\"]\n enable = true\n system = Windows\n systemVersion = 7\n path = C:/my/work/tree\n[untrackedCache \"windows8\"]\n enable = true\n system = Windows\n systemVersion = 8\n path = C:/my/windows8/work/tree\n[untrackedCache \"linux\"]\n enable = true\n system = Linux\n path = ~/work/tree\n\nThis is much more flexible and visible than a custom string list\ninside the untracked cache. But it is overkill for Booking.com's use\ncase (where the machines use Linux and behave well regarding mtime).\nSo if you are willing to implement that, then that would be great!\n\n> What I understand is that a kernel update of a Linux machine shouldn't matter,\n> but if a machine runs Windows or Linux should matter.\n\nEverything could matter. Some people might want to enable it or\ndisable it depending on the value of an environment variable or\nsomething.\n\nInstead of trying to think about all that could matter, we should\nhave, and still should, think about generic and flexible config based\nsolutions.\n\nThanks,\nChristian.\n"},{"id":"275208","messageId":"CAP8UFD1KcMOQ2M7=OnC3i2OKrzBigDphdOAgwjVWqMSU9VRo7g@mail.gmail.com","threadId":"41090","inReplyTo":"xmqqsi2kga1x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 08/10] dir: simplify untracked cache \"ident\" field","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-30T22:01:38Z","receivedAt":"2015-12-30T22:01:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Dec 29, 2015 at 11:32 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> -static int ident_in_untracked(const struct untracked_cache *uc)\n>> +static int ident_current_location_in_untracked(const struct untracked_cache *uc)\n>> {\n>> - const char *end = uc->ident.buf + uc->ident.len;\n>> - const char *p = uc->ident.buf;\n>> + struct strbuf cur_loc = STRBUF_INIT;\n>> + int res = 0;\n>>\n>> - for (p = uc->ident.buf; p < end; p += strlen(p) + 1)\n>> - if (!strcmp(p, get_ident_string()))\n>> - return 1;\n>> - return 0;\n>> + /*\n>> + * Previous git versions may have saved many strings in the\n>> + * \"ident\" field, but it is insane to manage many locations,\n>> + * so just take care of the first one.\n>> + */\n>> +\n>> + strbuf_addf(&cur_loc, \"Location %s, system \", get_git_work_tree());\n>> +\n>> + if (starts_with(uc->ident.buf, cur_loc.buf))\n>> + res = 1;\n>> +\n>> + strbuf_release(&cur_loc);\n>> +\n>> + return res;\n>> }\n>\n> The \", system \" part looks funny. I think I know what you are\n> trying to do, though.\n>\n> If the path to the working tree has \", system \" as a substring,\n> I wonder if funny things may happen with this starts_with()?\n\nYeah, I think in the next iteration I will do what Torsten suggests.\nIt looks like the only sane solution, other than just not using the\n\"ident\" field anymore and relying only on config variables like the\nexample I give in my reply to Torsten.\n"},{"id":"275213","messageId":"CAP8UFD0pOiqa5ZxwM0Vfzr_wMJ+HDrXyhzJ+TmRDED5GH+koMw@mail.gmail.com","threadId":"41090","inReplyTo":"xmqqlh8cg9y2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 09/10] config: add core.untrackedCache","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-30T22:47:44Z","receivedAt":"2015-12-30T22:47:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> +core.untrackedCache::\n>> + Determines if untracked cache will be automatically enabled or\n>> + disabled. It can be `keep`, `true` or `false`. Before setting\n>> + it to `true`, you should check that mtime is working properly\n>> + on your system.\n>> + See linkgit:git-update-index[1]. `keep` by default.\n>> +\n>\n> Before \"Before setting it to `true`\", the reader needs to be told\n> what would happen when this is set to each of these three values (as\n> well as what happens when this is not set at all).\n\nOk, then I think I will write something like:\n\ncore.untrackedCache::\n Determines what to do about the untracked cache feature of the\nindex. It will\n be kept, if this variable is unset or set to `keep`. It will\nautomatically be added\n if set to `true`. And it will automatically be removed, if set to\n`false`. Before\n setting it to `true`, you should check that mtime is working\nproperly on your\n system.\n See linkgit:git-update-index[1]. `keep` by default.\n\n>> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\n>> index a0afe17..813f6cc 100644\n>> --- a/Documentation/git-update-index.txt\n>> +++ b/Documentation/git-update-index.txt\n>> @@ -174,27 +174,31 @@ may not support it yet.\n>>\n>> --untracked-cache::\n>> --no-untracked-cache::\n>> - Enable or disable untracked cache extension. This could speed\n>> - up for commands that involve determining untracked files such\n>> - as `git status`. The underlying operating system and file\n>> - system must change `st_mtime` field of a directory if files\n>> - are added or deleted in that directory.\n>> + Enable or disable untracked cache extension. Please use\n>> + `--test-untracked-cache` before enabling it.\n>\n> \"extension\" is a term that is fairly close to the machinery. In\n> other parts of the documentation (like the added text below in this\n> patch), it is called \"untracked cache FEATURE\", which probably is a\n> better word to use here as well.\n\nOk, I will use \"feature\".\n\n>> +These options cannot override the `core.untrackedCache` configuration\n>> +variable when it is set to `true` or `false` (see\n>> +linkgit:git-config[1]). They can override it only when it is unset or\n>> +set to `keep`. If you want untracked cache to persist, it is better\n>> +anyway to just set this configuration variable to `true` or `false`\n>> +directly.\n>\n> While the above is not wrong per-se, from the point of those who\n> looked for these options (that is, those who wanted to do a one-shot\n> enabling or disabling of the feature, perhaps to try it out to see\n> how well it helps on their system), I think the explanation of the\n> interaction between the option and the config is backwards. For\n> their purpose, setting it to `true` or `false` will be hinderance,\n> because the options are made no-op by such a setting. IOW, the\n> advertisement \"once you decided that you want to use the feature,\n> configuration is a better place to set it\" does not belong here.\n>\n> If I were writing this documentation, I'd probably rephrase the\n> second paragraph like so:\n>\n> These options take effect only when the\n> `core.untrackedCache` configuration variable (see\n> linkgit:git-config[1]) is set to `keep` (or it is left\n> unset). When the configuration variable is set to `true`,\n> the untracked cache feature is always enabled (and when it\n> is set to `false`, it is always disabled), making these\n> options no-op.\n>\n> perhaps.\n\nYeah, but \"no-op\" is not technically true, as if you just set the\nconfig variable to true for example and then use \"--untracked-cache\"\nthen the cache will be added immediately. Also it does not explain\nthat for example \"git update-index --untracked-cache\" will die() if\nthe config variable is set to false.\n\n>> @@ -385,6 +389,34 @@ Although this bit looks similar to assume-unchanged bit, its goal is\n>> different from assume-unchanged bit's. Skip-worktree also takes\n>> precedence over assume-unchanged bit when both are set.\n>>\n>> +Untracked cache\n>> +---------------\n>> +\n>> +This cache could speed up commands that involve determining untracked\n>> +...\n>> +It is recommended to use the `core.untrackedCache` configuration\n>> +variable (see linkgit:git-config[1]) to enable or disable this feature\n>> +instead of using the `--[no-|force-]untracked-cache`, as it is more\n>> +persistant and visible with a configuration variable.\n>\n> s/persistant/persistent/; but more importantly, I do not think\n> persistence has much to do with the choice between the option and\n> configuration. Once it is set with `--untracked-cache`, it should\n> persist until you explicitly use `--no-untracked-cache` to disable\n> it, no? Otherwise you have a bug that needs to be fixed.\n\nYeah, it should persist except if you clone and copy the config file.\nI agree that it is not clear what \"persistent\" means, so maybe I can\njust remove \"as it is more persistant and visible with a configuration\nvariable\".\n\n> The reason you'd want to use a configuration is because the effect\n> of using the `--untracked-cache` option is per repository (rather,\n> per index-file).\n>\n> If you want to enable (or disable) this feature, it is easier to\n> use the `core.untrackedCache` configuration variable than using\n> the `--untracked-cache` option to `git update-index` in each\n> repository, especially if you want to do so across all\n> repositories you use, because you can set the configuration\n> variable to `true` (or `false`) in your `$HOME/.gitconfig` just\n> once and have it affect all repositories you touch.\n>\n> or something, perhaps.\n\nOk, I will use something like that.\n\n>> +When the `core.untrackedCache` configuration variable is changed, the\n>> +untracked cache is added to or removed from the index the next time\n>> +\"git status\" is run; while when `--[no-|force-]untracked-cache` are\n>> +used, the untracked cache is immediately added to or removed from the\n>> +index.\n>\n> Is it only \"git status\", or anything that writes/updates the index\n> file? The above makes it sound as if you are saying \"If you change\n> the variable, you must run `git status` for the change to take\n> effect\", and if that is indeed the case, perhaps you should say so\n> more explicitly. My cursory reading of the code suggests that the\n> user must run `git status` in a mode that shows untracked files\n> (i.e. \"git status -uno\" does not fix)?\n\nYeah, that's what the code does.\n\n> I somehow thought, per the previous discussion with Duy, that the\n> check and addition of an empty one (if missing in the index and\n> configuration is set to `true`) or removal of an existing one (if\n> configuration is set to `false`) were to be done when the index is\n> read by read_index_from(), though. If you have to say \"After\n> setting the configuration, you must run this command\", that does not\n> sound like a huge improvement over \"If you want to enable this\n> feature, you must run this command\".\n\nThe untracked-cache feature, as I tried to explain in an email in the\nprevious discussion, has an effect only on git status when it has to\nshow untracked files. Otherwise the untracked-cache is simply not\nused. It might be a goal to use it more often, but it's not my patch\nseries' goal.\n\nSo why should we have a check to see if maybe the cache should be\nadded or removed in all the other cases when the cache is not used?\n\n>> + switch (untracked_cache) {\n>> + case UC_UNSPECIFIED:\n>> + break;\n>> + case UC_DISABLE:\n>> + if (use_untracked_cache == 1)\n>> + die(\"core.untrackedCache is set to true; \"\n>> + \"remove or change it, if you really want to \"\n>> + \"disable the untracked cache\");\n>> remove_untracked_cache();\n>> report(_(\"Untracked cache disabled\"));\n>> + break;\n>> + case UC_TEST:\n>> + setup_work_tree();\n>> + return !test_if_untracked_cache_is_supported();\n>> + case UC_ENABLE:\n>> + case UC_FORCE:\n>> + if (use_untracked_cache == 0)\n>> + die(\"core.untrackedCache is set to false; \"\n>> + \"remove or change it, if you really want to \"\n>> + \"enable the untracked cache\");\n>> + add_untracked_cache();\n>> + report(_(\"Untracked cache enabled for '%s'\"), get_git_work_tree());\n>> + break;\n>\n> I do buy the decision to make these no-op when the configuration\n> says `true` or `false`, but I am not sure if these deserve die().\n>\n> Exiting with 0 (= success) after issuing a warning() might be more\n> appropriate.\n\nScripts users might just not look at the warnings and expect things to\nwork if the exit code is 0.\n\n> I'd especially anticipate that some newbies will\n> complain that they got \"fatal:\" when they used the \"--force-\"\n> variant, saying \"I know what I am doing, that is why I said 'force',\n> why does stupid Git refuse?\".\n\nI think this should be taken care of in the \"--force-\" documentation.\nMaybe we should say that it is deprecated and make it clear, if we\ndon't already, that it does nothing more than without \"force-\".\n"},{"id":"275215","messageId":"xmqqbn97cyh3.fsf@gitster.mtv.corp.google.com","threadId":"41090","inReplyTo":"CAP8UFD0pOiqa5ZxwM0Vfzr_wMJ+HDrXyhzJ+TmRDED5GH+koMw@mail.gmail.com","subject":"Re: [PATCH v4 09/10] config: add core.untrackedCache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-30T23:23:36Z","receivedAt":"2015-12-30T23:23:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano wrote:\n> ...\n>> While the above is not wrong per-se, from the point of those who\n>> looked for these options (that is, those who wanted to do a one-shot\n>> enabling or disabling of the feature, perhaps to try it out to see\n>> how well it helps on their system), I think the explanation of the\n>> interaction between the option and the config is backwards. For\n>> their purpose, setting it to `true` or `false` will be hinderance,\n>> because the options are made no-op by such a setting. IOW, the\n>> advertisement \"once you decided that you want to use the feature,\n>> configuration is a better place to set it\" does not belong here.\n>>\n>> If I were writing this documentation, I'd probably rephrase the\n>> second paragraph like so:\n>>\n>> These options take effect only when the\n>> `core.untrackedCache` configuration variable (see\n>> linkgit:git-config[1]) is set to `keep` (or it is left\n>> unset). When the configuration variable is set to `true`,\n>> the untracked cache feature is always enabled (and when it\n>> is set to `false`, it is always disabled), making these\n>> options no-op.\n>>\n>> perhaps.\n>\n> Yeah, but \"no-op\" is not technically true, as if you just set the\n> config variable to true for example and then use \"--untracked-cache\"\n> then the cache will be added immediately.\n\nThe \"update-index --[no-]untracked-cache\" command is made to no\nlonger follow the user's immediate desire (i.e. enable or disable)\nexpressed by the invocation of the command. That is what I meant by\n'no-op'.\n\n> ... for example \"git update-index --untracked-cache\" will die() if\n> the config variable is set to false.\n\nAs I think it is a BUG to make these die(), it is an irrelevant\nobjection ;-).\n\n>> I somehow thought, per the previous discussion with Duy, that the\n>> check and addition of an empty one (if missing in the index and\n>> configuration is set to `true`) or removal of an existing one (if\n>> configuration is set to `false`) were to be done when the index is\n>> read by read_index_from(), though. If you have to say \"After\n>> setting the configuration, you must run this command\", that does not\n>> sound like a huge improvement over \"If you want to enable this\n>> feature, you must run this command\".\n>\n> The untracked-cache feature, as I tried to explain in an email in the\n> previous discussion, has an effect only on git status when it has to\n> show untracked files. Otherwise the untracked-cache is simply not\n> used. It might be a goal to use it more often, but it's not my patch\n> series' goal.\n\nIn the future we may extend the system to allow \"git add somedir/\"\nto take advantage of the untracked cache (i.e. we may already know\nwhat untracked paths there are without letting readdir(3) to scan it\nin somedir/), for example. Is it reasonable to force users to say\n\"git status\", in such a future? And more importantly, when that\nfuture comes, is it reasonable to force users to re-learn Git to do\nsomething else to do things differently at that point?\n\nItt sounds like somewhat a short-sighted mindset to design the\nsystem, and I was hoping that by now you would have become better\nthan that.\n\nThe real question is what are the problems in implementing this in\nthe way Duy suggested in the previous discussion. The answer may\nfall into somewhere between \"that approach does not work in such and\nsuch cases, so this is the best I could come up with\" and \"I know\nthat approach is far superiour, but I have invested too much in this\ninferiour approach and refuse to rework it further to make it better.\"\n\nI unfortunately am not getting the sense that I know where your\nanswer would fall into that spectrum from your response.\n\n>> Exiting with 0 (= success) after issuing a warning() might be more\n>> appropriate.\n>\n> Scripts users might just not look at the warnings and expect things to\n> work if the exit code is 0.\n\nThat is exactly why I said it is inappropriate to error out.\n\nUsing of not using untracked-cache is (supposed to be) purely\nperformance and not correctness thing, and I do not think the users\nand the scripts do not deserve to see a failure from \"update-index\n--untracked-cache\" only because there is a stray core.untrackedCache\nset to 'false' somewhere.\n"},{"id":"275236","messageId":"CAP8UFD35VZ3QCLg525RCpacDrRHqt7EhxV1MeL-9xxHf8BCZ+A@mail.gmail.com","threadId":"41090","inReplyTo":"xmqqbn97cyh3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 09/10] config: add core.untrackedCache","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-12-31T12:33:24Z","receivedAt":"2015-12-31T12:33:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Dec 31, 2015 at 12:23 AM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano wrote:\n>> ...\n>>> While the above is not wrong per-se, from the point of those who\n>>> looked for these options (that is, those who wanted to do a one-shot\n>>> enabling or disabling of the feature, perhaps to try it out to see\n>>> how well it helps on their system), I think the explanation of the\n>>> interaction between the option and the config is backwards. For\n>>> their purpose, setting it to `true` or `false` will be hinderance,\n>>> because the options are made no-op by such a setting. IOW, the\n>>> advertisement \"once you decided that you want to use the feature,\n>>> configuration is a better place to set it\" does not belong here.\n>>>\n>>> If I were writing this documentation, I'd probably rephrase the\n>>> second paragraph like so:\n>>>\n>>> These options take effect only when the\n>>> `core.untrackedCache` configuration variable (see\n>>> linkgit:git-config[1]) is set to `keep` (or it is left\n>>> unset). When the configuration variable is set to `true`,\n>>> the untracked cache feature is always enabled (and when it\n>>> is set to `false`, it is always disabled), making these\n>>> options no-op.\n>>>\n>>> perhaps.\n>>\n>> Yeah, but \"no-op\" is not technically true, as if you just set the\n>> config variable to true for example and then use \"--untracked-cache\"\n>> then the cache will be added immediately.\n>\n> The \"update-index --[no-]untracked-cache\" command is made to no\n> longer follow the user's immediate desire (i.e. enable or disable)\n> expressed by the invocation of the command. That is what I meant by\n> 'no-op'.\n>\n>> ... for example \"git update-index --untracked-cache\" will die() if\n>> the config variable is set to false.\n>\n> As I think it is a BUG to make these die(), it is an irrelevant\n> objection ;-).\n\nOk I think I will just use what you suggest except the \"no-op\" part:\n\n These options take effect only when the\n `core.untrackedCache` configuration variable (see\n linkgit:git-config[1]) is set to `keep` (or it is left\n unset). When the configuration variable is set to `true`,\n the untracked cache feature is always enabled (and when it\n is set to `false`, it is always disabled).\n\n\n>>> I somehow thought, per the previous discussion with Duy, that the\n>>> check and addition of an empty one (if missing in the index and\n>>> configuration is set to `true`) or removal of an existing one (if\n>>> configuration is set to `false`) were to be done when the index is\n>>> read by read_index_from(), though. If you have to say \"After\n>>> setting the configuration, you must run this command\", that does not\n>>> sound like a huge improvement over \"If you want to enable this\n>>> feature, you must run this command\".\n>>\n>> The untracked-cache feature, as I tried to explain in an email in the\n>> previous discussion, has an effect only on git status when it has to\n>> show untracked files. Otherwise the untracked-cache is simply not\n>> used. It might be a goal to use it more often, but it's not my patch\n>> series' goal.\n>\n> In the future we may extend the system to allow \"git add somedir/\"\n> to take advantage of the untracked cache (i.e. we may already know\n> what untracked paths there are without letting readdir(3) to scan it\n> in somedir/), for example. Is it reasonable to force users to say\n> \"git status\", in such a future?\n\nIf a new feature takes advantage of the untracked cache, it is\nreasonable that this feature enables or disables it if should be\nenabled or disabled. Maybe we can just say in the doc something like:\n\n When the `core.untrackedCache` configuration variable is changed, the\n untracked cache is added to or removed from the index the next time\n a command that can use the untracked cache is run; while when\n `--[no-|force-]untracked-cache` are used, the untracked cache is\n immediately added to or removed from the index. Currently only\n \"git status\" can use the untracked cache.\n\n> And more importantly, when that\n> future comes, is it reasonable to force users to re-learn Git to do\n> something else to do things differently at that point?\n\nI don't think people will have to relearn anything or do things\ndifferently especially if things are explained like above.\n\nIf they want to use the untracked cache by setting a config option,\nand then want to enable it right away, they can still use git status\nfor that, so things will continue to work.\n\nWhat scenario do you have in mind where people would have to do things\ndifferently?\n\n> Itt sounds like somewhat a short-sighted mindset to design the\n> system, and I was hoping that by now you would have become better\n> than that.\n>\n> The real question is what are the problems in implementing this in\n> the way Duy suggested in the previous discussion. The answer may\n> fall into somewhere between \"that approach does not work in such and\n> such cases, so this is the best I could come up with\" and \"I know\n> that approach is far superiour, but I have invested too much in this\n> inferiour approach and refuse to rework it further to make it better.\"\n\nMy question is why should I invest time thinking about and testing\nanother approach when the current approach seems simpler, less bug\nprone, faster and without any downside?\n\n> I unfortunately am not getting the sense that I know where your\n> answer would fall into that spectrum from your response.\n>\n>>> Exiting with 0 (= success) after issuing a warning() might be more\n>>> appropriate.\n>>\n>> Scripts users might just not look at the warnings and expect things to\n>> work if the exit code is 0.\n>\n> That is exactly why I said it is inappropriate to error out.\n\nSuppose that we do as you suggest. This means that when \"git\nupdate-index --untracked-cache\" is called while the config variable is\nfalse, we issue a warning, add the UC to the index and exit 0.\n\nSo a legacy script calling \"git update-index --untracked-cache\" and\ngetting a 0 exit code from it will assume that every time \"git status\"\nis called the UC will be used because it has been enabled in the\nindex. But that would be wrong if the config variable is false,\nbecause in this case when git status is called, it will remove the UC.\n\n> Using of not using untracked-cache is (supposed to be) purely\n> performance and not correctness thing, and I do not think the users\n> and the scripts do not deserve to see a failure from \"update-index\n> --untracked-cache\" only because there is a stray core.untrackedCache\n> set to 'false' somewhere.\n\nThis \"stray core.untrackedCache\" could not have been put there by\nusers of previous git versions because it has no meaning before this\npatch series. So I don't understand why you call it \"a stray\ncore.untrackedCache\".\n\nIt is no more \"stray\" to me than the call to \"update-index --untracked-cache\".\n\nIf it has been put in /etc/git.config by an admin and if the user\nthinks he knows better, the user can still change the config locally\nto override the system one.\n"},{"id":"275311","messageId":"xmqqd1thp683.fsf@gitster.mtv.corp.google.com","threadId":"41090","inReplyTo":"CAP8UFD35VZ3QCLg525RCpacDrRHqt7EhxV1MeL-9xxHf8BCZ+A@mail.gmail.com","subject":"Re: [PATCH v4 09/10] config: add core.untrackedCache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-04T18:09:00Z","receivedAt":"2016-01-04T18:09:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> What scenario do you have in mind where people would have to do things\n> differently?\n\nThey eventually will see a system in which that they do not have do\nanything after flipping the configuration, yet will still see stale\n\"you must run 'git status'\" on their websearches and SO questions,\nwhich would be a cost for them to remember that they no longer have\nto do the extra 'git status'.\n\n>> Itt sounds like somewhat a short-sighted mindset to design the\n>> system, and I was hoping that by now you would have become better\n>> than that.\n>>\n>> The real question is what are the problems in implementing this in\n>> the way Duy suggested in the previous discussion. The answer may\n>> fall into somewhere between \"that approach does not work in such and\n>> such cases, so this is the best I could come up with\" and \"I know\n>> that approach is far superiour, but I have invested too much in this\n>> inferiour approach and refuse to rework it further to make it better.\"\n>\n> My question is why should I invest time thinking about and testing\n> another approach when the current approach seems simpler, less bug\n> prone, faster and without any downside?\n\nAs to the downside, I think Duy's \"at the time we read the index\" is\nthe least problematic route from the maintainability point of view.\nWhat are you doing to help people who make changes in the future to\nthe system to make \"git add $dir\" or \"git clean $dir\" aware of the\nuntracked cache not to forget doing the \"oh, the config says we want\nto add the untracked cache if missing, so we do it here\" in their\nnew codepath? Whey you say \"This is only about status\", you are\nessentially saying \"It's outside the scope of my job, I was hired to\nimprove the usability of 'git status' with untracked cache and I do\nnot care about the longer term overall health of the system\".\n\nNow, you said something about \"simpler\", \"less bug prone\" and\n\"faster\" (I doubt you can make such statements that involve\ncomparison without investing time thinking about other approaches,\nthough, but that is a separate topic). That would mean that you see\n\"complexity\", \"error prone-ness\", and \"slowness\" in the way Duy\nsuggested---that is exactly the question I asked you in the message\nyou are responding to. What are the problems in implementing this\nin the way Duy suggested? What kind of \"complexity\" do you see?\nWhich part is more \"error prone\"? Why does it have to be \"slower\"?\n\n>> Using of not using untracked-cache is (supposed to be) purely\n>> performance and not correctness thing, and I do not think the users\n>> and the scripts do not deserve to see a failure from \"update-index\n>> --untracked-cache\" only because there is a stray core.untrackedCache\n>> set to 'false' somewhere.\n>\n> This \"stray core.untrackedCache\" could not have been put there by\n> users of previous git versions because it has no meaning before this\n> patch series. So I don't understand why you call it \"a stray\n> core.untrackedCache\".\n>\n> It is no more \"stray\" to me than the call to \"update-index --untracked-cache\".\n>\n> If it has been put in /etc/git.config by an admin and if the user\n> thinks he knows better, the user can still change the config locally\n> to override the system one.\n\nYou are assuming that everybody constantly looks at /etc/git.config\nto make sure evil admins won't do things that affect their\nrepositories and use of Git in potentially negative way. I doubt\nanybody does.\n\nBy the way, I understand that this \"stray one affects without user\nbeing aware of it\" is the primary the reason why Ævar wants this\n'configuration automatically adds untracked cache even to the\nexisting index' feature---everybody will get it without even be\naware of the change their admins make. Which may be a good thing\nfor those who use the configuration variable.\n"},{"id":"275564","messageId":"CAP8UFD2eJjR4QxtE0x8BbKOAGwi4cwERG7bF89apPbJye356AQ@mail.gmail.com","threadId":"41090","inReplyTo":"xmqqd1thp683.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 09/10] config: add core.untrackedCache","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-01-08T17:52:31Z","receivedAt":"2016-01-08T17:52:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jan 4, 2016 at 7:09 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>>>\n>>> The real question is what are the problems in implementing this in\n>>> the way Duy suggested in the previous discussion. The answer may\n>>> fall into somewhere between \"that approach does not work in such and\n>>> such cases, so this is the best I could come up with\" and \"I know\n>>> that approach is far superiour, but I have invested too much in this\n>>> inferiour approach and refuse to rework it further to make it better.\"\n>>\n>> My question is why should I invest time thinking about and testing\n>> another approach when the current approach seems simpler, less bug\n>> prone, faster and without any downside?\n\nI just tried Duy's approach by moving into read_index_from() the code\nI had put in wt_status_collect_untracked() and it doesn't work because\n\"git status\" calls read_index_from() before calling\ngit_default_core_config().\n\nThe interesting parts of the backtraces for both calls are the following:\n\n#0 read_index_from (istate=0x8660a0 , path=0x868b90\n\".git/index\") at read-cache.c:1626\n#1 0x0000000000530ba3 in read_index (istate=0x8660a0 ) at\nread-cache.c:1404\n#2 0x00000000005743e1 in gitmodules_config () at submodule.c:188\n#3 0x0000000000431ed0 in status_init_config (s=0x83aa20 ,\nfn=0x434edd ) at builtin/commit.c:186\n\n#0 git_default_core_config (var=0x86ac00 \"core.untrackedcache\",\nvalue=0x86ac60 \"true\") at config.c:695\n#1 0x00000000004be761 in git_default_config (var=0x86ac00\n\"core.untrackedcache\", value=0x86ac60 \"true\", dummy=0x0) at\nconfig.c:1016\n#2 0x00000000004cfeba in git_diff_basic_config (var=0x86ac00\n\"core.untrackedcache\", value=0x86ac60 \"true\", cb=0x0) at diff.c:277\n#3 0x00000000004cfc9b in git_diff_ui_config (var=0x86ac00\n\"core.untrackedcache\", value=0x86ac60 \"true\", cb=0x0) at diff.c:230\n#4 0x000000000043524a in git_status_config (k=0x86ac00\n\"core.untrackedcache\", v=0x86ac60 \"true\", cb=0x83aa20 ) at\nbuiltin/commit.c:1313\n#5 0x00000000004bf183 in configset_iter (cs=0x8495e0\n, fn=0x434edd , data=0x83aa20 )\nat config.c:1305\n#6 0x00000000004bf206 in git_config (fn=0x434edd ,\ndata=0x83aa20 ) at config.c:1317\n#7 0x0000000000431ee3 in status_init_config (s=0x83aa20 ,\nfn=0x434edd ) at builtin/commit.c:187\n\nYou can see that in status_init_config(), gitmodules_config() is\ncalled just before git_config().\nAnd unfortunately gitmodules_config() indirectly calls read_index_from().\n\nSo indeed it seems very bug prone to me to rely on read_index_from()\nbeing called before git_default_core_config().\n"},{"id":"275572","messageId":"xmqqpoxb26xv.fsf@gitster.mtv.corp.google.com","threadId":"41090","inReplyTo":"CAP8UFD2eJjR4QxtE0x8BbKOAGwi4cwERG7bF89apPbJye356AQ@mail.gmail.com","subject":"Re: [PATCH v4 09/10] config: add core.untrackedCache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-08T19:43:24Z","receivedAt":"2016-01-08T19:43:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> I just tried Duy's approach by moving into read_index_from() the code\n> I had put in wt_status_collect_untracked() and it doesn't work because\n> \"git status\" calls read_index_from() before calling\n> git_default_core_config().\n\nI didn't expect to (and didn't want to) see the configuration be\ndead by git_default_core_config() in the first place, though.\nI instead was expecting that it would be done through the caching\nlayer, e.g. by calling git_config_get_maybe_bool() interface when\nread_index_from() wants to see if it needs to muck with the existing\n(or missing) extension.\n"}]}