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

@@ -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()

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
 }

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())
  }

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
  }

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)

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,81 +53,78 @@ 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
 }
 
 // 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(&params, &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(&params, &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
 }