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

View file

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

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
~some:Fun.id (Ptime.of_float_s (Int64.to_float (now ())))))
match t.committed, t.head with
| None, None ->
Lwt.return (Error (`Not_found key))
| Some _, _ ->
Lwt.return_ok
(Option.fold
~none:Ptime.epoch
~some:Fun.id (Ptime.of_float_s (Int64.to_float (now ()))))
| None, Some head ->
(* See https://github.com/ocaml/ocaml/issues/9301 why we have the
intermediate [r] value. *)
let+ r = Store.read_exn t.store head in
let[@warning "-8"] Commit c = r in
let author = Git_commit.author c in
let secs, tz_offset = author.Git.User.date in
let secs =
Option.fold ~none:secs
~some:(fun { Git.User.sign ; hours ; minutes } ->
let tz_off = Int64.(mul (add (mul (of_int hours) 60L) (of_int minutes)) 60L) in
match sign with
| `Plus -> Int64.(sub secs tz_off)
| `Minus -> Int64.(add secs tz_off))
tz_offset
in
let ts =
Option.fold
~none:Ptime.epoch
~some:Fun.id (Ptime.of_float_s (Int64.to_float secs))
in
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
Ok ts
let digest t key =
let open Lwt.Infix in