Re: [PATCH v3 10/24] make sure partially read index is not changed

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Aug 20, 2013, 08:46 UTC
Message-ID: <87ioz0iwbn.fsf@gmail.com>
In-Reply-To: <CAPig+cTJQrVm+r77utWzduU3KqC5WPX_b4Vbvxyjezd0Wc4LZw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 61 quoted lines

> On Sun, Aug 18, 2013 at 3:41 PM, Thomas Gummerer <t.gummerer@gmail.com> 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 write a
>
> s/,//
>
>> partially read index.  Do the same when trying to re-read a partially
>> read index without having discarded it first to avoid loosing any
>
> s/loosing/losing/
>
>> information.
>>
>> 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,
>
> s/it,/it/  (or s/file instead/file, instead/)
> s/advantage,/advantage/
>
>> by avoiding to read parts of the index twice.
>
> /to read/reading/
>
> More below...
>
>>
>> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
>> ---
>>  read-cache.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/read-cache.c b/read-cache.c
>> index 38b9a04..7a27f9b 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1332,6 +1332,8 @@ int read_index_filtered_from(struct index_state *istate, const char *path,
>>         void *mmap;
>>         size_t mmap_size;
>>
>> +       if (istate->filter_opts)
>> +               die("BUG: Can't re-read partially read index");
>>         errno = EBUSY;
>>         if (istate->initialized)
>>                 return istate->cache_nr;
>> @@ -1455,6 +1457,8 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile
>>
>>  int write_index(struct index_state *istate, int newfd)
>>  {
>> +       if (istate->filter_opts)
>> +               die("BUG: index: cannot write a partially read index");
>
> Consistency nit:
>
> In the preceding hunk, the error message starts "BUG: Can't...", but
> in this hunk we have "BUG: index: cannot...".
>
> So, "BUG:" is the prefix of one, but "BUG: index:" is the prefix of the other.
>
> Spelling difference: "Can't" vs. "cannot".
>
> Capitalization difference: "Can't" vs. "cannot".

Thanks for catching this. From quick grepping it seems the preferred version seems to be the one with only "BUG:" as prefix and starting with a lower case letter after this. Both can't and cannot are used in the codebase, but cannot seems to be used more often. I'll use that.

Will fix this and the rest of the style/spelling/grammar fixes you suggested. Thanks.

Show 6 quoted lines

>>         return istate->ops->write_index(istate, newfd);
>>  }
>>
>> --
>> 1.8.3.4.1231.g9fbf354.dirty
>>