Subject: Re: [PATCH v2 07/19] make sure partially read index is not changed

From: Thomas Gummerer <[email-masked]>
Date: Wed, 17 Jul 2013 14:56:52 +0200

Duy Nguyen <[email-masked]> writes:

> On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer <[email-masked]> wrote:
>> A partially read index file currently cannot be written to disk.  Make
>> sure that never happens, by erroring out when a caller tries to change a
>> partially read index.  The caller is responsible for reading the whole
>> index when it's trying to change it later.
>>
>> Forcing the caller to load the right part of the index file instead of
>> re-reading it when changing it, gives a bit of a performance advantage,
>> by avoiding to read parts of the index twice.
>
> If you want to be strict about this, put similar check in
> fill_stat_cache_info (used by entry.c), cache_tree_invalidate_path and
> convert the code base (maybe except unpack-trees.c, just put a check
> in the beginning of unpack_trees()) to use wrapper function to change
> ce_flags (some still update ce_flags directly, grep them). Some flags
> are in memory only and should be allowed to change in partial index,
> some are to be written on disk and should not be allowed.

I'm not sure anymore we should even be this strict.  A partially read
index will never make it to the disk, because write_index checks if it's
fully read.   I think the check in write_index and read_index_filtered
would be enough.

The only disadvantage would be that it takes a little longer to catch an
error that should never happen in the first place.  If it occurs the
user has bigger problems than the time that not having caught the
error earlier adds to the execution of the command.

I also don't see a clean way to add the check to fill_stat_cache_info or
cache_tree_invalidate_path, because we would need an additional
parameter for the index_state, or at least for index_state->filter_opts,
which doesn't do anything but check if the index is only partially loaded.