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

irmin-pack: move sigs to unix #2081

Merged
merged 1 commit into from Sep 9, 2022
Merged

irmin-pack: move sigs to unix #2081

merged 1 commit into from Sep 9, 2022

Conversation

icristescu
Copy link
Contributor

The signatures in irmin-pack/s.ml were common to both the in memory and on disk stores, but are actually sigs only for the on disk store (their implementation in irmin_pack_mem were empty). This PR moves most of the s.ml to the unix sub package.

The downside is the duplication of the Maker functor, since it has different S types for the in-memory and on disk stores.

Part of #2039.

@icristescu icristescu force-pushed the refactor branch 3 times, most recently from 066c8f3 to b0fd9ad Compare September 9, 2022 12:59
Ngoguey42
Ngoguey42 reviewed Sep 9, 2022
View reviewed changes
src/irmin-pack/unix/s.ml Outdated
_ t ->
(unit, [ `Wrong_hash | `Absent_value ]) result
end

(** [Irmin-pack]-specific extensions to the [Store] module type. *)
module type Specifics = sig
Copy link
Contributor

Choose a reason for hiding this comment

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

The Specifics level of abstraction exists solely for irmin_pack-unix. Have you tried to remove it and inline it's content in module type S below?

Copy link
Member

Choose a reason for hiding this comment

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

+1 to this idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks

metanivek
metanivek approved these changes Sep 9, 2022
View reviewed changes
Copy link
Member

@metanivek metanivek left a comment

Choose a reason for hiding this comment

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

This looks great (particularly excited about the deletions in the memory implementation -- no more useless GC!). Thanks for cleaning up. 🧹

@metanivek metanivek merged commit 0157dd8 into mirage:main Sep 9, 2022
@metanivek metanivek mentioned this pull request Sep 12, 2022
Merged
@icristescu icristescu mentioned this pull request Sep 26, 2022
Open
@metanivek metanivek mentioned this pull request Oct 6, 2022
Merged
metanivek added a commit to metanivek/opam-repository that referenced this pull request Oct 6, 2022
@metanivek
[new release] ppx_irmin, libirmin, irmin, irmin-tezos, irmin-tezos-ut…
029f725 
…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2)

CHANGES:

### Added

- **irmin**
  - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris)

- **irmin-graphql**
  -  Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris)
  -  Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris)

- **irmin-pack**
  - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w)

- **irmin-tezos-utils**
  - Add package `irmin-tezos-utils` containing a graphical tool for manual pack
    files analysis. (mirage/irmin#1939, @clecat)

### Changed

- **irmin-pack**
  - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081,
  @icristescu)
  - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack`
    to `irmin-pack.unix` (mirage/irmin#2084, @metanivek)
  - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w)

### Fixed

- **irmin-pack**
  - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42)
  - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106,
    @icristescu)

### Fixed

- **irmin-pack**
  - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with
    gced stores. (mirage/irmin#1919, @icristescu)
metanivek added a commit to metanivek/opam-repository that referenced this pull request Oct 6, 2022
@metanivek
[new release] ppx_irmin, libirmin, irmin, irmin-tezos, irmin-tezos-ut…
ff7795a 
…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2)

CHANGES:

### Added

- **irmin**
  - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris)

- **irmin-graphql**
  -  Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris)
  -  Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris)

- **irmin-pack**
  - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w)

- **irmin-tezos-utils**
  - Add package `irmin-tezos-utils` containing a graphical tool for manual pack
    files analysis. (mirage/irmin#1939, @clecat)

### Changed

- **irmin-pack**
  - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081,
  @icristescu)
  - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack`
    to `irmin-pack.unix` (mirage/irmin#2084, @metanivek)
  - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w)

### Fixed

- **irmin-pack**
  - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42)
  - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106,
    @icristescu)
  - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with
    gced stores. (mirage/irmin#1919, @icristescu)
metanivek added a commit to metanivek/opam-repository that referenced this pull request Oct 6, 2022
@metanivek
[new release] ppx_irmin, libirmin, irmin, irmin-tezos, irmin-tezos-ut…
fe1a48a 
…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2)

CHANGES:

### Added

- **irmin**
  - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris)

- **irmin-graphql**
  -  Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris)
  -  Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris)

- **irmin-pack**
  - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w)

- **irmin-tezos-utils**
  - Add package `irmin-tezos-utils` containing a graphical tool for manual pack
    files analysis. (mirage/irmin#1939, @clecat)

### Changed

- **irmin-pack**
  - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081,
  @icristescu)
  - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack`
    to `irmin-pack.unix` (mirage/irmin#2084, @metanivek)
  - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w)

### Fixed

- **irmin-pack**
  - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42)
  - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106,
    @icristescu)
  - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with
    gced stores. (mirage/irmin#1919, @icristescu)
icristescu pushed a commit to metanivek/opam-repository that referenced this pull request Oct 7, 2022
[new release] ppx_irmin, libirmin, irmin, irmin-tezos, irmin-tezos-ut…
8d6af4e 
…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2)

CHANGES:

### Added

- **irmin**
  - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris)

- **irmin-graphql**
  -  Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris)
  -  Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris)

- **irmin-pack**
  - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w)

- **irmin-tezos-utils**
  - Add package `irmin-tezos-utils` containing a graphical tool for manual pack
    files analysis. (mirage/irmin#1939, @clecat)

### Changed

- **irmin-pack**
  - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081,
  @icristescu)
  - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack`
    to `irmin-pack.unix` (mirage/irmin#2084, @metanivek)
  - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w)

### Fixed

- **irmin-pack**
  - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42)
  - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106,
    @icristescu)
  - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with
    gced stores. (mirage/irmin#1919, @icristescu)
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

@metanivek metanivek metanivek approved these changes

@clecat clecat clecat 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.

None yet
4 participants
@icristescu @Ngoguey42 @clecat @metanivek