Re: [jgit-dev] Grafts/replace design

[Robin Rosenberg <robin.rosenberg@xxxxxxxxxx>]

----- Ursprungligt meddelande -----
> On Mon, Oct 1, 2012 at 4:08 AM, Robin Rosenberg
> <robin.rosenberg@xxxxxxxxxx> wrote:
> > ----- Ursprungligt meddelande -----
> >> On Sat, Sep 29, 2012 at 7:35 AM, Robin Rosenberg
> >> <robin.rosenberg@xxxxxxxxxx> wrote:
> >>
> >> > 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.
> >
> > So what do you suggest? Even simple things like resolve and peeling
> > of refs sometimes need to be performed without graft/replacement in
> > the transport/gc methods. One could probably recode them to use
> > lower
> > level API's.
> 
> Within GC we should never be doing resolve or peeling. GC should be
> walking from exactly what a ref points to back through the object
> graph via an ObjectWalk that has graft/replace disabled. This is a
> non-issue.

> The git-upload-pack advertisement needs to send the peeled version of
> a tag, and needs to disable replace. So in RefAdvertiser we replace
> the repository.peel call with a RevWalk that has graft/replace
> disabled and peel the target dynamically through that. Given that
> Repository.peel() is implemented internally as creating a RevWalk,
> performing the peel, and caching the result, we could make that
> easier
> to call.

Add an optional argument (e.g. two methods).

> A big problem is actually any call to peel() and getPeeledObjectId().
> Most of these are relying on the value cached in the packed-refs
> file.
> That cached value needs to be the non-replaced version. Yet many of
> the call sites using getPeeledObjectId() would prefer to work with
> the
> replaced version. Which means the caching has limited value for them.

I think it still makes sense to cache the on-disk data, we just do
different things with it in different contexts, so I think caching
is irrelevant here.

> resolve() should never be used in fsck/gc/transport code. Where it
> can
> be used must be limited to an LHS for a push operation where its
> naming the SHA-1 to send. Within git-core, push does resolve the LHS
> using replace. So if you replace A->B and you do `git push origin
> A:master` you are actually going to send B. So resolve doesn't
> matter.

I can try that, makes sense.

> > Then we have the user-controlled options to disable the rewiring.
> > For
> > the CLI one can have a global flag (shudder), but then these thing
> > will not work within a container like Eclipse.
> 
> Obviously we cannot do a global flag. Its a non-starter. I don't even
> know why you suggested it.

We cannot do that, so what do your suggest, besides adding a parameter
to many places.

For "completing" the solution space, and discourage other people from suggesting
it, perhaps a bad thing to do.

I think it makes sense to toggle a switch in the history view to see the raw version
vs the grafted/replaced version of the history.

-- robin