Re: [PATCH v2 38/43] refs: make some files backend functions public

[David Turner <dturner@twopensource.com>]

On Mon, 2015-10-05 at 11:03 +0200, Michael Haggerty wrote:
> On 09/29/2015 12:02 AM, David Turner wrote:
> > Because HEAD and stash are per-worktree, other backends need to
> > go through the files backend to manage these refs and their reflogs.
> > 
> > To enable this, we make some files backend functions public.
> 
> I have a bad feeling about this change.
> 
> Naively I would expect a reference backend that cannot handle its own
> (e.g.) stash to instantiate internally a files backend object and to
> delegate stash-related calls to that object. That way neither class's
> interface has to be changed.
> 
> Here you are adding a separate interface to the files backend. That
> seems like a more complicated and less flexible design. But I'm open to
> be persuaded otherwise...

After some thought, here's a summary of the problem:

Some writes are cross-backend writes.  For example, if HEAD is symref to
refs/head/master, a commit is a cross-backend write (HEAD itself is not
updated, but its reflog is).  Ronnie's design of the ref backend
structure did not account for cross-backend writes, because we didn't
have per-worktree refs at the time (there was only HEAD, and there was
only one copy of it).

Cross-backend writes are complicated because there is no way to tell a
backend to do only part of a ref update -- for instance, to tell the
files backend to update HEAD and HEAD's reflog but not
refs/heads/master.  Maybe we could set a flag that would do this, but
the synchronization would be fairly complicated.  For instance, an
update to HEAD might need to confirm the old sha for HEAD, meaning that
we couldn't do the db write first.  But if we move the db write second,
then when the db code goes to do its check of the HEAD sha, it might see
a new value.  Perhaps there's a way to make it work, but it seems
fragile/complex.

Right now, for cross-backend reads/writes, the lmdb code cheats. It
simply does the write directly and immediately.  This means that these
portions of transactions cannot be rolled back.  That's clearly bad. 



The simplest solution would be for the lmdb code to simply acquire
locks, and write to lock files, and then commit those lock files just
before the db transaction commits. Then the lmdb code would handle all
of the orchestration without the files backend having to be rewritten to
handle this case.

We would still like to use files_log_ref_write, since that handles the
formatting details of writing to file-based reflogs.  If you want, we
could break that (and its callees) out to separate refs-be-common.[ch]
files, as you suggested for some #defines.  (The calls to
files_log_ref_write would also have to move to transaction_commit as
well, but I think that's straightforward).

Does this seem reasonable to you?