Re: [jgit-dev] Grafts/replace design

[Shawn Pearce <spearce@xxxxxxxxxxx>]

On Sat, Sep 29, 2012 at 7:35 AM, Robin Rosenberg
<robin.rosenberg@xxxxxxxxxx> wrote:
> This list is way too silent...

Yes. The library is shockingly stable, and most discussion happens
directly on code reviews.

> I want to discuss design issue that pops up in adding the
> grafts/replace mechanism. Unfortunately Gerrit
> is not ideal and the audience is perhaps too narrow.

:-)

> Usually you want the default behavior, i.e. apply grafts, replace
> the content of objects everywhere from peeling refs to checkout. The
> obvious exceptions are fsck, gc, push, fetch where content and refs
> should be handled the way the Creator formed them.

Unfortunately yes, most operations want the extra work implied by
honoring a graft and/or replace. Only a few need to avoid these, as
it would otherwise corrupt the object store.

> The core classes involved in the actual replacement are
> ObjectReader, RevWalk and ObjectWalk. We can do grafts within the
> latter two. We just need to pass the grafting info and only a few
> interfaces need to change, e.g. we can add a parameter to the
> constructor, or add getter/setters.

Unfortunately right now you can build a RevWalk or ObjectWalk from an
ObjectReader, where there is no Repository information available to
load the graft through. We could redefine these constructors to mean
they don't honor grafts, but this may break a number of call sites
want grafts.

I think with RevWalk/ObjectWalk you want them to default to using
grafts, and have a setter on the RevWalk that means "disable grafts".
Callers that can't use grafts will need to setUseGrafts(false) on the
walk instance before parsing the first object through it. This is a
behavior change, but most callers want grafts. The handful that can't
use them are internal to JGit (fsck, gc, PackWriter).

> For replacement of content, the ObjectReader is the core class. It
> does not always know about which repository or even object database it
> is working on so we need to pass information around from the caller so
> it knows what to replace.

ObjectReader has to know the ObjectDatabase it was created for.
This isn't true in the public API, but is true in the internal
implementations of ObjectReader. We can tie back to the Repository
so the ObjectReader can get the refs/replace namespace as needed.

> That's an extra parameter that needs to be added to any entry point
> from the user to the possible creation of an ObjectReader.

No. Look at how ObjectReader is created. Its always made from a
Repository or ObjectDatabase instance, and its construction is an
internal implementation detail. We can connect it to a Repository in
the implementation without inflicting pain on the caller.

> The next step is tricker.

> The Repository knows about grafts and replacements, so we can just
> ask it, but then we actually share instances, for efficiency and
> smoothness within the application. Since the Repositories are shared
> we cannot just flip a flag to turn off the replacement map.

Yes this is true. But the Repository doesn't actually care about
grafts or replacements, only specific ObjectReader, RevWalk, or
ObjectWalk instances. These are not thread safe and are created
per-operation that needs them. The flag belongs at this level, and the
flag should default to using the grafts and replacements.

> We can add parameter to most methods in Repository and pass it
> around

No. This is a horrible API.

> The other option I see is to create a new Repository instance
> working with the same git repository, but with a different opinion on
> what to replace.

This is also a horrible API, and one I rejected in the code review.