irmin: add pretty-printers to high-level Store that work with utop

[metanivek]

This PR is a follow up to #1836, with reduced scope to focus only on irmin and usage of built-in pretty-printing based on typereprs.

It adds pretty-printers for:

• Commit
• Branch
• Info
• Status
• Tree

An example output for a commit:

- : Store.commit =
{"key":"2703f0cf876e1859c79ae0cc28969810788eedb9","value":{"node":"db1d46bb4f8479641a796b638651485520b3ec01","info":{"date":1652198144,"author":"me","message":"update2"}}}

Closes #408

[metanivek]

@zshipko thanks for the feedback to re-evaluate the printer implementations. I'm using the typerepr ones now so should be resilient to future changes.

@Ngoguey42 based on the discussion from the previous PR, I decided to reduce scope to only impact irmin. I removed the pp values from the public signatures since it seems better to not put this burden on any existing implementations.

[Ngoguey42]

I tried the following and it didn't work. Did I do something wrong?

From your branch I launched dune utop and pasted this:

#require "digestif.ocaml";;
#require "checkseum.ocaml";;
#require "irmin";;
#require "irmin-unix";;
module Store = Irmin_unix.Git.FS.KV (Irmin.Contents.String);;

<abst> is still printed for a commit info:

[metanivek]

@Ngoguey42 I was surprised this didn't work but just confirmed the same behavior.

Based on the PR that added support for automatically installing printers, it appears to only work if the sig/attribute is present in a .mli file for the module. This was working for me because I created a local test package/module for testing that already had a store built, so when I run dune utop the printers would be installed from the store created in my test module. Interestingly, I confirmed that my file does not work if I start a plain utop session and use #use to load it.

It seems that the core problem may be that stores are created through functors, thus no module signatures are available at load time. I somewhat convinced myself of this by including a pre-built store in irmin-unix (the same one as yours), which worked as expected when used in utop.

I'm not sure how best to handle this, but I'll look into what can be done to make this work better with stores created directly in utop since that would be the best experience for people playing with the library.

[metanivek]

The best I have been able to come up with so far is to make a small package within irmin (locally named irmin.top) that exposes a way to install "known" printers.

For example:

module Store = Irmin_unix.Git.FS.KV (Irmin.Contents.String);;
Irmin_top.install_printers "Store";;

Interestingly, subsequent store modules created through similar makers will also pretty-print correctly (I assume because of how OCaml is able to see them as the same types).

What do you think of this approach?

[Ngoguey42]

That sounds promising. Could you share the code for this?

[metanivek]

@Ngoguey42 just pushed!

[Ngoguey42]

Thanks.

This is all black magic to me. @samoht would that be a problem to have an irmin subpackage that depends on compiler-libs.toplevel?

---

@metanivek It works now. However I'm noticing small inconsistencies in the things printed.

The following lines...

#require "digestif.ocaml";;
#require "checkseum.ocaml";;
#require "irmin-unix";;
module A = Irmin_unix.Git.FS.KV (Irmin.Contents.String);;
module B = Irmin_unix.Git.FS.KV (Irmin.Contents.String);;
module Maker = Irmin_pack_unix.Maker (Irmin_tezos.Conf);;
module C = Maker.Make (Irmin_tezos.Schema);;
Irmin_top.install_printers "A";;

A.Info.v 0L;;
B.Info.v 1L;;
C.Info.v 2L;;

...procude this output

─( 12:49:32 )─< command 11 >───────────────────────────────────────────────────────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # A.Info.v 0L;;
- : C.info = {"date":0,"author":"","message":""}
─( 12:49:32 )─< command 12 >───────────────────────────────────────────────────────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # B.Info.v 1L;;
- : C.info = {"date":1,"author":"","message":""}
─( 12:50:04 )─< command 13 >───────────────────────────────────────────────────────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # C.Info.v 2L;;
- : C.info = {"date":2,"author":"","message":""}

Namely,

• I only installed it for A and it works for all modules,
• I installed it for A and it prints C.

My opinion is that this is an acceptation situation and I think we can merge this patch as is, leaving room for future improvements.

[metanivek]

@Ngoguey42

The code/package is a modified version of core.top; I'm definitely learning more about OCaml through this little patch. 😄

My understanding for why it works for all your store modules is that the printers are matched based on type, and A.Info.t, B.Info.t, and C.Info.t are seen as all the same type. It is possible to build stores that do not match, but I'm not sure exactly how the type matching works. (I was able to build a store that did not match, using irmin-fs, I think)

It's surprising at first that it prints C.info, but I think that is just a side effect of how OCaml treats types that are the same. It's the same scenario as if you do something like type x = int;; and all subsequent ints are typed as x.

Does this patch deserve a mention in CHANGES.md?

[let-def]

The logic to autoload printers in utop (added here ocaml-community/utop#269) is a bit too limited to handle functors. I made a proof-of-concept fix here: https://github.com/let-def/autoprinter

opam pin add autoprinter https://github.com/let-def/autoprinter.git

This package should work well with Irmin printers, even if they are defined inside functors.
It is just a proof-of-concept, if it works well for you the best is probably to upstream the fix in utop (and maybe to the upstream top-level).

[metanivek]

@let-def thanks for the proof-of-concept and confirmation that the functors are the issue. I've verified it does work in utop-full with the annotated printers in this PR. As an aside, I did previously take a brief look at utop to see how hard it might be to update the functionality, but I was quickly out of my depth of understanding. With your code, and the linked issues/conversation, I might take another pass at trying to upstream a fix. Thanks!

@Ngoguey42 here's what I see outstanding for this PR before it is ready to merge:

1. Confirmation from @samoht that the irmin.top package is okay
2. Any documentation in CHANGES.md that is needed
3. Documentation (in README.md or perhaps a new/separate file) for how to use either the manual printer installation or autoprinter

Anything missing?

[codecov-commenter]

Codecov Report

Merging #1839 (42063c8) into main (e532ecf) will increase coverage by 0.01%.
The diff coverage is 50.00%.

❗ Current head 42063c8 differs from pull request most recent head ec6364e. Consider uploading reports for the commit ec6364e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #1839      +/-   ##
==========================================
+ Coverage   68.81%   68.82%   +0.01%     
==========================================
  Files         133      133              
  Lines       16208    16211       +3     
==========================================
+ Hits        11154    11158       +4     
+ Misses       5054     5053       -1     

Files Changed	Coverage Δ
src/irmin/store.ml	66.09% <50.00%> (-0.15%)	⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[samoht]

Thanks for your patches! I'm a bit sad to see so much magic in irmin.top. I know it's not your fault :-) But I don't think the install_printer <string> is really usable in practice (as module aliases will make it very confusing).

After discussing with @let-def it seems that the best UX for this feature would be to:

1. add annotations to the pp functions as you've already done (I'm also very tempted to accept your better pp printer as the JSON outputs are maybe not super great to read on the top-level? but it's not super important)
2. release autoprint (need to add support for 4.14, I'm happy to help - I'm also sure that @pitag-ha could also give nice advices :p)
3. create an empty irmin.top that only contains a dependency to autoprint so that #require "irmin.top" will prepare the necessary environment for the extensions to work.

What do you think?

[let-def]

For autoprint, I think that patching utop and ocaml toplevel is a more viable way (@nojb was looking at ways to add the feature upstream).

[metanivek]

I also think that the best path is to patch upstream(s) to handle the printers automatically. It's pretty far out from my current knowledge, but I'd love to see this happen so happy to help however I can. :)

I quickly tried a wrapper package locally -- unfortunately, it has the same caveats as autoprinter, so I think it is simpler to point directly to autoprinter until a patch can land. Once it does, that documentation can be removed, and everything will work magically (and as I naively expected at the beginning of this :).

So, here is my current list for getting this PR merge-ready:

1. Remove irmin.top
2. Add bullet to CHANGES.md (if necessary ?)
3. Add usage documentation to README.md for how to use autoprinter, for those that want pretty-printing. I could also see choosing to not document this and just letting them work automatically when a patch lands; I don't have a strong opinion.

This patch is (relatively) inconsequential, but I'm excited that it could result in improvements for the broader ecosystem (like this longstanding question about pretty-printing for functors). 🙌

---

As for the pp outputs from my previous PR: I like them too, but the JSON isn't so bad and has two benefits that I can see 1) much less code and 2) clearly showing the structure of the data (which is helpful if you are just getting started with the library). If we ever decide to properly format dates for git commits, we could revisit how the pps work overall.

[pitag-ha]

release autoprint (need to add support for 4.14, I'm happy to help - I'm also sure that @pitag-ha could also give nice advices :p)

Oh, can I? (I've just seen this) How is autoprint affected by the compiler's printer changes from 4.13 to 4.14?

Btw, TIL about the ocaml.toplevel_printer attributes. I'll update ppxlib to avoid it complaining about those attributes not being interpreted (not being interpreted by the ppxlib driver I mean).

[metanivek]

I opened a utop PR to attempt upstreaming @let-def's proof-of-concept ocaml-community/utop#378

[metanivek]

As of utop.2.11.0 the printer attribute works as expected for functors! Thanks @let-def for the initial proof of concept.

@samoht I think this small improvement is good to go!

[clecat]

LGTM, thx a lot for your work