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
hannes commented 2024-10-28 20:58:15 +00:00
Owner
Copy link

best applied after #2 -- then the t.committed is the indication whether we're in a change_and_push or not.

best applied after #2 -- then the `t.committed` is the indication whether we're in a `change_and_push` or not.
hannes added 1 commit 2024-10-28 20:58:15 +00:00
reynir reviewed 2024-10-29 09:20:26 +00:00
src/git_kv.ml Outdated
@ -717,0 +719,4 @@
| Some _ -> op t
| None ->
change_and_push t op >>! function
| Ok a -> Lwt.return a
reynir commented 2024-10-29 09:20:26 +00:00
Owner
Copy link

should this be | Ok _ as a -> Lwt.return a?

should this be `| Ok _ as a -> Lwt.return a`?
hannes commented 2024-10-29 09:34:54 +00:00
Author
Owner
Copy link

no, we need to unpack the inner result -- which is a (unit, _) result.

no, we need to unpack the inner result -- which is a (unit, _) result.
reynir marked this conversation as resolved
reynir reviewed 2024-10-29 09:30:56 +00:00
src/git_kv.ml Outdated
@ -649,4 +649,2 @@
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. *)
reynir commented 2024-10-29 09:21:51 +00:00
Owner
Copy link

Should we keep this comment?

Should we keep this comment?
hannes commented 2024-10-29 11:03:40 +00:00
Author
Owner
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
Owner
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
reynir marked this conversation as resolved
src/git_kv.ml Outdated
@ -717,0 +716,4 @@
set t dest contents
in
match t.committed with
| Some _ -> op t
reynir commented 2024-10-29 09:24:31 +00:00
Owner
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
Author
Owner
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
Author
Owner
Copy link

added a comment in 708cd7d

added a comment in 708cd7d
reynir commented 2024-10-29 11:28:57 +00:00
Owner
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
Author
Owner
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
Owner
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") ```
reynir marked this conversation as resolved
hannes force-pushed rename-single-commit from eab5511ab7 to 708cd7d008 2024-10-29 11:25:21 +00:00 Compare
hannes commented 2024-10-29 11:25:27 +00:00
Author
Owner
Copy link

rebased on main

rebased on main
reynir approved these changes 2024-10-29 11:32:55 +00:00
hannes merged commit e0b23b9458 into main 2024-10-29 11:37:58 +00:00
hannes deleted branch rename-single-commit 2024-10-29 11:37:59 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
reynir
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: robur/git-kv#3
No description provided.
Delete branch "rename-single-commit"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?