From: Christian Couder <christian.couder@gmail.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
Git <git@vger.kernel.org>, "Jeff King" <peff@peff.net>,
"David Turner" <dturner@twopensource.com>,
"Christian Couder" <chriscool@tuxfamily.org>
Subject: Re: [RFC/PATCH] config: add core.trustmtime
Date: Thu, 26 Nov 2015 06:21:40 +0100 [thread overview]
Message-ID: <CAP8UFD2V3nbY2-abW6cGDtB2PR9Q+sN+d0RgTVJORCPg6TPMcQ@mail.gmail.com> (raw)
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:
> 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.
next prev parent reply other threads:[~2015-11-26 5:21 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-25 6:35 [RFC/PATCH] config: add core.trustmtime Christian Couder
2015-11-25 9:00 ` Ævar Arnfjörð Bjarmason
2015-11-25 19:51 ` Duy Nguyen
2015-11-26 5:21 ` Christian Couder [this message]
2015-11-26 17:53 ` Duy Nguyen
2015-11-27 1:35 ` Ævar Arnfjörð Bjarmason
2015-11-30 19:05 ` Junio C Hamano
2015-11-30 19:12 ` Duy Nguyen
2015-12-01 5:57 ` Torsten Bögershausen
2015-12-02 19:28 ` Duy Nguyen
2015-12-07 5:42 ` Christian Couder
2015-11-25 10:25 ` Johannes Schindelin
2015-11-25 10:39 ` Christian Couder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAP8UFD2V3nbY2-abW6cGDtB2PR9Q+sN+d0RgTVJORCPg6TPMcQ@mail.gmail.com \
--to=christian.couder@gmail.com \
--cc=avarab@gmail.com \
--cc=chriscool@tuxfamily.org \
--cc=dturner@twopensource.com \
--cc=git@vger.kernel.org \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).