Re: [MirageOS-devel] Irmin API evolution

[Thomas Leonard <talex5@xxxxxxxxx>]

On 1 September 2015 at 18:51, Thomas Gazagnaire <thomas@xxxxxxxxxxxxxx> wrote:
>> 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
>
> I've added some comments to that issue. I'm mainly in favor of the change.

To answer them here:

> why do you need to remove the create function from AO? the BC store can just 
> redefine the function.
> it is not totally clear by reading the API why you gain by doing the change. 
> I understand the rational (caching repository connection) but I think we 
> should find a way to expose it a bit better. Maybe going further and having 
> separate Repo and Branch modules would make sense. Would make sense to rename 
> Tag as the same time :-)

"create" was originally in RO, so redefining it in BC would mean that
BC wasn't a store.

As you say, we might not want BC to be a store anyway. We could have
BC represent a repository, with Branch and Commit submodules. I'm
happy to do that, but was just aiming to minimise changes in these
patches.

However, I think we should remove "create" from the store interfaces
anyway. In Irmin-IndexedDB, I want to pass database connections to the
internal RW and AO stores, not a config. It looks like I can attach
arbitrary data to config objects, but doing this loses compile-time
type checking (it would be possible to call it without providing a
connection).

As another example, in the filesystem backend we really want a path
but have to cope if it's missing:

  let get_path config =
    match Irmin.Private.Conf.get config root_key with
    | None   -> IO.getcwd ()
    | Some p -> Lwt.return p

This is ugly (and maybe dangerous), and requires the caller to
implement getcwd just for this case.

In Mirage, this would be more awkward, because you'd want to pass the
FS.t somehow and there's no useful default.

More generally, I just dislike having constructors in interfaces.
Constructor arguments tend to be specific to a particular
implementation.

>> 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.
>
> The trick to solve that problem by putting the connection in the closure. See 
> https://github.com/mirage/irmin/blob/master/lib/git/irmin_git.ml#L157 for 
> instance when the Git handler is created outside of the closure which is 
> returned to the user, so this is shared on every invocation of `t`.

Currently, I have:

      let connect config [task] =
        Contents.create config task >>= fun contents ->
        Node.create config task     >>= fun node ->
        Commit.create config task   >>= fun commit ->
        Tag.create config task      >>= fun tag ->
        Lwt.return {config; contents; node; commit; tag}

It's not clear how to modify this so that e.g. Contents.create is
called when just the "config" argument is passed.

If we remove "create" from the interface then I think the whole
problem goes away, because none of these stores use the task argument
for anything anyway and we could just remove it.

>> [ 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. ]
>
> The initial idea was to use that task to (i) populate an audit log on all the 
> database operations (including reads) and (ii) attach the debug messages to 
> the task, instead of throwing them on the error channel. None of these have 
> been completed yet, but would be nice if they are still possible to do later.

These things don't sound very Irmin-specific, or connected to the Git
log message. Perhaps we could provide a wrapper for logging?

module AuditedStore (S : BC.STORE) : sig
  include BC.STORE
  val create : Logger.t -> S.t -> t
  ...
end


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