robur/git-kv
4
0
Fork 0
Code Issues Pull requests 1 Projects Releases Packages Wiki Activity Actions

only use a single commit in rename #3

Merged
hannes merged 2 commits from rename-single-commit into main 2024-10-29 11:37:58 +00:00
Conversation 1 Commits 2 Files changed 1 +16 -7
1 changed files with 16 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
src/git_kv.ml
View file

@ -652,13 +652,6 @@ module Make (Pclock : Mirage_clock.PCLOCK) = struct
let open Lwt.Infix in
reynir marked this conversation as resolved Outdated
reynir commented 2024-10-29 09:21:51 +00:00
Outdated
Review
Copy link

Should we keep this comment?

Should we keep this comment?
hannes commented 2024-10-29 11:03:40 +00:00
Outdated
Review
Copy link

we can. I'm not sure what we should do with the comment, to be honest.

we can. I'm not sure what we should do with the comment, to be honest.
reynir commented 2024-10-29 11:32:46 +00:00
Review
Copy link

I opened an issue on GitHub instead: https://github.com/robur-coop/git-kv/issues/4

I opened an issue on GitHub instead: https://github.com/robur-coop/git-kv/issues/4
remove ?and_commit:t.committed t key >|= Result.map_error to_write_error
let rename t ~source ~dest =
(* TODO(dinosaure): optimize it! It was done on the naive way. *)
let open Lwt_result.Infix in
get t source >>= fun contents ->
remove t source >>= fun () ->
set t dest contents
let allocate t key ?last_modified:_ size =
let open Lwt.Infix in
exists t key >>= function
@ -709,4 +702,20 @@ module Make (Pclock : Mirage_clock.PCLOCK) = struct
Lwt.return_ok res)
>|= Result.map_error
(fun err -> `Msg (Fmt.str "error pushing %a" Store.pp_error err)))
let rename t ~source ~dest =
let open Lwt_result.Infix in
let op t =
get t source >>= fun contents ->
remove t source >>= fun () ->
set t dest contents
in
(* (hannes) we check whether we're in a change_and_push or not, since
nested change_and_push are not supported. *)
match t.committed with
| Some _ -> op t
| None ->
change_and_push t op >>! function
| Ok a -> Lwt.return a
reynir marked this conversation as resolved Outdated
reynir commented 2024-10-29 09:24:31 +00:00
Outdated
Review
Copy link

I thought "why not unconditionally do change_and_push?" and the answer is we disallow nested change_and_push and this is how we check if we're inside a change_and_push.

Now I am curious if we don't have any yield points inside op?

I thought "why not unconditionally do `change_and_push`?" and the answer is we disallow nested `change_and_push` and this is how we check if we're inside a `change_and_push`. Now I am curious if we don't have any yield points inside `op`?
hannes commented 2024-10-29 11:04:57 +00:00
Outdated
Review
Copy link

Likely deserves a comment about why we check committed.

I may be slow - what do you mean by "don't have any yield points inside op"? We do have various yield points in there...

Likely deserves a comment about why we check committed. I may be slow - what do you mean by "don't have any yield points inside `op`"? We do have various yield points in there...
hannes commented 2024-10-29 11:25:49 +00:00
Outdated
Review
Copy link

added a comment in 708cd7d

added a comment in 708cd7d
reynir commented 2024-10-29 11:28:57 +00:00
Outdated
Review
Copy link

So my worry was that we could have an unfortunate task interleaving where rename just read /foo then another task (also inside change_and_push) sets /foo to something else and then the first task continues with remove & set. This was a problem before and this is an improvement.

So my worry was that we could have an unfortunate task interleaving where `rename` just read `/foo` then another task (also inside `change_and_push`) sets `/foo` to something else and then the first task continues with remove & set. This was a problem before and this is an improvement.
hannes commented 2024-10-29 11:33:35 +00:00
Outdated
Review
Copy link

that scenario shouldn't appear anymore, since we're always in a change_and_push now.

that scenario shouldn't appear anymore, since we're always in a change_and_push now.
reynir commented 2024-10-29 11:42:10 +00:00
Outdated
Review
Copy link

Well, it can appear if both tasks are within the change_and_push:

let* () = Store.set fs (Key.v "/foo") "value" in
Store.change_and_push fs @@ fun fs ->
Lwt.async (fun () -> Store.set fs (Key.v "/foo") "new value" |> Lwt.map ignore);
Store.rename fs ~source:(Key.v "/foo") ~dest:(Key.v "/bar")
Well, it can appear if both tasks are within the `change_and_push`: ```OCaml let* () = Store.set fs (Key.v "/foo") "value" in Store.change_and_push fs @@ fun fs -> Lwt.async (fun () -> Store.set fs (Key.v "/foo") "new value" |> Lwt.map ignore); Store.rename fs ~source:(Key.v "/foo") ~dest:(Key.v "/bar") ```
| Error _ as e -> Lwt.return e
end