Re: Some rough edges of core.fsmonitor

>
>
> On 1/28/2018 5:28 PM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Sun, Jan 28, 2018 at 9:44 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>>
>>> Hi,
>>>
>>> On Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:
>>>
>>>> I just got around to testing this since it landed, for context some
>>>> previous poking of mine in [1].
>>>>
>>>> Issues / stuff I've noticed:
>>>>
>>>> 1) We end up invalidating the untracked cache because stuff in .git/
>>>> changed. For example:
>>>>
>>>>      01:09:24.975524 fsmonitor.c:173         fsmonitor process
>>>> '.git/hooks/fsmonitor-watchman' returned success
>>>>      01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback
>>>> '.git'
>>>>      01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback
>>>> '.git/config'
>>>>      01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback
>>>> '.git/index'
>>>>      01:09:25.122726 fsmonitor.c:91          write fsmonitor extension
>>>> successful
>>>>
>>>> Am I missing something or should we do something like:
>>>>
>>>>      diff --git a/fsmonitor.c b/fsmonitor.c
>>>>      index 0af7c4edba..5067b89bda 100644
>>>>      --- a/fsmonitor.c
>>>>      +++ b/fsmonitor.c
>>>>      @@ -118,7 +118,12 @@ static int query_fsmonitor(int version,
>>>> uint64_t last_update, struct strbuf *que
>>>>
>>>>       static void fsmonitor_refresh_callback(struct index_state *istate,
>>>> const char *name)
>>>>       {
>>>>      -       int pos = index_name_pos(istate, name, strlen(name));
>>>>      +       int pos;
>>>>      +
>>>>      +       if (!strcmp(name, ".git") || starts_with(name, ".git/"))
>>>>      +               return;
>>>>      +
>>>>      +       pos = index_name_pos(istate, name, strlen(name));
>>>
>>>
>>> I would much rather have the fsmonitor hook already exclude those.
>>
>>
>> As documented the fsmonitor-watchman hook we ship doesn't work as
>> described in githooks(5), unless "files in the working directory" is
>> taken to include .git/, but I haven't seen that ever used.
>>
>> On the other hand relying on arbitrary user-provided hooks to do the
>> right thing at the cost of silent performance degradation is bad. If
>> we're going to expect the hook to remove these we should probably
>> warn/die here if it does send us .git/* files.
>>
>
> I'm not sure how often something is modified in the git directory when
> nothing was modified in the working directory but this seems like a nice
> optimization.
>
> We can't just blindly ignore changes under ".git" as the git directory may
> have been moved somewhere else.  Instead we'd need to use get_git_dir().

> Rather than assuming the hook will optimize for this particular case, I
> think a better solution would be to update untracked_cache_invalidate_path()
> so that it doesn't invalidate the untracked cache and mark the index as
> dirty when it was asked to invalidate a path under GIT_DIR.  I can't think
> of a case when that would be the desired behavior.

> Somewhat off topic but related to the overall performance discussion: I've
> also thought the untracked cache shouldn't mark the index as dirty except in
> the case where the extension is being added or removed.  We've observed that
> it causes unnecessary index writes that actually slows down overall
> performance.
>
> Since it is a cache, it does not require the index to be written out for
> correctness, it can simply update the cache again the next time it is
> needed. This is typically faster than the cost of the index write so makes
> things faster overall.  I adopted this same model with the fsmonitor
> extension.