fix(fsi): qri remove --all --force should not fail on low value files #1203

Arqu · 2020-03-19T10:24:50Z

Addresses issue #1190.

dustmop · 2020-03-19T15:11:42Z

fsi/fsi.go

+		err := removeLowValueFiles(dirPath)
+		if err != nil {
+			log.Errorf("removing directory: %s", err.Error())
+		}


I think this would be better if it lived here: https://github.com/qri-io/qri/blob/master/lib/datasets.go#L793. For one, this behavior isn't directly needed by fsi.Unlink, only by lib.Remove. If a user runs qri fsi unlink then this function also gets called, via the Unlink function in lib/fsi.go, but that shouldn't be deleting files. Furthermore, having this functionality here means that it doesn't respect the --keep-files flag, which if true shouldn't modify any files at all.

Would also be great to have a test for this in cmd/, which creates a directory, runs qri init, runs save, writes a .DS_Store file to that directory, then runs remove --all to make sure the whole thing is deleted. Copying TestRemoveOneRevisionFromWorkingDirectory then modifying it would be a great start.

Agreed on moving the call site to https://github.com/qri-io/qri/blob/master/lib/datasets.go#L793, but I think the logic of what we mean by RemoveAll should live in the fsi package. To me the best of both worlds is if fsi had an exported function:

package fsi // RemoveAll is like os.RemoveAll, but it also removes low-value files func RemoveAll(path string) error { // ... }

Then call that function from lib.

I took a bit of a split approach with datastore retaining control but moving the execution logic to fsi.

b5 · 2020-03-19T15:29:52Z

fsi/fsi.go

+			os.Remove(path)
+		}
+	}
+	return nil


My only other question here: should we also be removing directories if they are empty after removing low-value files? Personally, I think we should. when I run qri remove --all --force I'm trying to blow the directory away.

We could save that for another PR, but if we can get it knocked out now, I'd be all for it

Got covered as part of the update :)

Arqu · 2020-03-20T11:10:14Z

Still pending on adding the tests.

dustmop

The test TestRemoveAllVersionsWorkingDirectoryLowValueFiles is awesome, excellent work. I have a design concern with --force, however, that I think we need to figure out.

dustmop · 2020-03-22T21:50:18Z

cmd/fsi_integration_test.go

+	subDirPath := filepath.Join(run.WorkPath, subdir)
+	err := os.MkdirAll(subDirPath, os.ModePerm)
+	if err != nil {
+		panic(err)


Instead of panic'ing, pass a testing.T to CreateSubDir and call t.Fatal(err) here. See how MustWriteFile does it.

Yeah, lot's of these small bits we're just taken out of other examples. I'll look over it and appropriate this + if I find this setup anywhere else.

dustmop · 2020-03-22T21:53:07Z

cmd/remove_integration_test.go

+	}
+
+	// Remove all versions
+	run.MustExec(t, "qri remove --all=1")


Small style suggestion, just do --all without the =1. It's just a boolean flag, this makes it read as though it requires an integer.

Similar to the above, seen this in several places. For this PR will fix just this case but will open another to clean up the rest.

dustmop · 2020-03-22T21:55:12Z

cmd/remove_integration_test.go

+	run.MustWriteFile(t, "test.sh", "echo test\n")
+
+	// Remove all with force
+	run.MustExec(t, "qri remove --revisions=all --force")


I would suggest that, even with --force, we shouldn't try and delete files that we don't own. See my comments on the fsi.RemoveAll function for full justification.

dustmop · 2020-03-22T22:21:32Z

fsi/fsi.go

+	if err := fsi.Unlink(dirPath, refStr); err != nil {
+		return err
+	}
+	if err := os.RemoveAll(dirPath); err != nil {


I'm wary of ever calling RemoveAll, because it's potentially very destructive, especially when taking unknown / arbitrary input as a parameter. The only places we use it in the codebase right now are for tests that construct temporary directories, and one or two places that are part of rarely used commands, like a user deleting their whole repository. Since we're a data storage system, when it comes to deletes, I'd rather err on the side of being cautious over convenient, and whitelist the things we delete, rather than doing a RemoveAll.

In fact, this makes me realize, if a user at some point runs qri init in their home directory, and then later run remove --all --force, this codepath would try and delete their entire home directory. That alone is reason enough why we shouldn't make --force mean "remove even unknown files". The current semantics of --force are "delete things even if we get errors, including the working directory not being clean", and I think it should stay that way.

Agree on RemoveAll being very agressive and some bad input could wreak havoc.
On the alternative side, doing just RemoveAll and Remove will essentially result in the same behavior, unless Remove no longer removes low value files and it just triggers on --force which technically is the purpose of this PR.

Think the above might be a best blend between the two things while remaining cautious.

dustmop · 2020-03-22T22:25:18Z

lib/datasets.go

+			if p.Force {
+				err = r.inst.fsi.RemoveAll(ref.FSIPath, ref.AliasString())
+			} else if !p.KeepFiles {
+				err = r.inst.fsi.Remove(ref.FSIPath, ref.AliasString())


This call is happening before fsi.DeleteComponentFiles below, which is what removes "body.csv", "structure.json", "meta.json" etc. That means in nearly all cases, the working directory will not be empty, and the call to os.Remove within fsi.Remove will fail.

There's already a call to os.Remove below on the next line (after fsi.DeleteComponentFiles). I think the logic of removing low-value files should be happening over there, instead.

👍 Currently only works due to os.RemoveAll from above.

dustmop · 2020-03-22T22:30:59Z

fsi/fsi.go

+			"^__MACOSX$", // Resource fork
+
+			// linux specific files
+			"~$", // Backup file


Not fan of this one pattern. Any file ending with a ~ will be considered low-value, that's too broad and easy to get false positives.

Happy to remove it, mostly picked it up from some other list of "low value files"

dustmop · 2020-03-22T22:32:07Z

fsi/fsi.go

+}
+
+func getLowValueFiles(files *[]string) filepath.WalkFunc {
+	return func(path string, f os.FileInfo, err error) error {


Make sure to run go fmt ./... in order to apply the standard golang code style guide.

dustmop

LGTM!

Arqu force-pushed the qri-remove-force-1190 branch from d641225 to 4c0d472 Compare March 19, 2020 10:55

Arqu requested a review from b5 March 19, 2020 10:58

Arqu added the fix A bug fix label Mar 19, 2020

Arqu changed the title ~~fix(cmd): qri remove --all --force should not fail on low value files~~ fix(fsi): qri remove --all --force should not fail on low value files Mar 19, 2020

b5 assigned Arqu Mar 19, 2020

dustmop reviewed Mar 19, 2020

View reviewed changes

b5 reviewed Mar 19, 2020

View reviewed changes

fix(cmd): qri remove --all --force should not fail on low value files

5ea83ca

Arqu mentioned this pull request Mar 20, 2020

fix(remove): removing a dataset with no history should work #927

Merged

Arqu force-pushed the qri-remove-force-1190 branch from 4c0d472 to fd1c082 Compare March 20, 2020 11:05

fix(fsi): cleaning out fsi.Unlink behavior, moving removeLowValueFiles

7d0a3c5

Arqu force-pushed the qri-remove-force-1190 branch from fd1c082 to 7d0a3c5 Compare March 20, 2020 11:07

fix(fsi): added tests

13b68e0

dustmop reviewed Mar 22, 2020

View reviewed changes

fix(fsi): separated unlink and remove operations, added tests, cleanup

7437209

Arqu force-pushed the qri-remove-force-1190 branch from a4dba27 to 7437209 Compare March 23, 2020 16:18

Arqu added 2 commits March 23, 2020 17:37

fix(fsi): removing low value dir support for now

8f49e95

fix(fsi): updating tests

bb26d7c

Arqu force-pushed the qri-remove-force-1190 branch from 3f292d2 to bb26d7c Compare March 23, 2020 17:15

dustmop approved these changes Mar 23, 2020

View reviewed changes

Arqu merged commit 398e0ac into master Mar 23, 2020

Arqu deleted the qri-remove-force-1190 branch March 23, 2020 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(fsi): qri remove --all --force should not fail on low value files #1203

fix(fsi): qri remove --all --force should not fail on low value files #1203

Arqu commented Mar 19, 2020

dustmop Mar 19, 2020

b5 Mar 19, 2020 •

edited

Loading

Arqu Mar 20, 2020

b5 Mar 19, 2020

Arqu Mar 20, 2020

Arqu commented Mar 20, 2020 •

edited

Loading

dustmop left a comment

dustmop Mar 22, 2020

Arqu Mar 23, 2020

dustmop Mar 22, 2020

Arqu Mar 23, 2020

dustmop Mar 22, 2020

dustmop Mar 22, 2020

Arqu Mar 23, 2020

dustmop Mar 22, 2020

Arqu Mar 23, 2020

dustmop Mar 22, 2020

Arqu Mar 23, 2020

dustmop Mar 22, 2020

dustmop left a comment

fix(fsi): qri remove --all --force should not fail on low value files #1203

fix(fsi): qri remove --all --force should not fail on low value files #1203

Conversation

Arqu commented Mar 19, 2020

Choose a reason for hiding this comment

b5 Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arqu commented Mar 20, 2020 • edited Loading

dustmop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustmop left a comment

Choose a reason for hiding this comment

b5 Mar 19, 2020 •

edited

Loading

Arqu commented Mar 20, 2020 •

edited

Loading