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

From: Jeff King <peff@peff.net>
Date: Oct 1, 2021, 05:21 UTC
Message-ID: <YVaa650FehPmk0QD@coredump.intra.peff.net>
In-Reply-To: <CABPp-BEqcq0wYqNP1xUJj8+E5HUTnip7+asadRoeFLAZ34rTdQ@mail.gmail.com>

On Thu, Sep 30, 2021 at 07:31:44PM -0700, Elijah Newren wrote:

Show 12 quoted lines

> > Some ways it could go wrong:
> >
> >   - is it possible for the merge code to ever write an object? I kind of
> >     wonder if we'd ever do any cache-able transformations as part of a
> >     content-level merge. I don't think we do now, though.
> 
> Yes, of course -- otherwise there would have been no need for the
> tmp_objdir in the first place.  In particular, it needs to write
> three-way-content merges of individual files, and it needs to write
> new tree objects.  (And it needs to do this both for creating the
> virtual merge bases if the merge is recursive, as well as doing it for
> the outer merge.)

Right, sorry, I should have added: ...in addition to the merge-results we're writing. What I'm getting at is whether there might be _other_ parts of the code that the merge code would invoke. Say, if a .gitattribute caused us to convert a file's encoding in order to perform a more semantic merge, and we wanted to store that result somehow (e.g., in a similar cache).

I don't think anything like that exists now, but I don't find it outside the realm of possibility in the future. It's sort of the merge equivalent of "textconv"; we have external diff and external merge drivers, but being able to convert contents and feed them to the regular text diff/merge code simplifies things.

Show 13 quoted lines

> >   - in step 5, write_object_file() may still be confused by the presence
> >     of the to-be-thrown-away objects in the alternate. This is pretty
> >     unlikely, as it implies that the remerge-diff wrote a blob or tree
> >     that is byte-identical to something that the diff wants to write.
> 
> That's one reason it could be confused.  The textconv filtering in
> particular was creating a new object based on an existing one, and a
> tree, and a ref.  What if there was some other form of caching or
> statistic gathering that didn't write a new object based on an
> existing one, but did add trees and especially refs that referenced
> the existing object?  It's not that the diff wanted to write something
> byte-identical to what the merge wrote, it's just that the diff wants
> to reference the object somehow.

Yes, good point. It can help a little if the alternate-odb added by tmp_objdir was marked with a flag to say "hey, this is temporary". That would help write_object_file() decide not to depend on it. But the problem is so much wider than that that I think it will always be a bit dangerous; any code could say "can I access this object" and we don't know if its intent is simply to read it, or to create a new object that references it.

-Peff