irmin-pack: extract GC logic from ext #2063
Conversation
| let flush_and_raise () = Ao.flush new_suffix |> Errs.raise_if_error in | ||
| let* () = | ||
| Errs.catch (fun () -> | ||
| Worker.transfer_append_exn ~read_exn ~append_exn ~off:copy_end_offset |
There was a problem hiding this comment.
I think this might be a code smell for how we are reusing worker code but no longer in the context of the worker. I kept it as-is for now, but we might want to revisit in the future.
There was a problem hiding this comment.
I think that the answer to this concern and @icristescu's about the duplication of Args is to create a third file (gc_utils?) that would contain these
Codecov Report
@@ Coverage Diff @@
## main #2063 +/- ##
==========================================
- Coverage 64.34% 64.32% -0.03%
==========================================
Files 131 133 +2
Lines 15584 15580 -4
==========================================
- Hits 10028 10022 -6
- Misses 5556 5558 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/irmin-pack/unix/gc_worker.ml
Outdated
|
|
||
| exception Pack_error = Errors.Pack_error | ||
|
|
||
| module type Args = sig |
There was a problem hiding this comment.
if you include gc_intf here, you can avoid redefining this module type, I think. maybe even move module type S (renamed) to gc_intf. For me it make sense to have gc_intf shared between gc.ml and gc_worker, but if you disagree, you can leave it as is too :)
There was a problem hiding this comment.
I'll lump my response to @Ngoguey42's comment in with this.
Coming back to this with fresh eyes today, I think we ought to bring the worker functionality into gc.ml (as a private Worker module). The worker has no use outside of the gc.ml context, and all these little design tensions tell me that it would fit best as one module (and avoid adding another third module for sharing purposes), at least for now.
in preparation for extracting to its own file
|
Okay, I pushed the change that does not create a new @icristescu @Ngoguey42 can I get a final review? |
|
thanks LGTM! |
This PR is a part of #2039. It moves the core GC logic out of
ext.mland into its own set of files,gc.mlandgc_worker.ml. It also does a few cleanups in this area of the code. I recommend reviewing commit-by-commit to see the changes more easily.