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

From: David Turner <dturner@twopensource.com>
Date: Oct 7, 2015, 01:25 UTC
Message-ID: <1444181145.7739.70.camel@twopensource.com>
In-Reply-To: <56123CE3.9070909@alum.mit.edu>

On Mon, 2015-10-05 at 11:03 +0200, Michael Haggerty wrote:

Show 16 quoted lines

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