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.

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

irmin-git: fix decoding contents in the middle of a buffer #2277

Merged
merged 4 commits into from
Oct 5, 2023

Conversation

metanivek
Copy link
Member

This code previously assumed that it could read to the end of the buffer, but this is not always the case. This issue was discovered while testing the new batch API in irmin-client.

This PR updates irmin-git to use ocaml-git 3.14.0 or newer. This release of ocaml-git fixes the discovered bug and also exposes a function (length_with_header) that can be used to know the length of buffer read. I also updated the client/server example to use an in-memory git store, which can be used to manually verify this fix.

@metanivek metanivek requested review from art-w and clecat October 4, 2023 20:54
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2023

Codecov Report

Merging #2277 (08c4142) into main (2e7a6a2) will increase coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 08c4142 differs from pull request most recent head 39d14fe. Consider uploading reports for the commit 39d14fe to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main    #2277      +/-   ##
==========================================
+ Coverage   68.12%   68.14%   +0.01%     
==========================================
  Files         133      133              
  Lines       15982    15983       +1     
==========================================
+ Hits        10888    10891       +3     
+ Misses       5094     5092       -2     
Files Coverage Δ
src/irmin-git/contents.ml 45.00% <0.00%> (-2.37%) ⬇️

... and 3 files with indirect coverage changes

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

src/irmin-git/contents.ml Outdated Show resolved Hide resolved
@art-w
Copy link
Contributor

art-w commented Oct 5, 2023

Thanks, it looks good! (I'm guessing the CI doesn't yet see the new release of ocaml-git ... Hm but actually the commit used for the opam-repository is recent enough, so there might be a conflict in our dependencies Nevermind, the dependency analysis went through but now it seems stuck elsewhere)

@metanivek metanivek force-pushed the irmin-git/fix-decoding branch from 8be0141 to 39d14fe Compare October 5, 2023 14:22
@art-w art-w merged commit fddb6f0 into mirage:main Oct 5, 2023
4 checks passed
art-w added a commit to art-w/opam-repository that referenced this pull request Oct 9, 2023
CHANGES:

### Added

- **irmin-server**
  - Added `irmin-server` package (mirage/irmin#2031, @zshipko)
- **irmin-client**
  - Added `irmin-client` package to connect to `irmin-server` instances (mirage/irmin#2031,
    @zshipko)
- **irmin**
  - Add pretty printers for `Commit`, `Tree`, `Info`, `Status`, `Branch` when
    using `utop` (@metanivek, mirage/irmin#1839)

### Fixed

- **irmin-pack**
  - Fix index integrity check for v3 stores (mirage/irmin#2267, @metanivek)

### Removed

- **irmin-http**
  - Removed `irmin-http` since it is not compatible with generic keys.
    `irmin-grapqhl` or `irmin-server` should be used instead. (mirage/irmin#1902, @zshipko)
- **irmin**
  - Removed stream proofs. We now only have Merkle tree proofs. This simplifies
    the maintenance of that part of the code, as ensuring the correct order of
    cached IO operations was tricky for stream proofs (mirage/irmin#2275, @samoht)

### Changed

- **irmin-git**
  - Moved lower bounds to `git.3.14.0` to use new function (mirage/irmin#2277, @metanivek)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

### Added

- **irmin-server**
  - Added `irmin-server` package (mirage/irmin#2031, @zshipko)
- **irmin-client**
  - Added `irmin-client` package to connect to `irmin-server` instances (mirage/irmin#2031,
    @zshipko)
- **irmin**
  - Add pretty printers for `Commit`, `Tree`, `Info`, `Status`, `Branch` when
    using `utop` (@metanivek, mirage/irmin#1839)

### Fixed

- **irmin-pack**
  - Fix index integrity check for v3 stores (mirage/irmin#2267, @metanivek)

### Removed

- **irmin-http**
  - Removed `irmin-http` since it is not compatible with generic keys.
    `irmin-grapqhl` or `irmin-server` should be used instead. (mirage/irmin#1902, @zshipko)
- **irmin**
  - Removed stream proofs. We now only have Merkle tree proofs. This simplifies
    the maintenance of that part of the code, as ensuring the correct order of
    cached IO operations was tricky for stream proofs (mirage/irmin#2275, @samoht)

### Changed

- **irmin-git**
  - Moved lower bounds to `git.3.14.0` to use new function (mirage/irmin#2277, @metanivek)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants