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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 11, 2014, 19:24 UTC
Message-ID: <xmqqtx9zsm8p.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACsJy8A5VJM4gnJGazkzniv5YxRx0xR7YEFpQbU+kVGofQ1b_g@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 18 quoted lines

> 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.