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

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.