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

overbool · 2018-09-15T17:06:26Z

License: MIT
Signed-off-by: Overbool overbool.xu@gmail.com

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

overbool · 2018-09-16T02:55:52Z

@Stebalien, something seems to be incorrect in ci

schomatis · 2018-09-17T01:20:07Z

Yes, this definitely looks like the right approach, treat the nodes as IPLD nodes as long as possible and only convert them to ProtoNode when necessary: when we need to remove links (write to the node, something that can't be done with the IPLD interface), and at that point (if they have links to remove) we know they are not raw nodes. Am I interpreting this patch correctly?

Could you add a test for the raw nodes case?

(And also, if it's not too much trouble, could you add some comment lines explaining that this now supports raw nodes and why.)

schomatis · 2018-09-17T01:21:19Z

And great job!! (I should have started with that line :)

overbool · 2018-09-17T04:56:41Z

@schomatis Yes, thx for your reply. I add the comment and test cases. Could you help me review this again?

But something seems to be incorrect about our ci.

schomatis

Great @overbool!! This LGTM (minus the two code nits), I would like to get someone else to check the tests structure, overloading test cases (with more than one test_cmp call per case) seems sensible to me but we may be breaking some unwritten rule.

schomatis · 2018-09-17T14:13:10Z

core/commands/object/diff.go

 	coreiface "github.com/ipfs/go-ipfs/core/coreapi/interface"
 	"github.com/ipfs/go-ipfs/dagutils"

-	cmdkit "gx/ipfs/QmSP88ryZkHSRn1fnngAaV2Vcn63WUJzAavnRM9CVdU1Ky/go-ipfs-cmdkit"


I'm sure this was some IDE automatic feature (it happens to me also) but could you restore the e and cmdkit names?

schomatis · 2018-09-17T14:20:08Z

core/commands/object/diff.go

+	"gx/ipfs/QmSP88ryZkHSRn1fnngAaV2Vcn63WUJzAavnRM9CVdU1Ky/go-ipfs-cmdkit"
+)
+
+const (


I'm usually all for declaring constants and avoiding hard-coding inside functions but in this particular case of strings it doesn't seem to be worth it, those strings won't be repeated elsewhere and they make more sense inside the StringArg call instead of isolated here where we wouldn't now what they are used for, it will be just repeating the "obj_a" string as just the name of the string constant with the same value.

schomatis · 2018-09-17T14:23:09Z

@magik6k Could you take a look at the tests please?

schomatis · 2018-09-17T14:25:42Z

But something seems to be incorrect about our ci.

Yeah, I wouldn't worry too much about it, they tend to fail for no (apparent) reason fairly regularly, my main rule of thumb is that if another CI that is doing the same test is passing then I'm good.

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

magik6k · 2018-09-17T14:37:39Z

@magik6k Could you take a look at the tests please?

It was a random fail, most likely caused by #5481

overbool · 2018-09-17T14:38:19Z

@schomatis Thx for your suggestion. I had fixed them.

overbool · 2018-09-17T14:45:24Z

@schomatis Should I split the test cases to keep the rule (one test_cmp call per case)?

schomatis · 2018-09-17T14:55:36Z

It was a random fail, most likely caused by #5481

@magik6k Sorry, I wasn't very clear. I meant if you could take a look at the sharness tests in this PR: https://github.com/ipfs/go-ipfs/pull/5472/files#diff-c85948db06f72554f9c3153c8cfac4c9, particularly if it's ok to call test_cmp twice in the same test case (which seems reasonable to me but just to be sure).

magik6k

I'd prefer the test cases to be separate, makes it more obvious when something fails (slightly easier to read too).

magik6k · 2018-09-17T16:50:21Z

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


s/treats/treat

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

overbool · 2018-09-18T01:01:36Z

@magik6k I had separated the test cases and modified the spelling mistake，could you help me review it again?

schomatis · 2018-09-18T14:06:58Z

Nice work, thanks @overbool !

overbool · 2018-09-18T15:17:16Z

@schomatis thx for your help in this issue

Stebalien · 2018-09-18T17:05:03Z

How will this behave when passed, e.g., DagCBOR nodes? I think this'll just continue as if nothing is wrong and print out nonsensical results. We should fail when we encounter non-protobuf/non-raw nodes.

schomatis · 2018-09-18T19:47:29Z

I think this'll just continue as if nothing is wrong and print out nonsensical results.

Why?

We should fail when we encounter non-protobuf/non-raw nodes.

Where?

I don't have any experience with CBOR so I fail to see the relation between that type of node and this particular PR.

The title of the PR may be misleading, it emphasizes why we are doing this but not what we are changing here. This just doesn't force the ProtoNode type assertion unless it needs to: when modifying the links of placeholder nodes (cleanA/cleanB) which shouldn't even exist in the first place (we could just be using another container for the Add/Remove links). There is no especial support for proto/raw nodes, this function operates at the IPLD level comparing CIDs.

Stebalien · 2018-09-19T07:48:09Z

when modifying the links of placeholder nodes (cleanA/cleanB) which shouldn't even exist in the first place (we could just be using another container for the Add/Remove links)

This is how we're calculating the diff. We're removing all the links present in both nodes A and B from both A and B, leaving only the links present in only one or the other. However, this will fail for any format that's not ProtoNode as we won't be able to remove the links.

This is fine for raw nodes only because they don't have links.

One solution is to just treat all non-ProtoNodes as opaque "things" (i.e., stop diffing). Diff would, at the top, check to see if we have two protonodes. If not, it would compare the CIDs and emit a Mod change object instead of actually diffing.

Is this making any sense or am I still misunderstanding what this patch does.

overbool · 2018-09-19T08:37:14Z

However, this will fail for any format that's not ProtoNode as we won't be able to remove the links.

If other format Nodes have links and implement RemoveNodeLink function, maybe Diff() can support other format Nodes not just ProtoNode and RawNode.

Currently, Diff() just support ProtoNode and RawNode.

schomatis · 2018-09-19T21:41:59Z

This is how we're calculating the diff. We're removing all the links present in both nodes A and B from both A and B, leaving only the links present in only one or the other. However, this will fail for any format that's not ProtoNode as we won't be able to remove the links.

Yes, it seems like a reasonable compromise. @overbool Are you ok with making that change? If not, we can merge this PR as-is, since it's already solving #5470 (and the fact that this command is deeply broken is beyond responsibility of this PR).

overbool · 2018-09-20T00:54:30Z

Are you ok with making that change?

@schomatis What do you mean about change?

One solution is to just treat all non-ProtoNodes as opaque "things" (i.e., stop diffing). Diff would, at the top, check to see if we have two protonodes. If not, it would compare the CIDs and emit a Mod change object instead of actually diffing.

about this?

schomatis · 2018-09-20T02:59:56Z

Yes, instead of returning an error if the conversion to a ProtoNode fails generate a Mod entry instead (without diffing inside of those node's links).

overbool · 2018-09-20T17:38:17Z

@schomatis, i got it.
However, using new solution means that previous work and tests were wasted.

schomatis · 2018-09-21T00:43:59Z

dagutils/diff.go

@@ -112,8 +116,8 @@ func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, e
 	}



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?

overbool · 2018-09-22T02:07:27Z

@Stebalien @schomatis According to your suggestion, I made some changes in Diff().

It only traverses links in the following cases:

two node's links number are greater than 0.
both of two nodes are ProtoNode.

Otherwise, it compares the cid and emits a Mod change object.

schomatis · 2018-09-23T16:25:42Z

dagutils/diff.go

@@ -209,3 +197,16 @@ func MergeDiffs(a, b []*Change) ([]*Change, []Conflict) {
 	}
 	return out, conflicts
 }
+
+func getChange(a, b ipld.Node) ([]*Change, error) {


Nicely encapsulated!

schomatis · 2018-09-23T16:28:12Z

dagutils/diff.go

-// 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 only traverses links in the following cases:
+// 1. two node's links number are greater then 0.


nit: "then" -> "than"

OMG, forgive my frequent spelling mistakes. 😨

schomatis

@Stebalien Was that what you had in mind?

magik6k

2 small things in sharness, then LGTM

magik6k · 2018-09-24T12:25:55Z

test/sharness/t0052-object-diff.sh

  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


missing &&

magik6k · 2018-09-24T12:26:04Z

test/sharness/t0052-object-diff.sh

 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


also missing &&

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

overbool · 2018-09-24T12:42:24Z

@magik6k thx for your reply, I had fixed it.

overbool · 2018-09-29T08:04:59Z

@Stebalien Was this pr what you had in mind?

overbool requested a review from Kubuxu as a code owner September 15, 2018 17:06

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

26a3ebf

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

overbool force-pushed the fix/diff-raw-leaves branch from f7b14ba to 26a3ebf Compare September 15, 2018 17:16

overbool mentioned this pull request Sep 16, 2018

Add support for raw leaves in ipfs object diff #5470

Closed

overbool force-pushed the fix/diff-raw-leaves branch 2 times, most recently from 4254f14 to 1ce6e04 Compare September 17, 2018 11:02

schomatis approved these changes Sep 17, 2018

View reviewed changes

schomatis requested a review from magik6k September 17, 2018 14:22

fix(object): add Diff() comment and test case

5b0a948

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

overbool force-pushed the fix/diff-raw-leaves branch from 1ce6e04 to 5b0a948 Compare September 17, 2018 14:35

magik6k mentioned this pull request Sep 17, 2018

coreapi name: Increase test swarm size #5481

Merged

magik6k reviewed Sep 17, 2018

View reviewed changes

test(diff): separate diff test cases

7cda005

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

schomatis added the RFM label Sep 18, 2018

schomatis reviewed Sep 21, 2018

View reviewed changes

schomatis reviewed Sep 23, 2018

View reviewed changes

schomatis approved these changes Sep 23, 2018

View reviewed changes

schomatis added need/review Needs a review and removed RFM labels Sep 23, 2018

overbool force-pushed the fix/diff-raw-leaves branch from 1628e97 to df706f4 Compare September 23, 2018 16:44

magik6k reviewed Sep 24, 2018

View reviewed changes

fix(diff): modify diff logic and comment

c6daf93

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

overbool force-pushed the fix/diff-raw-leaves branch from df706f4 to c6daf93 Compare September 24, 2018 12:39

magik6k approved these changes Sep 24, 2018

View reviewed changes

eingenito added the status/in-progress In progress label Oct 2, 2018

schomatis requested a review from Stebalien October 3, 2018 11:22

Stebalien approved these changes Oct 4, 2018

View reviewed changes

Stebalien merged commit 181c399 into ipfs:master Oct 4, 2018

ghost removed the status/in-progress In progress label Oct 4, 2018

		@@ -112,8 +116,8 @@ func Diff(ctx context.Context, ds ipld.DAGService, a, b ipld.Node) ([]*Change, e
		}

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

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

Conversation

overbool commented Sep 15, 2018 • edited by hannahhoward Loading

overbool commented Sep 16, 2018

schomatis commented Sep 17, 2018

schomatis commented Sep 17, 2018

overbool commented Sep 17, 2018 • edited Loading

schomatis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schomatis commented Sep 17, 2018

schomatis commented Sep 17, 2018

magik6k commented Sep 17, 2018

overbool commented Sep 17, 2018

overbool commented Sep 17, 2018

schomatis commented Sep 17, 2018 • edited Loading

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overbool commented Sep 18, 2018

schomatis commented Sep 18, 2018

overbool commented Sep 18, 2018

Stebalien commented Sep 18, 2018

schomatis commented Sep 18, 2018

Stebalien commented Sep 19, 2018

overbool commented Sep 19, 2018

schomatis commented Sep 19, 2018

overbool commented Sep 20, 2018

schomatis commented Sep 20, 2018

overbool commented Sep 20, 2018

Choose a reason for hiding this comment

overbool commented Sep 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schomatis left a comment

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overbool commented Sep 24, 2018

overbool commented Sep 29, 2018

overbool commented Sep 15, 2018 •

edited by hannahhoward

Loading

overbool commented Sep 17, 2018 •

edited

Loading

schomatis commented Sep 17, 2018 •

edited

Loading

overbool commented Sep 22, 2018 •

edited

Loading