Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 14, 2014, 15:54 UTC
Message-ID: <xmqqr41oylyo.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACsJy8D0CdS5B5xNSSCk+LToXV9FnHFLkPzJ5f-7NTWiw9yn5w@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 15 quoted lines

> On Sat, Jul 12, 2014 at 11:44 AM, David Turner <dturner@twopensource.com> wrote:
>> @@ -342,6 +342,15 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
>>
>>                 discard_cache();
>>                 read_cache_from(index_lock.filename);
>> +               if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
>> +                       fd = open(index_lock.filename, O_WRONLY);
>> +                       if (fd >= 0)
>> +                               if (write_cache(fd, active_cache, active_nr) == 0) {
>> +                                       close_lock_file(&index_lock);
>
> If write_cache() returns a negative value, index.lock is probably
> corrupted. Should we die() instead of moving on and returning
> index_lock.filename to the caller? The caller may move index.lock to
> index later on and officially ruin "index".

Perhaps true, but worse yet, this will not play nicely together with your split index series, no? After taking the lock and writing and closing, we spawn the interactive while still holding the lock, and the "open" we see here is because we want to further update the same under the same lock. Perhaps write_locked_index() API in the split index series can notice that the underlying fd in index_lock has been closed earlier, realize that the call is to re-update the index under the same lock and open the file again for writing?

Then we can update the above "open() then write_cache()" sequence with just a call to "write_locked_index()".