threads / rfc / 40877

RFC patchconfig: add core.trustmtime

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

## tl;dr

13 messages between Nov 25, 2015 and Dec 7, 2015. Diffs are folded; open one to read it.

replies: 12people: 6as markdown or json

Christian Couder· Nov 25, 2015, 06:35 UTC · lore

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

Documentation/config.txt, Documentation/git-update-index.txt, builtin/update-index.c, cache.h, config.c, contrib/completion/git-completion.bash, dir.c, environment.c

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
Ævar Arnfjörð Bjarmason· Nov 25, 2015, 09:00 UTC · re: Christian Couder · lore

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

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.

Duy Nguyen· Nov 25, 2015, 19:51 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

Show 6 quoted lines
> 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) ;)

Show 7 quoted lines
>  * 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
Christian Couder· Nov 26, 2015, 05:21 UTC · re: Duy Nguyen · lore

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

Hi Duy,
On Wed, Nov 25, 2015 at 8:51 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 26 quoted lines
> 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.

Show 11 quoted lines
>> 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
Show 12 quoted lines
>>  * 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.
Show 7 quoted lines
>>  * 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.
Show 8 quoted lines
>> 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.

Duy Nguyen· Nov 26, 2015, 17:53 UTC · re: Christian Couder · lore

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

On Thu, Nov 26, 2015 at 6:21 AM, Christian Couder <christian.couder@gmail.com> wrote:

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

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

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

Show 19 quoted lines
>>>  * 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
Ævar Arnfjörð Bjarmason· Nov 27, 2015, 01:35 UTC · re: Duy Nguyen · lore

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

On Thu, Nov 26, 2015 at 6:53 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 9 quoted lines
> 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.

Junio C Hamano· Nov 30, 2015, 19:05 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 7 quoted lines
> 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.
Duy Nguyen· Nov 30, 2015, 19:12 UTC · re: Junio C Hamano · lore

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

On Mon, Nov 30, 2015 at 8:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Æ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
Torsten Bögershausen· Dec 1, 2015, 05:57 UTC · re: Junio C Hamano · lore

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

On 11/30/2015 08:05 PM, Junio C Hamano wrote:
Show 12 quoted lines
> Æ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.

Duy Nguyen· Dec 2, 2015, 19:28 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Christian Couder· Dec 7, 2015, 05:42 UTC · re: Duy Nguyen · lore

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

On Wed, Dec 2, 2015 at 8:28 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 12 quoted lines
> 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.

Johannes Schindelin· Nov 25, 2015, 10:25 UTC · re: Christian Couder · lore

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

Hi Christian,
On Wed, 25 Nov 2015, Christian Couder wrote:
Show 13 quoted lines
> 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

Christian Couder· Nov 25, 2015, 10:39 UTC · re: Johannes Schindelin · lore

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

Hi Johannes,

On Wed, Nov 25, 2015 at 11:25 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

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

← back to recent threads