fix(logbook): use sync.Once for rollback functions

cmd/log.go

@@ -159,8 +159,8 @@ The logbook command shows entries for a dataset, from newest to oldest.`,
 			}
 			if o.Raw {
 				return o.RawLogs()
-			} else if o.Diagnostic {
-				return o.LogbookDiagnostic()
+			} else if o.Summary {
+				return o.LogbookSummary()
 			}
 			return o.Logbook()
 		},
@@ -169,7 +169,7 @@ The logbook command shows entries for a dataset, from newest to oldest.`,
 	cmd.Flags().IntVar(&o.PageSize, "page-size", 25, "page size of results, default 25")
 	cmd.Flags().IntVar(&o.Page, "page", 1, "page number of results, default 1")
 	cmd.Flags().BoolVar(&o.Raw, "raw", false, "full logbook in raw JSON format. overrides all other flags")
-	cmd.Flags().BoolVar(&o.Diagnostic, "diagnostic", false, "print one oplog per line in the format 'MODEL ID OPCOUNT NAME'overrides all other flags")
+	cmd.Flags().BoolVar(&o.Summary, "summary", false, "print one oplog per line in the format 'MODEL ID OPCOUNT NAME'. overrides all other flags")
 
 	return cmd
 }
@@ -178,21 +178,21 @@ The logbook command shows entries for a dataset, from newest to oldest.`,
 type LogbookOptions struct {
 	ioes.IOStreams
 
-	PageSize        int
-	Page            int
-	Refs            *RefSelect
-	Raw, Diagnostic bool
+	PageSize     int
+	Page         int
+	Refs         *RefSelect
+	Raw, Summary bool
 
 	LogMethods *lib.LogMethods
 }
 
 // Complete adds any missing configuration that can only be added just before calling Run
 func (o *LogbookOptions) Complete(f Factory, args []string) (err error) {
-	if o.Raw && o.Diagnostic {
-		return fmt.Errorf("cannot use diagnostic & raw flags at once")
+	if o.Raw && o.Summary {
+		return fmt.Errorf("cannot use summary & raw flags at once")
 	}
 
-	if o.Raw || o.Diagnostic {
+	if o.Raw || o.Summary {
 		if len(args) != 0 {
 			return fmt.Errorf("can't use dataset reference. the raw flag shows the entire logbook")
 		}
@@ -255,10 +255,10 @@ func (o *LogbookOptions) RawLogs() error {
 	return nil
 }
 
-// LogbookDiagnostic prints a logbook overview
-func (o *LogbookOptions) LogbookDiagnostic() error {
+// LogbookSummary prints a logbook overview
+func (o *LogbookOptions) LogbookSummary() error {
 	var res string
-	if err := o.LogMethods.LogbookDiagnostic(&struct{}{}, &res); err != nil {
+	if err := o.LogMethods.LogbookSummary(&struct{}{}, &res); err != nil {
 		return err
 	}
 

lib/log.go

@@ -168,12 +168,12 @@ func (m *LogMethods) PlainLogs(p *PlainLogsParams, res *PlainLogs) (err error) {
 	return err
 }
 
-// LogbookDiagnostic returns a string overview of
-func (m *LogMethods) LogbookDiagnostic(p *struct{}, res *string) (err error) {
+// LogbookSummary returns a string overview of the logbook
+func (m *LogMethods) LogbookSummary(p *struct{}, res *string) (err error) {
 	if m.inst.rpc != nil {
 		return checkRPCError(m.inst.rpc.Call("LogMethods.Diagnostic", p, res))
 	}
 	ctx := context.TODO()
-	*res = m.inst.repo.Logbook().DiagnosticString(ctx)
+	*res = m.inst.repo.Logbook().SummaryString(ctx)
 	return nil
 }

lib/remote.go

@@ -75,12 +75,12 @@ func (r *RemoteMethods) Publish(p *PublicationParams, res *dsref.Ref) error {
 	pro, _ := r.inst.Repo().Profile()
 	ref.ProfileID = pro.ID.String()
 
-	rref := reporef.RefFromDsref(ref)
-	if err = r.inst.RemoteClient().PushDataset(ctx, rref, addr); err != nil {
+	datasetRef := reporef.RefFromDsref(ref)
+	if err = r.inst.RemoteClient().PushDataset(ctx, datasetRef, addr); err != nil {
 		return err
 	}
-	rref.Published = true
-	if err = base.SetPublishStatus(r.inst.node.Repo, &rref, true); err != nil {
+	datasetRef.Published = true
+	if err = base.SetPublishStatus(r.inst.node.Repo, &datasetRef, datasetRef.Published); err != nil {
 		return err
 	}
 

logbook/logbook.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"strings"
+	"sync"
 	"time"
 
 	logger "github.com/ipfs/go-log"
@@ -632,26 +633,28 @@ func (book *Book) WriteRemotePush(ctx context.Context, initID string, revisions
 		return nil, nil, err
 	}
 
+	var (
+		rollbackOnce  sync.Once
+		rollbackError error
+	)
 	// after successful save calling rollback drops the written push operation
-	// rollback can only be called once. it will overwrite itself with a noop func
-	// when called
 	rollback = func(ctx context.Context) error {
-		defer func() {
-			rollback = func(ctx context.Context) error { return nil }
-		}()
-
-		branchLog, err := book.branchLog(ctx, initID)
-		if err != nil {
-			return err
-		}
+		rollbackOnce.Do(func() {
+			branchLog, err := book.branchLog(ctx, initID)
+			if err != nil {
+				rollbackError = err
+				return
+			}
 
-		// TODO (b5) - the fact that this works means accessors are passing data that
-		// if modified will be persisted on save, which may be a *major* source of
-		// bugs if not handled correctly by packages that read & save logbook data
-		// we should consider returning copies, and adding explicit methods for
-		// modification.
-		branchLog.l.Ops = branchLog.l.Ops[:len(branchLog.l.Ops)-1]
-		return book.save(ctx)
+			// TODO (b5) - the fact that this works means accessors are passing data that
+			// if modified will be persisted on save, which may be a *major* source of
+			// bugs if not handled correctly by packages that read & save logbook data
+			// we should consider returning copies, and adding explicit methods for
+			// modification.
+			branchLog.l.Ops = branchLog.l.Ops[:len(branchLog.l.Ops)-1]
+			rollbackError = book.save(ctx)
+		})
+		return rollbackError
 	}
 
 	sparseLog, err := book.UserDatasetBranchesLog(ctx, initID)
@@ -690,20 +693,22 @@ func (book *Book) WriteRemoteDelete(ctx context.Context, initID string, revision
 		return nil, nil, err
 	}
 
+	var (
+		rollbackOnce  sync.Once
+		rollbackError error
+	)
 	// after successful save calling rollback drops the written push operation
-	// rollback can only be called once. it will overwrite itself with a noop func
-	// when called
 	rollback = func(ctx context.Context) error {
-		defer func() {
-			rollback = func(ctx context.Context) error { return nil }
-		}()
-
-		branchLog, err := book.branchLog(ctx, initID)
-		if err != nil {
-			return err
-		}
-		branchLog.l.Ops = branchLog.l.Ops[:len(branchLog.l.Ops)-1]
-		return book.save(ctx)
+		rollbackOnce.Do(func() {
+			branchLog, err := book.branchLog(ctx, initID)
+			if err != nil {
+				rollbackError = err
+				return
+			}
+			branchLog.l.Ops = branchLog.l.Ops[:len(branchLog.l.Ops)-1]
+			rollbackError = book.save(ctx)
+		})
+		return rollbackError
 	}
 
 	sparseLog, err := book.UserDatasetBranchesLog(ctx, initID)
@@ -1148,9 +1153,9 @@ func (book Book) PlainLogs(ctx context.Context) ([]PlainLog, error) {
 	return logs, nil
 }
 
-// DiagnosticString prints the entire hierarchy of logbook model/ID/opcount in a
-// single string
-func (book Book) DiagnosticString(ctx context.Context) string {
+// SummaryString prints the entire hierarchy of logbook model/ID/opcount/name in
+// a single string
+func (book Book) SummaryString(ctx context.Context) string {
 	logs, err := book.store.Logs(ctx, 0, -1)
 	if err != nil {
 		return fmt.Sprintf("error getting diagnostics: %q", err)

logbook/logbook_test.go

@@ -473,13 +473,16 @@ func TestPushModel(t *testing.T) {
 		t.Errorf("expected branch log to have 3 operations. got: %d", len(lg.Logs[0].Logs[0].Ops))
 	}
 
-	t.Log(tr.Book.DiagnosticString(ctx) + "\n\n")
+	t.Log(tr.Book.SummaryString(ctx) + "\n\n")
 
+	if err = rollback(ctx); err != nil {
+		t.Errorf("rollback error: %q", err)
+	}
 	if err = rollback(ctx); err != nil {
 		t.Errorf("rollback error: %q", err)
 	}
 
-	t.Log(tr.Book.DiagnosticString(ctx))
+	t.Log(tr.Book.SummaryString(ctx))
 
 	lg, err = tr.Book.UserDatasetBranchesLog(ctx, initID)
 	if err != nil {
@@ -514,7 +517,7 @@ func TestPushModel(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(lg.Logs[0].Logs[0].Ops) != 2 {
+	if len(lg.Logs[0].Logs[0].Ops) != 3 {
 		t.Errorf("expected branch log to have 3 operations after writing push & delete push. got: %d", len(lg.Logs[0].Logs[0].Ops))
 	}