[RFC/PATCH] config: add core.trustmtime

When we know that mtime is fully supported by the environment, we
don't want any slow down because we used --untracked-cache rather
than --force-untracked-cache, and we don't want untracked cache to
stop working because we updated a kernel.

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 it testing if it works (because it might
work on some systems using the repo over the network file system
but not others).

Signed-off-by: Christian Couder <redacted>
---
At Booking.com we know that mtime works everywhere and we don't
want the untracked cache to stop working when a kernel is upgraded
or when the repo is copied to a machine with a different kernel.
I will add tests later if people are ok with this.

 Documentation/config.txt               | 8 ++++++++
 Documentation/git-update-index.txt     | 6 +++++-
 builtin/update-index.c                 | 6 +++++-
 cache.h                                | 1 +
 config.c                               | 7 +++++++
 contrib/completion/git-completion.bash | 1 +
 dir.c                                  | 2 +-
 environment.c                          | 1 +
 8 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b4b0194..186ad17 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -308,6 +308,14 @@ core.trustctime::
 	crawlers and some backup systems).
 	See linkgit:git-update-index[1]. True by default.
 
+core.trustmtime::
+	If unset or set to 'default' or 'check', Git will test if
+	mtime is working properly before using features that need a
+	working mtime. If false, Git will refuse to use such
+	features. If set to true, Git will blindly use such features
+	without testing if they work.
+	See linkgit:git-update-index[1].
+
 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 1a296bc..8b4c5af 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt

@@ -182,7 +182,9 @@ may not support it yet.
 	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.
+	used to skip the tests. If you always want to skip those
+	tests, you can set the `core.trustmtime` configuration
+	variable to 'true', (see linkgit:git-config[1]).
 
 \--::
 	Do not interpret any more arguments as options.

@@ -398,6 +400,8 @@ 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]).
 
+Untracked cache needs a fully working mtime, so it will look at
+`core.trustmtime` configuration variable (see linkgit:git-config[1]).
 
 SEE ALSO
 --------

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7431938..c64439f 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c

@@ -1107,9 +1107,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	if (untracked_cache > 0) {
 		struct untracked_cache *uc;
 
+		if (trust_mtime == 0) {
+			fprintf_ln(stderr,_("core.trustmtime is set to false"));
+			return 1;
+		}
 		if (untracked_cache < 2) {
 			setup_work_tree();
-			if (!test_if_untracked_cache_is_supported())
+			if (trust_mtime != 1 && !test_if_untracked_cache_is_supported())
 				return 1;
 		}
 		if (!the_index.untracked) {

diff --git a/cache.h b/cache.h
index 736abc0..69a6197 100644
--- a/cache.h
+++ b/cache.h

@@ -601,6 +601,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 trust_mtime;
 extern int check_stat;
 extern int quote_path_fully;
 extern int has_symlinks;

diff --git a/config.c b/config.c
index 248a21a..d720b1f 100644
--- a/config.c
+++ b/config.c

@@ -691,6 +691,13 @@ 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.trustmtime")) {
+		if (!strcasecmp(value, "default") || !strcasecmp(value, "check"))
+			trust_mtime = -1;
+		else
+			trust_mtime = git_config_maybe_bool(var, 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 482ca84..39d1c9b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -2038,6 +2038,7 @@ _git_config ()
 		core.sparseCheckout
 		core.symlinks
 		core.trustctime
+		core.trustmtime
 		core.warnAmbiguousRefs
 		core.whitespace
 		core.worktree

diff --git a/dir.c b/dir.c
index d2a8f06..b06df1b 100644
--- a/dir.c
+++ b/dir.c

@@ -1994,7 +1994,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 (trust_mtime != 1 && !ident_in_untracked(dir->untracked)) {
 		warning(_("Untracked cache is disabled on this system."));
 		return NULL;
 	}

diff --git a/environment.c b/environment.c
index 2da7fe2..c899947 100644
--- a/environment.c
+++ b/environment.c

@@ -14,6 +14,7 @@
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
+int trust_mtime = -1;
 int check_stat = 1;
 int has_symlinks = 1;
 int minimum_abbrev = 4, default_abbrev = 7;

-- 
2.6.3.380.g494b52d

On Wed, Nov 25, 2015 at 7:35 AM, Christian Couder
[off-list ref] wrote:

At Booking.com we know that mtime works everywhere and we don't
want the untracked cache to stop working when a kernel is upgraded
or when the repo is copied to a machine with a different kernel.
I will add tests later if people are ok with this.

I bit more info: I rolled Git out internally with this patch:
https://github.com/avar/git/commit/c63f7c12c2664631961add7cf3da901b0b6aa2f2

The --untracked-cache feature hardcodes the equivalent of:

    pwd; uname --kernel-name --kernel-release --kernel-version

Into the index. If any of those change it prints out the "cache is
disabled" warning.

This patch will make it stop being so afraid of itself to the point of
disabling itself on minor kernel upgrades :)

A few other issues with this feature I've noticed:

 * There's no way to just enable it globally via the config. Makes it
a bit of a hassle to use it. I wanted to have a config option to
enable it via the config, how about "index.untracked_cache = true" for
the config variable name?

 * Doing "cd /tmp: git --git-dir=/git/somewhere/else/.git update-index
--untracked-cache" doesn't work how I'd expect. It hardcodes "/tmp" as
the directory that "works" into the index, so if you use the working
tree you'll never use the untracked cache. I spotted this because I
carry out a bunch of git maintenance commands with --git-dir instead
of cd-ing to the relevant directories. This works for most other
things in git, is it a bug that it doesn't work here?

 * If you "ctrl+c" git update-index --untracked-cache at an
inopportune time you'll end up with a mtime-test-XXXXXX directory in
your working tree. Perhaps this tempdir should be created in the .git
directory instead?

 * Maybe we should have a --test-untracked-cache option, so you can
run the tests without enabling it.

Aside from the slight hassle of enabling this and keeping it enabled
this feature is great. It's sped up "git status" across the board by
about 40%. Slightly less than that on faster spinning disks, slightly
more than that on slower ones.

Hi Christian,

On Wed, 25 Nov 2015, Christian Couder wrote:

If `core.trustmtime` is set to `OMG, never!`, `git_config_maybe_bool()`
will set trust_mtime to -1, i.e. the exact same value as if you had set
the variable to `default` or `check`...

Maybe you want to be a bit stricter here and either test the return value
of `git_config_maybe_bool()` for -1 (and warn in that case), or just
delete the `maybe_`?

Ciao,
Dscho

Hi Johannes,

On Wed, Nov 25, 2015 at 11:25 AM, Johannes Schindelin
[off-list ref] wrote:

Hi Christian,

On Wed, 25 Nov 2015, Christian Couder wrote:

quoted

diff --git a/config.c b/config.c
index 248a21a..d720b1f 100644
--- a/config.c
+++ b/config.c

@@ -691,6 +691,13 @@ 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.trustmtime")) {
+             if (!strcasecmp(value, "default") || !strcasecmp(value, "check"))
+                     trust_mtime = -1;
+             else
+                     trust_mtime = git_config_maybe_bool(var, value);

If `core.trustmtime` is set to `OMG, never!`, `git_config_maybe_bool()`
will set trust_mtime to -1, i.e. the exact same value as if you had set
the variable to `default` or `check`...

Yeah, I think returning -1 is the safe thing to do in this case.

Maybe you want to be a bit stricter here and either test the return value
of `git_config_maybe_bool()` for -1 (and warn in that case), or just
delete the `maybe_`?

I thought that if we ever add more options in the future then people
might be annoyed by older git versions warning about the new options.
But I am ok to add a warning.

Thanks,
Christian.

On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Wed, Nov 25, 2015 at 7:35 AM, Christian Couder
[off-list ref] wrote:

quoted

At Booking.com we know that mtime works everywhere and we don't
want the untracked cache to stop working when a kernel is upgraded
or when the repo is copied to a machine with a different kernel.
I will add tests later if people are ok with this.

I bit more info: I rolled Git out internally with this patch:
https://github.com/avar/git/commit/c63f7c12c2664631961add7cf3da901b0b6aa2f2

The --untracked-cache feature hardcodes the equivalent of:

    pwd; uname --kernel-name --kernel-release --kernel-version

Into the index. If any of those change it prints out the "cache is
disabled" warning.

This patch will make it stop being so afraid of itself to the point of
disabling itself on minor kernel upgrades :)

The problem is, there's no way to teach git to know it's a "minor"
upgrade.. but if there is a config key to say "don't be paranoid, I
know what I'm doing", then we can skip that check, or just warn
instead of disabling the cache.

A few other issues with this feature I've noticed:

 * There's no way to just enable it globally via the config. Makes it
a bit of a hassle to use it. I wanted to have a config option to
enable it via the config, how about "index.untracked_cache = true" for
the config variable name?

If you haven't noticed, all these experimental features have no real
UI (update-index is plumbing). I have been waiting for someone like
you to start using it and figure out the best UI (then implement it)
;)

 * Doing "cd /tmp: git --git-dir=/git/somewhere/else/.git update-index
--untracked-cache" doesn't work how I'd expect. It hardcodes "/tmp" as
the directory that "works" into the index, so if you use the working
tree you'll never use the untracked cache. I spotted this because I
carry out a bunch of git maintenance commands with --git-dir instead
of cd-ing to the relevant directories. This works for most other
things in git, is it a bug that it doesn't work here?

It needs the current directory at --untrack-cache time to test if the
directory satisfies the requirements. So either you cd to that
worktree, or you have to specify --worktree as well. Or am I missing
something?

 * If you "ctrl+c" git update-index --untracked-cache at an
inopportune time you'll end up with a mtime-test-XXXXXX directory in
your working tree. Perhaps this tempdir should be created in the .git
directory instead?

No because in theory .git could be on a separate file system with
different semantics. But we should probably clean those files at ^C.

 * Maybe we should have a --test-untracked-cache option, so you can
run the tests without enabling it.

I'd say patches welcome.

Aside from the slight hassle of enabling this and keeping it enabled
this feature is great. It's sped up "git status" across the board by
about 40%. Slightly less than that on faster spinning disks, slightly
more than that on slower ones.

I'm still waiting for the day when watchman support gets merged and
maybe poke Facebook guys to compare performance with Mercurial :)
Well, we are probably still behind Mercurial on that day.

Also, there's still work to be done. Right now it's optimized for
whole-tree "git status", Doing "git status -- abc" will not benefit
from untracked cache, similarly "git add" with pathspec..
-- 
Duy

Hi Duy,

On Wed, Nov 25, 2015 at 8:51 PM, Duy Nguyen [off-list ref] wrote:

On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

On Wed, Nov 25, 2015 at 7:35 AM, Christian Couder
[off-list ref] wrote:

quoted

At Booking.com we know that mtime works everywhere and we don't
want the untracked cache to stop working when a kernel is upgraded
or when the repo is copied to a machine with a different kernel.
I will add tests later if people are ok with this.

I bit more info: I rolled Git out internally with this patch:
https://github.com/avar/git/commit/c63f7c12c2664631961add7cf3da901b0b6aa2f2

The --untracked-cache feature hardcodes the equivalent of:

    pwd; uname --kernel-name --kernel-release --kernel-version

Into the index. If any of those change it prints out the "cache is
disabled" warning.

This patch will make it stop being so afraid of itself to the point of
disabling itself on minor kernel upgrades :)

The problem is, there's no way to teach git to know it's a "minor"
upgrade.. but if there is a config key to say "don't be paranoid, I
know what I'm doing", then we can skip that check, or just warn
instead of disabling the cache.

Yeah, in my patch if core.trustmtime is set to true or false the check
is skipped.

I am wondering why you didn't make it by default run the mtime checks
when a kernel change is detected. Maybe that would be better than
disabling itself.

quoted

A few other issues with this feature I've noticed:

 * There's no way to just enable it globally via the config. Makes it
a bit of a hassle to use it. I wanted to have a config option to
enable it via the config, how about "index.untracked_cache = true" for
the config variable name?

If you haven't noticed, all these experimental features have no real
UI (update-index is plumbing). I have been waiting for someone like
you to start using it and figure out the best UI (then implement it)
;)

Ok, we are happy to do that (including implementing it) :-)

I will take a look at something like index.untracked_cache. It will
probably also be a tristate like this:

- true: always enable it; die if core.trustmtime is false otherwise
warn if it is not true
- default/unset: same as current behavior
- false: die if it is enabled or when trying to enable to it

quoted

 * Doing "cd /tmp: git --git-dir=/git/somewhere/else/.git update-index
--untracked-cache" doesn't work how I'd expect. It hardcodes "/tmp" as
the directory that "works" into the index, so if you use the working
tree you'll never use the untracked cache. I spotted this because I
carry out a bunch of git maintenance commands with --git-dir instead
of cd-ing to the relevant directories. This works for most other
things in git, is it a bug that it doesn't work here?

It needs the current directory at --untrack-cache time to test if the
directory satisfies the requirements. So either you cd to that
worktree, or you have to specify --worktree as well. Or am I missing
something?

Maybe it could print out a message saying "Testing mtime in directory
$(pwd)" and if that works then "Untracked cache is enabled for
$(pwd)". That would make it clear that it will not work in other
directories.

Also maybe the mtime checks could be run when a directory change is detected.

quoted

 * If you "ctrl+c" git update-index --untracked-cache at an
inopportune time you'll end up with a mtime-test-XXXXXX directory in
your working tree. Perhaps this tempdir should be created in the .git
directory instead?

No because in theory .git could be on a separate file system with
different semantics. But we should probably clean those files at ^C.

Ok, I will have a look at cleaning the files at ^C.

quoted

 * Maybe we should have a --test-untracked-cache option, so you can
run the tests without enabling it.

I'd say patches welcome.

Ok, I wll have a look at that too.

quoted

Aside from the slight hassle of enabling this and keeping it enabled
this feature is great. It's sped up "git status" across the board by
about 40%. Slightly less than that on faster spinning disks, slightly
more than that on slower ones.

I'm still waiting for the day when watchman support gets merged and
maybe poke Facebook guys to compare performance with Mercurial :)
Well, we are probably still behind Mercurial on that day.

Yeah, it could be interesting to compare performance with Mercurial as
we move forward :-)

Also, there's still work to be done. Right now it's optimized for
whole-tree "git status", Doing "git status -- abc" will not benefit
from untracked cache, similarly "git add" with pathspec..

Thanks for these details. Yeah, it might be interesting to look at
"git add" too.

Best,
Christian.

On Thu, Nov 26, 2015 at 6:21 AM, Christian Couder
[off-list ref] wrote:

Hi Duy,

On Wed, Nov 25, 2015 at 8:51 PM, Duy Nguyen [off-list ref] wrote:

quoted

On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

On Wed, Nov 25, 2015 at 7:35 AM, Christian Couder
[off-list ref] wrote:

quoted

At Booking.com we know that mtime works everywhere and we don't
want the untracked cache to stop working when a kernel is upgraded
or when the repo is copied to a machine with a different kernel.
I will add tests later if people are ok with this.

I bit more info: I rolled Git out internally with this patch:
https://github.com/avar/git/commit/c63f7c12c2664631961add7cf3da901b0b6aa2f2

The --untracked-cache feature hardcodes the equivalent of:

    pwd; uname --kernel-name --kernel-release --kernel-version

Into the index. If any of those change it prints out the "cache is
disabled" warning.

This patch will make it stop being so afraid of itself to the point of
disabling itself on minor kernel upgrades :)

The problem is, there's no way to teach git to know it's a "minor"
upgrade.. but if there is a config key to say "don't be paranoid, I
know what I'm doing", then we can skip that check, or just warn
instead of disabling the cache.

Yeah, in my patch if core.trustmtime is set to true or false the check
is skipped.

I am wondering why you didn't make it by default run the mtime checks
when a kernel change is detected. Maybe that would be better than
disabling itself.

It takes about 10 seconds to go through the mtime check. Imagine you
have to wait 10s for some random "git status".. Plus I didn't want to
do anything fancy.

quoted

quoted

A few other issues with this feature I've noticed:

 * There's no way to just enable it globally via the config. Makes it
a bit of a hassle to use it. I wanted to have a config option to
enable it via the config, how about "index.untracked_cache = true" for
the config variable name?

If you haven't noticed, all these experimental features have no real
UI (update-index is plumbing). I have been waiting for someone like
you to start using it and figure out the best UI (then implement it)
;)

Ok, we are happy to do that (including implementing it) :-)

I will take a look at something like index.untracked_cache. It will

Nit. untrackedCache (underscores are avoided for config keys) and it's
probably core.untrackedCache instead..

probably also be a tristate like this:

- true: always enable it; die if core.trustmtime is false otherwise
warn if it is not true
- default/unset: same as current behavior
- false: die if it is enabled or when trying to enable to it

There could be a fourth option: let a hook do the checking. If the
hook returns ok, all is good.

quoted

quoted

 * Doing "cd /tmp: git --git-dir=/git/somewhere/else/.git update-index
--untracked-cache" doesn't work how I'd expect. It hardcodes "/tmp" as
the directory that "works" into the index, so if you use the working
tree you'll never use the untracked cache. I spotted this because I
carry out a bunch of git maintenance commands with --git-dir instead
of cd-ing to the relevant directories. This works for most other
things in git, is it a bug that it doesn't work here?

It needs the current directory at --untrack-cache time to test if the
directory satisfies the requirements. So either you cd to that
worktree, or you have to specify --worktree as well. Or am I missing
something?

Maybe it could print out a message saying "Testing mtime in directory
$(pwd)" and if that works then "Untracked cache is enabled for
$(pwd)". That would make it clear that it will not work in other
directories.

Also maybe the mtime checks could be run when a directory change is detected.

Yeah.. and we could also have a hook to do this test your own way too,
if you already know your system supports it, so you wait no time at
all.
-- 
Duy

On Thu, Nov 26, 2015 at 6:53 PM, Duy Nguyen [off-list ref] wrote:

On Thu, Nov 26, 2015 at 6:21 AM, Christian Couder
[off-list ref] wrote:

quoted

I am wondering why you didn't make it by default run the mtime checks
when a kernel change is detected. Maybe that would be better than
disabling itself.

It takes about 10 seconds to go through the mtime check. Imagine you
have to wait 10s for some random "git status".. Plus I didn't want to
do anything fancy.

I browsed through the commits that added the --untracked-cache and
tried to find the original mailing list discussion, but I couldn't
find the reason for why the default interface for enabling it is doing
these exhaustive tests.

Maybe I'm missing some really common breakage with st_mtime on some
system, but having a feature the user explicitly enables turn itself
off and doing FS-testing that takes 10 seconds when it's enabled seems
like the wrong default to me.

We don't do it with core.fileMode, core.ignorecase or core.trustctime
or core.symlinks. Do we really need to be treating this differently?

If that's a "no" then the default interface to this could be much
simpler. Rather than being a change you apply to .git/index (going
away if you nuke it etc.) it could just be a config option like the
rest.

On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

Aside from the slight hassle of enabling this and keeping it enabled
this feature is great. It's sped up "git status" across the board by
about 40%. Slightly less than that on faster spinning disks, slightly
more than that on slower ones.

Before I forget again, you should also enable split-index. That
feature was added because index update time cut so much into the
saving from untracked cache (unless you have small indexes, unlikely).
And untracked cache can update the index often. Then maybe you can
also think about improving the usability for it to ;-)
-- 
Duy

On Wed, Dec 2, 2015 at 8:28 PM, Duy Nguyen [off-list ref] wrote:

On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

Aside from the slight hassle of enabling this and keeping it enabled
this feature is great. It's sped up "git status" across the board by
about 40%. Slightly less than that on faster spinning disks, slightly
more than that on slower ones.

Before I forget again, you should also enable split-index. That
feature was added because index update time cut so much into the
saving from untracked cache (unless you have small indexes, unlikely).
And untracked cache can update the index often. Then maybe you can
also think about improving the usability for it to ;-)

Yeah, we will probably have a look at split-index next.

Thanks for developing it too,
Christian.