-
Notifications
You must be signed in to change notification settings - Fork 70
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
Fix test-and-set for packed references #291
Conversation
See moby/datakit#610 for details
/cc @talex5 |
src/git/fs.ml
Outdated
@@ -635,7 +642,13 @@ module Make (IO: IO) (D: Hash.DIGEST) (I: Inflate.S) = struct | |||
| Some v -> Some (Cstruct.of_string (Hash.to_hex v)) | |||
in | |||
let lock = lock_file t r in | |||
IO.test_and_set_file file ~temp_dir ~lock ~test:(raw test) ~set:(raw set) | |||
let is_read_packed_ref = Reference.Set.mem r t.read_packed_refs in | |||
let test = if is_read_packed_ref then None else raw test in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that when updating a packed ref, you can specify any value for test
and it will update? I guess if this code is going away soon it probably doesn't matter, but might be worth a warning comment in the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed now.
As ocaml-git 1.11 never unpack the packed references (instead it just writes loose references when it wants to update them), test_and_set_reference needs to be adapted. Note: remove_reference and test_and_set ~set:None are completely buggy when using packed references. This will be fixed by ocaml-git 2.0 but there is no easy fix in ocaml-git 1.11... So simply add a note in API docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes: - git: make `Git.FS.test_and_set_references` work better with packed references (mirage/ocaml-git#291, @samoht. Fixes moby/datakit#633) - git-unix: ignore trailing newlines when doing 'test and set' on reference files (backport of moby/datakit#611, @samoht. Fixes moby/datakit#610) - git-mirage: fix linking of tests (@samoht)
As ocaml-git 1.11 never unpack the packed references (instead it just writes
loose references when it wants to update them), test_and_set_reference needs
to be adapted.
Note: remove_reference and test_and_set ~set:None are completely buggy when
using packed references. This will be fixed by ocaml-git 2.0 but there is
no easy fix in ocaml-git 1.11... So simply add a note in API docs.