Re: [jgit-dev] Grafts/replace design

[Shawn Pearce <spearce@xxxxxxxxxxx>]

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.

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.

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.

> 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.