robur/git-kv
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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
Conversation 0 Commits 2 Files changed 1 +30 -27
1 changed files with 30 additions and 27 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
src/git_kv.ml
View file

@ -463,12 +463,19 @@ 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 ->
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 =
@ -486,10 +493,6 @@ module Make (Pclock : Mirage_clock.PCLOCK) = struct
~some:Fun.id (Ptime.of_float_s (Int64.to_float secs))
in
hannes marked this conversation as resolved Outdated
hannes commented 2024-12-18 15:57:36 +00:00
Outdated
Review
Copy link

this case shouldn't be needed

this case shouldn't be needed
reynir commented 2024-12-19 13:36:46 +00:00
Outdated
Review
Copy link

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 .. ```
hannes commented 2024-12-19 13:46:27 +00:00
Outdated
Review
Copy link

yes, as done in let pull

yes, as done in `let pull`
👍 1
reynir commented 2024-12-19 14:13:35 +00:00
Outdated
Review
Copy link

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
| _ ->
reynir commented 2024-12-18 15:25:29 +00:00
Outdated
Review
Copy link

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!)
hannes commented 2024-12-18 15:58:54 +00:00
Outdated
Review
Copy link

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...
reynir commented 2024-12-19 13:33:13 +00:00
Outdated
Review
Copy link

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?
hannes commented 2024-12-19 13:49:58 +00:00
Outdated
Review
Copy link

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 ())))))
let digest t key =
let open Lwt.Infix in