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

From: Junio C Hamano <[email-masked]>
Date: Wed, 28 May 2014 09:58:06 -0700

Michael Haggerty <[email-masked]> writes:

> 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()

Ugggly.  In general you would need to worry about the case where the
first commit succeeds and then the second commit fails, wouldn't
you?

The above happens not to break horribly wrong for the "Loose on top
of Packed" layering, in that the refs you wanted to delete are only
gone from Packed but still are in Loose, and the end result would be
some of them are really gone (because they weren't in Loose) and
some others remain (because they were in Loose), and at least you
did not lose any ref you did not want to discard.  But it still is
not what should happen in a proper "transaction".

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

That would mean that Loose (or Packed) class cannot be used as-is
and always needs to be wrapped with the layer that does the logging,
no?  In a system with "only packed-refs, no loose", you would want
Packed.deleteRefs() to remove the named refs ref and their reflogs,
but that would mean that the Layered wrapper that uses Loose
overlayed on Packed cannot call that method, because it does not
want reflogs of the refs in Packed covered by the ones in Loose.