Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2021, 20:00 UTC
Message-ID: <xmqqfstlbzq9.fsf@gitster.g>
In-Reply-To: <YVVmssXlaFM6yD5W@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 27 quoted lines

>> > If not, then I think the solution is probably not to install this as the
>> > "primary", but rather:
>> >
>> >   - do the specific remerge-diff writes we want using a special "write
>> >     to this object dir" variant of write_object_file()
>> >
>> >   - install the tmp_objdir as an alternate, so the reading side (which
>> >     is diff code that doesn't otherwise know about our remerge-diff
>> >     trickery) can find them
>> >
>> > And that lets other writers avoid writing into the temporary area
>> > accidentally.
>> 
>> ...
>
> The key thing here is in the first step, where remerge-diff is
> explicitly saying "I want to write to this temporary object-dir". But
> the textconv-cache code does not; it gets to write to the normal spot.
> So it avoids problem (1).
>
> You're right that it does not avoid problem (3) exactly. But triggering
> that would require some code not just writing other objects or
> references while the tmp-objdir is in place, but specifically
> referencing the objects that remerge-diff did put into the tmp-objdir.
> That seems a lot less likely to me (because the thing we're most worried
> about is unrelated code that just happens to write while the tmp-objdir
> is in place).

I do not offhand remember if a new object creation codepath that avoids writing an object that we already have considers an object we find in an alternate as "we have it". If it does not and we make a duplicated copy in our primary object store, then it would be OK, but otherwise, those that know tmp_objdir is an alternate may still try to write a new object and find that the same object already exists in an alternate. Once tmp_objdir is gone, we would end up with a corrupt repository, right?

freshen_loose_object() seems to go to check_and_freshen() which consult check_and_freshen_nonlocal() before returning yes/no, so having the same object in an alternate does seem to count as having one.

Show 9 quoted lines

> So hopefully it's clearer now from what I wrote above, but just
> connecting the dots:
>
>   1. Unrelated code calling write_object_file() would still write real,
>      durable objects, as usual.
>
>   2. The write_object_file() "skip this write" optimization already
>      ignores the pretend_object_file() objects while checking "do we
>      have this object".

Yup for (2)---the pretend interface is safer than alternate in that sense.

But the pretend interface was designed to just hold a handful of phoney objects in core. I am not sure if it is a good fit for this use case.

Show 8 quoted lines

> But this is all scary and error-prone enough that I'd prefer an approach
> that has the "least surprise". So any solution where random code calling
> write_object_file() _can't_ accidentally write to a throw-away directory
> seems like a safer less surprising thing.
>
> It does mean that the remerge-diff code needs to be explicit in its
> object writes to say "this needs to go to the temporary area" (whether
> it's a flag, or calling into a totally different function or subsystem).

... and also writes by others that happens to write the same object as what is already in the temporary area should be made to the real object store. Then we would be much safer.

> I'm hoping that's not hard to do (because its writes are done explicitly
> by the remerge-diff code), but I worry that it may be (because you are
> relying on more generic tree code to the write under the hood).

Thanks.