Re: [PATCH 05/22] read-cache: add index reading api

[Thomas Gummerer <t.gummerer@gmail.com>]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sun, Jul 7, 2013 at 3:11 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> +/*
>> + * Options by which the index should be filtered when read partially.
>> + *
>> + * pathspec: The pathspec which the index entries have to match
>> + * seen: Used to return the seen parameter from match_pathspec()
>> + * max_prefix, max_prefix_len: These variables are set to the longest
>> + *     common prefix, the length of the longest common prefix of the
>> + *     given pathspec
>> + *
>> + * read_staged: used to indicate if the conflicted entries (entries
>> + *     with a stage) should be included
>> + * read_cache_tree: used to indicate if the cache-tree should be read
>> + * read_resolve_undo: used to indicate if the resolve undo data should
>> + *     be read
>> + */
>> +struct filter_opts {
>> +       const char **pathspec;
>> +       char *seen;
>> +       char *max_prefix;
>> +       int max_prefix_len;
>> +
>> +       int read_staged;
>> +       int read_cache_tree;
>> +       int read_resolve_undo;
>> +};
>> +
>>  struct index_state {
>>         struct cache_entry **cache;
>>         unsigned int version;
>> @@ -270,6 +297,8 @@ struct index_state {
>>         struct hash_table name_hash;
>>         struct hash_table dir_hash;
>>         struct index_ops *ops;
>> +       struct internal_ops *internal_ops;
>> +       struct filter_opts *filter_opts;
>>  };
>
> ...
>
>> -/* remember to discard_cache() before reading a different cache! */
>> -int read_index_from(struct index_state *istate, const char *path)
>> +
>> +int read_index_filtered_from(struct index_state *istate, const char *path,
>> +                            struct filter_opts *opts)
>>  {
>>         int fd, err, i;
>>         struct stat st_old, st_new;
>> @@ -1337,7 +1425,7 @@ int read_index_from(struct index_state *istate, const char *path)
>>                 if (istate->ops->verify_hdr(mmap, mmap_size) < 0)
>>                         err = 1;
>>
>> -               if (istate->ops->read_index(istate, mmap, mmap_size) < 0)
>> +               if (istate->ops->read_index(istate, mmap, mmap_size, opts) < 0)
>>                         err = 1;
>>                 istate->timestamp.sec = st_old.st_mtime;
>>                 istate->timestamp.nsec = ST_MTIME_NSEC(st_old);
>> @@ -1345,6 +1433,7 @@ int read_index_from(struct index_state *istate, const char *path)
>>                         die_errno("cannot stat the open index");
>>
>>                 munmap(mmap, mmap_size);
>> +               istate->filter_opts = opts;
>>                 if (!index_changed(&st_old, &st_new) && !err)
>>                         return istate->cache_nr;
>>         }
>
> Putting filter_opts in index_state feels like a bad design. Iterator
> information should be separated from the iterated object, so that two
> callers can walk through the same index without stepping on each other
> (I'm not talking about multithreading, a caller may walk a bit, then
> the other caller starts walking, then the former caller resumes
> walking again in a call chain).

Yes, you're right.  We need the filter_opts to see what part of the
index has been loaded [1] and which part has been skipped, but it
shouldn't be used for filtering in the for_each_index_entry function.

I think there should be two versions of the for_each_index_entry
function then, where the for_each_index_entry function would behave the
same way as the for_each_index_entry_filtered function with the
filter_opts parameter set to NULL:
for_each_index_entry_filtered(struct index_state *, each_cache_entry_fn, void *cb_data, struct filter_opts *)
for_each_index_entry(struct index_state *, each_cache_entry_fn, void *cb_data)

Both of them then should call index_change_filter_opts to make sure all
the entries that are needed are loaded in the in-memory format.

Does that make sense?

[1] That is only important for the new index-v5 file format, which can
    be loaded partially.  The filter_opts could always be set to NULL,
    as the whole index is always loaded anyway.