From 26a3ebf00fc6bc3b9b92e1b145330d78ca9f7b01 Mon Sep 17 00:00:00 2001 From: Overbool Date: Sun, 16 Sep 2018 01:03:49 +0800 Subject: [PATCH 1/4] fix(object): add support for raw leaves in object diff License: MIT Signed-off-by: Overbool --- dagutils/diff.go | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/dagutils/diff.go b/dagutils/diff.go index 97d50c88e..9034db35c 100644 --- a/dagutils/diff.go +++ b/dagutils/diff.go @@ -112,8 +112,8 @@ func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, e } var out []*Change - cleanA := a.Copy().(*dag.ProtoNode) - cleanB := b.Copy().(*dag.ProtoNode) + cleanA := a.Copy() + cleanB := b.Copy() // strip out unchanged stuff for _, lnk := range a.Links() { @@ -132,17 +132,7 @@ func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, e return nil, err } - anodepb, ok := anode.(*dag.ProtoNode) - if !ok { - return nil, dag.ErrNotProtobuf - } - - bnodepb, ok := bnode.(*dag.ProtoNode) - if !ok { - return nil, dag.ErrNotProtobuf - } - - sub, err := Diff(ctx, ds, anodepb, bnodepb) + sub, err := Diff(ctx, ds, anode, bnode) if err != nil { return nil, err } @@ -152,8 +142,12 @@ func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, e out = append(out, subc) } } - cleanA.RemoveNodeLink(l.Name) - cleanB.RemoveNodeLink(l.Name) + if cleanA, ok := cleanA.(*dag.ProtoNode); ok { + cleanA.RemoveNodeLink(l.Name) + } + if cleanB, ok := cleanB.(*dag.ProtoNode); ok { + cleanB.RemoveNodeLink(l.Name) + } } } From 5b0a94895f87bfc4c8ff686846b5d4989d33b399 Mon Sep 17 00:00:00 2001 From: Overbool Date: Mon, 17 Sep 2018 11:24:33 +0800 Subject: [PATCH 2/4] fix(object): add Diff() comment and test case License: MIT Signed-off-by: Overbool --- dagutils/diff.go | 8 +++-- test/sharness/t0052-object-diff.sh | 54 +++++++++++++++++++++++------- 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/dagutils/diff.go b/dagutils/diff.go index 9034db35c..b1c468154 100644 --- a/dagutils/diff.go +++ b/dagutils/diff.go @@ -94,7 +94,11 @@ func ApplyChange(ctx context.Context, ds ipld.DAGService, nd *dag.ProtoNode, cs return e.Finalize(ctx, ds) } -// Diff returns a set of changes that transform node 'a' into node 'b' +// Diff returns a set of changes that transform node 'a' into node 'b'. +// It supports two nodes forms: ProtoNode and RawNode. Because we treats +// the nodes as IPLD nodes as long as possible and only convert them +// to ProtoNode when necessary: when we need to remove links, and at that point +// (if they have links to remove) we know they are not raw nodes. func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, error) { // Base case where both nodes are leaves, just compare // their CIDs. @@ -103,7 +107,7 @@ func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, e return []*Change{}, nil } return []*Change{ - &Change{ + { Type: Mod, Before: a.Cid(), After: b.Cid(), diff --git a/test/sharness/t0052-object-diff.sh b/test/sharness/t0052-object-diff.sh index 949088c27..eb6b0bf45 100755 --- a/test/sharness/t0052-object-diff.sh +++ b/test/sharness/t0052-object-diff.sh @@ -15,82 +15,112 @@ test_expect_success "create some objects for testing diffs" ' echo "stuff" > foo/bar && mkdir foo/baz && A=$(ipfs add -r -q foo | tail -n1) && + AR=$(ipfs add --raw-leaves -r -q foo | tail -n1) && echo "more things" > foo/cat && B=$(ipfs add -r -q foo | tail -n1) && + BR=$(ipfs add --raw-leaves -r -q foo | tail -n1) && echo "nested" > foo/baz/dog && C=$(ipfs add -r -q foo | tail -n1) + CR=$(ipfs add --raw-leaves -r -q foo | tail -n1) echo "changed" > foo/bar && D=$(ipfs add -r -q foo | tail -n1) && + DR=$(ipfs add --raw-leaves -r -q foo | tail -n1) && echo "" > single_file && SINGLE_FILE=$(ipfs add -r -q single_file | tail -n1) && + SINGLE_FILE_RAW=$(ipfs add --raw-leaves -r -q single_file | tail -n1) && mkdir empty_dir EMPTY_DIR=$(ipfs add -r -q empty_dir | tail -n1) + EMPTY_DIR_RAW=$(ipfs add --raw-leaves -r -q empty_dir | tail -n1) ' test_expect_success "diff against self is empty" ' - ipfs object diff $A $A > diff_out + ipfs object diff $A $A > diff_out && + ipfs object diff $AR $AR > diff_raw_out ' test_expect_success "identity diff output looks good" ' printf "" > diff_exp && + printf "" > diff_raw_exp && test_cmp diff_exp diff_out + test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff against self (single file) is empty" ' - ipfs object diff $SINGLE_FILE $SINGLE_FILE > diff_out + ipfs object diff $SINGLE_FILE $SINGLE_FILE > diff_out && + ipfs object diff $SINGLE_FILE_RAW $SINGLE_FILE_RAW > diff_raw_out printf "" > diff_exp && - test_cmp diff_exp diff_out + printf "" > diff_raw_exp && + test_cmp diff_exp diff_out && + test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff against self (empty dir) is empty" ' ipfs object diff $EMPTY_DIR $EMPTY_DIR > diff_out + ipfs object diff $EMPTY_DIR_RAW $EMPTY_DIR_RAW > diff_raw_out printf "" > diff_exp && - test_cmp diff_exp diff_out + printf "" > diff_raw_exp && + test_cmp diff_exp diff_out && + test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff added link works" ' ipfs object diff $A $B > diff_out + ipfs object diff $AR $BR > diff_raw_out ' test_expect_success "diff added link looks right" ' echo + QmUSvcqzhdfYM1KLDbM76eLPdS9ANFtkJvFuPYeZt73d7A \"cat\" > diff_exp && - test_cmp diff_exp diff_out + echo + zb2rhmWNFDCdMjJoCZPE5b5NuU38yoRzRmEtfzb4exxk3R8g4 \"cat\" > diff_raw_exp && + test_cmp diff_exp diff_out && + test_cmp diff_raw_exp diff_raw_out ' test_expect_success "verbose diff added link works" ' ipfs object diff -v $A $B > diff_out + ipfs object diff -v $AR $BR > diff_raw_out ' test_expect_success "verbose diff added link looks right" ' echo Added new link \"cat\" pointing to QmUSvcqzhdfYM1KLDbM76eLPdS9ANFtkJvFuPYeZt73d7A. > diff_exp && - test_cmp diff_exp diff_out + echo Added new link \"cat\" pointing to zb2rhmWNFDCdMjJoCZPE5b5NuU38yoRzRmEtfzb4exxk3R8g4. > diff_raw_exp && + test_cmp diff_exp diff_out && + test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff removed link works" ' - ipfs object diff -v $B $A > diff_out + ipfs object diff -v $B $A > diff_out && + ipfs object diff -v $BR $AR > diff_raw_out ' test_expect_success "diff removed link looks right" ' echo Removed link \"cat\" \(was QmUSvcqzhdfYM1KLDbM76eLPdS9ANFtkJvFuPYeZt73d7A\). > diff_exp && - test_cmp diff_exp diff_out + echo Removed link \"cat\" \(was zb2rhmWNFDCdMjJoCZPE5b5NuU38yoRzRmEtfzb4exxk3R8g4\). > diff_raw_exp && + test_cmp diff_exp diff_out && + test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff nested add works" ' - ipfs object diff -v $B $C > diff_out + ipfs object diff -v $B $C > diff_out && + ipfs object diff -v $BR $CR > diff_raw_out ' test_expect_success "diff looks right" ' echo Added new link \"baz/dog\" pointing to QmdNJQUTZuDpsUcec7YDuCfRfvw1w4J13DCm7YcU4VMZdS. > diff_exp && - test_cmp diff_exp diff_out + echo Added new link \"baz/dog\" pointing to zb2rhaM8wjDfi8A22dEqk89raWtViq8pjxvKQu2eaKtWhYKgE. > diff_raw_exp && + test_cmp diff_exp diff_out && + test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff changed link works" ' - ipfs object diff -v $C $D > diff_out + ipfs object diff -v $C $D > diff_out && + ipfs object diff -v $CR $DR > diff_raw_out ' test_expect_success "diff looks right" ' echo Changed \"bar\" from QmNgd5cz2jNftnAHBhcRUGdtiaMzb5Rhjqd4etondHHST8 to QmRfFVsjSXkhFxrfWnLpMae2M4GBVsry6VAuYYcji5MiZb. > diff_exp && - test_cmp diff_exp diff_out + echo Changed \"bar\" from zb2rhdUECGnPgMJNgmghaMKdqqGdpTe9GmEJiPna488ThfLBz to zb2rhfEA1M13SPoeayrsPcKhCezgMQPjguGFLH56G8qQ2qpDn. > diff_raw_exp && + test_cmp diff_exp diff_out && + test_cmp diff_raw_exp diff_raw_out ' test_done From 7cda005a5f8bd87f112f23e8d84becd7a3690169 Mon Sep 17 00:00:00 2001 From: Overbool Date: Tue, 18 Sep 2018 08:56:01 +0800 Subject: [PATCH 3/4] test(diff): separate diff test cases License: MIT Signed-off-by: Overbool --- dagutils/diff.go | 2 +- test/sharness/t0052-object-diff.sh | 82 ++++++++++++++++++++++-------- 2 files changed, 63 insertions(+), 21 deletions(-) diff --git a/dagutils/diff.go b/dagutils/diff.go index b1c468154..6fc921400 100644 --- a/dagutils/diff.go +++ b/dagutils/diff.go @@ -95,7 +95,7 @@ func ApplyChange(ctx context.Context, ds ipld.DAGService, nd *dag.ProtoNode, cs } // Diff returns a set of changes that transform node 'a' into node 'b'. -// It supports two nodes forms: ProtoNode and RawNode. Because we treats +// It supports two nodes forms: ProtoNode and RawNode. Because we treat // the nodes as IPLD nodes as long as possible and only convert them // to ProtoNode when necessary: when we need to remove links, and at that point // (if they have links to remove) we know they are not raw nodes. diff --git a/test/sharness/t0052-object-diff.sh b/test/sharness/t0052-object-diff.sh index eb6b0bf45..aa0bcd11d 100755 --- a/test/sharness/t0052-object-diff.sh +++ b/test/sharness/t0052-object-diff.sh @@ -34,92 +34,134 @@ test_expect_success "create some objects for testing diffs" ' ' test_expect_success "diff against self is empty" ' - ipfs object diff $A $A > diff_out && - ipfs object diff $AR $AR > diff_raw_out + ipfs object diff $A $A > diff_out ' test_expect_success "identity diff output looks good" ' printf "" > diff_exp && - printf "" > diff_raw_exp && test_cmp diff_exp diff_out +' + +test_expect_success "diff (raw-leaves) against self is empty" ' + ipfs object diff $AR $AR > diff_raw_out +' + +test_expect_success "identity diff (raw-leaves) output looks good" ' + printf "" > diff_raw_exp && test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff against self (single file) is empty" ' ipfs object diff $SINGLE_FILE $SINGLE_FILE > diff_out && - ipfs object diff $SINGLE_FILE_RAW $SINGLE_FILE_RAW > diff_raw_out printf "" > diff_exp && + test_cmp diff_exp diff_out +' + +test_expect_success "diff (raw-leaves) against self (single file) is empty" ' + ipfs object diff $SINGLE_FILE_RAW $SINGLE_FILE_RAW > diff_raw_out printf "" > diff_raw_exp && - test_cmp diff_exp diff_out && test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff against self (empty dir) is empty" ' ipfs object diff $EMPTY_DIR $EMPTY_DIR > diff_out - ipfs object diff $EMPTY_DIR_RAW $EMPTY_DIR_RAW > diff_raw_out printf "" > diff_exp && + test_cmp diff_exp diff_out +' + +test_expect_success "diff (raw-leaves) against self (empty dir) is empty" ' + ipfs object diff $EMPTY_DIR_RAW $EMPTY_DIR_RAW > diff_raw_out printf "" > diff_raw_exp && - test_cmp diff_exp diff_out && test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff added link works" ' ipfs object diff $A $B > diff_out - ipfs object diff $AR $BR > diff_raw_out ' test_expect_success "diff added link looks right" ' echo + QmUSvcqzhdfYM1KLDbM76eLPdS9ANFtkJvFuPYeZt73d7A \"cat\" > diff_exp && + test_cmp diff_exp diff_out +' + +test_expect_success "diff (raw-leaves) added link works" ' + ipfs object diff $AR $BR > diff_raw_out +' + +test_expect_success "diff (raw-leaves) added link looks right" ' echo + zb2rhmWNFDCdMjJoCZPE5b5NuU38yoRzRmEtfzb4exxk3R8g4 \"cat\" > diff_raw_exp && - test_cmp diff_exp diff_out && test_cmp diff_raw_exp diff_raw_out ' test_expect_success "verbose diff added link works" ' ipfs object diff -v $A $B > diff_out - ipfs object diff -v $AR $BR > diff_raw_out ' test_expect_success "verbose diff added link looks right" ' echo Added new link \"cat\" pointing to QmUSvcqzhdfYM1KLDbM76eLPdS9ANFtkJvFuPYeZt73d7A. > diff_exp && + test_cmp diff_exp diff_out +' + +test_expect_success "verbose diff (raw-leaves) added link works" ' + ipfs object diff -v $AR $BR > diff_raw_out +' + +test_expect_success "verbose diff (raw-leaves) added link looks right" ' echo Added new link \"cat\" pointing to zb2rhmWNFDCdMjJoCZPE5b5NuU38yoRzRmEtfzb4exxk3R8g4. > diff_raw_exp && - test_cmp diff_exp diff_out && test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff removed link works" ' - ipfs object diff -v $B $A > diff_out && - ipfs object diff -v $BR $AR > diff_raw_out + ipfs object diff -v $B $A > diff_out ' test_expect_success "diff removed link looks right" ' echo Removed link \"cat\" \(was QmUSvcqzhdfYM1KLDbM76eLPdS9ANFtkJvFuPYeZt73d7A\). > diff_exp && + test_cmp diff_exp diff_out +' + +test_expect_success "diff (raw-leaves) removed link works" ' + ipfs object diff -v $BR $AR > diff_raw_out +' + +test_expect_success "diff (raw-leaves) removed link looks right" ' echo Removed link \"cat\" \(was zb2rhmWNFDCdMjJoCZPE5b5NuU38yoRzRmEtfzb4exxk3R8g4\). > diff_raw_exp && - test_cmp diff_exp diff_out && test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff nested add works" ' - ipfs object diff -v $B $C > diff_out && - ipfs object diff -v $BR $CR > diff_raw_out + ipfs object diff -v $B $C > diff_out ' test_expect_success "diff looks right" ' echo Added new link \"baz/dog\" pointing to QmdNJQUTZuDpsUcec7YDuCfRfvw1w4J13DCm7YcU4VMZdS. > diff_exp && + test_cmp diff_exp diff_out +' + +test_expect_success "diff (raw-leaves) nested add works" ' + ipfs object diff -v $BR $CR > diff_raw_out +' + +test_expect_success "diff (raw-leaves) looks right" ' echo Added new link \"baz/dog\" pointing to zb2rhaM8wjDfi8A22dEqk89raWtViq8pjxvKQu2eaKtWhYKgE. > diff_raw_exp && - test_cmp diff_exp diff_out && test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff changed link works" ' - ipfs object diff -v $C $D > diff_out && - ipfs object diff -v $CR $DR > diff_raw_out + ipfs object diff -v $C $D > diff_out ' test_expect_success "diff looks right" ' echo Changed \"bar\" from QmNgd5cz2jNftnAHBhcRUGdtiaMzb5Rhjqd4etondHHST8 to QmRfFVsjSXkhFxrfWnLpMae2M4GBVsry6VAuYYcji5MiZb. > diff_exp && + test_cmp diff_exp diff_out +' + +test_expect_success "diff (raw-leaves) changed link works" ' + ipfs object diff -v $CR $DR > diff_raw_out +' + +test_expect_success "diff（raw-leaves) looks right" ' echo Changed \"bar\" from zb2rhdUECGnPgMJNgmghaMKdqqGdpTe9GmEJiPna488ThfLBz to zb2rhfEA1M13SPoeayrsPcKhCezgMQPjguGFLH56G8qQ2qpDn. > diff_raw_exp && - test_cmp diff_exp diff_out && test_cmp diff_raw_exp diff_raw_out ' From c6daf934ea355b6115ec7222e00aef969ef1783a Mon Sep 17 00:00:00 2001 From: Overbool Date: Sat, 22 Sep 2018 09:58:12 +0800 Subject: [PATCH 4/4] fix(diff): modify diff logic and comment License: MIT Signed-off-by: Overbool --- dagutils/diff.go | 47 ++++++++++++++++-------------- test/sharness/t0052-object-diff.sh | 6 ++-- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/dagutils/diff.go b/dagutils/diff.go index 6fc921400..c63aacafb 100644 --- a/dagutils/diff.go +++ b/dagutils/diff.go @@ -95,29 +95,23 @@ func ApplyChange(ctx context.Context, ds ipld.DAGService, nd *dag.ProtoNode, cs } // Diff returns a set of changes that transform node 'a' into node 'b'. -// It supports two nodes forms: ProtoNode and RawNode. Because we treat -// the nodes as IPLD nodes as long as possible and only convert them -// to ProtoNode when necessary: when we need to remove links, and at that point -// (if they have links to remove) we know they are not raw nodes. +// It only traverses links in the following cases: +// 1. two node's links number are greater than 0. +// 2. both of two nodes are ProtoNode. +// Otherwise, it compares the cid and emits a Mod change object. func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, error) { // Base case where both nodes are leaves, just compare // their CIDs. if len(a.Links()) == 0 && len(b.Links()) == 0 { - if a.Cid().Equals(b.Cid()) { - return []*Change{}, nil - } - return []*Change{ - { - Type: Mod, - Before: a.Cid(), - After: b.Cid(), - }, - }, nil + return getChange(a, b) } var out []*Change - cleanA := a.Copy() - cleanB := b.Copy() + cleanA, okA := a.Copy().(*dag.ProtoNode) + cleanB, okB := b.Copy().(*dag.ProtoNode) + if !okA || !okB { + return getChange(a, b) + } // strip out unchanged stuff for _, lnk := range a.Links() { @@ -146,12 +140,8 @@ func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, e out = append(out, subc) } } - if cleanA, ok := cleanA.(*dag.ProtoNode); ok { - cleanA.RemoveNodeLink(l.Name) - } - if cleanB, ok := cleanB.(*dag.ProtoNode); ok { - cleanB.RemoveNodeLink(l.Name) - } + cleanA.RemoveNodeLink(l.Name) + cleanB.RemoveNodeLink(l.Name) } } @@ -207,3 +197,16 @@ func MergeDiffs(a, b []*Change) ([]*Change, []Conflict) { } return out, conflicts } + +func getChange(a, b ipld.Node) ([]*Change, error) { + if a.Cid().Equals(b.Cid()) { + return []*Change{}, nil + } + return []*Change{ + { + Type: Mod, + Before: a.Cid(), + After: b.Cid(), + }, + }, nil +} diff --git a/test/sharness/t0052-object-diff.sh b/test/sharness/t0052-object-diff.sh index aa0bcd11d..e512c4b18 100755 --- a/test/sharness/t0052-object-diff.sh +++ b/test/sharness/t0052-object-diff.sh @@ -58,19 +58,19 @@ test_expect_success "diff against self (single file) is empty" ' ' test_expect_success "diff (raw-leaves) against self (single file) is empty" ' - ipfs object diff $SINGLE_FILE_RAW $SINGLE_FILE_RAW > diff_raw_out + ipfs object diff $SINGLE_FILE_RAW $SINGLE_FILE_RAW > diff_raw_out && printf "" > diff_raw_exp && test_cmp diff_raw_exp diff_raw_out ' test_expect_success "diff against self (empty dir) is empty" ' - ipfs object diff $EMPTY_DIR $EMPTY_DIR > diff_out + ipfs object diff $EMPTY_DIR $EMPTY_DIR > diff_out && printf "" > diff_exp && test_cmp diff_exp diff_out ' test_expect_success "diff (raw-leaves) against self (empty dir) is empty" ' - ipfs object diff $EMPTY_DIR_RAW $EMPTY_DIR_RAW > diff_raw_out + ipfs object diff $EMPTY_DIR_RAW $EMPTY_DIR_RAW > diff_raw_out && printf "" > diff_raw_exp && test_cmp diff_raw_exp diff_raw_out '