From ebcb676a5500d4b59b7a741a6169dcaec473284d Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Fri, 15 Nov 2019 16:55:36 -0500 Subject: [PATCH] fix(rename): Allow rename for dataset with no history. Update .qri-ref --- base/ref.go | 6 +- cmd/rename_integration_test.go | 124 +++++++++++++++++++++++++++++++++ fsi/fsi.go | 23 +++--- fsi/fsi_test.go | 10 +-- lib/datasets.go | 10 ++- lib/fsi.go | 30 ++++++-- 6 files changed, 177 insertions(+), 26 deletions(-) create mode 100644 cmd/rename_integration_test.go diff --git a/base/ref.go b/base/ref.go index f9679773e..556320c37 100644 --- a/base/ref.go +++ b/base/ref.go @@ -80,8 +80,10 @@ func ModifyDatasetRef(ctx context.Context, r repo.Repo, current, new *repo.Datas return err } if err := repo.CanonicalizeDatasetRef(r, current); err != nil { - log.Debug(err.Error()) - return fmt.Errorf("error with existing reference: %s", err.Error()) + if err != repo.ErrNoHistory { + log.Debug(err.Error()) + return fmt.Errorf("error with existing reference: %s", err.Error()) + } } err = repo.CanonicalizeDatasetRef(r, new) if err == nil { diff --git a/cmd/rename_integration_test.go b/cmd/rename_integration_test.go new file mode 100644 index 000000000..ba0f074d6 --- /dev/null +++ b/cmd/rename_integration_test.go @@ -0,0 +1,124 @@ +package cmd + +import ( + "os" + "regexp" + "path/filepath" + "testing" + + "github.com/google/go-cmp/cmp" +) + +// Test rename works if dataset has no history +func TestRenameNoHistory(t *testing.T) { + run := NewFSITestRunner(t, "qri_test_rename_no_history") + defer run.Delete() + + workDir := run.CreateAndChdirToWorkDir("remove_no_history") + + // Init as a linked directory + run.MustExec(t, "qri init --name remove_no_history --format csv") + + // Read .qri-ref file, it contains the reference this directory is linked to + actual := run.MustReadFile(t, filepath.Join(workDir, ".qri-ref")) + expect := "test_peer/remove_no_history" + if diff := cmp.Diff(expect, actual); diff != "" { + t.Errorf("qri list (-want +got):\n%s", diff) + } + + // Go up one directory + parentDir := filepath.Dir(workDir) + os.Chdir(parentDir) + + // Rename + run.MustExec(t, "qri rename me/remove_no_history me/remove_second_name") + + // Old dataset name can't be used + err := run.ExecCommand("qri get me/remove_no_history") + if err == nil { + t.Error("expected error, did not get one") + } + expect = "repo: not found" + if err.Error() != expect { + t.Errorf("error mismatch, expect: %s, got: %s", expect, err.Error()) + } + + // New dataset name can be used, but still has no history + err = run.ExecCommand("qri get me/remove_second_name") + if err == nil { + t.Error("expected error, did not get one") + } + expect = "repo: no history" + if err.Error() != expect { + t.Errorf("error mismatch, expect: %s, got: %s", expect, err.Error()) + } + + // Read .qri-ref file, it contains the new reference name + actual = run.MustReadFile(t, filepath.Join(workDir, ".qri-ref")) + expect = "test_peer/remove_second_name" + if diff := cmp.Diff(expect, actual); diff != "" { + t.Errorf("qri list (-want +got):\n%s", diff) + } + + // Test that `qri list` will only show the new ref. Still linked to the old directory name. + output := run.MustExec(t, "qri list") + // TODO(dlong): Move temp omissions into TestRunner framework + regex := regexp.MustCompile("linked: .*/remove_no_history") + replaced := string(regex.ReplaceAll([]byte(output), []byte("linked: /tmp/remove_no_history"))) + expect = `1 test_peer/remove_second_name + linked: /tmp/remove_no_history + +` + if diff := cmp.Diff(expect, replaced); diff != "" { + t.Errorf("qri list (-want +got):\n%s", diff) + } +} + +// Test rename updates the qri-ref link +func TestRenameUpdatesLink(t *testing.T) { + run := NewFSITestRunner(t, "qri_test_rename_update_link") + defer run.Delete() + + workDir := run.CreateAndChdirToWorkDir("remove_update_link") + + // Init as a linked directory + run.MustExec(t, "qri init --name remove_update_link --format csv") + + // Save a version + run.MustExec(t, "qri save") + + // Read .qri-ref file, it contains the reference this directory is linked to + actual := run.MustReadFile(t, filepath.Join(workDir, ".qri-ref")) + expect := "test_peer/remove_update_link" + if diff := cmp.Diff(expect, actual); diff != "" { + t.Errorf("qri list (-want +got):\n%s", diff) + } + + // Go up one directory + parentDir := filepath.Dir(workDir) + os.Chdir(parentDir) + + // Rename + run.MustExec(t, "qri rename me/remove_update_link me/remove_second_name") + + // Test that `qri list` will only show the new ref. Still linked to the old directory name. + output := run.MustExec(t, "qri list") + // TODO(dlong): Move temp omissions into TestRunner framework + regex := regexp.MustCompile("linked: .*/remove_update_link") + replaced := string(regex.ReplaceAll([]byte(output), []byte("linked: /tmp/remove_update_link"))) + expect = `1 test_peer/remove_second_name + linked: /tmp/remove_update_link + 22 B, 2 entries, 0 errors + +` + if diff := cmp.Diff(expect, replaced); diff != "" { + t.Errorf("qri list (-want +got):\n%s", diff) + } + + // Read .qri-ref file, it contains the new dataset reference + actual = run.MustReadFile(t, filepath.Join(workDir, ".qri-ref")) + expect = "test_peer/remove_second_name" + if diff := cmp.Diff(expect, actual); diff != "" { + t.Errorf("read .qri-ref (-want +got):\n%s", diff) + } +} diff --git a/fsi/fsi.go b/fsi/fsi.go index 57f8796e5..b99b2549b 100644 --- a/fsi/fsi.go +++ b/fsi/fsi.go @@ -143,20 +143,25 @@ func (fsi *FSI) CreateLink(dirPath, refStr string) (alias string, rollback func( return ref.AliasString(), removeLinkAndRemoveRefFunc, err } -// UpdateLink changes an existing link entry -func (fsi *FSI) UpdateLink(dirPath, refStr string) (string, error) { +// ModifyLinkDirectory changes the directory for the linked dataset reference +func (fsi *FSI) ModifyLinkDirectory(dirPath, refStr string) error { + return fmt.Errorf("TODO: Implement me") +} + +// ModifyLinkReference changes the reference for the existing linked working directory +func (fsi *FSI) ModifyLinkReference(dirPath, refStr string) error { ref, err := repo.ParseDatasetRef(refStr) if err != nil { - return "", err + return err } - - if err = repo.CanonicalizeDatasetRef(fsi.repo, &ref); err != nil { - return ref.String(), err + if err = repo.CanonicalizeDatasetRef(fsi.repo, &ref); err != nil && err != repo.ErrNoHistory { + return err } - ref.FSIPath = dirPath - err = fsi.repo.PutRef(ref) - return ref.String(), err + if _, err = writeLinkFile(dirPath, ref.AliasString()); err != nil { + return err + } + return nil } // Unlink breaks the connection between a directory and a dataset diff --git a/fsi/fsi_test.go b/fsi/fsi_test.go index 92ad0a2a4..32345d6a1 100644 --- a/fsi/fsi_test.go +++ b/fsi/fsi_test.go @@ -199,7 +199,7 @@ func TestCreateLinkAgainOnceQriRefRemoved(t *testing.T) { } } -func TestUpdateLink(t *testing.T) { +func TestModifyLink(t *testing.T) { paths := NewTmpPaths() defer paths.Close() @@ -208,13 +208,9 @@ func TestUpdateLink(t *testing.T) { if err != nil { t.Fatalf(err.Error()) } - link, err := fsi.UpdateLink(paths.firstDir, "me/test_ds@/ipfs/QmExample") + err = fsi.ModifyLinkReference(paths.firstDir, "me/test_ds@/ipfs/QmExample") if err != nil { - t.Fatalf(err.Error()) - } - expect := `peer/test_ds@/ipfs/QmExample` - if link != expect { - t.Errorf("error: link did not match, actual: %s, expect: %s", link, expect) + t.Errorf("expected ModifyLinkReference to succeed, got: %s", err.Error()) } } diff --git a/lib/datasets.go b/lib/datasets.go index 5632de204..c3c0f44d7 100644 --- a/lib/datasets.go +++ b/lib/datasets.go @@ -536,11 +536,19 @@ func (r *DatasetRequests) Rename(p *RenameParams, res *repo.DatasetRef) (err err return fmt.Errorf("current name is required to rename a dataset") } + // Update the reference stored in the repo if err := base.ModifyDatasetRef(ctx, r.node.Repo, &p.Current, &p.New, true /*isRename*/); err != nil { return err } - if err = base.ReadDataset(ctx, r.node.Repo, &p.New); err != nil { + // If the dataset is linked to a working directory, update the ref + if p.New.FSIPath != "" { + if err = r.inst.fsi.ModifyLinkReference(p.New.FSIPath, p.New.String()); err != nil { + return err + } + } + + if err = base.ReadDataset(ctx, r.node.Repo, &p.New); err != nil && err != repo.ErrNoHistory { log.Debug(err.Error()) return err } diff --git a/lib/fsi.go b/lib/fsi.go index 5c911f988..92a5a9db9 100644 --- a/lib/fsi.go +++ b/lib/fsi.go @@ -39,8 +39,9 @@ func (m *FSIMethods) LinkedRefs(p *ListParams, res *[]repo.DatasetRef) (err erro // LinkParams encapsulate parameters to the link method type LinkParams struct { - Dir string - Ref string + Dir string + Ref string + ToModify string } // CreateLink creates a connection between a working drirectory and a dataset history @@ -61,14 +62,29 @@ func (m *FSIMethods) CreateLink(p *LinkParams, res *string) (err error) { return err } -// UpdateLink creates a connection between a working drirectory and a dataset history -func (m *FSIMethods) UpdateLink(p *LinkParams, res *string) (err error) { +// ModifyLink changes an existing link by either updating the ref for a directory, or vice versa +func (m *FSIMethods) ModifyLink(p *LinkParams, res *bool) (err error) { + // absolutize path name + path, err := filepath.Abs(p.Dir) + if err != nil { + return err + } + + p.Dir = path + if m.inst.rpc != nil { - return m.inst.rpc.Call("FSIMethods.UpdateLink", p, res) + return m.inst.rpc.Call("FSIMethods.ModifyLink", p, res) } - *res, err = m.inst.fsi.UpdateLink(p.Dir, p.Ref) - return err + if p.ToModify == "dir" { + *res = true + return m.inst.fsi.ModifyLinkDirectory(p.Dir, p.Ref) + } else if p.ToModify == "ref" { + *res = true + return m.inst.fsi.ModifyLinkReference(p.Dir, p.Ref) + } else { + return fmt.Errorf("ToModify has unknown value %q", p.ToModify) + } } // Unlink rmeoves a connection between a working drirectory and a dataset history