fix(dagutils): rework diff algorithm #82
Conversation
| if !bytes.Equal(a.Data(), b.Data()) { | ||
| // FIXME: This is a very dirty way of reporting a difference in Data() | ||
| // but the current `Change` API doesn't support anything but links. | ||
| out = append(out, &Change{Type: Mod, Path: "<NODE-DATA>", Before: a.Cid(), After: b.Cid()}) |
There was a problem hiding this comment.
Wrote some thoughts and relevant examples in ipfs/kubo#4801 (comment)
Perhaps switching to Logical Format described there, with Data and Links/<index>/Name prefixes in Change.Path would remove the need for hacks like this.
Having that, ipfs object diff would ignore/strip this additional prefix data,
but ipfs dag diff could take advantage of it and show more detailed DAG diff.
There was a problem hiding this comment.
@lidel I'm not sure I follow the concrete direction you're requesting there; I think I'll need that to be lowered a bit more to the codebase I'm dealing with here.
From what I'm understanding in the linked proposal a dag diff would now be:
dag getto the JSON format already in place- diff the JSON outputs
Which would operate completely in the go-ipfs repo and not here (this work would could apply as the new, a little less broken, object diff command if you want).
There was a problem hiding this comment.
Indeed, generating the dag diff based on dag-json representation would make things easier and less error-prone.
Since everything would be serialized to dag-json representation, we could support dag-pb, raw amd dag-cbor CIDs out of the box.
👉 @schomatis I think we should give that a try and park this PR for now – focus on go-ipfs and its JSON-Patch-based dag diff instead.
Regarding this PR (when we revisit it later)
- is
Diffonly used inobject diffor are there other places where we use diff implementation from here? - if unused, perhaps we should remove
DiffandDiffEnumerate, and switchipfs object diffto usedag diffbehind the scenes?
There was a problem hiding this comment.
is Diff only used in object diff or are there other places where we use diff implementation from here?
AFAIK object diff ((*ObjectAPI)Diff) is the only consumer.
switch ipfs object diff to use dag diff behind the scenes?
Yes, that was the plan. Deprecate object diff and point it (either explicitly or implicitly as you propose) to dag diff.
|
@schomatis : correct me if this is wrong, but this will get reevaluated after ipfs/kubo#8711 lands. |
|
Yes, I think it's fair to close this at this point, we'll likely go with the JSON variant in ipfs/kubo#8711. |
Still some blocking fixmes to finalize but need a review at this point since this algorithm is tentatively filling in the blanks of some unsupported cases from before.
Toward ipfs/kubo#4801.