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 d2a0e526da - Show all commits

View file

@ -472,26 +472,27 @@ module Make (Pclock : Mirage_clock.PCLOCK) = struct
~none:Ptime.epoch ~none:Ptime.epoch
~some:Fun.id (Ptime.of_float_s (Int64.to_float (now ())))) ~some:Fun.id (Ptime.of_float_s (Int64.to_float (now ()))))
| None, Some head -> | None, Some head ->
Store.read_exn t.store head >|= function (* See https://github.com/ocaml/ocaml/issues/9301 why we have the
| Commit c -> intermediate [r] value. *)
let author = Git_commit.author c in let+ r = Store.read_exn t.store head in
let secs, tz_offset = author.Git.User.date in let[@warning "-8"] Commit c = r in
let secs = let author = Git_commit.author c in
Option.fold ~none:secs let secs, tz_offset = author.Git.User.date in
~some:(fun { Git.User.sign ; hours ; minutes } -> let secs =
let tz_off = Int64.(mul (add (mul (of_int hours) 60L) (of_int minutes)) 60L) in Option.fold ~none:secs
match sign with ~some:(fun { Git.User.sign ; hours ; minutes } ->
| `Plus -> Int64.(sub secs tz_off) let tz_off = Int64.(mul (add (mul (of_int hours) 60L) (of_int minutes)) 60L) in
| `Minus -> Int64.(add secs tz_off)) match sign with
tz_offset | `Plus -> Int64.(sub secs tz_off)
in | `Minus -> Int64.(add secs tz_off))
let ts = tz_offset
Option.fold in

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...
~none:Ptime.epoch let ts =
~some:Fun.id (Ptime.of_float_s (Int64.to_float secs)) Option.fold
in ~none:Ptime.epoch
Ok ts ~some:Fun.id (Ptime.of_float_s (Int64.to_float secs))
| _ -> assert false 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 digest t key =
let open Lwt.Infix in let open Lwt.Infix in