Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split the unix specific part out of irmin-pack #1783

Merged
merged 4 commits into from Feb 25, 2022
Merged

Split the unix specific part out of irmin-pack #1783

merged 4 commits into from Feb 25, 2022

Conversation

hhugo
Copy link

@hhugo hhugo commented Feb 24, 2022 •
edited

Fix #1772

Ngoguey42
Ngoguey42 reviewed Feb 24, 2022
View reviewed changes
Copy link
Contributor

@Ngoguey42 Ngoguey42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good

src/irmin-pack/unix/import.ml Outdated
Comment on lines 17 to 32
include Irmin.Export_for_backends

let src = Logs.Src.create "irmin.pack" ~doc:"irmin-pack backend"

module Log = (val Logs.src_log src : Logs.LOG)

module Int63 = struct
include Optint.Int63

let t = Irmin.Type.int63
end

type int63 = Int63.t [@@deriving irmin]

let ( ++ ) = Int63.add
let ( -- ) = Int63.sub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
include Irmin.Export_for_backends
let src = Logs.Src.create "irmin.pack" ~doc:"irmin-pack backend"
module Log = (val Logs.src_log src : Logs.LOG)
module Int63 = struct
include Optint.Int63
let t = Irmin.Type.int63
end
type int63 = Int63.t [@@deriving irmin]
let ( ++ ) = Int63.add
let ( -- ) = Int63.sub
include Irmin_pack.Import

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import is not exported in irmin-pack

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine to define twice Logs.Src.create "irmin.pack" ~doc:"irmin-pack backend"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, but it probably make sense to change the name. I've pushed a change

hhugo
hhugo commented Feb 24, 2022
View reviewed changes
src/irmin-pack/unix/inode.ml Outdated Show resolved Hide resolved
@hhugo
Copy link
Author

hhugo commented Feb 24, 2022

I've fixed the rest of the codebase.
dune runtest works on my machine

@codecov-commenter
Copy link

Codecov Report

Merging #1783 (0f9595a) into main (640d609) will decrease coverage by 0.02%.
The diff coverage is 84.41%.

❗ Current head 0f9595a differs from pull request most recent head 36e3eb7. Consider uploading reports for the commit 36e3eb7 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1783      +/-   ##
==========================================
- Coverage   64.70%   64.68%   -0.03%     
==========================================
  Files         111      115       +4     
  Lines       14060    14063       +3     
==========================================
- Hits         9098     9097       -1     
- Misses       4962     4966       +4
Impacted Files Coverage Δ
src/irmin-pack/IO.ml 92.59% <ø> (+9.12%) ⬆️
src/irmin-pack/atomic_write.ml 75.86% <ø> (-8.88%) ⬇️
src/irmin-pack/inode.ml 80.59% <ø> (ø)
src/irmin-pack/pack_store.ml 45.45% <ø> (-37.81%) ⬇️
src/irmin-pack/unix/checks.ml 25.09% <ø> (ø)
src/irmin-pack/unix/ext.ml 59.09% <ø> (ø)
src/irmin-pack/unix/pack_dict.ml 100.00% <ø> (ø)
src/irmin-pack/unix/pack_index.ml 88.88% <ø> (ø)
src/irmin-pack/unix/traverse_pack_file.ml 49.26% <0.00%> (ø)
src/irmin-pack/unix/utils.ml 85.71% <ø> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 640d609...36e3eb7. Read the comment docs.

@Ngoguey42 Ngoguey42 mentioned this pull request Feb 25, 2022
Merged
craigfe
craigfe approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@craigfe craigfe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@craigfe craigfe merged commit 46f1396 into mirage:main Feb 25, 2022
@Ngoguey42 Ngoguey42 mentioned this pull request Feb 25, 2022
Closed
craigfe added a commit to craigfe/opam-repository that referenced this pull request Feb 25, 2022
@craigfe
[new release] ppx_irmin, libirmin, irmin, irmin-unix, irmin-tezos, ir…
f4a7712 
…min-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-chunk and irmin-bench (3.1.0)

CHANGES:

### Fixed

- **irmin-pack**
  - Drop unnecessary runtime dependency on `ppx_irmin`. (mirage/irmin#1782, @hhugo)
  - Split the unix part of irmin-pack into irmin-pack.unix (mirage/irmin#1783, @hhugo)

- **irmin-unix**
  - Fix conflicting command line arguments for `push`, `pull`, `fetch` and
    `clone` (mirage/irmin#1776, @zshipko)
  - Fix issues with Sync functions by provided a better default `Mimic.ctx`. A
    side-effect of this update is that the `remote` function now returns an Lwt
    promise. (mirage/irmin#1778, @zshipko)

### Added

- **libirmin**
  - Create `libirmin` package providing a C interface to the irmin API
    (mirage/irmin#1713, @zshipko)

### Changed

- **irmin-bench**
  - Make trace replay API public and simpler (mirage/irmin#1781, @Ngoguey42)
@craigfe craigfe mentioned this pull request Feb 25, 2022
Merged
@hhugo hhugo deleted the irmin-pack-unix branch February 25, 2022 16:47
Ngoguey42 added a commit to Ngoguey42/irmin that referenced this pull request May 3, 2022
@tomjridge @Ngoguey42
irmin-pack: Move some signatures to irmin-pack.unix
adabda3 
The mirage#1783 PR introduced `irmin-pack.unix` by moving code out of
`irmin-pack`. This commit goes further by moving more code.

Partially extracted from mirage#1832

Co-authored-by: Nicolas Goguey <ngoguey42@free.fr>
Co-authored-by: Tom Ridge <tom.j.ridge@googlemail.com>
@Ngoguey42 Ngoguey42 mentioned this pull request May 3, 2022
Merged
Ngoguey42 added a commit to Ngoguey42/irmin that referenced this pull request May 4, 2022
@tomjridge @Ngoguey42
irmin-pack: Move some signatures to irmin-pack.unix
21f3bba 
The mirage#1783 PR introduced `irmin-pack.unix` by moving code out of
`irmin-pack`. This commit goes further by moving more code.

Partially extracted from mirage#1832

Co-authored-by: Nicolas Goguey <ngoguey42@free.fr>
Co-authored-by: Tom Ridge <tom.j.ridge@googlemail.com>
icristescu pushed a commit to icristescu/irmin that referenced this pull request May 23, 2022
@tomjridge @Ngoguey42
irmin-pack: Move some signatures to irmin-pack.unix
f30f2f3 
The mirage#1783 PR introduced `irmin-pack.unix` by moving code out of
`irmin-pack`. This commit goes further by moving more code.

Partially extracted from mirage#1832

Co-authored-by: Nicolas Goguey <ngoguey42@free.fr>
Co-authored-by: Tom Ridge <tom.j.ridge@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ngoguey42 Ngoguey42 Ngoguey42 left review comments

@craigfe craigfe craigfe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problematic dependencies to target jsoo
4 participants
@hhugo @codecov-commenter @Ngoguey42 @craigfe