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

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Feb 6, 2018 at 12:44 AM, Ben Peart <peartben@gmail.com> wrote:
>
>
> 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.

Right. I focused too much on ".git" in the middle and the end of the
path and forgot that it's also at the beginning.

> 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.

Removing "/" could catch things like abc/lala.git/def, which
treat_path does not consider special and may show up as untracked
entries. In that sense, the updated invalidate_... is broken if we
don't invalidate them properly.

I think what we need here is something like

    if (!fspathcmp(path, ".git") || starts_with(path, ".git/"))

but can handle case-insensitivity as well (starts_with can't).

> I wondered what would happen if there was some other directory named ".git"
> and how that would behave.  With or without this patch and with or without
> the untracked cache, a file "dir1/.git/foo" is ignored by git so no change
> in behavior there either.

That's what I meant by treat_path() and invisibility in my commit
message. We always ignore directories (or even files) named ".git". It
does not matter if it contains anything remotely related to git. I'm
not saying it's a good thing, but it's how it is and changing that
requires a lot more work, possibly some performance degradation if you
start to check if ".git" is a valid repo before ignoring. So I focus
on improving this function alone first.
-- 
Duy