[PATCH v4 00/10] Untracked cache improvements

[PATCH v4 00/10] Untracked cache improvements

[Christian Couder]

Here is a new version of a patch series to improve the untracked cache
feature.

This v4 implements core.untrackedCache as a tristate config
variable. When it's `true`, Git commands, especially `git status`,
should always add the untracked cache and use it, and when `false`,
Git commands should remove it. The default though is now `keep` in
which case the untracked cache is neither removed nor added, and used
if it is there.

Patch 1/10 is a small bugfix that has not changed since v3.

Patch 2/10 to 4/10 add some small features that are missing. The only
chqnge there since v3 is that we are now using `report()` to display
verbose information, thanks to Duy.

Patchs 5/10 to 7/10 are some refactoring to prepare for the following
patchs. Among them 6/10 is the result of merging two patchs from v3,
thanks to Eric.

Patch 8/10 deals with the "ident" field in "struct untracked_cache"
and is mostly the same as in v3. The difference is just a small bug
fix to prevent a crash.

Patch 9/10 adds core.untrackedCache. It has been changed compared to
v3 in the following ways:
  - the config variable is now a tristate, thanks to Junio,
  - we use `switch` to deal with different values, thanks to Torsten,
  - documentation for --test-untracked-cache is improved, thanks to
    David.

Patch 10/10, which contains tests, has been changed to reflect changes
in 9/10 and to add a few tests.

So the changes compared to v3 are mostly small updates, and patchs
6/10, 9/10 and 10/10.

The patch series is also available there:

https://github.com/chriscool/git/tree/uc-notifs40

Thanks to the reviewers and helpers.

Christian Couder (10):
  dir: free untracked cache when removing it
  update-index: use enum for untracked cache options
  update-index: add --test-untracked-cache
  update-index: add untracked cache notifications
  update-index: move 'uc' var declaration
  dir: add {new,add}_untracked_cache()
  dir: add remove_untracked_cache()
  dir: simplify untracked cache "ident" field
  config: add core.untrackedCache
  t7063: add tests for core.untrackedCache

 Documentation/config.txt               |  7 +++
 Documentation/git-update-index.txt     | 65 +++++++++++++++++++++++-----
 builtin/update-index.c                 | 62 ++++++++++++++++----------
 cache.h                                |  1 +
 config.c                               | 11 +++++
 contrib/completion/git-completion.bash |  1 +
 dir.c                                  | 70 ++++++++++++++++++++++++------
 dir.h                                  |  2 +
 environment.c                          |  1 +
 t/t7063-status-untracked-cache.sh      | 79 +++++++++++++++++++++++++++++++---
 wt-status.c                            | 13 ++++++
 11 files changed, 260 insertions(+), 52 deletions(-)

-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 01/10] dir: free untracked cache when removing it

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7431938..a6fff87 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1123,6 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		add_untracked_ident(the_index.untracked);
 		the_index.cache_changed |= UNTRACKED_CHANGED;
 	} else if (!untracked_cache && the_index.untracked) {
+		free_untracked_cache(the_index.untracked);
 		the_index.untracked = NULL;
 		the_index.cache_changed |= UNTRACKED_CHANGED;
 	}
-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 02/10] update-index: use enum for untracked cache options

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index a6fff87..1e546a3 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -35,6 +35,14 @@ static int mark_skip_worktree_only;
 #define UNMARK_FLAG 2
 static struct strbuf mtime_dir = STRBUF_INIT;
 
+/* Untracked cache mode */
+enum uc_mode {
+	UC_UNSPECIFIED = -1,
+	UC_DISABLE = 0,
+	UC_ENABLE,
+	UC_FORCE
+};
+
 __attribute__((format (printf, 1, 2)))
 static void report(const char *fmt, ...)
 {
@@ -902,7 +910,7 @@ static int reupdate_callback(struct parse_opt_ctx_t *ctx,
 int cmd_update_index(int argc, const char **argv, const char *prefix)
 {
 	int newfd, entries, has_errors = 0, line_termination = '\n';
-	int untracked_cache = -1;
+	enum uc_mode untracked_cache = UC_UNSPECIFIED;
 	int read_from_stdin = 0;
 	int prefix_length = prefix ? strlen(prefix) : 0;
 	int preferred_index_format = 0;
@@ -997,7 +1005,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "untracked-cache", &untracked_cache,
 			N_("enable/disable untracked cache")),
 		OPT_SET_INT(0, "force-untracked-cache", &untracked_cache,
-			    N_("enable untracked cache without testing the filesystem"), 2),
+			    N_("enable untracked cache without testing the filesystem"), UC_FORCE),
 		OPT_END()
 	};
 
@@ -1104,10 +1112,10 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		the_index.split_index = NULL;
 		the_index.cache_changed |= SOMETHING_CHANGED;
 	}
-	if (untracked_cache > 0) {
+	if (untracked_cache > UC_DISABLE) {
 		struct untracked_cache *uc;
 
-		if (untracked_cache < 2) {
+		if (untracked_cache < UC_FORCE) {
 			setup_work_tree();
 			if (!test_if_untracked_cache_is_supported())
 				return 1;
@@ -1122,7 +1130,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		}
 		add_untracked_ident(the_index.untracked);
 		the_index.cache_changed |= UNTRACKED_CHANGED;
-	} else if (!untracked_cache && the_index.untracked) {
+	} else if (untracked_cache == UC_DISABLE && the_index.untracked) {
 		free_untracked_cache(the_index.untracked);
 		the_index.untracked = NULL;
 		the_index.cache_changed |= UNTRACKED_CHANGED;
-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 03/10] update-index: add --test-untracked-cache

[Christian Couder]

It is nice to just be able to test if untracked cache is
supported without enabling it.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-update-index.txt | 12 +++++++++++-
 builtin/update-index.c             |  5 +++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index f4e5a85..a0afe17 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -18,7 +18,7 @@ SYNOPSIS
 	     [--[no-]skip-worktree]
 	     [--ignore-submodules]
 	     [--[no-]split-index]
-	     [--[no-|force-]untracked-cache]
+	     [--[no-|test-|force-]untracked-cache]
 	     [--really-refresh] [--unresolve] [--again | -g]
 	     [--info-only] [--index-info]
 	     [-z] [--stdin] [--index-version <n>]
@@ -180,6 +180,16 @@ may not support it yet.
 	system must change `st_mtime` field of a directory if files
 	are added or deleted in that directory.
 
+--test-untracked-cache::
+	Only perform tests on the working directory to make sure
+	untracked cache can be used. You have to manually enable
+	untracked cache using `--force-untracked-cache` (or
+	`--untracked-cache` but this will run the tests again)
+	afterwards if you really want to use it. If a test fails
+	the exit code is 1 and a message explains what is not
+	working as needed, otherwise the exit code is 0 and OK is
+	printed.
+
 --force-untracked-cache::
 	For safety, `--untracked-cache` performs tests on the working
 	directory to make sure untracked cache can be used. These
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 1e546a3..62222dd 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -40,6 +40,7 @@ enum uc_mode {
 	UC_UNSPECIFIED = -1,
 	UC_DISABLE = 0,
 	UC_ENABLE,
+	UC_TEST,
 	UC_FORCE
 };
 
@@ -1004,6 +1005,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			N_("enable or disable split index")),
 		OPT_BOOL(0, "untracked-cache", &untracked_cache,
 			N_("enable/disable untracked cache")),
+		OPT_SET_INT(0, "test-untracked-cache", &untracked_cache,
+			    N_("test if the filesystem supports untracked cache"), UC_TEST),
 		OPT_SET_INT(0, "force-untracked-cache", &untracked_cache,
 			    N_("enable untracked cache without testing the filesystem"), UC_FORCE),
 		OPT_END()
@@ -1119,6 +1122,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			setup_work_tree();
 			if (!test_if_untracked_cache_is_supported())
 				return 1;
+			if (untracked_cache == UC_TEST)
+				return 0;
 		}
 		if (!the_index.untracked) {
 			uc = xcalloc(1, sizeof(*uc));
-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 04/10] update-index: add untracked cache notifications

[Christian Couder]

Attempting to flip the untracked-cache feature on for a random index
file with

    cd /random/unrelated/place
    git --git-dir=/somewhere/else/.git update-index --untracked-cache

would not work as you might expect. Because flipping the feature on
in the index also records the location of the corresponding working
tree (/random/unrelated/place in the above example), when the index
is subsequently used to keep track of files in the working tree in
/somewhere/else, the feature is disabled.

With this patch "git update-index --[test-]untracked-cache" tells the
user in which directory tests are performed. This makes it easy to
spot any problem.

Also in verbose mode, let's tell the user when the cache is enabled
or disabled.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 62222dd..369c207 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -130,7 +130,7 @@ static int test_if_untracked_cache_is_supported(void)
 	if (!mkdtemp(mtime_dir.buf))
 		die_errno("Could not make temporary directory");
 
-	fprintf(stderr, _("Testing "));
+	fprintf(stderr, _("Testing mtime in '%s' "), xgetcwd());
 	atexit(remove_test_directory);
 	xstat_mtime_dir(&st);
 	fill_stat_data(&base, &st);
@@ -1135,10 +1135,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		}
 		add_untracked_ident(the_index.untracked);
 		the_index.cache_changed |= UNTRACKED_CHANGED;
-	} else if (untracked_cache == UC_DISABLE && the_index.untracked) {
-		free_untracked_cache(the_index.untracked);
-		the_index.untracked = NULL;
-		the_index.cache_changed |= UNTRACKED_CHANGED;
+		report(_("Untracked cache enabled for '%s'"), get_git_work_tree());
+	} else if (untracked_cache == UC_DISABLE) {
+		if (the_index.untracked) {
+			free_untracked_cache(the_index.untracked);
+			the_index.untracked = NULL;
+			the_index.cache_changed |= UNTRACKED_CHANGED;
+		}
+		report(_("Untracked cache disabled"));
 	}
 
 	if (active_cache_changed) {
-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 05/10] update-index: move 'uc' var declaration

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 369c207..fe7aaa3 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1116,8 +1116,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		the_index.cache_changed |= SOMETHING_CHANGED;
 	}
 	if (untracked_cache > UC_DISABLE) {
-		struct untracked_cache *uc;
-
 		if (untracked_cache < UC_FORCE) {
 			setup_work_tree();
 			if (!test_if_untracked_cache_is_supported())
@@ -1126,7 +1124,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 				return 0;
 		}
 		if (!the_index.untracked) {
-			uc = xcalloc(1, sizeof(*uc));
+			struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
 			strbuf_init(&uc->ident, 100);
 			uc->exclude_per_dir = ".gitignore";
 			/* should be the same flags used by git-status */
-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 06/10] dir: add {new,add}_untracked_cache()

[Christian Couder]

Factor out code into new_untracked_cache() and
add_untracked_cache(), which will be used
in later commits.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 11 +----------
 dir.c                  | 18 ++++++++++++++++++
 dir.h                  |  1 +
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index fe7aaa3..74a6f8d 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1123,16 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			if (untracked_cache == UC_TEST)
 				return 0;
 		}
-		if (!the_index.untracked) {
-			struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
-			strbuf_init(&uc->ident, 100);
-			uc->exclude_per_dir = ".gitignore";
-			/* should be the same flags used by git-status */
-			uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
-			the_index.untracked = uc;
-		}
-		add_untracked_ident(the_index.untracked);
-		the_index.cache_changed |= UNTRACKED_CHANGED;
+		add_untracked_cache();
 		report(_("Untracked cache enabled for '%s'"), get_git_work_tree());
 	} else if (untracked_cache == UC_DISABLE) {
 		if (the_index.untracked) {
diff --git a/dir.c b/dir.c
index d2a8f06..4227ba6 100644
--- a/dir.c
+++ b/dir.c
@@ -1938,6 +1938,24 @@ void add_untracked_ident(struct untracked_cache *uc)
 	strbuf_addch(&uc->ident, 0);
 }
 
+static void new_untracked_cache(void)
+{
+	struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
+	strbuf_init(&uc->ident, 100);
+	uc->exclude_per_dir = ".gitignore";
+	/* should be the same flags used by git-status */
+	uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
+	the_index.untracked = uc;
+}
+
+void add_untracked_cache(void)
+{
+	if (!the_index.untracked)
+		new_untracked_cache();
+	add_untracked_ident(the_index.untracked);
+	the_index.cache_changed |= UNTRACKED_CHANGED;
+}
+
 static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,
 						      int base_len,
 						      const struct pathspec *pathspec)
diff --git a/dir.h b/dir.h
index 7b5855d..ee94c76 100644
--- a/dir.h
+++ b/dir.h
@@ -308,4 +308,5 @@ void free_untracked_cache(struct untracked_cache *);
 struct untracked_cache *read_untracked_extension(const void *data, unsigned long sz);
 void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
 void add_untracked_ident(struct untracked_cache *);
+void add_untracked_cache(void);
 #endif
-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 07/10] dir: add remove_untracked_cache()

[Christian Couder]

Factor out code into remove_untracked_cache(), which will be used
in a later commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 6 +-----
 dir.c                  | 9 +++++++++
 dir.h                  | 1 +
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 74a6f8d..7844991 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1126,11 +1126,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		add_untracked_cache();
 		report(_("Untracked cache enabled for '%s'"), get_git_work_tree());
 	} else if (untracked_cache == UC_DISABLE) {
-		if (the_index.untracked) {
-			free_untracked_cache(the_index.untracked);
-			the_index.untracked = NULL;
-			the_index.cache_changed |= UNTRACKED_CHANGED;
-		}
+		remove_untracked_cache();
 		report(_("Untracked cache disabled"));
 	}
 
diff --git a/dir.c b/dir.c
index 4227ba6..dba1ad0 100644
--- a/dir.c
+++ b/dir.c
@@ -1956,6 +1956,15 @@ void add_untracked_cache(void)
 	the_index.cache_changed |= UNTRACKED_CHANGED;
 }
 
+void remove_untracked_cache(void)
+{
+	if (the_index.untracked) {
+		free_untracked_cache(the_index.untracked);
+		the_index.untracked = NULL;
+		the_index.cache_changed |= UNTRACKED_CHANGED;
+	}
+}
+
 static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,
 						      int base_len,
 						      const struct pathspec *pathspec)
diff --git a/dir.h b/dir.h
index ee94c76..3e5114d 100644
--- a/dir.h
+++ b/dir.h
@@ -309,4 +309,5 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
 void add_untracked_ident(struct untracked_cache *);
 void add_untracked_cache(void);
+void remove_untracked_cache(void);
 #endif
-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 08/10] dir: simplify untracked cache "ident" field

[Christian Couder]

It is not a good idea to compare kernel versions and disable
the untracked cache if it changes as people may upgrade and
still want the untracked cache to work. So let's just
compare work tree locations to decide if we should disable
it.

Also it's not useful to store many locations in the ident
field and compare to any of them. It can even be dangerous
if GIT_WORK_TREE is used with different values. So let's
just store one location, the location of the current work
tree.

If this location changed and we still want an untracked
cache, let's delete the cache and recreate it.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 dir.c | 49 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/dir.c b/dir.c
index dba1ad0..1a8b5a2 100644
--- a/dir.c
+++ b/dir.c
@@ -1918,23 +1918,36 @@ static const char *get_ident_string(void)
 	return sb.buf;
 }
 
-static int ident_in_untracked(const struct untracked_cache *uc)
+static int ident_current_location_in_untracked(const struct untracked_cache *uc)
 {
-	const char *end = uc->ident.buf + uc->ident.len;
-	const char *p   = uc->ident.buf;
+	struct strbuf cur_loc = STRBUF_INIT;
+	int res = 0;
 
-	for (p = uc->ident.buf; p < end; p += strlen(p) + 1)
-		if (!strcmp(p, get_ident_string()))
-			return 1;
-	return 0;
+	/*
+	 * Previous git versions may have saved many strings in the
+	 * "ident" field, but it is insane to manage many locations,
+	 * so just take care of the first one.
+	 */
+
+	strbuf_addf(&cur_loc, "Location %s, system ", get_git_work_tree());
+
+	if (starts_with(uc->ident.buf, cur_loc.buf))
+		res = 1;
+
+	strbuf_release(&cur_loc);
+
+	return res;
 }
 
-void add_untracked_ident(struct untracked_cache *uc)
+static void set_untracked_ident(struct untracked_cache *uc)
 {
-	if (ident_in_untracked(uc))
-		return;
+	strbuf_reset(&uc->ident);
 	strbuf_addstr(&uc->ident, get_ident_string());
-	/* this strbuf contains a list of strings, save NUL too */
+
+	/*
+	 * This strbuf used to contain a list of NUL separated
+	 * strings, so save NUL too for backward compatibility.
+	 */
 	strbuf_addch(&uc->ident, 0);
 }
 
@@ -1945,15 +1958,21 @@ static void new_untracked_cache(void)
 	uc->exclude_per_dir = ".gitignore";
 	/* should be the same flags used by git-status */
 	uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
+	set_untracked_ident(uc);
 	the_index.untracked = uc;
+	the_index.cache_changed |= UNTRACKED_CHANGED;
 }
 
 void add_untracked_cache(void)
 {
-	if (!the_index.untracked)
+	if (!the_index.untracked) {
 		new_untracked_cache();
-	add_untracked_ident(the_index.untracked);
-	the_index.cache_changed |= UNTRACKED_CHANGED;
+	} else {
+		if (!ident_current_location_in_untracked(the_index.untracked)) {
+			free_untracked_cache(the_index.untracked);
+			new_untracked_cache();
+		}
+	}
 }
 
 void remove_untracked_cache(void)
@@ -2021,7 +2040,7 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
 	if (dir->exclude_list_group[EXC_CMDL].nr)
 		return NULL;
 
-	if (!ident_in_untracked(dir->untracked)) {
+	if (!ident_current_location_in_untracked(dir->untracked)) {
 		warning(_("Untracked cache is disabled on this system."));
 		return NULL;
 	}
-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 09/10] config: add core.untrackedCache

[Christian Couder]

When we know that mtime is fully supported by the environment, we
might want the untracked cache to be always used by default without
any mtime test or kernel version check being performed.

Also when we know that mtime is not supported by the environment,
for example because the repo is shared over a network file system,
then we might want 'git update-index --untracked-cache' to fail
immediately instead of performing tests (because it might work on
some systems using the repo over the network file system but not
others).

The normal way to achieve the above in Git is to use a config
variable. That's why this patch introduces "core.untrackedCache".

This variable is a tristate, `keep`, `false` and `true`, which
default to `keep`.

When "git status" is run, it now adds or removes the untracked
cache in the index to respect the value of this variable. So it
does nothing if the value is `keep` or if the variable is unset;
it adds the untracked cache if the value is `true`; and it
removes the cache if the value is `false`.

`git update-index --[no-|force-]untracked-cache` still adds or
removes the untracked cache from the index, but this now fails
if it goes against the value of core.untrackedCache.

Also `--untracked-cache` used to check that the underlying
operating system and file system change `st_mtime` field of a
directory if files are added or deleted in that directory. But
those tests take a long time and there is now
`--test-untracked-cache` to perform them.

That's why, to be more consistent with other git commands, this
patch prevents `--untracked-cache` to perform tests, so that
after this patch there is no difference any more between
`--untracked-cache` and `--force-untracked-cache`.

This last change is backward incompatible and should be
mentioned in the release notes.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt               |  7 ++++
 Documentation/git-update-index.txt     | 65 ++++++++++++++++++++++++++--------
 builtin/update-index.c                 | 35 ++++++++++++------
 cache.h                                |  1 +
 config.c                               | 11 ++++++
 contrib/completion/git-completion.bash |  1 +
 environment.c                          |  1 +
 t/t7063-status-untracked-cache.sh      |  4 +--
 wt-status.c                            | 13 +++++++
 9 files changed, 109 insertions(+), 29 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f617886..d892127 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -308,6 +308,13 @@ core.trustctime::
 	crawlers and some backup systems).
 	See linkgit:git-update-index[1]. True by default.
 
+core.untrackedCache::
+	Determines if untracked cache will be automatically enabled or
+	disabled. It can be `keep`, `true` or `false`. Before setting
+	it to `true`, you should check that mtime is working properly
+	on your system.
+	See linkgit:git-update-index[1]. `keep` by default.
+
 core.checkStat::
 	Determines which stat fields to match between the index
 	and work tree. The user can set this to 'default' or
diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index a0afe17..813f6cc 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -174,27 +174,31 @@ may not support it yet.
 
 --untracked-cache::
 --no-untracked-cache::
-	Enable or disable untracked cache extension. This could speed
-	up for commands that involve determining untracked files such
-	as `git status`. The underlying operating system and file
-	system must change `st_mtime` field of a directory if files
-	are added or deleted in that directory.
+	Enable or disable untracked cache extension. Please use
+	`--test-untracked-cache` before enabling it.
++
+These options cannot override the `core.untrackedCache` configuration
+variable when it is set to `true` or `false` (see
+linkgit:git-config[1]). They can override it only when it is unset or
+set to `keep`. If you want untracked cache to persist, it is better
+anyway to just set this configuration variable to `true` or `false`
+directly.
 
 --test-untracked-cache::
 	Only perform tests on the working directory to make sure
 	untracked cache can be used. You have to manually enable
-	untracked cache using `--force-untracked-cache` (or
-	`--untracked-cache` but this will run the tests again)
-	afterwards if you really want to use it. If a test fails
-	the exit code is 1 and a message explains what is not
-	working as needed, otherwise the exit code is 0 and OK is
-	printed.
+	untracked cache using `--untracked-cache` or
+	`--force-untracked-cache` or the `core.untrackedCache`
+	configuration variable afterwards if you really want to use
+	it. If a test fails the exit code is 1 and a message
+	explains what is not working as needed, otherwise the exit
+	code is 0 and OK is printed.
 
 --force-untracked-cache::
-	For safety, `--untracked-cache` performs tests on the working
-	directory to make sure untracked cache can be used. These
-	tests can take a few seconds. `--force-untracked-cache` can be
-	used to skip the tests.
+	Same as `--untracked-cache`. Provided for backwards
+	compatibility with older versions of Git where
+	`--untracked-cache` used to imply `--test-untracked-cache` but
+	this option would enable the extension unconditionally.
 
 \--::
 	Do not interpret any more arguments as options.
@@ -385,6 +389,34 @@ Although this bit looks similar to assume-unchanged bit, its goal is
 different from assume-unchanged bit's. Skip-worktree also takes
 precedence over assume-unchanged bit when both are set.
 
+Untracked cache
+---------------
+
+This cache could speed up commands that involve determining untracked
+files such as `git status`.
+
+This feature works by recording the mtime of the working tree
+directories and then omitting reading directories and stat calls
+against files in those directories whose mtime hasn't changed. For
+this to work the underlying operating system and file system must
+change the `st_mtime` field of directories if files in the directory
+are added, modified or deleted.
+
+You can test whether the filesystem supports that with the
+`--test-untracked-cache` option. The `--untracked-cache` option used
+to implicitly perform that test in older versions of Git, but that's
+no longer the case.
+
+It is recommended to use the `core.untrackedCache` configuration
+variable (see linkgit:git-config[1]) to enable or disable this feature
+instead of using the `--[no-|force-]untracked-cache`, as it is more
+persistant and visible with a configuration variable.
+
+When the `core.untrackedCache` configuration variable is changed, the
+untracked cache is added to or removed from the index the next time
+"git status" is run; while when `--[no-|force-]untracked-cache` are
+used, the untracked cache is immediately added to or removed from the
+index.
 
 Configuration
 -------------
@@ -410,6 +442,9 @@ It can be useful when the inode change time is regularly modified by
 something outside Git (file system crawlers and backup systems use
 ctime for marking files processed) (see linkgit:git-config[1]).
 
+The untracked cache extension can be enabled by the
+`core.untrackedCache` configuration variable (see
+linkgit:git-config[1]).
 
 SEE ALSO
 --------
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7844991..497f904 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1115,19 +1115,32 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		the_index.split_index = NULL;
 		the_index.cache_changed |= SOMETHING_CHANGED;
 	}
-	if (untracked_cache > UC_DISABLE) {
-		if (untracked_cache < UC_FORCE) {
-			setup_work_tree();
-			if (!test_if_untracked_cache_is_supported())
-				return 1;
-			if (untracked_cache == UC_TEST)
-				return 0;
-		}
-		add_untracked_cache();
-		report(_("Untracked cache enabled for '%s'"), get_git_work_tree());
-	} else if (untracked_cache == UC_DISABLE) {
+
+	switch (untracked_cache) {
+	case UC_UNSPECIFIED:
+		break;
+	case UC_DISABLE:
+		if (use_untracked_cache == 1)
+			die("core.untrackedCache is set to true; "
+			    "remove or change it, if you really want to "
+			    "disable the untracked cache");
 		remove_untracked_cache();
 		report(_("Untracked cache disabled"));
+		break;
+	case UC_TEST:
+		setup_work_tree();
+		return !test_if_untracked_cache_is_supported();
+	case UC_ENABLE:
+	case UC_FORCE:
+		if (use_untracked_cache == 0)
+			die("core.untrackedCache is set to false; "
+			    "remove or change it, if you really want to "
+			    "enable the untracked cache");
+		add_untracked_cache();
+		report(_("Untracked cache enabled for '%s'"), get_git_work_tree());
+		break;
+	default:
+		die("Bug: bad untracked_cache value: %d", untracked_cache);
 	}
 
 	if (active_cache_changed) {
diff --git a/cache.h b/cache.h
index c63fcc1..8d09858 100644
--- a/cache.h
+++ b/cache.h
@@ -619,6 +619,7 @@ extern void set_alternate_index_output(const char *);
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int trust_ctime;
+extern int use_untracked_cache;
 extern int check_stat;
 extern int quote_path_fully;
 extern int has_symlinks;
diff --git a/config.c b/config.c
index 86a5eb2..b7d7b25 100644
--- a/config.c
+++ b/config.c
@@ -691,6 +691,17 @@ static int git_default_core_config(const char *var, const char *value)
 		trust_ctime = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "core.untrackedcache")) {
+		if (!strcasecmp(value, "keep"))
+			use_untracked_cache = -1;
+		else {
+			use_untracked_cache = git_config_maybe_bool(var, value);
+			if (use_untracked_cache == -1)
+				error("unknown core.untrackedCache value '%s'; "
+				      "using 'keep' default value", value);
+		}
+		return 0;
+	}
 	if (!strcmp(var, "core.checkstat")) {
 		if (!strcasecmp(value, "default"))
 			check_stat = 1;
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6956807..84bcec3 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2055,6 +2055,7 @@ _git_config ()
 		core.sparseCheckout
 		core.symlinks
 		core.trustctime
+		core.untrackedCache
 		core.warnAmbiguousRefs
 		core.whitespace
 		core.worktree
diff --git a/environment.c b/environment.c
index 2da7fe2..394b15c 100644
--- a/environment.c
+++ b/environment.c
@@ -14,6 +14,7 @@
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
+int use_untracked_cache = -1; /* keep */
 int check_stat = 1;
 int has_symlinks = 1;
 int minimum_abbrev = 4, default_abbrev = 7;
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 0e8d0d4..253160a 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -8,10 +8,8 @@ avoid_racy() {
 	sleep 1
 }
 
-# It's fine if git update-index returns an error code other than one,
-# it'll be caught in the first test.
 test_lazy_prereq UNTRACKED_CACHE '
-	{ git update-index --untracked-cache; ret=$?; } &&
+	{ git update-index --test-untracked-cache; ret=$?; } &&
 	test $ret -ne 1
 '
 
diff --git a/wt-status.c b/wt-status.c
index bba2596..75fafe5 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -578,6 +578,19 @@ static void wt_status_collect_untracked(struct wt_status *s)
 	if (!s->show_untracked_files)
 		return;
 
+	switch (use_untracked_cache) {
+	case -1: /* keep: do nothing */
+		break;
+	case 0: /* false */
+		remove_untracked_cache();
+		break;
+	case 1: /* true */
+		add_untracked_cache();
+		break;
+	default: /* unknown value: do nothing */
+		break;
+	}
+
 	memset(&dir, 0, sizeof(dir));
 	if (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)
 		dir.flags |=
-- 
2.7.0.rc2.10.g544ad6b

[PATCH v4 10/10] t7063: add tests for core.untrackedCache

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7063-status-untracked-cache.sh | 75 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 4 deletions(-)

diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 253160a..67e049e 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -18,6 +18,10 @@ if ! test_have_prereq UNTRACKED_CACHE; then
 	test_done
 fi
 
+test_expect_success 'core.untrackedCache is unset' '
+	test_must_fail git config --get core.untrackedCache
+'
+
 test_expect_success 'setup' '
 	git init worktree &&
 	cd worktree &&
@@ -30,13 +34,13 @@ test_expect_success 'setup' '
 
 test_expect_success 'untracked cache is empty' '
 	test-dump-untracked-cache >../actual &&
-	cat >../expect <<EOF &&
+	cat >../expect-empty <<EOF &&
 info/exclude 0000000000000000000000000000000000000000
 core.excludesfile 0000000000000000000000000000000000000000
 exclude_per_dir .gitignore
 flags 00000006
 EOF
-	test_cmp ../expect ../actual
+	test_cmp ../expect-empty ../actual
 '
 
 cat >../status.expect <<EOF &&
@@ -506,7 +510,7 @@ EOF
 
 test_expect_success 'verify untracked cache dump (sparse/subdirs)' '
 	test-dump-untracked-cache >../actual &&
-	cat >../expect <<EOF &&
+	cat >../expect-from-test-dump <<EOF &&
 info/exclude 13263c0978fb9fad16b2d580fb800b6d811c3ff0
 core.excludesfile 0000000000000000000000000000000000000000
 exclude_per_dir .gitignore
@@ -525,7 +529,7 @@ file
 /dtwo/ 0000000000000000000000000000000000000000 recurse check_only valid
 two
 EOF
-	test_cmp ../expect ../actual
+	test_cmp ../expect-from-test-dump ../actual
 '
 
 test_expect_success 'test sparse status again with untracked cache and subdir' '
@@ -569,4 +573,67 @@ EOF
 	test_cmp ../status.expect ../status.actual
 '
 
+test_expect_success '--no-untracked-cache removes the cache' '
+	git update-index --no-untracked-cache &&
+	test-dump-untracked-cache >../actual &&
+	echo "no untracked cache" >../expect &&
+	test_cmp ../expect ../actual
+'
+
+test_expect_success 'git status does not change anything' '
+	git status &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect ../actual
+'
+
+test_expect_success 'setting core.untrackedCache to true and using git status creates the cache' '
+	git config core.untrackedCache true &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect ../actual &&
+	git status &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect-from-test-dump ../actual
+'
+
+test_expect_success 'using --no-untracked-cache fails when core.untrackedCache is true' '
+	test_must_fail git update-index --no-untracked-cache &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect-from-test-dump ../actual
+'
+
+test_expect_success 'setting core.untrackedCache to false and using git status removes the cache' '
+	git config core.untrackedCache false &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect-from-test-dump ../actual &&
+	git status &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect ../actual
+'
+
+test_expect_success 'using --untracked-cache fails when core.untrackedCache is false' '
+	test_must_fail git update-index --untracked-cache &&
+	test_must_fail git update-index --force-untracked-cache &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect ../actual
+'
+
+test_expect_success 'setting core.untrackedCache to keep' '
+	git config core.untrackedCache keep &&
+	git update-index --untracked-cache &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect-empty ../actual &&
+	git status &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect-from-test-dump ../actual &&
+	git update-index --no-untracked-cache &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect ../actual &&
+	git update-index --force-untracked-cache &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect-empty ../actual &&
+	git status &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect-from-test-dump ../actual
+'
+
 test_done
-- 
2.7.0.rc2.10.g544ad6b

Re: [PATCH v4 01/10] dir: free untracked cache when removing it

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/update-index.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 7431938..a6fff87 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1123,6 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  		add_untracked_ident(the_index.untracked);
>  		the_index.cache_changed |= UNTRACKED_CHANGED;
>  	} else if (!untracked_cache && the_index.untracked) {
> +		free_untracked_cache(the_index.untracked);
>  		the_index.untracked = NULL;
>  		the_index.cache_changed |= UNTRACKED_CHANGED;
>  	}

Looks good.

Re: [PATCH v4 03/10] update-index: add --test-untracked-cache

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> It is nice to just be able to test if untracked cache is
> supported without enabling it.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/git-update-index.txt | 12 +++++++++++-
>  builtin/update-index.c             |  5 +++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
> index f4e5a85..a0afe17 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -18,7 +18,7 @@ SYNOPSIS
>  	     [--[no-]skip-worktree]
>  	     [--ignore-submodules]
>  	     [--[no-]split-index]
> -	     [--[no-|force-]untracked-cache]
> +	     [--[no-|test-|force-]untracked-cache]
>  	     [--really-refresh] [--unresolve] [--again | -g]
>  	     [--info-only] [--index-info]
>  	     [-z] [--stdin] [--index-version <n>]
> @@ -180,6 +180,16 @@ may not support it yet.
>  	system must change `st_mtime` field of a directory if files
>  	are added or deleted in that directory.
>  
> +--test-untracked-cache::
> +	Only perform tests on the working directory to make sure
> +	untracked cache can be used. You have to manually enable
> +	untracked cache using `--force-untracked-cache` (or
> +	`--untracked-cache` but this will run the tests again)
> +	afterwards if you really want to use it. If a test fails
> +	the exit code is 1 and a message explains what is not
> +	working as needed, otherwise the exit code is 0 and OK is
> +	printed.
> +

Looks good.

>  --force-untracked-cache::
>  	For safety, `--untracked-cache` performs tests on the working
>  	directory to make sure untracked cache can be used. These
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 1e546a3..62222dd 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -40,6 +40,7 @@ enum uc_mode {
>  	UC_UNSPECIFIED = -1,
>  	UC_DISABLE = 0,
>  	UC_ENABLE,
> +	UC_TEST,
>  	UC_FORCE
>  };
>  
> @@ -1004,6 +1005,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  			N_("enable or disable split index")),
>  		OPT_BOOL(0, "untracked-cache", &untracked_cache,
>  			N_("enable/disable untracked cache")),
> +		OPT_SET_INT(0, "test-untracked-cache", &untracked_cache,
> +			    N_("test if the filesystem supports untracked cache"), UC_TEST),
>  		OPT_SET_INT(0, "force-untracked-cache", &untracked_cache,
>  			    N_("enable untracked cache without testing the filesystem"), UC_FORCE),
>  		OPT_END()
> @@ -1119,6 +1122,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  			setup_work_tree();
>  			if (!test_if_untracked_cache_is_supported())
>  				return 1;
> +			if (untracked_cache == UC_TEST)
> +				return 0;
>  		}
>  		if (!the_index.untracked) {
>  			uc = xcalloc(1, sizeof(*uc));

Re: [PATCH v4 08/10] dir: simplify untracked cache "ident" field

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> -static int ident_in_untracked(const struct untracked_cache *uc)
> +static int ident_current_location_in_untracked(const struct untracked_cache *uc)
>  {
> -	const char *end = uc->ident.buf + uc->ident.len;
> -	const char *p   = uc->ident.buf;
> +	struct strbuf cur_loc = STRBUF_INIT;
> +	int res = 0;
>  
> -	for (p = uc->ident.buf; p < end; p += strlen(p) + 1)
> -		if (!strcmp(p, get_ident_string()))
> -			return 1;
> -	return 0;
> +	/*
> +	 * Previous git versions may have saved many strings in the
> +	 * "ident" field, but it is insane to manage many locations,
> +	 * so just take care of the first one.
> +	 */
> +
> +	strbuf_addf(&cur_loc, "Location %s, system ", get_git_work_tree());
> +
> +	if (starts_with(uc->ident.buf, cur_loc.buf))
> +		res = 1;
> +
> +	strbuf_release(&cur_loc);
> +
> +	return res;
>  }

The ", system " part looks funny.  I think I know what you are
trying to do, though.

If the path to the working tree has ", system " as a substring,
I wonder if funny things may happen with this starts_with()?

Re: [PATCH v4 09/10] config: add core.untrackedCache

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> +core.untrackedCache::
> +	Determines if untracked cache will be automatically enabled or
> +	disabled. It can be `keep`, `true` or `false`. Before setting
> +	it to `true`, you should check that mtime is working properly
> +	on your system.
> +	See linkgit:git-update-index[1]. `keep` by default.
> +

Before "Before setting it to `true`", the reader needs to be told
what would happen when this is set to each of these three values (as
well as what happens when this is not set at all).

> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
> index a0afe17..813f6cc 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -174,27 +174,31 @@ may not support it yet.
>  
>  --untracked-cache::
>  --no-untracked-cache::
> -	Enable or disable untracked cache extension. This could speed
> -	up for commands that involve determining untracked files such
> -	as `git status`. The underlying operating system and file
> -	system must change `st_mtime` field of a directory if files
> -	are added or deleted in that directory.
> +	Enable or disable untracked cache extension. Please use
> +	`--test-untracked-cache` before enabling it.

"extension" is a term that is fairly close to the machinery.  In
other parts of the documentation (like the added text below in this
patch), it is called "untracked cache FEATURE", which probably is a
better word to use here as well.

> ++
> +These options cannot override the `core.untrackedCache` configuration
> +variable when it is set to `true` or `false` (see
> +linkgit:git-config[1]). They can override it only when it is unset or
> +set to `keep`. If you want untracked cache to persist, it is better
> +anyway to just set this configuration variable to `true` or `false`
> +directly.

While the above is not wrong per-se, from the point of those who
looked for these options (that is, those who wanted to do a one-shot
enabling or disabling of the feature, perhaps to try it out to see
how well it helps on their system), I think the explanation of the
interaction between the option and the config is backwards.  For
their purpose, setting it to `true` or `false` will be hinderance,
because the options are made no-op by such a setting.  IOW, the
advertisement "once you decided that you want to use the feature,
configuration is a better place to set it" does not belong here.

If I were writing this documentation, I'd probably rephrase the
second paragraph like so:

	These options take effect only when the
	`core.untrackedCache` configuration variable (see
	linkgit:git-config[1]) is set to `keep` (or it is left
	unset).  When the configuration variable is set to `true`,
	the untracked cache feature is always enabled (and when it
	is set to `false`, it is always disabled), making these
	options no-op.

perhaps.

> @@ -385,6 +389,34 @@ Although this bit looks similar to assume-unchanged bit, its goal is
>  different from assume-unchanged bit's. Skip-worktree also takes
>  precedence over assume-unchanged bit when both are set.
>  
> +Untracked cache
> +---------------
> +
> +This cache could speed up commands that involve determining untracked
> +...
> +It is recommended to use the `core.untrackedCache` configuration
> +variable (see linkgit:git-config[1]) to enable or disable this feature
> +instead of using the `--[no-|force-]untracked-cache`, as it is more
> +persistant and visible with a configuration variable.

s/persistant/persistent/; but more importantly, I do not think
persistence has much to do with the choice between the option and
configuration.  Once it is set with `--untracked-cache`, it should
persist until you explicitly use `--no-untracked-cache` to disable
it, no?  Otherwise you have a bug that needs to be fixed.

The reason you'd want to use a configuration is because the effect
of using the `--untracked-cache` option is per repository (rather,
per index-file).

    If you want to enable (or disable) this feature, it is easier to
    use the `core.untrackedCache` configuration variable than using
    the `--untracked-cache` option to `git update-index` in each
    repository, especially if you want to do so across all
    repositories you use, because you can set the configuration
    variable to `true` (or `false`) in your `$HOME/.gitconfig` just
    once and have it affect all repositories you touch.

or something, perhaps.

> +When the `core.untrackedCache` configuration variable is changed, the
> +untracked cache is added to or removed from the index the next time
> +"git status" is run; while when `--[no-|force-]untracked-cache` are
> +used, the untracked cache is immediately added to or removed from the
> +index.

Is it only "git status", or anything that writes/updates the index
file?  The above makes it sound as if you are saying "If you change
the variable, you must run `git status` for the change to take
effect", and if that is indeed the case, perhaps you should say so
more explicitly.  My cursory reading of the code suggests that the
user must run `git status` in a mode that shows untracked files
(i.e. "git status -uno" does not fix)?

I somehow thought, per the previous discussion with Duy, that the
check and addition of an empty one (if missing in the index and
configuration is set to `true`) or removal of an existing one (if
configuration is set to `false`) were to be done when the index is
read by read_index_from(), though.  If you have to say "After
setting the configuration, you must run this command", that does not
sound like a huge improvement over "If you want to enable this
feature, you must run this command".

> +	switch (untracked_cache) {
> +	case UC_UNSPECIFIED:
> +		break;
> +	case UC_DISABLE:
> +		if (use_untracked_cache == 1)
> +			die("core.untrackedCache is set to true; "
> +			    "remove or change it, if you really want to "
> +			    "disable the untracked cache");
>  		remove_untracked_cache();
>  		report(_("Untracked cache disabled"));
> +		break;
> +	case UC_TEST:
> +		setup_work_tree();
> +		return !test_if_untracked_cache_is_supported();
> +	case UC_ENABLE:
> +	case UC_FORCE:
> +		if (use_untracked_cache == 0)
> +			die("core.untrackedCache is set to false; "
> +			    "remove or change it, if you really want to "
> +			    "enable the untracked cache");
> +		add_untracked_cache();
> +		report(_("Untracked cache enabled for '%s'"), get_git_work_tree());
> +		break;

I do buy the decision to make these no-op when the configuration
says `true` or `false`, but I am not sure if these deserve die().

Exiting with 0 (= success) after issuing a warning() might be more
appropriate.  I'd especially anticipate that some newbies will
complain that they got "fatal:" when they used the "--force-"
variant, saying "I know what I am doing, that is why I said 'force',
why does stupid Git refuse?".

Re: [PATCH v4 08/10] dir: simplify untracked cache "ident" field

[Torsten Bögershausen]

On 2015-12-29 08.09, Christian Couder wrote:
> It is not a good idea to compare kernel versions and disable
> the untracked cache if it changes as people may upgrade and
> still want the untracked cache to work. So let's just
> compare work tree locations to decide if we should disable
> it.
OK with that.
> 
> Also it's not useful to store many locations in the ident
> field and compare to any of them. It can even be dangerous
> if GIT_WORK_TREE is used with different values. So let's
> just store one location, the location of the current work
> tree.
I don't think I fully agree in all details.
The list is there to distinguish between different clients when sharing
a Git workspace over a network to different clients:

A file system is exported from Linux to Linux via NFS,
including a Git workspace)
The same Workspace is exported via SAMBA to Windows and, in an extreme case,
AFS to Mac OS.

Other combinations could be
Linux - SAMBA - Linux
Linux - AFP - Linux

Mac OS - NFS - Linux
Mac OS - AFP - Mac OS
Mac OS - SMB - Linux, Mac OS, Windows
The list is incomplete (BSD and other Unix is missing),
there are other combinations of network protocols,
there are NAS boxes which mostly Linux under the hood, but
may have vendor specific tweaks.

Now we want to know, which of the combinations work.
The different clients need to test separately.
E.g. for a given server:

NFS - Linux
SMB - Linux
SMB Windows.

At this point I would agree that the old code from dir.c:

static const char *get_ident_string(void)
{
	static struct strbuf sb = STRBUF_INIT;
	struct utsname uts;

	if (sb.len)
		return sb.buf;
	if (uname(&uts) < 0)
		die_errno(_("failed to get kernel name and information"));
	strbuf_addf(&sb, "Location %s, system %s %s %s", get_git_work_tree(),
		    uts.sysname, uts.release, uts.version);
	return sb.buf;
}
------------------
is unneccessary picky, and the sysname should be enough:

strbuf_addf(&sb, "Location %s, system %s", get_git_work_tree(),
uts.sysname);


The old code did not find out, which network protocol that we used,
(you need to call "mount", and grep for the directory, and try to get
the FS information, which may be ext4, btrfs, smbfs, nfs....)
This is unnecessary complicated, so we endet up in using the path.

If I was a system administrator, (I sometimes am), I would set up a
bunch of Linux boxes in a similar way, so that the repo is under
/nfs/server1/projects/myproject
(and the same path is used for all Linux boxes)

The Windows machines may use something like
N:/projects/myproject
or
//server1/projects/myproject

and Mac OS may use
/Volumes/projects/myproject
(If mounted from the finder)
or
/nfs/projects/myproject
(When autofs is used)


I may have missed the discussion somewhat, could you explain why having
different uname/location combinations are not usable at your site ?

How much burden is actually put on your system, and is there a way to keep a
list of different clients ?


What I understand is that a kernel update of a Linux machine shouldn't matter,
but if a machine runs Windows or Linux should matter.

Re: [PATCH v4 08/10] dir: simplify untracked cache "ident" field

[Christian Couder]

On Wed, Dec 30, 2015 at 12:47 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 2015-12-29 08.09, Christian Couder wrote:
>> It is not a good idea to compare kernel versions and disable
>> the untracked cache if it changes as people may upgrade and
>> still want the untracked cache to work. So let's just
>> compare work tree locations to decide if we should disable
>> it.
> OK with that.
>>
>> Also it's not useful to store many locations in the ident
>> field and compare to any of them. It can even be dangerous
>> if GIT_WORK_TREE is used with different values. So let's
>> just store one location, the location of the current work
>> tree.
> I don't think I fully agree in all details.

Suppose I use "git update-index --untracked-cache" first with
GIT_WORK_TREE=/foo and then with GIT_WORK_TREE=/bar. It will store
both /foo and /bar as valid locations for the worktree in the "ident"
field.

Then I work a bit with GIT_WORK_TREE=/foo and then a bit with
GIT_WORK_TREE=/bar. Now does the untracked cache info in the index
reflect the state of /foo or the state of /bar?

With a config variable, the problem is worse, because, as we
automatically add the untracked cache when git status is used, we are
even less likely to pay attention to GIT_WORK_TREE when the cache is
added.

> The list is there to distinguish between different clients when sharing
> a Git workspace over a network to different clients:
>
> A file system is exported from Linux to Linux via NFS,
> including a Git workspace)
> The same Workspace is exported via SAMBA to Windows and, in an extreme case,
> AFS to Mac OS.
>
> Other combinations could be
> Linux - SAMBA - Linux
> Linux - AFP - Linux
>
> Mac OS - NFS - Linux
> Mac OS - AFP - Mac OS
> Mac OS - SMB - Linux, Mac OS, Windows
> The list is incomplete (BSD and other Unix is missing),
> there are other combinations of network protocols,
> there are NAS boxes which mostly Linux under the hood, but
> may have vendor specific tweaks.
>
> Now we want to know, which of the combinations work.

In my opinion at this point it is a bit insane to want untracked cache
to work when it is accessed by different clients over the network. It
is much safer to just disable it (with a config option) in this case.
But I am ok to try to improve things for your use case to work if it
doesn't complicate things too much.

> The different clients need to test separately.
> E.g. for a given server:
>
> NFS - Linux
> SMB - Linux
> SMB Windows.
>
> At this point I would agree that the old code from dir.c:
>
> static const char *get_ident_string(void)
> {
>         static struct strbuf sb = STRBUF_INIT;
>         struct utsname uts;
>
>         if (sb.len)
>                 return sb.buf;
>         if (uname(&uts) < 0)
>                 die_errno(_("failed to get kernel name and information"));
>         strbuf_addf(&sb, "Location %s, system %s %s %s", get_git_work_tree(),
>                     uts.sysname, uts.release, uts.version);
>         return sb.buf;
> }
> ------------------
> is unneccessary picky, and the sysname should be enough:
>
> strbuf_addf(&sb, "Location %s, system %s", get_git_work_tree(),
> uts.sysname);
>
> The old code did not find out, which network protocol that we used,
> (you need to call "mount", and grep for the directory, and try to get
> the FS information, which may be ext4, btrfs, smbfs, nfs....)
> This is unnecessary complicated, so we endet up in using the path.

I am ok with the change to compare only the location and the system
name. I wonder a bit though if this change is completely backward
compatible.

> If I was a system administrator, (I sometimes am), I would set up a
> bunch of Linux boxes in a similar way, so that the repo is under
> /nfs/server1/projects/myproject
> (and the same path is used for all Linux boxes)
>
> The Windows machines may use something like
> N:/projects/myproject
> or
> //server1/projects/myproject
>
> and Mac OS may use
> /Volumes/projects/myproject
> (If mounted from the finder)
> or
> /nfs/projects/myproject
> (When autofs is used)
>
> I may have missed the discussion somewhat, could you explain why having
> different uname/location combinations are not usable at your site ?

I tried to explain above why using a config option to set the
untracked cache implies that you have to be careful at what happens
when people use GIT_WORK_TREE.

But it should be usable if indeed we put only the system name as you
suggest. So I think I will do that.

> How much burden is actually put on your system, and is there a way to keep a
> list of different clients ?

About a list of different client, instead of just one, I don't think
it is sane given the GIT_WORK_TREE problem.

If you really have many clients that should use the untracked cache
with different paths, then what you might want is a set of config
options like this:

[untrackedCache "windows7"]
        enable = true
        system = Windows
        systemVersion = 7
        path = C:/my/work/tree
[untrackedCache "windows8"]
        enable = true
        system = Windows
        systemVersion = 8
        path = C:/my/windows8/work/tree
[untrackedCache "linux"]
        enable = true
        system = Linux
        path = ~/work/tree

This is much more flexible and visible than a custom string list
inside the untracked cache. But it is overkill for Booking.com's use
case (where the machines use Linux and behave well regarding mtime).
So if you are willing to implement that, then that would be great!

> What I understand is that a kernel update of a Linux machine shouldn't matter,
> but if a machine runs Windows or Linux should matter.

Everything could matter. Some people might want to enable it or
disable it depending on the value of an environment variable or
something.

Instead of trying to think about all that could matter, we should
have, and still should, think about generic and flexible config based
solutions.

Thanks,
Christian.

Re: [PATCH v4 08/10] dir: simplify untracked cache "ident" field

[Christian Couder]

On Tue, Dec 29, 2015 at 11:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> -static int ident_in_untracked(const struct untracked_cache *uc)
>> +static int ident_current_location_in_untracked(const struct untracked_cache *uc)
>>  {
>> -     const char *end = uc->ident.buf + uc->ident.len;
>> -     const char *p   = uc->ident.buf;
>> +     struct strbuf cur_loc = STRBUF_INIT;
>> +     int res = 0;
>>
>> -     for (p = uc->ident.buf; p < end; p += strlen(p) + 1)
>> -             if (!strcmp(p, get_ident_string()))
>> -                     return 1;
>> -     return 0;
>> +     /*
>> +      * Previous git versions may have saved many strings in the
>> +      * "ident" field, but it is insane to manage many locations,
>> +      * so just take care of the first one.
>> +      */
>> +
>> +     strbuf_addf(&cur_loc, "Location %s, system ", get_git_work_tree());
>> +
>> +     if (starts_with(uc->ident.buf, cur_loc.buf))
>> +             res = 1;
>> +
>> +     strbuf_release(&cur_loc);
>> +
>> +     return res;
>>  }
>
> The ", system " part looks funny.  I think I know what you are
> trying to do, though.
>
> If the path to the working tree has ", system " as a substring,
> I wonder if funny things may happen with this starts_with()?

Yeah, I think in the next iteration I will do what Torsten suggests.
It looks like the only sane solution, other than just not using the
"ident" field anymore and relying only on config variables like the
example I give in my reply to Torsten.

Re: [PATCH v4 09/10] config: add core.untrackedCache

[Christian Couder]

On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> +core.untrackedCache::
>> +     Determines if untracked cache will be automatically enabled or
>> +     disabled. It can be `keep`, `true` or `false`. Before setting
>> +     it to `true`, you should check that mtime is working properly
>> +     on your system.
>> +     See linkgit:git-update-index[1]. `keep` by default.
>> +
>
> Before "Before setting it to `true`", the reader needs to be told
> what would happen when this is set to each of these three values (as
> well as what happens when this is not set at all).

Ok, then I think I will write something like:

core.untrackedCache::
     Determines what to do about the untracked cache feature of the
index. It will
     be kept, if this variable is unset or set to `keep`. It will
automatically be added
     if set to `true`. And it will automatically be removed, if set to
`false`. Before
     setting it to `true`, you should check that mtime is working
properly on your
     system.
     See linkgit:git-update-index[1]. `keep` by default.

>> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
>> index a0afe17..813f6cc 100644
>> --- a/Documentation/git-update-index.txt
>> +++ b/Documentation/git-update-index.txt
>> @@ -174,27 +174,31 @@ may not support it yet.
>>
>>  --untracked-cache::
>>  --no-untracked-cache::
>> -     Enable or disable untracked cache extension. This could speed
>> -     up for commands that involve determining untracked files such
>> -     as `git status`. The underlying operating system and file
>> -     system must change `st_mtime` field of a directory if files
>> -     are added or deleted in that directory.
>> +     Enable or disable untracked cache extension. Please use
>> +     `--test-untracked-cache` before enabling it.
>
> "extension" is a term that is fairly close to the machinery.  In
> other parts of the documentation (like the added text below in this
> patch), it is called "untracked cache FEATURE", which probably is a
> better word to use here as well.

Ok, I will use "feature".

>> +These options cannot override the `core.untrackedCache` configuration
>> +variable when it is set to `true` or `false` (see
>> +linkgit:git-config[1]). They can override it only when it is unset or
>> +set to `keep`. If you want untracked cache to persist, it is better
>> +anyway to just set this configuration variable to `true` or `false`
>> +directly.
>
> While the above is not wrong per-se, from the point of those who
> looked for these options (that is, those who wanted to do a one-shot
> enabling or disabling of the feature, perhaps to try it out to see
> how well it helps on their system), I think the explanation of the
> interaction between the option and the config is backwards.  For
> their purpose, setting it to `true` or `false` will be hinderance,
> because the options are made no-op by such a setting.  IOW, the
> advertisement "once you decided that you want to use the feature,
> configuration is a better place to set it" does not belong here.
>
> If I were writing this documentation, I'd probably rephrase the
> second paragraph like so:
>
>         These options take effect only when the
>         `core.untrackedCache` configuration variable (see
>         linkgit:git-config[1]) is set to `keep` (or it is left
>         unset).  When the configuration variable is set to `true`,
>         the untracked cache feature is always enabled (and when it
>         is set to `false`, it is always disabled), making these
>         options no-op.
>
> perhaps.

Yeah, but "no-op" is not technically true, as if you just set the
config variable to true for example and then use "--untracked-cache"
then the cache will be added immediately. Also it does not explain
that for example "git update-index --untracked-cache" will die() if
the config variable is set to false.

>> @@ -385,6 +389,34 @@ Although this bit looks similar to assume-unchanged bit, its goal is
>>  different from assume-unchanged bit's. Skip-worktree also takes
>>  precedence over assume-unchanged bit when both are set.
>>
>> +Untracked cache
>> +---------------
>> +
>> +This cache could speed up commands that involve determining untracked
>> +...
>> +It is recommended to use the `core.untrackedCache` configuration
>> +variable (see linkgit:git-config[1]) to enable or disable this feature
>> +instead of using the `--[no-|force-]untracked-cache`, as it is more
>> +persistant and visible with a configuration variable.
>
> s/persistant/persistent/; but more importantly, I do not think
> persistence has much to do with the choice between the option and
> configuration.  Once it is set with `--untracked-cache`, it should
> persist until you explicitly use `--no-untracked-cache` to disable
> it, no?  Otherwise you have a bug that needs to be fixed.

Yeah, it should persist except if you clone and copy the config file.
I agree that it is not clear what "persistent" means, so maybe I can
just remove "as it is more persistant and visible with a configuration
variable".

> The reason you'd want to use a configuration is because the effect
> of using the `--untracked-cache` option is per repository (rather,
> per index-file).
>
>     If you want to enable (or disable) this feature, it is easier to
>     use the `core.untrackedCache` configuration variable than using
>     the `--untracked-cache` option to `git update-index` in each
>     repository, especially if you want to do so across all
>     repositories you use, because you can set the configuration
>     variable to `true` (or `false`) in your `$HOME/.gitconfig` just
>     once and have it affect all repositories you touch.
>
> or something, perhaps.

Ok, I will use something like that.

>> +When the `core.untrackedCache` configuration variable is changed, the
>> +untracked cache is added to or removed from the index the next time
>> +"git status" is run; while when `--[no-|force-]untracked-cache` are
>> +used, the untracked cache is immediately added to or removed from the
>> +index.
>
> Is it only "git status", or anything that writes/updates the index
> file?  The above makes it sound as if you are saying "If you change
> the variable, you must run `git status` for the change to take
> effect", and if that is indeed the case, perhaps you should say so
> more explicitly.  My cursory reading of the code suggests that the
> user must run `git status` in a mode that shows untracked files
> (i.e. "git status -uno" does not fix)?

Yeah, that's what the code does.

> I somehow thought, per the previous discussion with Duy, that the
> check and addition of an empty one (if missing in the index and
> configuration is set to `true`) or removal of an existing one (if
> configuration is set to `false`) were to be done when the index is
> read by read_index_from(), though.  If you have to say "After
> setting the configuration, you must run this command", that does not
> sound like a huge improvement over "If you want to enable this
> feature, you must run this command".

The untracked-cache feature, as I tried to explain in an email in the
previous discussion, has an effect only on git status when it has to
show untracked files. Otherwise the untracked-cache is simply not
used. It might be a goal to use it more often, but it's not my patch
series' goal.

So why should we have a check to see if maybe the cache should be
added or removed in all the other cases when the cache is not used?

>> +     switch (untracked_cache) {
>> +     case UC_UNSPECIFIED:
>> +             break;
>> +     case UC_DISABLE:
>> +             if (use_untracked_cache == 1)
>> +                     die("core.untrackedCache is set to true; "
>> +                         "remove or change it, if you really want to "
>> +                         "disable the untracked cache");
>>               remove_untracked_cache();
>>               report(_("Untracked cache disabled"));
>> +             break;
>> +     case UC_TEST:
>> +             setup_work_tree();
>> +             return !test_if_untracked_cache_is_supported();
>> +     case UC_ENABLE:
>> +     case UC_FORCE:
>> +             if (use_untracked_cache == 0)
>> +                     die("core.untrackedCache is set to false; "
>> +                         "remove or change it, if you really want to "
>> +                         "enable the untracked cache");
>> +             add_untracked_cache();
>> +             report(_("Untracked cache enabled for '%s'"), get_git_work_tree());
>> +             break;
>
> I do buy the decision to make these no-op when the configuration
> says `true` or `false`, but I am not sure if these deserve die().
>
> Exiting with 0 (= success) after issuing a warning() might be more
> appropriate.

Scripts users might just not look at the warnings and expect things to
work if the exit code is 0.

> I'd especially anticipate that some newbies will
> complain that they got "fatal:" when they used the "--force-"
> variant, saying "I know what I am doing, that is why I said 'force',
> why does stupid Git refuse?".

I think this should be taken care of in the "--force-" documentation.
Maybe we should say that it is deprecated and make it clear, if we
don't already, that it does nothing more than without "force-".

Re: [PATCH v4 09/10] config: add core.untrackedCache

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> While the above is not wrong per-se, from the point of those who
>> looked for these options (that is, those who wanted to do a one-shot
>> enabling or disabling of the feature, perhaps to try it out to see
>> how well it helps on their system), I think the explanation of the
>> interaction between the option and the config is backwards.  For
>> their purpose, setting it to `true` or `false` will be hinderance,
>> because the options are made no-op by such a setting.  IOW, the
>> advertisement "once you decided that you want to use the feature,
>> configuration is a better place to set it" does not belong here.
>>
>> If I were writing this documentation, I'd probably rephrase the
>> second paragraph like so:
>>
>>         These options take effect only when the
>>         `core.untrackedCache` configuration variable (see
>>         linkgit:git-config[1]) is set to `keep` (or it is left
>>         unset).  When the configuration variable is set to `true`,
>>         the untracked cache feature is always enabled (and when it
>>         is set to `false`, it is always disabled), making these
>>         options no-op.
>>
>> perhaps.
>
> Yeah, but "no-op" is not technically true, as if you just set the
> config variable to true for example and then use "--untracked-cache"
> then the cache will be added immediately.

The "update-index --[no-]untracked-cache" command is made to no
longer follow the user's immediate desire (i.e. enable or disable)
expressed by the invocation of the command.  That is what I meant by
'no-op'.

> ... for example "git update-index --untracked-cache" will die() if
> the config variable is set to false.

As I think it is a BUG to make these die(), it is an irrelevant
objection ;-).

>> I somehow thought, per the previous discussion with Duy, that the
>> check and addition of an empty one (if missing in the index and
>> configuration is set to `true`) or removal of an existing one (if
>> configuration is set to `false`) were to be done when the index is
>> read by read_index_from(), though.  If you have to say "After
>> setting the configuration, you must run this command", that does not
>> sound like a huge improvement over "If you want to enable this
>> feature, you must run this command".
>
> The untracked-cache feature, as I tried to explain in an email in the
> previous discussion, has an effect only on git status when it has to
> show untracked files. Otherwise the untracked-cache is simply not
> used. It might be a goal to use it more often, but it's not my patch
> series' goal.

In the future we may extend the system to allow "git add somedir/"
to take advantage of the untracked cache (i.e. we may already know
what untracked paths there are without letting readdir(3) to scan it
in somedir/), for example.  Is it reasonable to force users to say
"git status", in such a future?  And more importantly, when that
future comes, is it reasonable to force users to re-learn Git to do
something else to do things differently at that point?

Itt sounds like somewhat a short-sighted mindset to design the
system, and I was hoping that by now you would have become better
than that.

The real question is what are the problems in implementing this in
the way Duy suggested in the previous discussion.  The answer may
fall into somewhere between "that approach does not work in such and
such cases, so this is the best I could come up with" and "I know
that approach is far superiour, but I have invested too much in this
inferiour approach and refuse to rework it further to make it better."

I unfortunately am not getting the sense that I know where your
answer would fall into that spectrum from your response.

>> Exiting with 0 (= success) after issuing a warning() might be more
>> appropriate.
>
> Scripts users might just not look at the warnings and expect things to
> work if the exit code is 0.

That is exactly why I said it is inappropriate to error out.

Using of not using untracked-cache is (supposed to be) purely
performance and not correctness thing, and I do not think the users
and the scripts do not deserve to see a failure from "update-index
--untracked-cache" only because there is a stray core.untrackedCache
set to 'false' somewhere.

Re: [PATCH v4 09/10] config: add core.untrackedCache

[Christian Couder]

On Thu, Dec 31, 2015 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>> While the above is not wrong per-se, from the point of those who
>>> looked for these options (that is, those who wanted to do a one-shot
>>> enabling or disabling of the feature, perhaps to try it out to see
>>> how well it helps on their system), I think the explanation of the
>>> interaction between the option and the config is backwards.  For
>>> their purpose, setting it to `true` or `false` will be hinderance,
>>> because the options are made no-op by such a setting.  IOW, the
>>> advertisement "once you decided that you want to use the feature,
>>> configuration is a better place to set it" does not belong here.
>>>
>>> If I were writing this documentation, I'd probably rephrase the
>>> second paragraph like so:
>>>
>>>         These options take effect only when the
>>>         `core.untrackedCache` configuration variable (see
>>>         linkgit:git-config[1]) is set to `keep` (or it is left
>>>         unset).  When the configuration variable is set to `true`,
>>>         the untracked cache feature is always enabled (and when it
>>>         is set to `false`, it is always disabled), making these
>>>         options no-op.
>>>
>>> perhaps.
>>
>> Yeah, but "no-op" is not technically true, as if you just set the
>> config variable to true for example and then use "--untracked-cache"
>> then the cache will be added immediately.
>
> The "update-index --[no-]untracked-cache" command is made to no
> longer follow the user's immediate desire (i.e. enable or disable)
> expressed by the invocation of the command.  That is what I meant by
> 'no-op'.
>
>> ... for example "git update-index --untracked-cache" will die() if
>> the config variable is set to false.
>
> As I think it is a BUG to make these die(), it is an irrelevant
> objection ;-).

Ok I think I will just use what you suggest except the "no-op" part:

      These options take effect only when the
      `core.untrackedCache` configuration variable (see
      linkgit:git-config[1]) is set to `keep` (or it is left
      unset).  When the configuration variable is set to `true`,
      the untracked cache feature is always enabled (and when it
      is set to `false`, it is always disabled).


>>> I somehow thought, per the previous discussion with Duy, that the
>>> check and addition of an empty one (if missing in the index and
>>> configuration is set to `true`) or removal of an existing one (if
>>> configuration is set to `false`) were to be done when the index is
>>> read by read_index_from(), though.  If you have to say "After
>>> setting the configuration, you must run this command", that does not
>>> sound like a huge improvement over "If you want to enable this
>>> feature, you must run this command".
>>
>> The untracked-cache feature, as I tried to explain in an email in the
>> previous discussion, has an effect only on git status when it has to
>> show untracked files. Otherwise the untracked-cache is simply not
>> used. It might be a goal to use it more often, but it's not my patch
>> series' goal.
>
> In the future we may extend the system to allow "git add somedir/"
> to take advantage of the untracked cache (i.e. we may already know
> what untracked paths there are without letting readdir(3) to scan it
> in somedir/), for example.  Is it reasonable to force users to say
> "git status", in such a future?

If a new feature takes advantage of the untracked cache, it is
reasonable that this feature enables or disables it if should be
enabled or disabled. Maybe we can just say in the doc something like:

    When the `core.untrackedCache` configuration variable is changed, the
    untracked cache is added to or removed from the index the next time
    a command that can use the untracked cache is run; while when
    `--[no-|force-]untracked-cache` are used, the untracked cache is
    immediately added to or removed from the index. Currently only
    "git status" can use the untracked cache.

> And more importantly, when that
> future comes, is it reasonable to force users to re-learn Git to do
> something else to do things differently at that point?

I don't think people will have to relearn anything or do things
differently especially if things are explained like above.

If they want to use the untracked cache by setting a config option,
and then want to enable it right away, they can still use git status
for that, so things will continue to work.

What scenario do you have in mind where people would have to do things
differently?

> Itt sounds like somewhat a short-sighted mindset to design the
> system, and I was hoping that by now you would have become better
> than that.
>
> The real question is what are the problems in implementing this in
> the way Duy suggested in the previous discussion.  The answer may
> fall into somewhere between "that approach does not work in such and
> such cases, so this is the best I could come up with" and "I know
> that approach is far superiour, but I have invested too much in this
> inferiour approach and refuse to rework it further to make it better."

My question is why should I invest time thinking about and testing
another approach when the current approach seems simpler, less bug
prone, faster and without any downside?

> I unfortunately am not getting the sense that I know where your
> answer would fall into that spectrum from your response.
>
>>> Exiting with 0 (= success) after issuing a warning() might be more
>>> appropriate.
>>
>> Scripts users might just not look at the warnings and expect things to
>> work if the exit code is 0.
>
> That is exactly why I said it is inappropriate to error out.

Suppose that we do as you suggest. This means that when "git
update-index --untracked-cache" is called while the config variable is
false, we issue a warning, add the UC to the index and exit 0.

So a legacy script calling "git update-index --untracked-cache" and
getting a 0 exit code from it will assume that every time "git status"
is called the UC will be used because it has been enabled in the
index. But that would be wrong if the config variable is false,
because in this case when git status is called, it will remove the UC.

> Using of not using untracked-cache is (supposed to be) purely
> performance and not correctness thing, and I do not think the users
> and the scripts do not deserve to see a failure from "update-index
> --untracked-cache" only because there is a stray core.untrackedCache
> set to 'false' somewhere.

This "stray core.untrackedCache" could not have been put there by
users of previous git versions because it has no meaning before this
patch series. So I don't understand why you call it "a stray
core.untrackedCache".

It is no more "stray" to me than the call to "update-index --untracked-cache".

If it has been put in /etc/git.config by an admin and if the user
thinks he knows better, the user can still change the config locally
to override the system one.

Re: [PATCH v4 09/10] config: add core.untrackedCache

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> What scenario do you have in mind where people would have to do things
> differently?

They eventually will see a system in which that they do not have do
anything after flipping the configuration, yet will still see stale
"you must run 'git status'" on their websearches and SO questions,
which would be a cost for them to remember that they no longer have
to do the extra 'git status'.

>> Itt sounds like somewhat a short-sighted mindset to design the
>> system, and I was hoping that by now you would have become better
>> than that.
>>
>> The real question is what are the problems in implementing this in
>> the way Duy suggested in the previous discussion.  The answer may
>> fall into somewhere between "that approach does not work in such and
>> such cases, so this is the best I could come up with" and "I know
>> that approach is far superiour, but I have invested too much in this
>> inferiour approach and refuse to rework it further to make it better."
>
> My question is why should I invest time thinking about and testing
> another approach when the current approach seems simpler, less bug
> prone, faster and without any downside?

As to the downside, I think Duy's "at the time we read the index" is
the least problematic route from the maintainability point of view.
What are you doing to help people who make changes in the future to
the system to make "git add $dir" or "git clean $dir" aware of the
untracked cache not to forget doing the "oh, the config says we want
to add the untracked cache if missing, so we do it here" in their
new codepath?  Whey you say "This is only about status", you are
essentially saying "It's outside the scope of my job, I was hired to
improve the usability of 'git status' with untracked cache and I do
not care about the longer term overall health of the system".

Now, you said something about "simpler", "less bug prone" and
"faster" (I doubt you can make such statements that involve
comparison without investing time thinking about other approaches,
though, but that is a separate topic).  That would mean that you see
"complexity", "error prone-ness", and "slowness" in the way Duy
suggested---that is exactly the question I asked you in the message
you are responding to.  What are the problems in implementing this
in the way Duy suggested?  What kind of "complexity" do you see?
Which part is more "error prone"?  Why does it have to be "slower"?

>> Using of not using untracked-cache is (supposed to be) purely
>> performance and not correctness thing, and I do not think the users
>> and the scripts do not deserve to see a failure from "update-index
>> --untracked-cache" only because there is a stray core.untrackedCache
>> set to 'false' somewhere.
>
> This "stray core.untrackedCache" could not have been put there by
> users of previous git versions because it has no meaning before this
> patch series. So I don't understand why you call it "a stray
> core.untrackedCache".
>
> It is no more "stray" to me than the call to "update-index --untracked-cache".
>
> If it has been put in /etc/git.config by an admin and if the user
> thinks he knows better, the user can still change the config locally
> to override the system one.

You are assuming that everybody constantly looks at /etc/git.config
to make sure evil admins won't do things that affect their
repositories and use of Git in potentially negative way.  I doubt
anybody does.

By the way, I understand that this "stray one affects without user
being aware of it" is the primary the reason why Ævar wants this
'configuration automatically adds untracked cache even to the
existing index' feature---everybody will get it without even be
aware of the change their admins make.  Which may be a good thing
for those who use the configuration variable.

Re: [PATCH v4 09/10] config: add core.untrackedCache

[Christian Couder]

On Mon, Jan 4, 2016 at 7:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>> The real question is what are the problems in implementing this in
>>> the way Duy suggested in the previous discussion.  The answer may
>>> fall into somewhere between "that approach does not work in such and
>>> such cases, so this is the best I could come up with" and "I know
>>> that approach is far superiour, but I have invested too much in this
>>> inferiour approach and refuse to rework it further to make it better."
>>
>> My question is why should I invest time thinking about and testing
>> another approach when the current approach seems simpler, less bug
>> prone, faster and without any downside?

I just tried Duy's approach by moving into read_index_from() the code
I had put in wt_status_collect_untracked() and it doesn't work because
"git status" calls read_index_from() before calling
git_default_core_config().

The interesting parts of the backtraces for both calls are the following:

#0  read_index_from (istate=0x8660a0 <the_index>, path=0x868b90
".git/index") at read-cache.c:1626
#1  0x0000000000530ba3 in read_index (istate=0x8660a0 <the_index>) at
read-cache.c:1404
#2  0x00000000005743e1 in gitmodules_config () at submodule.c:188
#3  0x0000000000431ed0 in status_init_config (s=0x83aa20 <s>,
fn=0x434edd <git_status_config>) at builtin/commit.c:186

#0  git_default_core_config (var=0x86ac00 "core.untrackedcache",
value=0x86ac60 "true") at config.c:695
#1  0x00000000004be761 in git_default_config (var=0x86ac00
"core.untrackedcache", value=0x86ac60 "true", dummy=0x0) at
config.c:1016
#2  0x00000000004cfeba in git_diff_basic_config (var=0x86ac00
"core.untrackedcache", value=0x86ac60 "true", cb=0x0) at diff.c:277
#3  0x00000000004cfc9b in git_diff_ui_config (var=0x86ac00
"core.untrackedcache", value=0x86ac60 "true", cb=0x0) at diff.c:230
#4  0x000000000043524a in git_status_config (k=0x86ac00
"core.untrackedcache", v=0x86ac60 "true", cb=0x83aa20 <s>) at
builtin/commit.c:1313
#5  0x00000000004bf183 in configset_iter (cs=0x8495e0
<the_config_set>, fn=0x434edd <git_status_config>, data=0x83aa20 <s>)
at config.c:1305
#6  0x00000000004bf206 in git_config (fn=0x434edd <git_status_config>,
data=0x83aa20 <s>) at config.c:1317
#7  0x0000000000431ee3 in status_init_config (s=0x83aa20 <s>,
fn=0x434edd <git_status_config>) at builtin/commit.c:187

You can see that in status_init_config(), gitmodules_config() is
called just before git_config().
And unfortunately gitmodules_config() indirectly calls read_index_from().

So indeed it seems very bug prone to me to rely on read_index_from()
being called before git_default_core_config().

Re: [PATCH v4 09/10] config: add core.untrackedCache

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> I just tried Duy's approach by moving into read_index_from() the code
> I had put in wt_status_collect_untracked() and it doesn't work because
> "git status" calls read_index_from() before calling
> git_default_core_config().

I didn't expect to (and didn't want to) see the configuration be
dead by git_default_core_config() in the first place, though.
I instead was expecting that it would be done through the caching
layer, e.g. by calling git_config_get_maybe_bool() interface when
read_index_from() wants to see if it needs to muck with the existing
(or missing) extension.