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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2017, 20:52 UTC
Message-ID: <xmqqsho6amm7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAP8UFD3KXOgVOhuMtws36XPiek56U4mQKdUs07hzKc-dC=ppmA@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

> Well, when we cannot freshen a loose file (with
> freshen_loose_object()), we don't warn or die, we just write the loose
> file. But here we cannot write the shared index file.

I think that is an excellent point. Let me make sure I got you right. For loose object files, we may attempt to freshen and when it fails we stay silent and do not talk about the failure. Instead, we write the same file again. That will have two potential outcomes:

 1. write fails and we fail the whole thing.  It is very clear to
    the user that there is something wrong going on.

 2. write succeeds, and because we just wrote it, we know that the
    file is fresh and is protected from gc.

So the "freshen and if fails just write" is sufficient. It is absolutely the right thing to do for loose object files.

When we are forking off a new split index file based on an old shared index file, we may attempt to "touch" the old shared one, but we cannot write the old shared one again, because other split index may be based on that, and we do not have enough information to recreate the old one [*1*]. The fallback safety is not available.

> And this doesn't lead to a catastrophic failure right away.

Exactly.

Show 5 quoted lines

> There
> could be a catastrophic failure if the shared index file is needed
> again later, but we are not sure that it's going to be needed later.
> In fact it may have just been removed because it won't be needed
> later.

You are listing only the irrelevant cases. The shared one may be used immediately, and the user can keep using it for a while without "touching". Perhaps the shared one was chowned to "root" while the user is looking the other way, and its timestamp is now frozen to the time of chown. It is a ticking time-bomb that will go off when your expiry logic kicks in.

Show 9 quoted lines

> So I am not sure it's a good idea to anticipate a catastrophic failure
> that may not happen. Perhaps we could just warn, but I am not sure it
> will help the user. If the catastrophic failure doesn't happen because
> the shared index file is not needed, I can't see how the warning could
> help. And if there is a catastrophic failure, the following will be
> displayed:
>
> fatal: .git/sharedindex.cbfe41352a4623d4d3e9757861fed53f3094e0ac:
> index file open failed: No such file or directory

That "fatal" is given _ONLY_ after time passes and our failure to "touch" the file that is still-in-use left it subject to "gc". Of course, when "fatal" is given, it is too late to warn about ticking time bomb.

At the time we notice a ticking time bomb is the only sensible time to warn. Or better yet take a corrective action.

As I expect (and you seem to agree) that a failure to "touch" would be a very rare event (like, sysadmin chowning it to "root" by mistake), I do not mind if the "corrective action" were "immediately unsplit the index, so that at least the current and the latest index contents will be written out safely to a new single unsplit index file". That won't help _other_ split index files that are based on the same "untouchable" shared index, but I do not think that is a problem we need to solve---if they are still in use, the code that use them will notice it, and otherwise they are not in use and can be aged away safely.

[Footnote]

*1* My understanding is that we lose information on stale entries in
    the shared file that are covered by the split index overlay
    after read_index() returns, so we _might_ be able to write the
    "old" one that is sufficient for _our_ split index, but we do
    not have good enough information to recreate "old" one usable by
    other split index files.