Iter on node and commit graphs #1077
Conversation
| nodes and bounded by the [min] nodes, as specified by | ||
| {{!Irmin__.S.NODE_GRAPH.iter} GRAPH.iter}. *) | ||
|
|
||
| val iter_commits : |
There was a problem hiding this comment.
I have a feeling that this function is also going to iterate over the nodes and the contents referenced by the closure of commits given, it's just not going to do anything when it visits them; is that right?
There was a problem hiding this comment.
Yes, it just ignores them. So we can add ?node:(node -> unit Lwt.t) and ?contents:(contents * metadata -> unit Lwt.t). I didn't do it because I don't need it in the layered store, but maybe we should add them for future usage?
There was a problem hiding this comment.
Not having iterators for node and contents seems fine. I was mostly concerned about performance: iterating over all objects in the closure when you're only making progress for the commits feels like it might be quite inefficient.
There was a problem hiding this comment.
Good point, it actually only iterates over commits, as the predecessors of a commit are its parents. Unlike for nodes where predecessors can be both nodes and contents. So I was wrong above, we cannot iterate over nodes and contents with this function.
There was a problem hiding this comment.
Hmm... indeed. This iter function never seems to behave the way I expect it to 😛
Can you add something like `Node _ | `Contents _ -> assert false in the implementation, just to make that clear?
There was a problem hiding this comment.
Nice suggestions, thanks!
icristescu
force-pushed
the
iter
branch
2 times, most recently
from
078b7a1 to
52d4551
Compare
The two
iterfunctions are used in the layered store, to iterate over the commi graph and the node graph. Having two separate functions allows us to choose in which order we iterate over the commit graph, whereas for the node graph the order is fixed: we have to iterate in reverse order to keep the compression of inodes.