Re: [PATCH v8 41/44] refs.c: add a new flag for transaction delete for refs we know are packed only

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 28, 2014, 14:21 UTC
Message-ID: <5385F0E5.4010306@alum.mit.edu>
In-Reply-To: <xmqqzji3f55i.fsf@gitster.dls.corp.google.com>

On 05/27/2014 08:27 PM, Junio C Hamano wrote:

Show 12 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This suggests to me that our current structure is best modeled as two
>> independent reference back ends, with a third implementation of the same
>> reference API whose job it is to compose the first two.  In pseudocode,
>> ...
> 
> That is an interesting view.
> 
> How does reflog fit into the picture?  Is it a completely
> independent thing that is called from any implementation of
> ReferenceBackend interface?

That's a good question. It certainly wouldn't work for each of the loose and packed backends to try to do logging. I think the best solution would be to have a logging wrapper as explained below.

Show 20 quoted lines

>> From this point of view it is clear that packing refs is not an
>> operation that belongs in the ReferenceBackend API, but rather in the
>> StackedReferenceBackend interface.
> 
> When an implementation of ReferenceBackend has skewed performance
> characteristics (e.g. PackedReferenceBackend really prefers to be
> modified in bulk), how would that interact with the abstraction?
> 
> For example, when the application does:
> 
>     begin_transaction()
>     for ref in many_refs():
> 	delete_reference(ref)
>     commit_transaction()
> 
> StackedReferenceBackend() that consists of Loose on top of Packed
> may want to implement the commit phase like so:
> 
>     - tell Packed backend to repack without the deleted refs
>     - tell Loose backend to delete the deleted refs

I think for any two backends that are stacked, you would need to break down a transaction as follows (here generalized to include not only deletions):

    packed->begin_transaction()
    loose->begin_transaction()

    # And this part is done within stacked->commit_transaction():
    for entry in many_ref_updates():
        if have_old:
            stacked->verify_reference(ref, old_sha1)

        if entry is a delete:
            packed->delete_reference(entry)
        loose->update_reference(entry)

    if (!packed->commit_transaction())
        loose->commit_transaction()

Verifying old values is impossible to do batchwise with the current API, because the old value of the packed ref has to be verified if and only if there is no corresponding loose ref.

Show 5 quoted lines

> But the above would not quite work, as somebody needs to remove logs
> for refs that were only in the Packed backend, and "repack without
> these refs" API supported by the Packed backend cannot be that
> somebody---"repack packed-refs without A B C" cannot unconditionally
> remove logs for A B C without checking if A B C exists as Loose.

Correct. That's another reason that logging has to be the responsibility of something at the "stacked" level of abstraction or higher.

I think the logging should be done by yet another outer layer of wrapper that only does the logging, while also passing all updates down 1:1 to the backend that it wraps (which in our case would be a stacked backend). Then the loose and packed backends could remain completely ignorant of the fact that reference updates can be logged. I think the logging layer could implement the same reference backend API as the other backends.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu