From 67f5ad5fd03747f8da86167ecd45b4a42cfd159b Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Fri, 8 Nov 2019 19:18:12 -0500 Subject: [PATCH] fix(remove): Fix multiple problems with remove 1) remove now works with FSI, by reading the ref in .qri-ref 2) remove with no reference is an error, before it was silently ignored 3) removing a single version no longer unlinks the dsref in FSI 4) change --files to --keep-files, which has the opposite meaning 5) in FSI, removing a dataset version will update files so the wd is clean 6) but removing a dataset is not allowed if the wd is dirty 7) unless the --keep-files flag is enabled, which won't modify the wd Fixes all of issue #1000 --- api/datasets.go | 7 +- api/testdata/api.snapshot | Bin 204295 -> 204265 bytes base/dataset.go | 14 +- base/dataset_test.go | 6 +- base/ref.go | 2 +- cmd/fsi_integration_test.go | 35 +++ cmd/remove.go | 90 +++--- cmd/remove_integration_test.go | 538 +++++++++++++++++++++++++++++++++ cmd/remove_test.go | 14 +- fsi/fsi.go | 21 ++ fsi/status.go | 18 ++ lib/datasets.go | 147 ++++----- lib/datasets_test.go | 24 +- remote/remote.go | 2 +- 14 files changed, 757 insertions(+), 161 deletions(-) create mode 100644 cmd/remove_integration_test.go diff --git a/api/datasets.go b/api/datasets.go index dc1066699..d04ddc216 100644 --- a/api/datasets.go +++ b/api/datasets.go @@ -475,10 +475,9 @@ func (h *DatasetHandlers) saveHandler(w http.ResponseWriter, r *http.Request) { func (h *DatasetHandlers) removeHandler(w http.ResponseWriter, r *http.Request) { p := lib.RemoveParams{ - Ref: HTTPPathToQriPath(r.URL.Path[len("/remove"):]), - Revision: dsref.Rev{Field: "ds", Gen: -1}, - Unlink: r.FormValue("unlink") == "true", - DeleteFSIFiles: r.FormValue("files") == "true", + Ref: HTTPPathToQriPath(r.URL.Path[len("/remove"):]), + Revision: dsref.Rev{Field: "ds", Gen: -1}, + KeepFiles: r.FormValue("keep-files") == "true", } if r.FormValue("all") == "true" { p.Revision = dsref.NewAllRevisions() diff --git a/api/testdata/api.snapshot b/api/testdata/api.snapshot index 3aeb3d67ac6d78c5a55ebbbbc549089cfa2cbb6b..8df85e9b3c66c9191acc21306e270cc8bc977d7f 100755 GIT binary patch delta 19 bcmZqQ!}D@CPeTji7AD@R?b=mLcNPNxQ$`3M delta 44 zcmaF4o2PvbPeTji7AD>*376EI)RNQ`w_s1V%$(F>B`bxr#GK+(o$c&ZOpg`=0CkiQ ASO5S3 diff --git a/base/dataset.go b/base/dataset.go index 02ebe3277..e074b4150 100644 --- a/base/dataset.go +++ b/base/dataset.go @@ -278,24 +278,24 @@ func UnpinDataset(ctx context.Context, r repo.Repo, ref repo.DatasetRef) error { // the most recent version // when n == -1, remove all versions // does not remove the dataset reference -func RemoveNVersionsFromStore(ctx context.Context, r repo.Repo, ref *repo.DatasetRef, n int) error { +func RemoveNVersionsFromStore(ctx context.Context, r repo.Repo, ref *repo.DatasetRef, n int) (*repo.DatasetRef, error) { var err error if r == nil { - return fmt.Errorf("need a repo") + return nil, fmt.Errorf("need a repo") } // ref is nil or ref has no path err if ref == nil || ref.Path == "" { - return fmt.Errorf("need a dataset reference with a path") + return nil, fmt.Errorf("need a dataset reference with a path") } if n < -1 { - return fmt.Errorf("invalid 'n', n should be n >= 0 or n == -1 to indicate removing all versions") + return nil, fmt.Errorf("invalid 'n', n should be n >= 0 or n == -1 to indicate removing all versions") } // load previous dataset into prev ref.Dataset, err = dsfs.LoadDatasetRefs(ctx, r.Store(), ref.Path) if err != nil { - return err + return nil, err } curr := *ref @@ -307,7 +307,7 @@ func RemoveNVersionsFromStore(ctx context.Context, r repo.Repo, ref *repo.Datase i-- // unpin dataset, ignoring "not pinned" errors if err = UnpinDataset(ctx, r, curr); err != nil && !strings.Contains(err.Error(), "not pinned") { - return err + return nil, err } // if no previous path, break if curr.Dataset.PreviousPath == "" { @@ -334,5 +334,5 @@ func RemoveNVersionsFromStore(ctx context.Context, r repo.Repo, ref *repo.Datase err = nil } - return nil + return &curr, nil } diff --git a/base/dataset_test.go b/base/dataset_test.go index acef71089..6f4896807 100644 --- a/base/dataset_test.go +++ b/base/dataset_test.go @@ -147,7 +147,7 @@ func TestRemoveNVersionsFromStore(t *testing.T) { } for _, c := range bad { - err := RemoveNVersionsFromStore(ctx, c.store, c.ref, c.n) + _, err := RemoveNVersionsFromStore(ctx, c.store, c.ref, c.n) if err == nil { t.Errorf("case %s expected: '%s', got no error", c.description, c.err) continue @@ -181,7 +181,7 @@ func TestRemoveNVersionsFromStore(t *testing.T) { for _, c := range good { // remove latestRef := refs[len(refs)-1] - err := RemoveNVersionsFromStore(ctx, r, latestRef, c.n) + _, err := RemoveNVersionsFromStore(ctx, r, latestRef, c.n) if err != nil { t.Errorf("case '%s', unexpected err: %s", c.description, err.Error()) } @@ -210,7 +210,7 @@ func TestRemoveNVersionsFromStore(t *testing.T) { update := updateCitiesDataset(t, r, fmt.Sprintf("example city data version %d", i)) refs = append(refs, &update) } - err := RemoveNVersionsFromStore(ctx, r, refs[len(refs)-1], -1) + _, err := RemoveNVersionsFromStore(ctx, r, refs[len(refs)-1], -1) if err != nil { t.Errorf("case 'remove all', unexpected err: %s", err.Error()) } diff --git a/base/ref.go b/base/ref.go index db91bcb94..f9679773e 100644 --- a/base/ref.go +++ b/base/ref.go @@ -99,7 +99,7 @@ func ModifyDatasetRef(ctx context.Context, r repo.Repo, current, new *repo.Datas return err } } - + new.FSIPath = current.FSIPath if err = r.DeleteRef(*current); err != nil { return err } diff --git a/cmd/fsi_integration_test.go b/cmd/fsi_integration_test.go index e91e2f0b4..f7bb64b93 100644 --- a/cmd/fsi_integration_test.go +++ b/cmd/fsi_integration_test.go @@ -80,6 +80,30 @@ func (fr *FSITestRunner) GetCommandOutput() string { return fr.RepoRoot.GetOutput() } +// MustExec runs a command, returning standard output, failing the test if there's an error +func (fr *FSITestRunner) MustExec(cmdText string) string { + if err := fr.ExecCommand(cmdText); err != nil { + fr.RepoRoot.t.Fatal(err) + } + return fr.GetCommandOutput() +} + +// MustWriteFile writes to a file, failing the test if there's an error +func (fr *FSITestRunner) MustWriteFile(filename, contents string) { + if err := ioutil.WriteFile(filename, []byte(contents), os.FileMode(0644)); err != nil { + fr.RepoRoot.t.Fatal(err) + } +} + +// MustReadFile reads a file, failing the test if there's an error +func (fr *FSITestRunner) MustReadFile(filename string) string { + bytes, err := ioutil.ReadFile(filename) + if err != nil { + fr.RepoRoot.t.Fatal(err) + } + return string(bytes) +} + // ChdirToRoot changes the current directory to the temporary root func (fr *FSITestRunner) ChdirToRoot() { os.Chdir(fr.RootPath) @@ -163,6 +187,17 @@ run ` + "`qri save`" + ` to commit this dataset // TODO: Verify that files are in ipfs repo. + // Verify that the .qri-ref contains the full path for the saved dataset. + bytes, err := ioutil.ReadFile(".qri-ref") + if err != nil { + t.Fatal(err) + } + // TODO(dlong): Fix me, should write the updated FSI link with the dsref head + expect = "test_peer/brand_new" + if diff := cmp.Diff(expect, string(bytes)); diff != "" { + t.Errorf(".qri-ref contents (-want +got):\n%s", diff) + } + // Status again, check that the working directory is clean. if err := fr.ExecCommand("qri status"); err != nil { t.Fatal(err) diff --git a/cmd/remove.go b/cmd/remove.go index 251024537..cf84e81b0 100644 --- a/cmd/remove.go +++ b/cmd/remove.go @@ -44,8 +44,7 @@ both qri & IPFS. Promise.`, cmd.Flags().StringVarP(&o.RevisionsText, "revisions", "r", "", "revisions to delete") cmd.Flags().BoolVarP(&o.All, "all", "a", false, "synonym for --revisions=all") - cmd.Flags().BoolVar(&o.DeleteFSIFiles, "files", false, "delete linked files in dataset directory") - cmd.Flags().BoolVar(&o.Unlink, "unlink", false, "break link to directory") + cmd.Flags().BoolVar(&o.KeepFiles, "keep-files", false, "") return cmd } @@ -54,48 +53,48 @@ both qri & IPFS. Promise.`, type RemoveOptions struct { ioes.IOStreams - Args []string + Refs *RefSelect - RevisionsText string - Revision dsref.Rev - All bool - DeleteFSIFiles bool - Unlink bool + RevisionsText string + Revision dsref.Rev + All bool + KeepFiles bool DatasetRequests *lib.DatasetRequests } // Complete adds any missing configuration that can only be added just before calling Run func (o *RemoveOptions) Complete(f Factory, args []string) (err error) { - o.Args = args if o.DatasetRequests, err = f.DatasetRequests(); err != nil { return err } + if o.Refs, err = GetCurrentRefSelect(f, args, -1); err != nil { + return err + } if o.All { o.Revision = dsref.NewAllRevisions() } else { if o.RevisionsText == "" { - o.Revision = dsref.Rev{Field: "ds", Gen: 0} - } else { - revisions, err := dsref.ParseRevs(o.RevisionsText) - if err != nil { - return err - } - if len(revisions) != 1 { - return fmt.Errorf("need exactly 1 revision parameter to remove") - } - if revisions[0] == nil { - return fmt.Errorf("invalid nil revision") - } - o.Revision = *revisions[0] + return fmt.Errorf("need --all or --revisions to specify how many revisions to remove") } + revisions, err := dsref.ParseRevs(o.RevisionsText) + if err != nil { + return err + } + if len(revisions) != 1 { + return fmt.Errorf("need exactly 1 revision parameter to remove") + } + if revisions[0] == nil { + return fmt.Errorf("invalid nil revision") + } + o.Revision = *revisions[0] } return err } // Validate checks that all user input is valid func (o *RemoveOptions) Validate() error { - if len(o.Args) == 0 { + if o.Refs.Ref() == "" { return lib.NewError(lib.ErrBadArgs, "please specify a dataset path or name you would like to remove from your qri node") } return nil @@ -103,32 +102,29 @@ func (o *RemoveOptions) Validate() error { // Run executes the remove command func (o *RemoveOptions) Run() (err error) { - for _, arg := range o.Args { - params := lib.RemoveParams{ - Ref: arg, - Revision: o.Revision, - DeleteFSIFiles: o.DeleteFSIFiles, - Unlink: o.Unlink, - } + printRefSelect(o.Out, o.Refs) - res := lib.RemoveResponse{} - if err = o.DatasetRequests.Remove(¶ms, &res); err != nil { - if err.Error() == "repo: not found" { - return lib.NewError(err, fmt.Sprintf("could not find dataset '%s'", arg)) - } - return err - } - if res.NumDeleted == dsref.AllGenerations { - printSuccess(o.Out, "removed entire dataset '%s'", res.Ref) - } else if res.NumDeleted != 0 { - printSuccess(o.Out, "removed %d revisions of dataset '%s'", res.NumDeleted, res.Ref) - } - if res.DeletedFSIFiles { - printSuccess(o.Out, "deleted dataset files") - } - if res.Unlinked { - printSuccess(o.Out, "removed dataset link") + params := lib.RemoveParams{ + Ref: o.Refs.Ref(), + Revision: o.Revision, + KeepFiles: o.KeepFiles, + } + + res := lib.RemoveResponse{} + if err = o.DatasetRequests.Remove(¶ms, &res); err != nil { + if err.Error() == "repo: not found" { + return lib.NewError(err, fmt.Sprintf("could not find dataset '%s'", o.Refs.Ref())) } + return err + } + + if res.NumDeleted == dsref.AllGenerations { + printSuccess(o.Out, "removed entire dataset '%s'", res.Ref) + } else if res.NumDeleted != 0 { + printSuccess(o.Out, "removed %d revisions of dataset '%s'", res.NumDeleted, res.Ref) + } + if res.Unlinked { + printSuccess(o.Out, "removed dataset link") } return nil } diff --git a/cmd/remove_integration_test.go b/cmd/remove_integration_test.go new file mode 100644 index 000000000..d724d08b3 --- /dev/null +++ b/cmd/remove_integration_test.go @@ -0,0 +1,538 @@ +package cmd + +import ( + "context" + "io/ioutil" + "os" + "regexp" + "strings" + "time" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/qri-io/qri/base/dsfs" + "github.com/spf13/cobra" +) + +// RemoveTestRunner holds test info integration tests +type RemoveTestRunner struct { + RepoRoot *TestRepoRoot + Context context.Context + ContextDone func() + TsFunc func() time.Time + LocOrig *time.Location + CmdR *cobra.Command +} + +// newRemoveTestRunner returns a new FSITestRunner. +func newRemoveTestRunner(t *testing.T, peerName, testName string) *RemoveTestRunner { + root := NewTestRepoRoot(t, peerName, testName) + + run := RemoveTestRunner{} + run.RepoRoot = &root + run.Context, run.ContextDone = context.WithCancel(context.Background()) + + // To keep hashes consistent, artificially specify the timestamp by overriding + // the dsfs.Timestamp func + counter := 0 + run.TsFunc = dsfs.Timestamp + dsfs.Timestamp = func() time.Time { + counter++ + return time.Date(2001, 01, 01, 01, 01, counter, 01, time.UTC) + } + + // Set the location to New York so that timezone printing is consistent + location, err := time.LoadLocation("America/New_York") + if err != nil { + panic(err) + } + run.LocOrig = location + StringerLocation = location + + return &run +} + +// Delete cleans up after a RemoveTestRunner is done being used. +func (run *RemoveTestRunner) Delete() { + dsfs.Timestamp = run.TsFunc + StringerLocation = run.LocOrig + run.ContextDone() + run.RepoRoot.Delete() +} + +// ExecCommand executes the given command string +func (run *RemoveTestRunner) ExecCommand(cmdText string) error { + run.CmdR = run.RepoRoot.CreateCommandRunner(run.Context) + return executeCommand(run.CmdR, cmdText) +} + +// MustExec runs a command, returning standard output, failing the test if there's an error +func (run *RemoveTestRunner) MustExec(cmdText string) string { + if err := run.ExecCommand(cmdText); err != nil { + run.RepoRoot.t.Fatal(err) + } + return run.GetCommandOutput() +} + +// MustWriteFile writes to a file, failing the test if there's an error +func (run *RemoveTestRunner) MustWriteFile(filename, contents string) { + if err := ioutil.WriteFile(filename, []byte(contents), os.FileMode(0644)); err != nil { + run.RepoRoot.t.Fatal(err) + } +} + +// MustReadFile reads a file, failing the test if there's an error +func (run *RemoveTestRunner) MustReadFile(filename string) string { + bytes, err := ioutil.ReadFile(filename) + if err != nil { + run.RepoRoot.t.Fatal(err) + } + return string(bytes) +} + +// GetCommandOutput returns the standard output from the previously executed command +func (run *RemoveTestRunner) GetCommandOutput() string { + return run.RepoRoot.GetOutput() +} + +func parsePathFromRef(ref string) string { + pos := strings.Index(ref, "@") + if pos == -1 { + return ref + } + return ref[pos+1:] +} + +// Test that adding two versions, then deleting one, ends up with only the first version +func TestRemoveOneRevisionFromRepo(t *testing.T) { + run := newRemoveTestRunner(t, "test_peer", "qri_test_remove_one_rev_from_repo") + defer run.Delete() + + // Save a dataset containing a body.json, no meta, nothing special. + output := run.MustExec("qri save --body=testdata/movies/body_two.json me/remove_test") + ref1 := parsePathFromRef(parseRefFromSave(output)) + dsPath1 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath1 { + t.Fatal("ref from first save should match what is in qri repo") + } + + // Save another version + output = run.MustExec("qri save --body=testdata/movies/body_four.json me/remove_test") + ref2 := parsePathFromRef(parseRefFromSave(output)) + dsPath2 := run.RepoRoot.GetPathForDataset(0) + if ref2 != dsPath2 { + t.Fatal("ref from second save should match what is in qri repo") + } + + // Remove one version + run.MustExec("qri remove --revisions=1 me/remove_test") + + // Verify that dsref of HEAD is the same as the result of the first save command + dsPath3 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath3 { + t.Errorf("after delete, ref should match the first version, expected: %s\n, got: %s\n", + ref1, dsPath3) + } +} + +// Test that adding two versions, then deleting all will end up with nothing left +func TestRemoveAllRevisionsFromRepo(t *testing.T) { + run := newRemoveTestRunner(t, "test_peer", "qri_test_remove_all_rev_from_repo") + defer run.Delete() + + // Save a dataset containing a body.json, no meta, nothing special. + output := run.MustExec("qri save --body=testdata/movies/body_two.json me/remove_test") + ref1 := parsePathFromRef(parseRefFromSave(output)) + dsPath1 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath1 { + t.Fatal("ref from first save should match what is in qri repo") + } + + // Save another version + output = run.MustExec("qri save --body=testdata/movies/body_four.json me/remove_test") + ref2 := parsePathFromRef(parseRefFromSave(output)) + dsPath2 := run.RepoRoot.GetPathForDataset(0) + if ref2 != dsPath2 { + t.Fatal("ref from second save should match what is in qri repo") + } + + // Remove one version + run.MustExec("qri remove --all me/remove_test") + + // Verify that dsref of HEAD is the same as the result of the first save command + dsPath3 := run.RepoRoot.GetPathForDataset(0) + if dsPath3 != "" { + t.Errorf("after delete, dataset should not exist, got: %s\n", dsPath3) + } +} + +// Test that remove from a repo can't be used with --keep-files flag +func TestRemoveRepoCantUseKeepFiles(t *testing.T) { + run := newRemoveTestRunner(t, "test_peer", "qri_test_remove_repo_cant_use_keep_files") + defer run.Delete() + + // Save a dataset containing a body.json, no meta, nothing special. + output := run.MustExec("qri save --body=testdata/movies/body_two.json me/remove_test") + ref1 := parsePathFromRef(parseRefFromSave(output)) + dsPath1 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath1 { + t.Fatal("ref from first save should match what is in qri repo") + } + + // Save another version + output = run.MustExec("qri save --body=testdata/movies/body_four.json me/remove_test") + ref2 := parsePathFromRef(parseRefFromSave(output)) + dsPath2 := run.RepoRoot.GetPathForDataset(0) + if ref2 != dsPath2 { + t.Fatal("ref from second save should match what is in qri repo") + } + + // Try to remove with the --keep-files flag should produce an error + err := run.ExecCommand("qri remove --revisions=1 --keep-files me/remove_test") + if err == nil { + t.Fatal("expected error trying to remove with --keep-files, did not get an error") + } + expect := `dataset is not linked to filesystem, cannot use keep-files` + if err.Error() != expect { + t.Errorf("error mismatch, expect: %s, got: %s", expect, err.Error()) + } +} + +// Test removing a revision from a linked directory +func TestRemoveOneRevisionFromWorkingDirectory(t *testing.T) { + run := NewFSITestRunner(t, "qri_test_remove_one_work_dir") + defer run.Delete() + + _ = run.CreateAndChdirToWorkDir("remove_one") + + // Init as a linked directory. + run.MustExec("qri init --name remove_one --format csv") + + // Add a meta.json. + run.MustWriteFile("meta.json", "{\"title\":\"one\"}\n") + + // Save the new dataset. + output := run.MustExec("qri save") + ref1 := parsePathFromRef(parseRefFromSave(output)) + dsPath1 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath1 { + t.Fatal("ref from first save should match what is in qri repo") + } + + // Modify meta.json and body.csv. + run.MustWriteFile("meta.json", "{\"title\":\"two\"}\n") + run.MustWriteFile("body.csv", "seven,eight,9\n") + + // Save the new dataset. + output = run.MustExec("qri save") + ref2 := parsePathFromRef(parseRefFromSave(output)) + dsPath2 := run.RepoRoot.GetPathForDataset(0) + if ref2 != dsPath2 { + t.Fatal("ref from second save should match what is in qri repo") + } + + // Remove one revision + run.MustExec("qri remove --revisions=1") + + // Verify that dsref of HEAD is the same as the result of the first save command + dsPath3 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath3 { + t.Errorf("after delete, ref should match the first version, expected: %s\n, got: %s\n", + ref1, dsPath3) + } + + // Verify the meta.json contains the original contents, not `{"title":"two"}` + actual := run.MustReadFile("meta.json") + expect := `{ + "qri": "md:0", + "title": "one" +}` + if diff := cmp.Diff(expect, actual); diff != "" { + t.Errorf("meta.json contents (-want +got):\n%s", diff) + } + + // Verify the body.csv contains the original contents, not "seven,eight,9" + actual = run.MustReadFile("body.csv") + expect = "one,two,3\nfour,five,6\n" + if diff := cmp.Diff(expect, actual); diff != "" { + t.Errorf("body.csv contents (-want +got):\n%s", diff) + } + + // Verify that status is clean + output = run.MustExec("qri status") + expect = `for linked dataset [test_peer/remove_one] + +working directory clean +` + if diff := cmpTextLines(expect, output); diff != "" { + t.Errorf("qri status (-want +got):\n%s", diff) + } + + // Verify that we can access the working directory. This would not be the case if the + // delete operation caused the FSIPath to be moved from the dataset ref in the repo. + actual = run.MustExec("qri get") + // TODO(dlong): Move temp omissions into TestRunner framework + regex := regexp.MustCompile("bodyPath: .*/remove_one/body.csv") + replaced := string(regex.ReplaceAll([]byte(actual), []byte("bodyPath: remove_one/body.csv"))) + expect = `for linked dataset [test_peer/remove_one] + +bodyPath: remove_one/body.csv +meta: + qri: md:0 + title: one +name: remove_one +peername: test_peer +qri: ds:0 +structure: + format: csv + formatConfig: + lazyQuotes: true + qri: st:0 + schema: + items: + items: + - title: field_1 + type: string + - title: field_2 + type: string + - title: field_3 + type: integer + type: array + type: array + +` + if diff := cmp.Diff(expect, replaced); diff != "" { + t.Errorf("dataset result from get: (-want +got):\n%s", diff) + } +} + +// Test removing a revision which added a component will cause that component's file to be removed +func TestRemoveOneRevisionWillDeleteFilesThatWereNotThereBefore(t *testing.T) { + run := NewFSITestRunner(t, "qri_test_remove_one_that_wasnt_there") + defer run.Delete() + + workDir := run.CreateAndChdirToWorkDir("remove_one") + + // Init as a linked directory. + run.MustExec("qri init --name remove_one --format csv") + + // Save the new dataset. + output := run.MustExec("qri save") + ref1 := parsePathFromRef(parseRefFromSave(output)) + dsPath1 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath1 { + t.Fatal("ref from first save should match what is in qri repo") + } + + // Modify meta.json and body.csv. + run.MustWriteFile("meta.json", "{\"title\":\"two\"}\n") + run.MustWriteFile("body.csv", "seven,eight,9\n") + + // Save the new dataset. + output = run.MustExec("qri save") + ref2 := parsePathFromRef(parseRefFromSave(output)) + dsPath2 := run.RepoRoot.GetPathForDataset(0) + if ref2 != dsPath2 { + t.Fatal("ref from second save should match what is in qri repo") + } + + // Remove one revision + run.MustExec("qri remove --revisions=1") + + // Verify that dsref of HEAD is the same as the result of the first save command + dsPath3 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath3 { + t.Errorf("after delete, ref should match the first version, expected: %s\n, got: %s\n", + ref1, dsPath3) + } + + // Verify that status is clean + output = run.MustExec("qri status") + expect := `for linked dataset [test_peer/remove_one] + +working directory clean +` + if diff := cmpTextLines(expect, output); diff != "" { + t.Errorf("qri status (-want +got):\n%s", diff) + } + + // Verify the directory contains the body, but not the meta + dirContents := listDirectory(workDir) + // TODO(dlong): meta.json is written, but is empty. Need to figure out how to determine + // not to write it, without breaking other things. + expectContents := []string{".qri-ref", "body.csv", "meta.json", "structure.json"} + if diff := cmp.Diff(expectContents, dirContents); diff != "" { + t.Errorf("directory contents (-want +got):\n%s", diff) + } +} + +// Test removing a revision while keeping the files the same +func TestRemoveKeepFiles(t *testing.T) { + run := NewFSITestRunner(t, "qri_test_remove_one_keep_files") + defer run.Delete() + + _ = run.CreateAndChdirToWorkDir("remove_one") + + // Init as a linked directory. + run.MustExec("qri init --name remove_one --format csv") + + // Save the new dataset. + output := run.MustExec("qri save") + ref1 := parsePathFromRef(parseRefFromSave(output)) + dsPath1 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath1 { + t.Fatal("ref from first save should match what is in qri repo") + } + + // Modify body.csv. + run.MustWriteFile("body.csv", "seven,eight,9\n") + + // Save the new dataset. + output = run.MustExec("qri save") + ref2 := parsePathFromRef(parseRefFromSave(output)) + dsPath2 := run.RepoRoot.GetPathForDataset(0) + if ref2 != dsPath2 { + t.Fatal("ref from second save should match what is in qri repo") + } + + // Modify body.csv again. + run.MustWriteFile("body.csv", "ten,eleven,12\n") + + // Try to remove, but this will result an error because working directory is not clean + err := run.ExecCommand("qri remove --revisions=1") + if err == nil { + t.Fatal("expected error because working directory is not clean") + } + expect := `cannot remove from dataset while working directory is dirty` + if err.Error() != expect { + t.Errorf("error mismatch, expect: %s, got: %s", expect, err.Error()) + } + + // Verify that dsref of HEAD is still the result of the second save + dsPath3 := run.RepoRoot.GetPathForDataset(0) + if ref2 != dsPath3 { + t.Errorf("no commits should have been removed, expected: %s\n, got: %s\n", + ref2, dsPath3) + } + + // Remove is possible using --keep-files + run.MustExec("qri remove --revisions=1 --keep-files") + + // Verify that dsref is now the result of the first save because one commit was removed + dsPath4 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath4 { + t.Errorf("no commits should have been removed, expected: %s\n, got: %s\n", + ref1, dsPath4) + } + + // Verify the body.csv contains the newest version and was not removed + actual := run.MustReadFile("body.csv") + expect = "ten,eleven,12\n" + if diff := cmp.Diff(expect, actual); diff != "" { + t.Errorf("body.csv contents (-want +got):\n%s", diff) + } + + // Verify that status is dirty because we kept the files + output = run.MustExec("qri status") + expect = `for linked dataset [test_peer/remove_one] + + modified: body (source: body.csv) + +run ` + "`qri save`" + ` to commit this dataset +` + if diff := cmpTextLines(expect, output); diff != "" { + t.Errorf("qri status (-want +got):\n%s", diff) + } +} + +// Test removing all versions from a working directory +func TestRemoveAllVersionsWorkingDirectory(t *testing.T) { + run := NewFSITestRunner(t, "qri_test_remove_all_work_dir") + defer run.Delete() + + workDir := run.CreateAndChdirToWorkDir("remove_all") + + // Init as a linked directory. + run.MustExec("qri init --name remove_all --format csv") + + // Save the new dataset. + output := run.MustExec("qri save") + ref1 := parsePathFromRef(parseRefFromSave(output)) + dsPath1 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath1 { + t.Fatal("ref from first save should match what is in qri repo") + } + + // Modify body.csv. + run.MustWriteFile("body.csv", "seven,eight,9\n") + + // Save the new dataset. + output = run.MustExec("qri save") + ref2 := parsePathFromRef(parseRefFromSave(output)) + dsPath2 := run.RepoRoot.GetPathForDataset(0) + if ref2 != dsPath2 { + t.Fatal("ref from second save should match what is in qri repo") + } + + // Remove all versions + run.MustExec("qri remove --all=1") + + // Verify that dsref of HEAD is empty + dsPath3 := run.RepoRoot.GetPathForDataset(0) + if dsPath3 != "" { + t.Errorf("after delete, ref should be empty, got: %s", dsPath3) + } + + // Verify the directory contains none of the component files + dirContents := listDirectory(workDir) + expectContents := []string{} + if diff := cmp.Diff(expectContents, dirContents); diff != "" { + t.Errorf("directory contents (-want +got):\n%s", diff) + } +} + +// Test removing all versions while keeping files +func TestRemoveAllAndKeepFiles(t *testing.T) { + run := NewFSITestRunner(t, "qri_test_remove_all_keep_files") + defer run.Delete() + + workDir := run.CreateAndChdirToWorkDir("remove_all") + + // Init as a linked directory. + run.MustExec("qri init --name remove_all --format csv") + + // Save the new dataset. + output := run.MustExec("qri save") + ref1 := parsePathFromRef(parseRefFromSave(output)) + dsPath1 := run.RepoRoot.GetPathForDataset(0) + if ref1 != dsPath1 { + t.Fatal("ref from first save should match what is in qri repo") + } + + // Modify body.csv. + run.MustWriteFile("body.csv", "seven,eight,9\n") + + // Save the new dataset. + output = run.MustExec("qri save") + ref2 := parsePathFromRef(parseRefFromSave(output)) + dsPath2 := run.RepoRoot.GetPathForDataset(0) + if ref2 != dsPath2 { + t.Fatal("ref from second save should match what is in qri repo") + } + + // Remove all but --keep-files + run.MustExec("qri remove --revisions=all --keep-files") + + // Verify that dsref of HEAD is empty + dsPath3 := run.RepoRoot.GetPathForDataset(0) + if dsPath3 != "" { + t.Errorf("after delete, ref should be empty, got: %s", dsPath3) + } + + // Verify the directory contains the files that we expect. + dirContents := listDirectory(workDir) + expectContents := []string{"body.csv", "meta.json", "structure.json"} + if diff := cmp.Diff(expectContents, dirContents); diff != "" { + t.Errorf("directory contents (-want +got):\n%s", diff) + } +} diff --git a/cmd/remove_test.go b/cmd/remove_test.go index 6e6380796..90e655478 100644 --- a/cmd/remove_test.go +++ b/cmd/remove_test.go @@ -42,12 +42,6 @@ func TestRemoveComplete(t *testing.T) { continue } - if !testSliceEqual(c.args, opt.Args) { - t.Errorf("case %d, opt.Args not set correctly. Expected: '%s', Got: '%s'", i, c.args, opt.Args) - ioReset(in, out, errs) - continue - } - if opt.DatasetRequests == nil { t.Errorf("case %d, opt.DatasetRequests not set.", i) ioReset(in, out, errs) @@ -69,7 +63,7 @@ func TestRemoveValidate(t *testing.T) { } for i, c := range cases { opt := &RemoveOptions{ - Args: c.args, + Refs: NewListOfRefSelects(c.args), } err := opt.Validate() @@ -112,10 +106,10 @@ func TestRemoveRun(t *testing.T) { err string msg string }{ - {[]string{}, -1, "", "", ""}, + {[]string{}, -1, "", "repo: empty dataset reference", ""}, {[]string{"me/bad_dataset"}, -1, "", "repo: not found", "could not find dataset 'me/bad_dataset'"}, {[]string{"me/movies"}, -1, "removed entire dataset 'peer/movies@/map/QmcnKQTWNuAYpcqUa4Ss85WiNp4c9ySem4DNipxQzUqw5J'\n", "", ""}, - {[]string{"me/cities", "me/counter"}, -1, "removed entire dataset 'peer/cities@/map/QmQAvmHB7gE5jvXyzqYeAZfAz2dnWxghEjq7hDN8aaJWZM'\nremoved entire dataset 'peer/counter@/map/QmVB2MQoYXQq4ouDuxmeDa1GvCzvRpYUN1PFRsqgi39TGp'\n", "", ""}, + {[]string{"me/cities", "me/counter"}, -1, "removed entire dataset 'peer/cities@/map/QmQAvmHB7gE5jvXyzqYeAZfAz2dnWxghEjq7hDN8aaJWZM'\n", "", ""}, {[]string{"me/movies"}, -1, "", "repo: not found", "could not find dataset 'me/movies'"}, } @@ -128,7 +122,7 @@ func TestRemoveRun(t *testing.T) { opt := &RemoveOptions{ IOStreams: streams, - Args: c.args, + Refs: NewListOfRefSelects(c.args), Revision: dsref.Rev{Field: "ds", Gen: c.revision}, DatasetRequests: dsr, } diff --git a/fsi/fsi.go b/fsi/fsi.go index 83db02186..f7c727e4d 100644 --- a/fsi/fsi.go +++ b/fsi/fsi.go @@ -20,6 +20,7 @@ import ( golog "github.com/ipfs/go-log" "github.com/qri-io/qri/base" + "github.com/qri-io/qri/base/component" "github.com/qri-io/qri/repo" ) @@ -196,3 +197,23 @@ func removeLinkFile(dir string) error { dir = filepath.Join(dir, QriRefFilename) return os.Remove(dir) } + +// DeleteComponentFiles deletes all component files in the directory. Should only be used if +// removing an entire dataset, or if the dataset is about to be rewritten back to the filesystem. +func DeleteComponentFiles(dir string) error { + dirComps, err := component.ListDirectoryComponents(dir) + if err != nil { + return err + } + for _, compName := range component.AllSubcomponentNames() { + comp := dirComps.Base().GetSubcomponent(compName) + if comp == nil { + continue + } + err = os.Remove(comp.Base().SourceFile) + if err != nil { + return err + } + } + return nil +} diff --git a/fsi/status.go b/fsi/status.go index 9cdb377a0..42b7a80a9 100644 --- a/fsi/status.go +++ b/fsi/status.go @@ -26,6 +26,8 @@ var ( STParseError = "parse error" // STConflictError is a component with a conflict STConflictError = "conflict error" + // ErrWorkingDirectoryDirty is the error for when the working directory is not clean + ErrWorkingDirectoryDirty = fmt.Errorf("working directory is dirty") ) // StatusItem is a component that has status representation on the filesystem @@ -249,3 +251,19 @@ func (fsi *FSI) StatusAtVersion(ctx context.Context, refStr string) (changes []S } return changes, nil } + +// IsWorkingDirectoryClean returns nil if the directory is clean, or ErrWorkingDirectoryDirty if +// it is dirty +func (fsi *FSI) IsWorkingDirectoryClean(ctx context.Context, dir string) error { + changes, err := fsi.Status(ctx, dir) + if err != nil { + return err + } + for _, ch := range changes { + if ch.Type != STUnmodified { + return ErrWorkingDirectoryDirty + } + } + return nil + +} diff --git a/lib/datasets.go b/lib/datasets.go index b693dbec0..aae87cb50 100644 --- a/lib/datasets.go +++ b/lib/datasets.go @@ -546,18 +546,16 @@ func (r *DatasetRequests) Rename(p *RenameParams, res *repo.DatasetRef) (err err // RemoveParams defines parameters for remove command type RemoveParams struct { - Ref string - Revision dsref.Rev - Unlink bool // If true, break any FSI link - DeleteFSIFiles bool // If true, delete tracked files from the designated FSI link + Ref string + Revision dsref.Rev + KeepFiles bool } // RemoveResponse gives the results of a remove type RemoveResponse struct { - Ref string - NumDeleted int - Unlinked bool // true if the remove unlinked an FSI-linked dataset - DeletedFSIFiles bool // true if the remove deleted FSI-linked files + Ref string + NumDeleted int + Unlinked bool } // Remove a dataset entirely or remove a certain number of revisions @@ -567,6 +565,10 @@ func (r *DatasetRequests) Remove(p *RemoveParams, res *RemoveResponse) error { } ctx := context.TODO() + if p.Revision.Gen == 0 { + return fmt.Errorf("invalid number of revisions to delete: 0") + } + if p.Revision.Field != "ds" { return fmt.Errorf("can only remove whole dataset versions, not individual components") } @@ -576,45 +578,42 @@ func (r *DatasetRequests) Remove(p *RemoveParams, res *RemoveResponse) error { return err } - noHistory := false - if canonErr := repo.CanonicalizeDatasetRef(r.node.Repo, &ref); canonErr != nil && canonErr != repo.ErrNoHistory { + if canonErr := repo.CanonicalizeDatasetRef(r.node.Repo, &ref); canonErr != nil { return canonErr - } else if canonErr == repo.ErrNoHistory { - noHistory = true } res.Ref = ref.String() - if ref.FSIPath == "" && p.Unlink { - return fmt.Errorf("cannot unlink, dataset is not linked to a directory") - } - if ref.FSIPath == "" && p.DeleteFSIFiles { - return fmt.Errorf("can't delete files, dataset is not linked to a directory") - } - if ref.FSIPath != "" { - if p.DeleteFSIFiles { - if err := fsi.DeleteDatasetFiles(ref.FSIPath); err != nil { - return err + // Dataset is linked in a working directory. + if !p.KeepFiles { + // Make sure that status is clean, otherwise, refuse to remove any revisions. + wdErr := r.inst.fsi.IsWorkingDirectoryClean(ctx, ref.FSIPath) + if wdErr != nil { + if wdErr == fsi.ErrWorkingDirectoryDirty { + return fmt.Errorf("cannot remove from dataset while working directory is dirty") + } + return wdErr } - res.DeletedFSIFiles = true - } - - // running remove on a dataset that has no history must always unlink - if p.Unlink || noHistory { - if err := r.inst.fsi.Unlink(ref.FSIPath, ref.AliasString()); err != nil { - return err - } - res.Unlinked = true } + } else if p.KeepFiles { + // If dataset is not linked in a working directory, --keep-files can't be used. + return fmt.Errorf("dataset is not linked to filesystem, cannot use keep-files") } - if noHistory { - return nil + // Get the revisions that will be deleted. + history, err := base.DatasetLog(ctx, r.node.Repo, ref, p.Revision.Gen+1, 0, false) + if err != nil { + if err == repo.ErrNoHistory { + p.Revision.Gen = dsref.AllGenerations + } else { + return err + } } - removeEntireDataset := func() error { + if p.Revision.Gen == dsref.AllGenerations || p.Revision.Gen >= len(history) { // removing all revisions of a dataset must unlink it - if ref.FSIPath != "" && !p.Unlink { + if ref.FSIPath != "" { + fmt.Printf("about to Unlink\n") if err := r.inst.fsi.Unlink(ref.FSIPath, ref.AliasString()); err != nil { return err } @@ -622,7 +621,7 @@ func (r *DatasetRequests) Remove(p *RemoveParams, res *RemoveResponse) error { } // Delete entire dataset for all generations. - if err := base.RemoveNVersionsFromStore(ctx, r.inst.Repo(), &ref, -1); err != nil { + if _, err := base.RemoveNVersionsFromStore(ctx, r.inst.Repo(), &ref, -1); err != nil { return err } // Write the deletion to the logbook. @@ -636,43 +635,55 @@ func (r *DatasetRequests) Remove(p *RemoveParams, res *RemoveResponse) error { } res.NumDeleted = dsref.AllGenerations - return nil - } - - if p.Revision.Gen == dsref.AllGenerations { - return removeEntireDataset() - } else if p.Revision.Gen < 1 { - return fmt.Errorf("invalid number of revisions to delete: %d", p.Revision.Gen) - } + if ref.FSIPath != "" && !p.KeepFiles { + // Remove all files + fsi.DeleteComponentFiles(ref.FSIPath) + } + } else { + // Delete the specific number of revisions. + dsr := history[p.Revision.Gen] + replace := &repo.DatasetRef{ + Peername: dsr.Ref.Username, + Name: dsr.Ref.Name, + ProfileID: ref.ProfileID, // TODO (b5) - this is a cheat for now + Path: dsr.Ref.Path, + Published: dsr.Published, + } + err = base.ModifyDatasetRef(ctx, r.node.Repo, &ref, replace, false /*isRename*/) + if err != nil { + return err + } + head, err := base.RemoveNVersionsFromStore(ctx, r.inst.Repo(), &ref, p.Revision.Gen) + if err != nil { + return err + } + res.NumDeleted = p.Revision.Gen - // Get the revisions that will be deleted. - log, err := base.DatasetLog(ctx, r.node.Repo, ref, p.Revision.Gen+1, 0, false) - if err != nil { - return err - } + if ref.FSIPath != "" && !p.KeepFiles { + // Load dataset version that is at head after newer versions are removed + ds, err := dsfs.LoadDataset(ctx, r.inst.Repo().Store(), head.Path) + if err != nil { + return err + } + ds.Name = head.Name + ds.Peername = head.Peername + if err = base.OpenDataset(ctx, r.inst.Repo().Filesystem(), ds); err != nil { + return err + } - // If deleting more revisions then exist, delete the entire dataset. - if p.Revision.Gen >= len(log) { - return removeEntireDataset() - } + // TODO(dlong): Add a method to FSI called ProjectOntoDirectory, use it here + // and also for Restore() in lib/fsi.go and also maybe WriteComponents in fsi/mapping.go - // Delete the specific number of revisions. - dsr := log[p.Revision.Gen] - replace := &repo.DatasetRef{ - Peername: dsr.Ref.Username, - Name: dsr.Ref.Name, - ProfileID: ref.ProfileID, // TODO (b5) - this is a cheat for now - Path: dsr.Ref.Path, - Published: dsr.Published, - } + // Delete the old files + err = fsi.DeleteComponentFiles(ref.FSIPath) + if err != nil { + log.Debug("deleting component files: %s", err) + } - if err := base.ModifyDatasetRef(ctx, r.node.Repo, &ref, replace, false /*isRename*/); err != nil { - return err - } - if err := base.RemoveNVersionsFromStore(ctx, r.inst.Repo(), &ref, p.Revision.Gen); err != nil { - return err + // Update the files in the working directory + fsi.WriteComponents(ds, ref.FSIPath, r.inst.node.Repo.Filesystem()) + } } - res.NumDeleted = p.Revision.Gen return nil } diff --git a/lib/datasets_test.go b/lib/datasets_test.go index 8c52c4b67..4be19fb79 100644 --- a/lib/datasets_test.go +++ b/lib/datasets_test.go @@ -729,20 +729,19 @@ func TestDatasetRequestsRemove(t *testing.T) { {"repo: not found", RemoveParams{Ref: "abc/ABC", Revision: allRevs}}, {"can only remove whole dataset versions, not individual components", RemoveParams{Ref: "abc/ABC", Revision: dsref.Rev{Field: "st", Gen: -1}}}, {"invalid number of revisions to delete: 0", RemoveParams{Ref: "peer/movies", Revision: dsref.Rev{Field: "ds", Gen: 0}}}, - {"cannot unlink, dataset is not linked to a directory", RemoveParams{Ref: "peer/movies", Revision: allRevs, Unlink: true}}, - {"can't delete files, dataset is not linked to a directory", RemoveParams{Ref: "peer/movies", Revision: allRevs, DeleteFSIFiles: true}}, + {"dataset is not linked to filesystem, cannot use keep-files", RemoveParams{Ref: "peer/movies", Revision: allRevs, KeepFiles: true}}, } - for _, c := range badCases { + for i, c := range badCases { t.Run(fmt.Sprintf("bad_case_%s", c.err), func(t *testing.T) { res := RemoveResponse{} err := req.Remove(&c.params, &res) if err == nil { - t.Errorf("expected error. got nil") + t.Errorf("case %d: expected error. got nil", i) return } else if c.err != err.Error() { - t.Errorf("error mismatch: expected: %s, got: %s", c.err, err) + t.Errorf("case %d: error mismatch: expected: %s, got: %s", i, c.err, err) } }) } @@ -760,18 +759,6 @@ func TestDatasetRequestsRemove(t *testing.T) { RemoveParams{Ref: "peer/counter", Revision: dsref.Rev{Field: "ds", Gen: 20}}, RemoveResponse{NumDeleted: -1}, }, - {"all generations of peer/cities, remove link, delete files", - RemoveParams{Ref: "peer/cities", Revision: allRevs, DeleteFSIFiles: true}, - RemoveResponse{NumDeleted: -1, Unlinked: true, DeletedFSIFiles: true}, - }, - {"one commit of peer/craigslist, remove link, delete files", - RemoveParams{Ref: "peer/craigslist", Revision: dsref.Rev{Field: "ds", Gen: 1}, Unlink: true, DeleteFSIFiles: true}, - RemoveResponse{NumDeleted: 1, Unlinked: true, DeletedFSIFiles: true}, - }, - {"no history dataset, remove link, delete files", - RemoveParams{Ref: noHistoryName, Revision: dsref.Rev{Field: "ds", Gen: 0}, DeleteFSIFiles: true}, - RemoveResponse{NumDeleted: 0, Unlinked: true, DeletedFSIFiles: true}, - }, } for _, c := range goodCases { @@ -789,9 +776,6 @@ func TestDatasetRequestsRemove(t *testing.T) { if c.res.Unlinked != res.Unlinked { t.Errorf("res.Unlinked mismatch. want %t, got %t", c.res.Unlinked, res.Unlinked) } - if c.res.DeletedFSIFiles != res.DeletedFSIFiles { - t.Errorf("res.DeletedFSIFiles mismatch. want %t, got %t", c.res.DeletedFSIFiles, res.DeletedFSIFiles) - } }) } } diff --git a/remote/remote.go b/remote/remote.go index feaa636ea..2282de014 100644 --- a/remote/remote.go +++ b/remote/remote.go @@ -187,7 +187,7 @@ func (r *Remote) RemoveDataset(ctx context.Context, params map[string]string) er } // remove all the versions of this dataset from the store - if err := base.RemoveNVersionsFromStore(ctx, r.node.Repo, &ref, -1); err != nil { + if _, err := base.RemoveNVersionsFromStore(ctx, r.node.Repo, &ref, -1); err != nil { return err }