Christian Couder <christian.couder@xxxxxxxxx> 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.
> 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.
> 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.