last_modified: don't find_blob; find the commit! #10

Merged
reynir merged 2 commits from fix-last_modified into main 2024-12-20 12:11:32 +00:00
Showing only changes of commit e342f8539a - Show all commits

View file

@ -463,33 +463,35 @@ module Make (Pclock : Mirage_clock.PCLOCK) = struct
let last_modified t key = let last_modified t key =
let open Lwt.Infix in let open Lwt.Infix in
find_blob t key >>= match t.committed, t.head with
Option.fold | None, None ->
~none:(Lwt.return (Error (`Not_found key))) Lwt.return (Error (`Not_found key))
~some:(fun head -> | Some _, _ ->
Store.read_exn t.store head >|= function Lwt.return_ok
| Commit c -> (Option.fold
let author = Git_commit.author c in ~none:Ptime.epoch
let secs, tz_offset = author.Git.User.date in ~some:Fun.id (Ptime.of_float_s (Int64.to_float (now ()))))
let secs = | None, Some head ->
Option.fold ~none:secs Store.read_exn t.store head >|= function
~some:(fun { Git.User.sign ; hours ; minutes } -> | Commit c ->
let tz_off = Int64.(mul (add (mul (of_int hours) 60L) (of_int minutes)) 60L) in let author = Git_commit.author c in
match sign with let secs, tz_offset = author.Git.User.date in
| `Plus -> Int64.(sub secs tz_off) let secs =
| `Minus -> Int64.(add secs tz_off)) Option.fold ~none:secs
tz_offset ~some:(fun { Git.User.sign ; hours ; minutes } ->
in let tz_off = Int64.(mul (add (mul (of_int hours) 60L) (of_int minutes)) 60L) in
let ts = match sign with
Option.fold | `Plus -> Int64.(sub secs tz_off)
~none:Ptime.epoch | `Minus -> Int64.(add secs tz_off))
~some:Fun.id (Ptime.of_float_s (Int64.to_float secs)) tz_offset
in in
Ok ts let ts =
| _ -> Option.fold

We ended up here due to find_blob returning a Blob _ and not a Commit _ (how surprising given the name!)

We ended up here due to `find_blob` returning a `Blob _` and not a `Commit _` (how surprising given the name!)

should the same be applied to digest -- where !6 and !8 introduced the val commit since we ended up in https://github.com/robur-coop/git-kv/issues/6...

should the same be applied to `digest` -- where !6 and !8 introduced the `val commit` since we ended up in https://github.com/robur-coop/git-kv/issues/6...

Hm. To me it seems fine that digest returns the blob. As I understand it it's a digest that represents the file data. Maybe @dinosaure, who has a much better understanding of both git and ocaml-git, has an opinion?

Hm. To me it seems fine that `digest` returns the blob. As I understand it it's a digest that represents the file data. Maybe @dinosaure, who has a much better understanding of both git and ocaml-git, has an opinion?

What digest should return is a unique hash of the data. This can be achieved by the last commit sha1. This can as well be done by computing a hash of the content.

Here, it is nowadays computed by the commit if we're not in change_and_push, and by this Blob thingy if we're in change_and_push. Of course we can leave that as is -- but I think that having a divergence in what the digest is computed over depending on whether we're in change_and_push or not may lead to unexpected strange error situations.

But this is off-topic to the PR you propose here...

What `digest` should return is a unique hash of the data. This can be achieved by the last commit sha1. This can as well be done by computing a hash of the content. Here, it is nowadays computed by the commit if we're not in change_and_push, and by this Blob thingy if we're in change_and_push. Of course we can leave that as is -- but I think that having a divergence in what the digest is computed over depending on whether we're in change_and_push or not may lead to unexpected strange error situations. But this is off-topic to the PR you propose here...
Ok (Option.fold ~none:Ptime.epoch
~none:Ptime.epoch ~some:Fun.id (Ptime.of_float_s (Int64.to_float secs))
~some:Fun.id (Ptime.of_float_s (Int64.to_float (now ()))))) in
Ok ts
| _ -> assert false
hannes marked this conversation as resolved Outdated

this case shouldn't be needed

this case shouldn't be needed

What do you suggest instead? Mute partial match warnings?

let [@warning "-8"] (`Commit c) = Store.read_exn .. in
..
What do you suggest instead? Mute partial match warnings? ```OCaml let [@warning "-8"] (`Commit c) = Store.read_exn .. in .. ```

yes, as done in let pull

yes, as done in `let pull`

This is done in d2a0e526da. I found that there's no support for infix attributes on binding operators (.e.g let+) so I went with an intermediate binding.

https://github.com/ocaml/ocaml/issues/9301

This is done in d2a0e526dad259f71fc7f741129aba8e476c8f7a. I found that there's no support for infix attributes on binding operators (.e.g `let+`) so I went with an intermediate binding. https://github.com/ocaml/ocaml/issues/9301
let digest t key = let digest t key =
let open Lwt.Infix in let open Lwt.Infix in