Re: [PATCH v2 09/19] ls-files.c: use index api

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Jul 17, 2013, 08:07 UTC
Message-ID: <87wqopsj84.fsf@gmail.com>
In-Reply-To: <CACsJy8DA=AD8=_kVYLNMS53fb8QwqKG4=8fA1cYAQBTn1gcj0Q@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 26 quoted lines

> On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> +       if (!with_tree) {
>> +               memset(opts, 0, sizeof(*opts));
>> +               opts->pathspec = &pathspec_struct;
>> +               opts->read_staged = 1;
>> +               if (show_resolve_undo)
>> +                       opts->read_resolve_undo = 1;
>> +               read_cache_filtered(opts);
>
> So you load partial index here.
>
>> +       } else {
>> +               read_cache();
>> +       }
>> +       /* be nice with submodule paths ending in a slash */
>> +       if (pathspec)
>> +               strip_trailing_slash_from_submodules();
>
> Then strip_trailing_slash_from_submodules will attempt to convert
> pathspec "foo/" to "foo" if "foo" exists in the index and is a
> gitlink. But becaues "foo/" is used to load the partial index, "foo"
> is not loaded (is it?) and this could become incorrect no-op. I
> suggest you go through the pathspec once checking for ones ending with
> '/'. If so strip_trailing_... may potentially update pathspec, just
> load full index. If no pathspec ends with '/', strip_trail... is no-op
> and we can do partial loading safely.

It was loaded, because the adjusted_pathspec algorithm stripped the trailing slash and then everything until the next slash. That's overkill except when the trailing slash had to be stripped.

I'll make the adjusted_pathspec algorithm more restrictive, so the last trailing slash is no longer stripped. If a pathspec contains a trailing slash I'll load the whole index, as you suggested.