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

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

Merged
merged 3 commits into from
Sep 13, 2019
Merged

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

merged 3 commits into from
Sep 13, 2019

Conversation

b5
Copy link
Member

@b5 b5 commented Sep 12, 2019

need to be able to remove a dataset reference that has no history

@b5 b5 added the fix A bug fix label Sep 12, 2019
@b5 b5 self-assigned this Sep 12, 2019
@b5
fix(api): remove files field is 'files', not 'delete'
3017723
@b5
fix(fsi.DeleteFiles): attempt to remove files, even on early return
711f733 
early-bails were skipping the call to os.Remove, which meant ErrNoHistory ref removal wasn't trying to remove a (very possibly) empty directory.
@b5 b5 marked this pull request as ready for review September 12, 2019 21:05
@b5 b5 added the FSI file system integration label Sep 13, 2019
@b5 b5 merged commit 1e462d3 into master Sep 13, 2019
@b5 b5 deleted the fix_remove_no_history branch September 13, 2019 02:05
@dustmop
Copy link
Contributor

dustmop commented Mar 19, 2020

Looking back at this, I think it's putting too much work into qri fsi unlink. That command should only be removing the link file (.qri-ref) and removing the FSIPath from the repository. Removing the directory should be the purview of qri remove only. I also think the remove functionality (that is, calling os.Remove in Unlink) has been broken because the Dir is always an empty string, but the lack of tests means it wasn't caught. I'll make a follow-up PR that removes the call to os.Remove, and also puts more tests into place that validate the fact that the directory remains after qri fsi unlink.

@Arqu
Copy link
Contributor

Arqu commented Mar 20, 2020

@dustmop I've actually addressed this as part of PR #1203, we can continue the conversation there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@b5 b5

Labels
fix A bug fix FSI file system integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@b5 @dustmop @Arqu