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

From: Christian Couder <christian.couder@gmail.com>
Date: Nov 26, 2015, 05:21 UTC
Message-ID: <CAP8UFD2V3nbY2-abW6cGDtB2PR9Q+sN+d0RgTVJORCPg6TPMcQ@mail.gmail.com>
In-Reply-To: <CACsJy8DhAfu7J=WpPAp8HYGLuFQC5+DZyZj6Hs6vruEJEeVKig@mail.gmail.com>

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.