Allow mfs files.write command to create parent directories #5359

achingbrain · 2018-08-09T11:14:24Z

Adds support for a -p/--parents flag to the files.write command similar to the one supported by the files.mkdir command. If this is true and the directory for the file is not "/", try to create the containing directory before writing to the file.

Also adds a test. Already supported in `js-ipfs`, `go-ipfs` requires ipfs/kubo#5359 to be merged and released

Stebalien

Definitely something we want. My main concern is this might go better in getFileHandle but I won't be a stickler on that.

Stebalien · 2018-08-09T23:26:15Z

core/commands/files.go

@@ -735,6 +736,7 @@ stat' on the file or any of its ancestors.
 		}

 		create, _ := req.Options["create"].(bool)
+		dashp, _ := req.Options["parents"].(bool)


Let's call this "mkParents" or something like that. Contrary to what the rest of the go-ipfs code might indicate, we don't pay by the letter in variable names 😄.

Stebalien · 2018-08-09T23:28:35Z

core/commands/files.go

@@ -757,6 +759,22 @@ stat' on the file or any of its ancestors.
 			return
 		}

+		dirtomake := gopath.Dir(path)
+
+		if dirtomake != "/" && dashp == true {


No need for the dashp == true, dashp will suffice.

Stebalien · 2018-08-09T23:37:18Z

core/commands/files.go

+		if dirtomake != "/" && dashp == true {
+			root := nd.FilesRoot
+
+			err = mfs.Mkdir(root, dirtomake, mfs.MkdirOpts{


Given that getFileHandle already has special logic for creating missing files, I'd prefer to add this logic to getFileHandle, replacing the the create bool with a constant. However, I don't feel too strongly about this.

Stebalien · 2018-08-09T23:37:32Z

core/commands/files.go

+			root := nd.FilesRoot
+
+			err = mfs.Mkdir(root, dirtomake, mfs.MkdirOpts{
+				Mkparents: dashp,


I'd just use the true constant. Easier to follow.

achingbrain · 2018-08-10T07:50:37Z

Thanks for the review - I was concerned that adding the mkdir type behaviour to getFileHandle might make it responsible for a little too much so split it out into a separate function. Let me know if you'd like any further changes made.

Stebalien

One nit but this otherwise looks good to me.

Stebalien · 2018-08-10T18:37:19Z

core/commands/files.go

@@ -757,6 +759,14 @@ stat' on the file or any of its ancestors.
 			return
 		}

+		if mkParents {
+			err := ensureContainingDirectoryExists(nd.FilesRoot, path, flush, prefix)


No point in flushing here. We'll flush when we actually create the file.

Ok, I've changed it to not pass a Flush option to mfs.Mkdir

Stebalien · 2018-08-13T16:14:49Z

@achingbrain mind rebasing?

Stebalien · 2018-08-13T19:16:32Z

I'd like to get a second signoff from either @schomatis or @kevina and then I'll merge.

kevina

Needs one more test as I am unsure of the behavior of mfs.Mkdir when all the directories already exist.

kevina · 2018-08-13T21:41:17Z

test/sharness/t0250-files-api.sh

+  test_expect_success "can write file and create intermediate directories with short flags $EXTRA" '
+    echo "ipfs rocks" | ipfs files write -e -p /parents/foo/bar/baz/qux/quux/garply/ipfs.txt &&
+    ipfs files stat "/parents/foo/bar/baz/qux/quux/garply/ipfs.txt" | grep -q "^Type: file"
+  '


Please add another test something like:

test_expect_success "can write another file in the same directory with -e -p $EXTRA" ' echo "ipfs rocks" | ipfs files write -e -p /parents/foo/bar/baz/qux/quux/garply/ipfs2.txt && ipfs files stat "/parents/foo/bar/baz/qux/quux/garply/ipfs2.txt" | grep -q "^Type: file" '

To make sure it won't error if you use -p (or --parents) and all the parent directory already exists.

I've added the extra test.

Okay. Assuming the test passes this LGTM.

It worked locally, hopefully CI will agree.

kevina

LGTM Now.

kevina

One more test change.

Sorry I didn't catch it the first time around.

kevina · 2018-08-14T00:02:54Z

test/sharness/t0250-files-api.sh

@@ -597,9 +597,29 @@ test_files_api() {
    ipfs files ls /adir | grep foobar
  '

+  test_expect_failure "should fail to write file and create intermediate directories with no --parents flag set $EXTRA" '
+    echo "ipfs rocks" | ipfs files write --create /parents/foo/ipfs.txt
+  '


This is not the correct way to do this. Use test_must_fail. Some thing like:

test_expect_success "should fail to write file and create intermediate directories with no --parents flag set $EXTRA" ' echo "ipfs rocks" | test_must_fail ipfs files write --create /parents/foo/ipfs.txt '

test_expect_failure is for broken test that need eventually be fixed.

schomatis

I've always wanted this options since the first time I used the ipfs files write command, thanks @achingbrain!

Adds support for a `-p/--parents` flag to the `files.write` command similar to the one supported by the `files.mkdir` command. If this is true and the directory for the file is not `"/"`, try to create the containing directory before writing to the file. License: MIT Signed-off-by: Alex Potsides <[email protected]>

achingbrain · 2018-08-22T09:24:54Z

@Stebalien @kevina @schomatis Do you think this will make it into the next release?

Stebalien · 2018-08-23T03:34:27Z

@achingbrain yes.

Also adds a test. Already supported in `js-ipfs`, `go-ipfs` requires ipfs/kubo#5359 to be merged and released

achingbrain requested a review from Kubuxu as a code owner August 9, 2018 11:14

achingbrain added a commit to ipfs-inactive/interface-js-ipfs-core that referenced this pull request Aug 9, 2018

docs: document --parents flag for files.write command

8b91cff

Also adds a test. Already supported in `js-ipfs`, `go-ipfs` requires ipfs/kubo#5359 to be merged and released

achingbrain mentioned this pull request Aug 9, 2018

docs: document --parents flag for files.write command ipfs-inactive/interface-js-ipfs-core#349

Merged

Stebalien reviewed Aug 9, 2018

View reviewed changes

achingbrain force-pushed the allow-files-write-to-create-parent-directories branch 2 times, most recently from e9bd6fb to e756c7d Compare August 10, 2018 07:15

Stebalien requested changes Aug 10, 2018

View reviewed changes

achingbrain force-pushed the allow-files-write-to-create-parent-directories branch 3 times, most recently from 5c0c918 to d439c5a Compare August 13, 2018 15:27

Stebalien approved these changes Aug 13, 2018

View reviewed changes

Stebalien requested a review from schomatis August 13, 2018 16:14

achingbrain force-pushed the allow-files-write-to-create-parent-directories branch 5 times, most recently from bc1433b to 9b6eea9 Compare August 13, 2018 18:58

Stebalien requested a review from kevina August 13, 2018 19:16

kevina suggested changes Aug 13, 2018

View reviewed changes

achingbrain force-pushed the allow-files-write-to-create-parent-directories branch from 9b6eea9 to bd7d437 Compare August 13, 2018 21:46

kevina approved these changes Aug 13, 2018

View reviewed changes

kevina suggested changes Aug 14, 2018

View reviewed changes

schomatis approved these changes Aug 14, 2018

View reviewed changes

achingbrain force-pushed the allow-files-write-to-create-parent-directories branch from bd7d437 to 4e61193 Compare August 14, 2018 06:01

achingbrain force-pushed the allow-files-write-to-create-parent-directories branch from 4e61193 to c97c44e Compare August 14, 2018 06:59

kevina approved these changes Aug 14, 2018

View reviewed changes

Stebalien merged commit 0be1f5f into ipfs:master Aug 23, 2018

alanshaw pushed a commit to ipfs-inactive/interface-js-ipfs-core that referenced this pull request Aug 28, 2018

docs: document --parents flag for files.write command (#349)

e5f5b59

Also adds a test. Already supported in `js-ipfs`, `go-ipfs` requires ipfs/kubo#5359 to be merged and released

achingbrain deleted the allow-files-write-to-create-parent-directories branch September 20, 2018 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow mfs files.write command to create parent directories #5359

Allow mfs files.write command to create parent directories #5359

achingbrain commented Aug 9, 2018

Stebalien left a comment

Stebalien Aug 9, 2018

Stebalien Aug 9, 2018

Stebalien Aug 9, 2018

Stebalien Aug 9, 2018

achingbrain commented Aug 10, 2018

Stebalien left a comment

Stebalien Aug 10, 2018

achingbrain Aug 13, 2018

Stebalien commented Aug 13, 2018

Stebalien commented Aug 13, 2018

kevina left a comment

kevina Aug 13, 2018

achingbrain Aug 13, 2018

kevina Aug 13, 2018

achingbrain Aug 13, 2018

kevina left a comment

kevina left a comment

kevina Aug 14, 2018

achingbrain Aug 14, 2018

schomatis left a comment

achingbrain commented Aug 22, 2018

Stebalien commented Aug 23, 2018 •

edited

Loading

Allow mfs files.write command to create parent directories #5359

Allow mfs files.write command to create parent directories #5359

Conversation

achingbrain commented Aug 9, 2018

Stebalien 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

achingbrain commented Aug 10, 2018

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Aug 13, 2018

Stebalien commented Aug 13, 2018

kevina 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

kevina left a comment

Choose a reason for hiding this comment

kevina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schomatis left a comment

Choose a reason for hiding this comment

achingbrain commented Aug 22, 2018

Stebalien commented Aug 23, 2018 • edited Loading

Stebalien commented Aug 23, 2018 •

edited

Loading