-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pre_hash should recursively rely on pre_hash #39
Comments
This issue seems to have resulted in #60 having introduced an incompatibility with Irmin master. The following diff seems to fix it, but it's a hack: diff --git a/src/repr/type.ml b/src/repr/type.ml
index 3b89218..965e45e 100644
--- a/src/repr/type.ml
+++ b/src/repr/type.ml
@@ -21,6 +21,7 @@ open Utils
let pre_hash t =
let rec aux : type a. a t -> a encode_bin = function
+ | Attributes { attr_type; _ } -> aux attr_type
| Self s -> aux s.self_fix
| Map m ->
let dst = unstage (aux m.x) in We should fix this, as it's a very subtle behaviour that Repr's test suite does not cover. |
craigfe
added a commit
to craigfe/repr
that referenced
this issue
Jun 11, 2021
Implementing boxing in terms of annotations is conceptually correct, but interacts with an existing bug in the definition of `pre_hash` (see mirage#39) that makes this change a breaking inside Irmin. Pending a solution to mirage#39, we should avoid changing the behaviour of `pre_hash` in this way. This reverts commit b09dddf.
craigfe
added a commit
to craigfe/repr
that referenced
this issue
Jun 11, 2021
Implementing boxing in terms of annotations is conceptually correct, but interacts with an existing bug in the definition of `pre_hash` (see mirage#39) that makes this change breaking inside Irmin. Pending a solution to mirage#39, we should avoid changing the behaviour of `pre_hash` in this way. This restores compatibility with Irmin `master`. This reverts commit b09dddf.
craigfe
added a commit
to craigfe/repr
that referenced
this issue
Jun 11, 2021
This reverts commit b09dddf. Implementing boxing in terms of annotations is conceptually correct, but interacts with an existing bug in the definition of `pre_hash` (see mirage#39) that makes this change breaking inside Irmin. Pending a solution to mirage#39, we should avoid changing the behaviour of `pre_hash` in this way. This restores compatibility with Irmin `master`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Follow up on https://gitlab.com/tezos/tezos/-/merge_requests/2543.
pre_hash
isUnboxed.encode_bin
unless when the user provides a custom one through thev
,like
andmap
contructors.The problem is that when computing the
pre_hash
of a container type (e.g. list), the custompre_hash
of the substructures contained in that container are simply ignored.I guess a solution would be to add a
is_pre_hash : bool
parameter to all the function inEncode
that traverse the runtime types so that the| Custom
and| Map
cases ofEncode.unboxed
andEncode.t
select the right function given that boolean.The text was updated successfully, but these errors were encountered: