[RFC/PATCH] config: add core.trustmtime

[RFC/PATCH] config: add core.trustmtime

[Christian Couder]

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 <chriscool@tuxfamily.org>
---
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

Re: [RFC/PATCH] config: add core.trustmtime

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 25, 2015 at 7:35 AM, Christian Couder
<christian.couder@gmail.com> 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.

Re: [RFC/PATCH] config: add core.trustmtime

[Duy Nguyen]

On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Wed, Nov 25, 2015 at 7:35 AM, Christian Couder
> <christian.couder@gmail.com> 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 :)

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

Re: [RFC/PATCH] config: add core.trustmtime

[Christian Couder]

Hi Duy,

On Wed, Nov 25, 2015 at 8:51 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Wed, Nov 25, 2015 at 7:35 AM, Christian Couder
>> <christian.couder@gmail.com> 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 :)
>
> 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.

>> 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

>>  * 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.

>>  * 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.

>>  * 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.

>> 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.

Re: [RFC/PATCH] config: add core.trustmtime

[Duy Nguyen]

On Thu, Nov 26, 2015 at 6:21 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> Hi Duy,
>
> On Wed, Nov 25, 2015 at 8:51 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>> On Wed, Nov 25, 2015 at 7:35 AM, Christian Couder
>>> <christian.couder@gmail.com> 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 :)
>>
>> 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.

>
>>> 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.

>>>  * 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

Re: [RFC/PATCH] config: add core.trustmtime

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 26, 2015 at 6:53 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Thu, Nov 26, 2015 at 6:21 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> 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.

Re: [RFC/PATCH] config: add core.trustmtime

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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?

I share the exact thought.  I was looking the other way when
untracked-cache was done originally ;-), and I would also want to
know the answers to the above questions.

Thanks.

Re: [RFC/PATCH] config: add core.trustmtime

[Duy Nguyen]

On Mon, Nov 30, 2015 at 8:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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?
>
> I share the exact thought.  I was looking the other way when
> untracked-cache was done originally ;-), and I would also want to
> know the answers to the above questions.

OK I was just paranoid (and having to look at filesystem source code
to determine if it supported this didn't help either). So I guess that
means we can make the test a separate option, only invoked by the
user, then.
-- 
Duy

Re: [RFC/PATCH] config: add core.trustmtime

[Torsten Bögershausen]

On 11/30/2015 08:05 PM, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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?
> I share the exact thought.  I was looking the other way when
> untracked-cache was done originally ;-), and I would also want to
> know the answers to the above questions.
Maybe somewhat off topic, but:

Create a repo under Linux:
Linux will probe the FS and will set
core.filemode true
core.ignorecase false.

Export it using SAMBA to Windows or Mac OS X.
In some magical way the mounted repo becomes case-insensitive under
both Mac and Windows, even if core.ignorecase is true in the config file.

Same for filemode:
(Git for Windows doesn't see the execute bit at all, but relies on 
core.filemode anyway)
Depending how the repo is mounted on the Mac, the execute bits may work, 
be always 0 or always 1.

Relying on this kind of config files is not ideal in a networked 
environment.
It is not ideal when different implementations of Git access the same repo,
(Git for Windows vs cygwin vs Egit/Jgit)

So I think that the original patch could make Git unreliable under some 
circumstances.

And some day I may send a patch which does a quick auto-probe for 
filemode and ignorecase,
to adjust Git to what the underlying "local OS - network - remote OS - 
remote FS" really achieves.

Re: [RFC/PATCH] config: add core.trustmtime

[Duy Nguyen]

On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> 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

Re: [RFC/PATCH] config: add core.trustmtime

[Christian Couder]

On Wed, Dec 2, 2015 at 8:28 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> 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 ;-)

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

Thanks for developing it too,
Christian.

Re: [RFC/PATCH] config: add core.trustmtime

[Johannes Schindelin]

Hi Christian,

On Wed, 25 Nov 2015, Christian Couder wrote:

> 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`...

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

Re: [RFC/PATCH] config: add core.trustmtime

[Christian Couder]

Hi Johannes,

On Wed, Nov 25, 2015 at 11:25 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Christian,
>
> On Wed, 25 Nov 2015, Christian Couder wrote:
>
>> 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.