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
reynir commented 2024-12-18 15:24:38 +00:00
Owner
Copy link

To get the last_modified timestamp we need the commit object not the blob. This is closer to the old behavior, and will return a correct timestamp on an existing file outside change_and_push like before.

To get the last_modified timestamp we need the commit object not the blob. This is closer to the old behavior, and will return a correct timestamp on an existing file outside change_and_push like before.
reynir added 1 commit 2024-12-18 15:24:38 +00:00 
To get the last_modified timestamp we need the commit object not the
blob. This is closer to the old behavior.
reynir reviewed 2024-12-18 15:25:29 +00:00
src/git_kv.ml Outdated
@ -486,4 +466,0 @@
~some:Fun.id (Ptime.of_float_s (Int64.to_float secs))
in
Ok ts
| _ ->
reynir commented 2024-12-18 15:25:29 +00:00
Author
Owner
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
Owner
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
Author
Owner
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
Owner
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...
hannes reviewed 2024-12-18 15:57:36 +00:00
src/git_kv.ml Outdated
@ -493,0 +491,4 @@
~some:Fun.id (Ptime.of_float_s (Int64.to_float secs))
in
Ok ts
| _ -> assert false
hannes commented 2024-12-18 15:57:36 +00:00
Owner
Copy link

this case shouldn't be needed

this case shouldn't be needed
reynir commented 2024-12-19 13:36:46 +00:00
Author
Owner
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
Owner
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
Author
Owner
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
hannes marked this conversation as resolved
reynir added 1 commit 2024-12-19 14:06:31 +00:00
dinosaure approved these changes 2024-12-20 11:08:56 +00:00
dinosaure left a comment
Owner
Copy link

LGTM

LGTM
reynir merged commit aec833f991 into main 2024-12-20 12:11:32 +00:00
reynir deleted branch fix-last_modified 2024-12-20 12:11:33 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dinosaure
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: robur/git-kv#10
No description provided.
Delete branch "fix-last_modified"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?