Re: [PATCH v3 14/21] read-cache: touch shared index files when used

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 23, 2017, 18:14 UTC
Message-ID: <CAP8UFD2LWGtNtdhtQTZXqsACBvK=jD25vt8M4HzBRBVS1sJ+=Q@mail.gmail.com>
In-Reply-To: <xmqqlgu627uj.fsf@gitster.mtv.corp.google.com>

On Thu, Jan 19, 2017 at 8:00 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Mon, Jan 9, 2017 at 9:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Duy Nguyen <pclouds@gmail.com> writes:
>>>
>>>> On Sun, Jan 8, 2017 at 4:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Christian Couder <christian.couder@gmail.com> writes:
>>>>>
>>>>>> So what should we do if freshen_file() returns 0 which means that the
>>>>>> freshening failed?
>>>>>
>>>>> You tell me ;-)  as you are the one who is proposing this feature.

Before the above question I already had given my opinion about what we should do.

There are the following cases:

- Freshening failed because the shared index file does not exist
anymore. In this case it could have been removed for a good reason
(for example maybe the user wants to remove all the shared index
files), or it could be a bug somewhere else. Anyway we cannot know and
the user will get an error if the shared index file that disappeared
is read from afterwards, so I don't think we need to warn or do
anything.

- Freshening failed because the mtime of the shared index cannot be
changed. You already in a previous email wrote that we shoudn't warn
if the file system is read only, and I agree with that, as anyway if
the file system is read only, the shared index file cannot be deleted,
so there is no risk from the current user. Also the split index mode
is useful to speed up index writing at the cost of making index
reading a little slower, so its use in a read only mode should not be
the primary way it is used.

So my opinion is that there are good reasons not to do anything if freshening fails.

Show 5 quoted lines

>>>> My answer is, we are not worse than freshening loose objects case
>>>> (especially since I took the idea from there).
>>>
>>> I do not think so, unfortunately.  Loose object files with stale
>>> timestamps are not removed as long as objects are still reachable.

As the current index is read, which freshens its shared index file, before a new index is created, the number of shared index files doesn't go below 2. This can be seen in the tests changed in patch 19/21. So the risk of deleting interesting shared index files is quite low in my opinion.

Also in general the split-index mode is useful when you often write new indexes, and in this case shared index files that are used will often be freshened, so the risk of deleting interesting shared index files should be low.

Show 14 quoted lines

>> But there are plenty of unreachable loose objects, added in index,
>> then got replaced with new versions. cache-tree can create loose trees
>> too and it's been run more often, behind user's back, to take
>> advantage of the shortcut in unpack-trees.
>
> I am not sure if I follow.  Aren't objects reachable from the
> cache-tree in the index protected from gc?
>
> Not that I think freshening would actually fail in a repository
> where you can actually write into to update the index or its refs to
> make a difference (iow, even if we make it die() loudly when shared
> index cannot be "touched" because we are paranoid, no real life
> usage will trigger that die(), and if a repository does trigger the
> die(), I think you would really want to know about it).

As I wrote above, I think if we can actually write the shared index file but its freshening fails, it probably means that the shared index file has been removed behind us, and this case is equivalent as when loose files have been removed behind us.