Re: [MirageOS-devel] Irmin API evolution

[Thomas Leonard <talex5@xxxxxxxxx>]

On 25 August 2015 at 17:10, Thomas Leonard <talex5@xxxxxxxxx> wrote:
> On 10 August 2015 at 15:05, Thomas Leonard <talex5@xxxxxxxxx> wrote:
>> On 10 August 2015 at 13:51, Thomas Gazagnaire <thomas@xxxxxxxxxxxxxx> wrote:
>>> Hi all,
>>>
>>> There are still parts in the Irmin API that I am not very happy about, so I 
>>> send an email to get feedback from all the early users to check if they 
>>> share my views.
> [...]
>>> 4. The Irmin API conflates the Git repository configuration and branch 
>>> state into an Irmin store handler. For some operations (as listing all the 
>>> branches in a repository) it doesn't make really sense. Maybe it's too 
>>> confusing and you don't want that. See Cuekeeper'API[2] which exposes a 
>>> different API, closer to what Git offers.
>>
>> No surprise that I agree with this ;-)
>>
>> Another reason to make an explicit Repository.t: Irmin currently makes
>> some "global" state when you apply the functors. For example, I have
>> to re-apply the Irmin.Basic functor to Irmin_mem.Make every time I
>> want a fresh in-memory store. Having a Repository.t lets you do the
>> set up operations for a repository once, not once per branch (too
>> often) or once per functor application (surprising hidden state).
>
> Another reason for an explicit Repository.t is that for
> database-backed stores you only want to open one database connection
> per repository.
>
> Currently, when you do Store.create/of_tag/of_head, BC creates the
> internal Contents, Node, Commit and Tag stores, each of which opens a
> separate connection.
>
> Is there any way with the current API I can share this? Currently, I'm
> thinking of creating a hash table that maps database names to
> connections and caching them there, but that's quite ugly.
>
> It's a particular issue at the moment because I want Irmin-IndexedDB
> to test for an old Irmin-format repository and upgrade it to
> Git-format automatically, but there's no obvious place to do the test.
> I guess I could have Contents notice the upgrade is needed and update
> the Tags table at the same time, but that's messy too.
>
> (also, a "Repository.close" function would be nice)

I'm currently experimenting with adding a "connect" method to BC so
that you connect to a repository first, and then use that to get
additional branches:

  https://github.com/mirage/irmin/pull/275

One problem I have is with the task-maker functions that get passed
around everywhere.

Currently, you specify the task-maker when you open a branch. You then
pass a value of the appropriate type to the response to get the store,
e.g.

  let task_of_msg x = Irmin.Task.create ... in
  Store.create conf task_of_msg >>= fun store_maker ->
  let store = store_maker "unused string" in
  Store.read store ...

I think this API is pretty strange and confusing anyway, but it's a
particular problem for sharing connections because we don't want to
create a new connection every time the user wants a new commit
message.

I know this design came out of previous discussions:

http://lists.xenproject.org/archives/html/mirageos-devel/2014-11/msg00154.html

But I'm not really clear on what the original aims were and whether
the current design meets them (it looks like it was intended to convey
some other context, but now it's just the commit message).

The problem is that my BC.connect method shouldn't take a task-maker
argument, but internally it needs to create the various backing stores
(contents, node, commit and tag) and these currently require a
task-maker.

[ And ideally, it would make more sense to me if you only specified
the commit message when making a commit. The rest of the strings just
get thrown away, I think. ]


-- 
Dr Thomas Leonard        http://roscidus.com/blog/
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/mirageos-devel