From f0ba1a1533a63de44c34b62d6f45e7773470c8c6 Mon Sep 17 00:00:00 2001 From: b5 Date: Thu, 12 Sep 2019 14:54:23 -0400 Subject: [PATCH 1/3] fix(remove): removing a dataset with no history should work --- cmd/remove.go | 25 +++++++++++---------- lib/datasets.go | 53 ++++++++++++++++++++++---------------------- lib/datasets_test.go | 16 +++++++++++++ 3 files changed, 56 insertions(+), 38 deletions(-) diff --git a/cmd/remove.go b/cmd/remove.go index db97bd761..81b73f55e 100644 --- a/cmd/remove.go +++ b/cmd/remove.go @@ -75,19 +75,20 @@ func (o *RemoveOptions) Complete(f Factory, args []string) (err error) { o.Revision = rev.NewAllRevisions() } else { if o.RevisionsText == "" { - return fmt.Errorf("--revisions flag is requried") - } - revisions, err := rev.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 = rev.Rev{Field: "ds", Gen: 0} + } else { + revisions, err := rev.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] } - o.Revision = *revisions[0] } return err } diff --git a/lib/datasets.go b/lib/datasets.go index 1e81e67f1..f8cc68118 100644 --- a/lib/datasets.go +++ b/lib/datasets.go @@ -527,8 +527,11 @@ func (r *DatasetRequests) Remove(p *RemoveParams, res *RemoveResponse) error { return err } - if err = repo.CanonicalizeDatasetRef(r.node.Repo, &ref); err != nil { - return err + noHistory := false + if canonErr := repo.CanonicalizeDatasetRef(r.node.Repo, &ref); canonErr != nil && canonErr != repo.ErrNoHistory { + return canonErr + } else if canonErr == repo.ErrNoHistory { + noHistory = true } res.Ref = ref.String() @@ -539,16 +542,30 @@ func (r *DatasetRequests) Remove(p *RemoveParams, res *RemoveResponse) error { return fmt.Errorf("can't delete files, dataset is not linked to a directory") } - removeEntireDataset := func() error { - // removing all revisions of a dataset must unlink it - if ref.FSIPath != "" { - if p.DeleteFSIFiles { - if _, err := fsi.DeleteDatasetFiles(ref.FSIPath); err != nil { - return err - } - res.DeletedFSIFiles = true + if ref.FSIPath != "" { + if p.DeleteFSIFiles { + if _, err := fsi.DeleteDatasetFiles(ref.FSIPath); err != nil { + return err } + 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 + } + } + + if noHistory { + return nil + } + removeEntireDataset := func() error { + // removing all revisions of a dataset must unlink it + if ref.FSIPath != "" && !p.Unlink { if err := r.inst.fsi.Unlink(ref.FSIPath, ref.AliasString()); err != nil { return err } @@ -581,22 +598,6 @@ func (r *DatasetRequests) Remove(p *RemoveParams, res *RemoveResponse) error { return removeEntireDataset() } - if ref.FSIPath != "" { - if p.DeleteFSIFiles { - if _, err := fsi.DeleteDatasetFiles(ref.FSIPath); err != nil { - return err - } - res.DeletedFSIFiles = true - } - - if p.Unlink { - if err := r.inst.fsi.Unlink(ref.FSIPath, ref.AliasString()); err != nil { - return err - } - res.Unlinked = true - } - } - // Delete the specific number of revisions. replace := log[p.Revision.Gen] if err := actions.ModifyDataset(r.node, &ref, &replace, false /*isRename*/); err != nil { diff --git a/lib/datasets_test.go b/lib/datasets_test.go index bd208b009..2ac4e1b43 100644 --- a/lib/datasets_test.go +++ b/lib/datasets_test.go @@ -652,6 +652,18 @@ func TestDatasetRequestsRemove(t *testing.T) { } defer os.RemoveAll(datasetsDir) + // initialize an example no-history dataset + initp := &InitFSIDatasetParams{ + Name: "no_history", + Dir: datasetsDir, + Format: "csv", + Mkdir: "no_history", + } + var noHistoryName string + if err := fsim.InitDataset(initp, &noHistoryName); err != nil { + t.Fatal(err) + } + // link cities dataset with a checkout checkoutp := &CheckoutParams{ Dir: filepath.Join(datasetsDir, "cities"), @@ -724,6 +736,10 @@ func TestDatasetRequestsRemove(t *testing.T) { RemoveParams{Ref: "peer/craigslist", Revision: rev.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: rev.Rev{Field: "ds", Gen: 0}, DeleteFSIFiles: true}, + RemoveResponse{NumDeleted: 0, Unlinked: true, DeletedFSIFiles: true}, + }, } for _, c := range goodCases { From 30177230cd44e584d2529bd6a4f94a41f3dd920d Mon Sep 17 00:00:00 2001 From: b5 Date: Thu, 12 Sep 2019 16:20:43 -0400 Subject: [PATCH 2/3] fix(api): remove files field is 'files', not 'delete' --- api/datasets.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/datasets.go b/api/datasets.go index 91a5ab0cc..2705ec1ad 100644 --- a/api/datasets.go +++ b/api/datasets.go @@ -459,7 +459,7 @@ func (h *DatasetHandlers) removeHandler(w http.ResponseWriter, r *http.Request) Ref: HTTPPathToQriPath(r.URL.Path[len("/remove"):]), Revision: rev.Rev{Field: "ds", Gen: -1}, Unlink: r.FormValue("unlink") == "true", - DeleteFSIFiles: r.FormValue("delete") == "true", + DeleteFSIFiles: r.FormValue("files") == "true", } if r.FormValue("all") == "true" { p.Revision = rev.NewAllRevisions() From 711f733bb2b325d32e6dd4638b3e99eeec0607db Mon Sep 17 00:00:00 2001 From: b5 Date: Thu, 12 Sep 2019 17:00:39 -0400 Subject: [PATCH 3/3] fix(fsi.DeleteFiles): attempt to remove files, even on early return early-bails were skipping the call to os.Remove, which meant ErrNoHistory ref removal wasn't trying to remove a (very possibly) empty directory. --- cmd/remove.go | 8 ++++---- fsi/fsi.go | 15 +++++++++------ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/cmd/remove.go b/cmd/remove.go index 81b73f55e..2034e0476 100644 --- a/cmd/remove.go +++ b/cmd/remove.go @@ -120,15 +120,15 @@ func (o *RemoveOptions) Run() (err error) { } if res.NumDeleted == rev.AllGenerations { printSuccess(o.Out, "removed entire dataset '%s'", res.Ref) - } else { + } 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") - } if res.DeletedFSIFiles { printSuccess(o.Out, "deleted dataset files") } + if res.Unlinked { + printSuccess(o.Out, "removed dataset link") + } } return nil } diff --git a/fsi/fsi.go b/fsi/fsi.go index 3de28d5c0..ecba43136 100644 --- a/fsi/fsi.go +++ b/fsi/fsi.go @@ -143,17 +143,20 @@ func (fsi *FSI) Unlink(dirPath, refStr string) error { log.Debugf("removing link file: %s", removeLinkErr.Error()) } + defer func() { + // always attempt to remove the directory, ignoring "directory not empty" errors + // os.Remove will fail if the directory isn't empty, which is the behaviour + // we want + if err := os.Remove(dirPath); err != nil && !strings.Contains(err.Error(), "directory not empty") { + log.Errorf("removing directory: %s", err.Error()) + } + }() + if err = repo.CanonicalizeDatasetRef(fsi.repo, &ref); err != nil { if err == repo.ErrNoHistory { // if we're unlinking a ref without history, delete it return fsi.repo.DeleteRef(ref) } - - return err - } - - // attempt to remove the directory, ignoring "directory not empty" errors - if err := os.Remove(dirPath); err != nil && !strings.Contains(err.Error(), "directory not empty") { return err }