threads / patch / 41196

v5, 11 partsUntracked cache improvements

Subject: [PATCH v5 00/11] Untracked cache improvements

## tl;dr

20 messages between Jan 15, 2016 and Jan 20, 2016. Diffs are folded; open one to read it.

replies: 19people: 3as markdown or json

Christian Couder· Jan 15, 2016, 06:59 UTC · lore

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

This v5 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 is `keep` in which case the untracked cache is neither removed nor added, and used if it is there.

Patch 1/11 is a small bugfix that has not changed.

Patch 2/11 to 4/11 add some small features that are missing. There are no changes compared to v4.

Patchs 5/11 to 7/11 are some refactoring to prepare for the following patchs. The interface of the new functions has changed as they are now passed a "struct index_state *istate" parameter on which they will operate instead of modifying "the_index".

Patch 8/11 deals with the "ident" field in "struct untracked_cache". Following Torsten and Junio's suggestions, the differences compared to v4 are the following:

	- we store only the kernel "sysname" and not the full kernel version,
	- we match the full "ident" string instead of just the beginning of the string,
	- a warning has been improved.

Patch 9/11 adds core.untrackedCache. Following Junio's suggestions, it has been changed compared to v4 in the following ways:

  - we don't die() and we exit 0, if "git update-index
  --[no-|force-]untracked-cache" is used in a way that goes against
  the value of core.untrackedCache,
  - we add or remove the untracked cache in read_index_from() instead
    of wt_status_collect_untracked().

Patch 10/11 is a new hack that is needed to have test-dump-untracked-cache work like it used to, now that we add or remove the untracked cache in read_index_from(). Because test-dump-untracked-cache indirectly calls read_index_from(), we have to prevent it from addind or removing the untracked cache.

Patch 11/11, which contains tests, has been changed to reflect changes in 9/11.

So the changes compared to v4 are mostly small updates, and patchs 8/11, 9/11 and 10/11.

The patch series is also available there:
https://github.com/chriscool/git/tree/uc-notifs63
Thanks to the reviewers and helpers.
Christian Couder (11):
  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
  test-dump-untracked-cache: don't modify the untracked cache
  t7063: add tests for core.untrackedCache
 Documentation/config.txt               |  9 ++++
 Documentation/git-update-index.txt     | 67 +++++++++++++++++++++----
 builtin/update-index.c                 | 62 ++++++++++++++---------
 cache.h                                |  3 ++
 config.c                               | 23 +++++++++
 contrib/completion/git-completion.bash |  1 +
 dir.c                                  | 62 ++++++++++++++++++-----
 dir.h                                  |  3 +-
 environment.c                          |  6 +++
 read-cache.c                           | 14 ++++++
 t/t7063-status-untracked-cache.sh      | 89 +++++++++++++++++++++++++++++++---
 test-dump-untracked-cache.c            |  2 +
 12 files changed, 287 insertions(+), 54 deletions(-)
-- 
2.7.0.36.g20612a7
Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 01/11] dir: free untracked cache when removing it

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to builtin/update-index.c +1 −0
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.36.g20612a7
Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 02/11] update-index: use enum for untracked cache options

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Duy Nguyen <pclouds@gmail.com>
---
 builtin/update-index.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
Show changes to builtin/update-index.c +13 −5
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.36.g20612a7
Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 03/11] update-index: add --test-untracked-cache

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>
Helped-by: David Turner <dturner@twopensource.com>
---
 Documentation/git-update-index.txt | 12 +++++++++++-
 builtin/update-index.c             |  5 +++++
 2 files changed, 16 insertions(+), 1 deletion(-)
Show changes to 2 files +16 −1

Documentation/git-update-index.txt, builtin/update-index.c

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.36.g20612a7
Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 04/11] update-index: add untracked cache notifications

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>
Helped-by: Duy Nguyen <pclouds@gmail.com>
---
 builtin/update-index.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
Show changes to builtin/update-index.c +9 −5
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.36.g20612a7
Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 05/11] update-index: move 'uc' var declaration

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
Show changes to builtin/update-index.c +1 −3
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.36.g20612a7
Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 06/11] dir: add {new,add}_untracked_cache()

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>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/update-index.c | 11 +----------
 dir.c                  | 18 ++++++++++++++++++
 dir.h                  |  1 +
 3 files changed, 20 insertions(+), 10 deletions(-)
Show changes to 3 files +20 −10

builtin/update-index.c, dir.c, dir.h

diff --git a/builtin/update-index.c b/builtin/update-index.c
index fe7aaa3..5f8630c 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(&the_index);
 		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..31eae37 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(struct index_state *istate)
+{
+	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;
+	istate->untracked = uc;
+}
+
+void add_untracked_cache(struct index_state *istate)
+{
+	if (!istate->untracked) {
+		new_untracked_cache(istate);
+	add_untracked_ident(istate->untracked);
+	istate->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..cfd3636 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(struct index_state *istate);
 #endif
-- 
2.7.0.36.g20612a7
Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 07/11] dir: add remove_untracked_cache()

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(-)
Show changes to 3 files +11 −5

builtin/update-index.c, dir.c, dir.h

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5f8630c..d90154c 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(&the_index);
 		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(&the_index);
 		report(_("Untracked cache disabled"));
 	}
 
diff --git a/dir.c b/dir.c
index 31eae37..0d069c9 100644
--- a/dir.c
+++ b/dir.c
@@ -1956,6 +1956,15 @@ void add_untracked_cache(struct index_state *istate)
 	istate->cache_changed |= UNTRACKED_CHANGED;
 }
 
+void remove_untracked_cache(struct index_state *istate)
+{
+	if (istate->untracked) {
+		free_untracked_cache(istate->untracked);
+		istate->untracked = NULL;
+		istate->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 cfd3636..a3dacdb 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(struct index_state *istate);
+void remove_untracked_cache(struct index_state *istate);
 #endif
-- 
2.7.0.36.g20612a7
Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 08/11] dir: simplify untracked cache "ident" field

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 and kernel name 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.

Note that if an untracked cache has been created by a previous Git version, then the kernel version is stored in the ident field. As we now compare with just the kernel name the comparison will fail and the untracked cache will be disabled until it's recreated.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Torsten Bögershausen <tboegi@web.de>
---
 dir.c | 39 ++++++++++++++++++++++++---------------
 dir.h |  1 -
 2 files changed, 24 insertions(+), 16 deletions(-)
Show changes to 2 files +24 −16

dir.c, dir.h

diff --git a/dir.c b/dir.c
index 0d069c9..42d3b6b 100644
--- a/dir.c
+++ b/dir.c
@@ -1913,28 +1913,31 @@ static const char *get_ident_string(void)
 		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);
+	strbuf_addf(&sb, "Location %s, system %s", get_git_work_tree(),
+		    uts.sysname);
 	return sb.buf;
 }
 
 static int ident_in_untracked(const struct untracked_cache *uc)
 {
-	const char *end = uc->ident.buf + uc->ident.len;
-	const char *p   = uc->ident.buf;
+	/*
+	 * Previous git versions may have saved many NUL separated
+	 * strings in the "ident" field, but it is insane to manage
+	 * many locations, so just take care of the first one.
+	 */
 
-	for (p = uc->ident.buf; p < end; p += strlen(p) + 1)
-		if (!strcmp(p, get_ident_string()))
-			return 1;
-	return 0;
+	return !strcmp(uc->ident.buf, get_ident_string());
 }
 
-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 +1948,21 @@ static void new_untracked_cache(struct index_state *istate)
 	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);
 	istate->untracked = uc;
+	istate->cache_changed |= UNTRACKED_CHANGED;
 }
 
 void add_untracked_cache(struct index_state *istate)
 {
 	if (!istate->untracked) {
 		new_untracked_cache(istate);
-	add_untracked_ident(istate->untracked);
-	istate->cache_changed |= UNTRACKED_CHANGED;
+	} else {
+		if (!ident_in_untracked(istate->untracked)) {
+			free_untracked_cache(istate->untracked);
+			new_untracked_cache(istate);
+		}
+	}
 }
 
 void remove_untracked_cache(struct index_state *istate)
@@ -2022,7 +2031,7 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
 		return NULL;
 
 	if (!ident_in_untracked(dir->untracked)) {
-		warning(_("Untracked cache is disabled on this system."));
+		warning(_("Untracked cache is disabled on this system or location."));
 		return NULL;
 	}
 
diff --git a/dir.h b/dir.h
index a3dacdb..cd46f30 100644
--- a/dir.h
+++ b/dir.h
@@ -307,7 +307,6 @@ void untracked_cache_add_to_index(struct index_state *, const char *);
 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(struct index_state *istate);
 void remove_untracked_cache(struct index_state *istate);
 #endif
-- 
2.7.0.36.g20612a7
Torsten Bögershausen· Jan 16, 2016, 14:55 UTC · re: Christian Couder · lore

Re: [PATCH v5 08/11] dir: simplify untracked cache "ident" field

On 2016-01-15 07.59, Christian Couder wrote:
Show 23 quoted lines
> 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 and kernel name 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.
>
> Note that if an untracked cache has been created by a
> previous Git version, then the kernel version is stored in
> the ident field. As we now compare with just the kernel
> name the comparison will fail and the untracked cache will
> be disabled until it's recreated.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> Helped-by: Torsten Bögershausen <tboegi@web.de>
This could be my version of a commit message:

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.

Drop the major and minor version from uname stored in ident.
It can be dangerous if GIT_WORK_TREE is used with different values.

So let's simplify the handling and store just store one line, the uname of the system 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.

The untracked cache can only be used by one type of OS, exporting a git repo to different clients via a network to e.g. Linux and Windows means that only one can use the untracked cache.

Note that if an untracked cache has been created by a previous Git version, then the kernel version is stored in the ident field. As we now compare with just the kernel name the comparison will fail and the untracked cache will be disabled until it's recreated.

Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 09/11] config: add core.untrackedCache

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, we might want the untracked-cache to be automatically remove from the index.

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 read_index_from() is called, 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 shows a warning if it goes against the value of core.untrackedCache, as the untracked cache will go back to its previous state the next time read_index_from() is called.

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>
Helped-by: Duy Nguyen <pclouds@gmail.com>
Helped-by: Torsten Bögershausen <tboegi@web.de>
---
 Documentation/config.txt               |  9 +++++
 Documentation/git-update-index.txt     | 67 ++++++++++++++++++++++++++--------
 builtin/update-index.c                 | 35 ++++++++++++------
 cache.h                                |  1 +
 config.c                               | 20 ++++++++++
 contrib/completion/git-completion.bash |  1 +
 read-cache.c                           | 14 +++++++
 t/t7063-status-untracked-cache.sh      |  4 +-
 8 files changed, 122 insertions(+), 29 deletions(-)
Show changes to 8 files +122 −29

Documentation/config.txt, Documentation/git-update-index.txt, builtin/update-index.c, cache.h, config.c, contrib/completion/git-completion.bash, read-cache.c, t/t7063-status-untracked-cache.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index beb18da..485aa5e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -308,6 +308,15 @@ core.trustctime::
 	crawlers and some backup systems).
 	See linkgit:git-update-index[1]. True by default.
 
+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.
+
 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..5853dcd 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -174,27 +174,30 @@ 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 feature. Please use
+	`--test-untracked-cache` before enabling it.
++
+These options take effect whatever the value of the `core.untrackedCache`
+configuration variable (see linkgit:git-config[1]). But a warning is
+emitted when the change goes against the configured value, as the
+configured value will take effect next time the index is read and this
+will remove the intended effect of the option.
 
 --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 +388,37 @@ 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.
+
+If you want to enable (or disable) this feature, it is easier to use
+the `core.untrackedCache` configuration variable (see
+linkgit:git-config[1]) 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.
+
+When the `core.untrackedCache` configuration variable is changed, the
+untracked cache is added to or removed from the index the next time a
+command reads the index; while when `--[no-|force-]untracked-cache`
+are used, the untracked cache is immediately added to or removed from
+the index.
 
 Configuration
 -------------
@@ -410,6 +444,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 d90154c..7a55334 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(&the_index);
-		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 (git_config_get_untracked_cache() == 1)
+			warning("core.untrackedCache is set to true; "
+				"remove or change it, if you really want to "
+				"disable the untracked cache");
 		remove_untracked_cache(&the_index);
 		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 (git_config_get_untracked_cache() == 0)
+			warning("core.untrackedCache is set to false; "
+				"remove or change it, if you really want to "
+				"enable the untracked cache");
+		add_untracked_cache(&the_index);
+		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..59a15fd 100644
--- a/cache.h
+++ b/cache.h
@@ -1603,6 +1603,7 @@ extern int git_config_get_bool(const char *key, int *dest);
 extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);
 extern int git_config_get_maybe_bool(const char *key, int *dest);
 extern int git_config_get_pathname(const char *key, const char **dest);
+extern int git_config_get_untracked_cache(void);
 
 struct key_value_info {
 	const char *filename;
diff --git a/config.c b/config.c
index 86a5eb2..647a15e 100644
--- a/config.c
+++ b/config.c
@@ -1594,6 +1594,26 @@ int git_config_get_pathname(const char *key, const char **dest)
 	return ret;
 }
 
+int git_config_get_untracked_cache(void)
+{
+	int val = -1;
+	const char *v;
+
+	if (!git_config_get_maybe_bool("core.untrackedcache", &val))
+		return val;
+
+	if (!git_config_get_value("core.untrackedcache", &v)) {
+		if (!strcasecmp(v, "keep"))
+			return -1;
+
+		error("unknown core.untrackedCache value '%s'; "
+		      "using 'keep' default value", v);
+		return -1;
+	}
+
+	return -1; /* default value */
+}
+
 NORETURN
 void git_die_config_linenr(const char *key, const char *filename, int linenr)
 {
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ab4da7f..de4b5ed 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2056,6 +2056,7 @@ _git_config ()
 		core.sparseCheckout
 		core.symlinks
 		core.trustctime
+		core.untrackedCache
 		core.warnAmbiguousRefs
 		core.whitespace
 		core.worktree
diff --git a/read-cache.c b/read-cache.c
index 84616c8..1d5696c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1622,6 +1622,20 @@ int read_index_from(struct index_state *istate, const char *path)
 		return istate->cache_nr;
 
 	ret = do_read_index(istate, path, 0);
+
+	switch (git_config_get_untracked_cache()) {
+	case -1: /* keep: do nothing */
+		break;
+	case 0: /* false */
+		remove_untracked_cache(istate);
+		break;
+	case 1: /* true */
+		add_untracked_cache(istate);
+		break;
+	default: /* unknown value: do nothing */
+		break;
+	}
+
 	split_index = istate->split_index;
 	if (!split_index || is_null_sha1(split_index->base_sha1)) {
 		check_ce_order(istate);
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
 '
 
-- 
2.7.0.36.g20612a7
Junio C Hamano· Jan 15, 2016, 20:15 UTC · re: Christian Couder · lore

Re: [PATCH v5 09/11] config: add core.untrackedCache

Christian Couder <christian.couder@gmail.com> writes:
Show 8 quoted lines
> 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,
> we might want the untracked-cache to be automatically remove from
> the index.

I'd say "fully supported" and "not supported" are bad phrasing. Network file system may give you mtime, but for the purpose of uc, it may not be usable, and that is when you do not want to rely on it. So perhaps "When we know that mtime on directory the environment gives us is usable for the purpose of untracked cache", and "Also when we know that mtime is not usable" or something like that?

I think "we might want" can become a stronger "we may want" in both paragraphs. The second paragraph needs s/remove/removed/.

Show 5 quoted lines
> 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`.
The above makes a reader wonder what are abnormal ways ;-).
    Allow the user to express such preference by setting the
    'core.untrackedCache' configuration variable, which can take
    'keep', 'false', or 'true'.

would be clearer without being muddied by an unnecessary value judgement.

Show 5 quoted lines
> When read_index_from() is called, 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`.
OK.
> `git update-index --[no-|force-]untracked-cache` still adds or
> removes the untracked cache from the index,

The above is meant to be a contraction of "... still adds UC to the index or removes UC from the index", but reads as if the original were "... still adds UC from the index or removes UC from the index".

"... still adds UC to or removes from the index" perhaps?
> but this shows a
> warning if it goes against the value of core.untrackedCache,
> as the untracked cache will go back to its previous state the
> next time read_index_from() is called.
Not incorrect per-se, but
    ... shows a warning ... because the next time the index is read
    UC will be added or removed if the configuration is set to do so.

may make it more clear that `keep` in core.untrackedCache does not have to issue a warning (I saw your code correctly handles the warning in this patch).

Show 9 quoted lines
> 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`.
    But because those tests take a long time, `--untracked-cache` no
    longer performs them.  Instead, there is now `--test-untracked-cache`
    to perform the tests.  This change makes `--untracked-cache` the
    same as `--force-untracked-cache`.
Show 5 quoted lines
> +Untracked cache
> +---------------
> +
> +This cache could speed up commands that involve determining untracked
> +files such as `git status`.

You are being a bit too modest by saying "could" here, I think. If you do not want to be overly assertive, perhaps say "is meant to"?

Show 6 quoted lines
> +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.

A reader who read only this without reading the code would wonder if the code works correctly after a untracked path is added to a diretory A/B (the timestamp of B would be updated, while A would stay the same) and we ask what are the untracked things inside the whole tree. Of course we shouldn't "omit" scanning A down to B, but it is a bit unclear that we didn't have such a design bug from this description.

Unfortunately I don't have a good rephrasing suggestion.
Thanks.
Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 10/11] test-dump-untracked-cache: don't modify the untracked cache

To correctly perform its testing function, test-dump-untracked-cache should not change the state of the untracked cache in the index.

As a previous patch makes read_index_from() change the untracked cache status and as test-dump-untracked-cache indirectly calls this function, we need a mechanism to prevent read_index_from() to change the untracked cache state when it's called from test-dump-untracked-cache.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 cache.h                     | 2 ++
 config.c                    | 3 +++
 environment.c               | 6 ++++++
 test-dump-untracked-cache.c | 2 ++
 4 files changed, 13 insertions(+)
Show changes to 4 files +13 −0

cache.h, config.c, environment.c, test-dump-untracked-cache.c

diff --git a/cache.h b/cache.h
index 59a15fd..89c7e10 100644
--- a/cache.h
+++ b/cache.h
@@ -1605,6 +1605,8 @@ extern int git_config_get_maybe_bool(const char *key, int *dest);
 extern int git_config_get_pathname(const char *key, const char **dest);
 extern int git_config_get_untracked_cache(void);
 
+extern int ignore_untracked_cache_config;
+
 struct key_value_info {
 	const char *filename;
 	int linenr;
diff --git a/config.c b/config.c
index 647a15e..a4f70f7 100644
--- a/config.c
+++ b/config.c
@@ -1599,6 +1599,9 @@ int git_config_get_untracked_cache(void)
 	int val = -1;
 	const char *v;
 
+	if (ignore_untracked_cache_config)
+		return -1;
+
 	if (!git_config_get_maybe_bool("core.untrackedcache", &val))
 		return val;
 
diff --git a/environment.c b/environment.c
index 1cc4aab..74294ee 100644
--- a/environment.c
+++ b/environment.c
@@ -87,6 +87,12 @@ int auto_comment_line_char;
 /* Parallel index stat data preload? */
 int core_preload_index = 1;
 
+/*
+ * This is to ensure that the untracked cache is not modified, for
+ * example in test programs like test-dump-untracked-cache.
+ */
+int ignore_untracked_cache_config;
+
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
 static char *work_tree;
diff --git a/test-dump-untracked-cache.c b/test-dump-untracked-cache.c
index 25d855d..8d1293c 100644
--- a/test-dump-untracked-cache.c
+++ b/test-dump-untracked-cache.c
@@ -44,6 +44,8 @@ int main(int ac, char **av)
 {
 	struct untracked_cache *uc;
 	struct strbuf base = STRBUF_INIT;
+
+	ignore_untracked_cache_config = 1;
 	setup_git_directory();
 	if (read_cache() < 0)
 		die("unable to read index file");
-- 
2.7.0.36.g20612a7
Junio C Hamano· Jan 15, 2016, 20:16 UTC · re: Christian Couder · lore

Re: [PATCH v5 10/11] test-dump-untracked-cache: don't modify the untracked cache

Christian Couder <christian.couder@gmail.com> writes:
Show 10 quoted lines
> diff --git a/cache.h b/cache.h
> index 59a15fd..89c7e10 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1605,6 +1605,8 @@ extern int git_config_get_maybe_bool(const char *key, int *dest);
>  extern int git_config_get_pathname(const char *key, const char **dest);
>  extern int git_config_get_untracked_cache(void);
>  
> +extern int ignore_untracked_cache_config;
> +

I know you said this is a hack to support the test, but I really do not like to have a test-only global variable exposed to everybody like this, as I do not think "ignore_untracked_cache_config" should be necessary outside the context of testing [*1*].

If the config cache layer that is used by the implementation of git_config_get_untracked_cache() had a way to be told to pretend that the value of a particular configuration variable is a given value, then we could do

	git_config_pretend_value("core.untrackedcache", "keep");
at the beginning of the test program without harming anybody else.

The above is just me "thinking aloud", without assessing if the damage to the entire codebase with that approach to extend the config layer would be larger than the damabe by this patch, and it is certainly not a suggestion to redo this patch along that line. But I am saying it aloud because it may turn out to be a good direction to go in the larger picture once this series is done--it may be a solution that is applicable to a class of similar problems in a more general way.

Inside the scope of this series, can we at least add a comment next to this variable telling everybody to never use it in normal programs, or something?

Thanks.
[Footnote]

*1* Otherwise, we have a larger problem; it would mean that "we trust the configuration variable better than the fact that the user said the feature must (or must not) be used with this index file" is flawed, which would contradict with the whole premise of this series.

Show 44 quoted lines
> diff --git a/config.c b/config.c
> index 647a15e..a4f70f7 100644
> --- a/config.c
> +++ b/config.c
> @@ -1599,6 +1599,9 @@ int git_config_get_untracked_cache(void)
>  	int val = -1;
>  	const char *v;
>  
> +	if (ignore_untracked_cache_config)
> +		return -1;
> +
>  	if (!git_config_get_maybe_bool("core.untrackedcache", &val))
>  		return val;
>  
> diff --git a/environment.c b/environment.c
> index 1cc4aab..74294ee 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -87,6 +87,12 @@ int auto_comment_line_char;
>  /* Parallel index stat data preload? */
>  int core_preload_index = 1;
>  
> +/*
> + * This is to ensure that the untracked cache is not modified, for
> + * example in test programs like test-dump-untracked-cache.
> + */
> +int ignore_untracked_cache_config;
> +
>  /* This is set by setup_git_dir_gently() and/or git_default_config() */
>  char *git_work_tree_cfg;
>  static char *work_tree;
> diff --git a/test-dump-untracked-cache.c b/test-dump-untracked-cache.c
> index 25d855d..8d1293c 100644
> --- a/test-dump-untracked-cache.c
> +++ b/test-dump-untracked-cache.c
> @@ -44,6 +44,8 @@ int main(int ac, char **av)
>  {
>  	struct untracked_cache *uc;
>  	struct strbuf base = STRBUF_INIT;
> +
> +	ignore_untracked_cache_config = 1;
>  	setup_git_directory();
>  	if (read_cache() < 0)
>  		die("unable to read index file");
Christian Couder· Jan 20, 2016, 10:12 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 10/11] test-dump-untracked-cache: don't modify the untracked cache

On Fri, Jan 15, 2016 at 9:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 39 quoted lines
> Christian Couder <christian.couder@gmail.com> writes:
>
>> diff --git a/cache.h b/cache.h
>> index 59a15fd..89c7e10 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1605,6 +1605,8 @@ extern int git_config_get_maybe_bool(const char *key, int *dest);
>>  extern int git_config_get_pathname(const char *key, const char **dest);
>>  extern int git_config_get_untracked_cache(void);
>>
>> +extern int ignore_untracked_cache_config;
>> +
>
> I know you said this is a hack to support the test, but I really do
> not like to have a test-only global variable exposed to everybody
> like this, as I do not think "ignore_untracked_cache_config" should
> be necessary outside the context of testing [*1*].
>
> If the config cache layer that is used by the implementation of
> git_config_get_untracked_cache() had a way to be told to pretend
> that the value of a particular configuration variable is a given
> value, then we could do
>
>         git_config_pretend_value("core.untrackedcache", "keep");
>
> at the beginning of the test program without harming anybody else.
>
> The above is just me "thinking aloud", without assessing if the
> damage to the entire codebase with that approach to extend the
> config layer would be larger than the damabe by this patch, and it
> is certainly not a suggestion to redo this patch along that line.
> But I am saying it aloud because it may turn out to be a good
> direction to go in the larger picture once this series is done--it
> may be a solution that is applicable to a class of similar problems
> in a more general way.
>
> Inside the scope of this series, can we at least add a comment next
> to this variable telling everybody to never use it in normal
> programs, or something?

Ok, the comments have been added or rewritten telling that in the v6 I just sent, thanks.

Christian Couder· Jan 15, 2016, 06:59 UTC · re: Christian Couder · lore

[PATCH v5 11/11] t7063: add tests for core.untrackedCache

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7063-status-untracked-cache.sh | 85 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 81 insertions(+), 4 deletions(-)
Show changes to t/t7063-status-untracked-cache.sh +81 −4
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 253160a..b1dc860 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,77 @@ 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 does not fail when core.untrackedCache is true' '
+	git update-index --no-untracked-cache &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect ../actual &&
+	git update-index --untracked-cache &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect-empty ../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-empty ../actual &&
+	git status &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect ../actual
+'
+
+test_expect_success 'using --untracked-cache does not fail when core.untrackedCache is false' '
+	git update-index --untracked-cache &&
+	test-dump-untracked-cache >../actual &&
+	test_cmp ../expect-empty ../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_expect_success 'test ident field is working' '
+	mkdir ../other_worktree &&
+	cp -R done dthree dtwo four three ../other_worktree &&
+	GIT_WORK_TREE=../other_worktree git status 2>../err &&
+	echo "warning: Untracked cache is disabled on this system or location." >../expect &&
+	test_cmp ../expect ../err
+'
+
 test_done
-- 
2.7.0.36.g20612a7
Junio C Hamano· Jan 15, 2016, 20:16 UTC · re: Christian Couder · lore

Re: [PATCH v5 00/11] Untracked cache improvements

Christian Couder <christian.couder@gmail.com> writes:
> Here is a new version of a patch series to improve the untracked cache
> feature.

Will replace. This mostly looks ready for 'next' modulo minor nits I sent separately.

Thanks.
Torsten Bögershausen· Jan 16, 2016, 14:57 UTC · re: Christian Couder · lore

Re: [PATCH v5 00/11] Untracked cache improvements

General question after testing UC on a slow network share:

time ./git status On branch uc-notifs63 Your branch is up-to-date with 'cc/uc-notifs63'.

It took 6.27 seconds to enumerate untracked files. 'status -uno' may speed it up, but you have to be careful not to forget to add new files yourself (see 'git help status'). nothing to commit, working directory clean

real 0m19.159s user 0m0.085s sys 0m0.293s ---------------------- Does it makes sense to hint the user about the untracked cache ?

Christian Couder· Jan 19, 2016, 14:43 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v5 00/11] Untracked cache improvements

On Sat, Jan 16, 2016 at 3:57 PM, Torsten Bögershausen <tboegi@web.de> wrote:
Show 16 quoted lines
> General question after testing UC on a slow network share:
>
> time ./git status
> On branch uc-notifs63
> Your branch is up-to-date with 'cc/uc-notifs63'.
>
> It took 6.27 seconds to enumerate untracked files. 'status -uno'
> may speed it up, but you have to be careful not to forget to add
> new files yourself (see 'git help status').
> nothing to commit, working directory clean
>
> real    0m19.159s
> user    0m0.085s
> sys     0m0.293s
> ----------------------
> Does it makes sense  to hint the user about the untracked cache ?

I don't think it makes sense to hint about it for now. When more people will have used it for a significant time perhaps.

Junio C Hamano· Jan 19, 2016, 17:47 UTC · re: Christian Couder · lore

Re: [PATCH v5 00/11] Untracked cache improvements

Christian Couder <christian.couder@gmail.com> writes:
Show 20 quoted lines
> On Sat, Jan 16, 2016 at 3:57 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>> General question after testing UC on a slow network share:
>>
>> time ./git status
>> On branch uc-notifs63
>> Your branch is up-to-date with 'cc/uc-notifs63'.
>>
>> It took 6.27 seconds to enumerate untracked files. 'status -uno'
>> may speed it up, but you have to be careful not to forget to add
>> new files yourself (see 'git help status').
>> nothing to commit, working directory clean
>>
>> real    0m19.159s
>> user    0m0.085s
>> sys     0m0.293s
>> ----------------------
>> Does it makes sense  to hint the user about the untracked cache ?
>
> I don't think it makes sense to hint about it for now.
> When more people will have used it for a significant time perhaps.
Chicken or Egg?

I tend to think updating this message is outside of the scope of your series, but you would not get wider adoption (hence more guinea pigs that 'have used it for a significant time') unless you advertise it, and I agree with Torsten that this is the message that matters most--it is shown when we know we spent a lot of time on what UC is meant to speed up and no other time.

← back to recent threads