Merge pull request #45 from ipfs/fix/flush/remove-fullsync

remove the `fullSync` option from `updateChildEntry`

dir.go

@@ -76,45 +76,27 @@ func (d *Directory) SetCidBuilder(b cid.Builder) {
 	d.unixfsDir.SetCidBuilder(b)
 }
 
-// This method implements the `parent` interface. It first updates
-// the child entry in the underlying UnixFS directory and then, if `fullSync`
-// is set, it:
-//   1. DAG: saves the newly created directory node with the updated entry.
-//   2. MFS: propagates the update upwards (through this same interface)
-//           repeating the whole process in the parent.
-func (d *Directory) updateChildEntry(c child, fullSync bool) error {
-
-	// There's a local flush (`closeChildUpdate`) and a propagated flush (`updateChildEntry`).
-
-	newDirNode, err := d.closeChildUpdate(c, fullSync)
+// This method implements the `parent` interface. It first does the local
+// update of the child entry in the underlying UnixFS directory and saves
+// the newly created directory node with the updated entry in the DAG
+// service. Then it propagates the update upwards (through this same
+// interface) repeating the whole process in the parent.
+func (d *Directory) updateChildEntry(c child) error {
+	newDirNode, err := d.localUpdate(c)
 	if err != nil {
 		return err
 	}
 
-	// TODO: The `sync` seems to be tightly coupling this two pieces of code,
-	// we use the node returned by `closeChildUpdate` (which entails a copy)
-	// only if `sync` is set, and we are discarding it otherwise. At the very
-	// least the `if sync {` clause at the end of `closeChildUpdate` should
-	// be merged with this one (the use of the `lock` is stopping this at the
-	// moment, re-evaluate when its purpose has been better understood).
-
-	if fullSync {
-		return d.parent.updateChildEntry(child{d.name, newDirNode}, true)
-		// Setting `fullSync` to true here means, if the original child that
-		// initiated the update process wanted to propagate it upwards then
-		// continue to do so all the way up to the root, that is, the only
-		// time `fullSync` can be false is in the first call (which will be
-		// the *only* call), we either update the first parent entry or *all*
-		// the parent's.
-	}
-
-	return nil
+	// Continue to propagate the update process upwards
+	// (all the way up to the root).
+	return d.parent.updateChildEntry(child{d.name, newDirNode})
 }
 
 // This method implements the part of `updateChildEntry` that needs
 // to be locked around: in charge of updating the UnixFS layer and
-// generating the new node reflecting the update.
-func (d *Directory) closeChildUpdate(c child, fullSync bool) (*dag.ProtoNode, error) {
+// generating the new node reflecting the update. It also stores the
+// new node in the DAG layer.
+func (d *Directory) localUpdate(c child) (*dag.ProtoNode, error) {
 	d.lock.Lock()
 	defer d.lock.Unlock()
 
@@ -125,34 +107,20 @@ func (d *Directory) closeChildUpdate(c child, fullSync bool) (*dag.ProtoNode, er
 	// TODO: Clearly define how are we propagating changes to lower layers
 	// like UnixFS.
 
-	if fullSync {
-		return d.flushCurrentNode()
-	}
-	return nil, nil
-}
-
-// Recreate the underlying UnixFS directory node and save it in the DAG layer.
-func (d *Directory) flushCurrentNode() (*dag.ProtoNode, error) {
 	nd, err := d.unixfsDir.GetNode()
 	if err != nil {
 		return nil, err
 	}
 
-	err = d.dagService.Add(d.ctx, nd)
-	if err != nil {
-		return nil, err
-	}
-	// TODO: This method is called in `closeChildUpdate` while the lock is
-	// taken, we need the lock while operating on `unixfsDir` to create the
-	// new node but do we also need to keep the lock while adding it to the
-	// DAG service? Evaluate refactoring these two methods together and better
-	// redistributing the node.
-
 	pbnd, ok := nd.(*dag.ProtoNode)
 	if !ok {
 		return nil, dag.ErrNotProtobuf
 	}
-	// TODO: Why do we check the node *after* adding it to the DAG service?
+
+	err = d.dagService.Add(d.ctx, nd)
+	if err != nil {
+		return nil, err
+	}
 
 	return pbnd.Copy().(*dag.ProtoNode), nil
 	// TODO: Why do we need a copy?
@@ -374,7 +342,7 @@ func (d *Directory) Flush() error {
 		return err
 	}
 
-	return d.parent.updateChildEntry(child{d.name, nd}, true)
+	return d.parent.updateChildEntry(child{d.name, nd})
 }
 
 // AddChild adds the node 'nd' under this directory giving it the name 'name'

fd.go

@@ -123,9 +123,8 @@ func (fi *fileDescriptor) Flush() error {
 
 // flushUp syncs the file and adds it to the dagservice
 // it *must* be called with the File's lock taken
-// TODO: What is `fullsync`? Propagate the changes upward
-// to the root flushing every node in the path (the "up"
-// part of `flushUp`).
+// If `fullSync` is set the changes are propagated upwards
+// (the `Up` part of `flushUp`).
 func (fi *fileDescriptor) flushUp(fullSync bool) error {
 	nd, err := fi.mod.GetNode()
 	if err != nil {
@@ -151,7 +150,11 @@ func (fi *fileDescriptor) flushUp(fullSync bool) error {
 	fi.inode.nodeLock.Unlock()
 	// TODO: Maybe all this logic should happen in `File`.
 
-	return parent.updateChildEntry(child{name, nd}, fullSync)
+	if fullSync {
+		return parent.updateChildEntry(child{name, nd})
+	}
+
+	return nil
 }
 
 // Seek implements io.Seeker

root.go

@@ -45,13 +45,12 @@ type child struct {
 // the entry/link of the directory that pointed to the modified node).
 type parent interface {
 	// Method called by a child to its parent to signal to update the content
-	// pointed to in the entry by that child's name. The child sends as
-	// arguments its own information (under the `child` structure) and a flag
-	// (`fullsync`) indicating whether or not to propagate the update upwards:
-	// modifying a directory entry entails modifying its contents which means
-	// that its parent (the parent's parent) will also need to be updated (and
-	// so on).
-	updateChildEntry(c child, fullSync bool) error
+	// pointed to in the entry by that child's name. The child sends its own
+	// information in the `child` structure. As modifying a directory entry
+	// entails modifying its contents the parent will also call *its* parent's
+	// `updateChildEntry` to update the entry pointing to the new directory,
+	// this mechanism is in turn repeated until reaching the `Root`.
+	updateChildEntry(c child) error
 }
 
 type NodeType int
@@ -189,7 +188,7 @@ func (kr *Root) FlushMemFree(ctx context.Context) error {
 // TODO: The `sync` argument isn't used here (we've already reached
 // the top), document it and maybe make it an anonymous variable (if
 // that's possible).
-func (kr *Root) updateChildEntry(c child, fullSync bool) error {
+func (kr *Root) updateChildEntry(c child) error {
 	err := kr.GetDirectory().dagService.Add(context.TODO(), c.Node)
 	if err != nil {
 		return err