Re: [PATCH] dir.c: ignore paths containing .git when invalidating untracked cache

>
>
> On 2/4/2018 4:38 AM, Nguyễn Thái Ngọc Duy wrote:
>>
>> read_directory() code ignores all paths named ".git" even if it's not
>> a valid git repository. See treat_path() for details. Since ".git" is
>> basically invisible to read_directory(), when we are asked to
>> invalidate a path that contains ".git", we can safely ignore it
>> because the slow path would not consider it anyway.
>>
>> This helps when fsmonitor is used and we have a real ".git" repo at
>> worktree top. Occasionally .git/index will be updated and if the
>> fsmonitor hook does not filter it, untracked cache is asked to
>> invalidate the path ".git/index".
>>
>> Without this patch, we invalidate the root directory unncessarily,
>> which:
>>
>> - makes read_directory() fall back to slow path for root directory
>>    (slower)
>>
>> - makes the index dirty (because UNTR extension is updated). Depending
>>    on the index size, writing it down could also be slow.
>>
>> Noticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>   Sorry for the resend, I forgot git@vger.
>>
>>   dir.c             | 13 ++++++++++++-
>>   git-compat-util.h |  2 ++
>>   wrapper.c         | 12 ++++++++++++
>>   3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 7c4b45e30e..f8b4cabba9 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct
>> dir_struct *dir,
>>         if (!de)
>>                 return treat_path_fast(dir, untracked, cdir, istate, path,
>>                                        baselen, pathspec);
>> -       if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
>> +       if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name,
>> ".git"))
>>                 return path_none;
>>         strbuf_setlen(path, baselen);
>>         strbuf_addstr(path, de->d_name);
>> @@ -2970,8 +2970,19 @@ static int invalidate_one_component(struct
>> untracked_cache *uc,
>>   void untracked_cache_invalidate_path(struct index_state *istate,
>>                                      const char *path)
>>   {
>> +       const char *end;
>> +       int skipped;
>> +
>>         if (!istate->untracked || !istate->untracked->root)
>>                 return;
>
>
> Thank you for this patch!  It's great to see people helping improve the
> performance of git.
>
> I ran a quick test and this is not sufficient to prevent the index from
> getting flagged as dirty and written out to disk when fsmonitor detects
> changes for files under the .git folder.  What I'm seeing is that when
> ".git/index" is returned, the test below doesn't catch them and end early as
> would be expected.

> As a result, invalidate_one_component() is called which calls
> invalidate_one_directory() which increments dir_invalidated counter and the
> regular dirty process continues which triggers the index to be written to
> disk unnecessarily.
>
>> +       if (!fspathcmp(path, ".git"))
>> +               return;
>> +       if (ignore_case)
>> +               skipped = skip_caseprefix(path, "/.git", &end);
>> +       else
>> +               skipped = skip_prefix(path, "/.git", &end);
>> +       if (skipped && (*end == '\0' || *end == '/'))
>> +               return;
>
>
> If I replace the above lines with:
>
>         if (ignore_case)
>                 skipped = skip_caseprefix(path, ".git", &end);
>         else
>                 skipped = skip_prefix(path, ".git", &end);
>
> Then it correctly skips _all_ files under ".git".  I'm not sure if by
> removing the leading slash, I'm breaking some other case but I was not able
> to find where that was expected or needed. Removing the leading slash also
> allows me to remove the fsmpathcmp() call as it is now redundant.