Re: [PATCH v7 2/2] Verify index file before we opportunistically update it

[Junio C Hamano <gitster@pobox.com>]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Apr 11, 2014 at 2:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Yiannis Marangos <yiannis.marangos@gmail.com> writes:
>>
>>> +     n = xpread(fd, sha1, 20, st.st_size - 20);
>>> +     if (n != 20)
>>> +             goto out;
>>
>> I think it is possible for pread(2) to give you a short-read.
>>
>> The existing callers of emulated mmap and index-pack are prepared to
>> handle a short-read correctly, but I do not think this code does.
>>
>> I'll queue this instead in the meantime.
>
> There are two things to sort out (sorry I can't spend much time on it
> right now): should the same sha1 test be done in write_index(), in
> addition to update_index_if_able(). And what do we do about
> istate->sha1[] in discard_index(), keep it or clear it.

Yeah, I was hoping that the real write codepath (as opposed to "this
is read only and we read the index without holding a lock---now we
noticed that the index needs refreshing, and we know how the
resulting refreshed index should look like, perhaps we can write it
to save cycles for other processes" codepath where we cannot and
should not take a lock early) would take the lock and then read, but
because that is not the way they work, the need the same protection,
I would think.  As discard_index() is not "we are removing all the
entries because the user told us to 'rm -r .'" but is "for the
purpose of our internal processing we do not need the old contents
of the index", my knee-jerk reaction is that we should not clear it.

Any operation that makes the resulting index empty (i.e. no path
being tracked) should still write an empty index (with the usual
header and the most importantly the trailing file checksum), but
that goes without saying.