Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(command): add force flag for files rm #5555

Merged
merged 2 commits into from
Oct 5, 2018
Merged

feat(command): add force flag for files rm #5555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd23e7d
feat(command): add force flag for files rm
overbool Oct 3, 2018
a86cde5
test(files): add test for force flag
overbool Oct 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 20 additions & 13 deletions core/commands/files.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,7 @@ Remove files or directories.
},
Options: []cmdkit.Option{
cmdkit.BoolOption("recursive", "r", "Recursively remove directories."),
cmdkit.BoolOption("force", "Forcibly remove target at path; implies -r for directories"),
},
Run: func(req oldcmds.Request, res oldcmds.Response) {
defer res.SetOutput(nil)
Expand Down Expand Up @@ -1041,8 +1042,6 @@ Remove files or directories.
return
}

dashr, _, _ := req.Option("r").Bool()

var success bool
defer func() {
if success {
Expand All @@ -1054,8 +1053,10 @@ Remove files or directories.
}
}()

// if '-r' specified, don't check file type (in bad scenarios, the block may not exist)
if dashr {
// if '--force' specified, it will remove anything else,
// including file, directory, corrupted node, etc
force, _, _ := req.Option("force").Bool()
if force {
err := pdir.Unlink(name)
if err != nil {
res.SetError(err, cmdkit.ErrNormal)
Expand All @@ -1066,25 +1067,31 @@ Remove files or directories.
return
}

childi, err := pdir.Child(name)
// get child node by name, when the node is corrupted and nonexistent,
// it will return specific error.
child, err := pdir.Child(name)
if err != nil {
res.SetError(err, cmdkit.ErrNormal)
return
}

switch childi.(type) {
dashr, _, _ := req.Option("r").Bool()

switch child.(type) {
case *mfs.Directory:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can now turn this single-case switch into an if

res.SetError(fmt.Errorf("%s is a directory, use -r to remove directories", path), cmdkit.ErrNormal)
return
default:
err := pdir.Unlink(name)
if err != nil {
res.SetError(err, cmdkit.ErrNormal)
if !dashr {
res.SetError(fmt.Errorf("%s is a directory, use -r to remove directories", path), cmdkit.ErrNormal)
return
}
}

success = true
err = pdir.Unlink(name)
if err != nil {
res.SetError(err, cmdkit.ErrNormal)
return
}

success = true
},
}

Expand Down
14 changes: 14 additions & 0 deletions test/sharness/t0250-files-api.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,20 @@ test_files_api() {
test_expect_success "repo gc $EXTRA" '
ipfs repo gc
'

# test rm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to add a test with an actual corrupted node, which could just be an empty proto-node, but I'm not sure how could we add that to MFS, ipfs files cp does check the type:

ipfs object new
# QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n
ipfs files cp /ipfs/QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n /corrupted
# Error: cp: cannot flush the created file /corrupted: proto: required field "Type" not set

Copy link
Contributor

@schomatis schomatis Oct 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, turn off flush:

ipfs files -f=0 cp /ipfs/QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n /corrupted
ipfs files rm /corrupted
# Error: proto: required field "Type" not set

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OT: What's the difference of adding a file in MFS without flushing? I still don't know how that works (and I'm supposedly the one in charge of reviewing the MFS API 😬)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this some more, after adding the checks to PutNode this won't work anyways (even with flush turned off), so scratch that, I'm fine with the existing tests.


test_expect_success "remove file forcibly" '
echo "hello world" | ipfs files write --create /forcibly &&
ipfs files rm --force /forcibly &&
verify_dir_contents /
'

test_expect_success "remove directory forcibly" '
ipfs files mkdir /forcibly-dir &&
ipfs files rm --force /forcibly-dir &&
verify_dir_contents /
'
}

# test offline and online
Expand Down