Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(object): add support for raw leaves in object diff #5472

Merged
merged 4 commits into from
Oct 4, 2018
Merged

fix(object): add support for raw leaves in object diff #5472

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26a3ebf
fix(object): add support for raw leaves in object diff
overbool Sep 15, 2018
5b0a948
fix(object): add Diff() comment and test case
overbool Sep 17, 2018
7cda005
test(diff): separate diff test cases
overbool Sep 18, 2018
c6daf93
fix(diff): modify diff logic and comment
overbool Sep 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 15 additions & 17 deletions dagutils/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 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.
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.
Expand All @@ -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(),
Expand All @@ -112,8 +116,8 @@ func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, e
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that much (I think), we could add a check here to verify that, if they have links, i.e., they are not raw nodes because they passed the first check (hence we would still be supporting them), they must be proto-nodes; if not, you could also return a Mod as before. Actually you could even merge this proto-node check in the previous if, WDYT?

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() {
Expand All @@ -132,17 +136,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
}
Expand All @@ -152,8 +146,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)
}
}
}

Expand Down
74 changes: 73 additions & 1 deletion test/sharness/t0052-object-diff.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,22 @@ 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" '
Expand All @@ -36,18 +42,39 @@ test_expect_success "identity diff output looks good" '
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 $SINGLE_FILE > diff_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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing &&

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
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also missing &&

printf "" > diff_raw_exp &&
test_cmp diff_raw_exp diff_raw_out
'

test_expect_success "diff added link works" '
ipfs object diff $A $B > diff_out
'
Expand All @@ -57,6 +84,15 @@ test_expect_success "diff added link looks right" '
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_raw_exp diff_raw_out
'

test_expect_success "verbose diff added link works" '
ipfs object diff -v $A $B > diff_out
'
Expand All @@ -66,6 +102,15 @@ test_expect_success "verbose diff added link looks right" '
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_raw_exp diff_raw_out
'

test_expect_success "diff removed link works" '
ipfs object diff -v $B $A > diff_out
'
Expand All @@ -75,6 +120,15 @@ test_expect_success "diff removed link looks right" '
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_raw_exp diff_raw_out
'

test_expect_success "diff nested add works" '
ipfs object diff -v $B $C > diff_out
'
Expand All @@ -84,6 +138,15 @@ test_expect_success "diff looks right" '
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_raw_exp diff_raw_out
'

test_expect_success "diff changed link works" '
ipfs object diff -v $C $D > diff_out
'
Expand All @@ -93,4 +156,13 @@ test_expect_success "diff looks right" '
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_raw_exp diff_raw_out
'

test_done