fix: allow mtime and mode to be optional #38

achingbrain · 2019-12-12T16:03:41Z

Upgrades protons to take advantage of new hasFoo type methods to detect if the user has set mode and mtime or not.

Also updates the constructor:

// old
new UnixFS('file', buf)

// new
new UnixFS('file', buf) // still supported
new UnixFS() // defaults to 'file' without data
new UnixFS({ // all fields are optional, apart from `type`
  type: 'file',
  data: buf,
  blockSizes: [],
  mode: parseInt('0755', 8),
  mtime: new Date()
})

This is to not have the boolean-trap style constructor of:

new UnixFS(type, data, bufferSizes, mode, mtime)

Uses protobuf messages to enable having mode and mtime as actually optional values.

ribasushi · 2019-12-12T16:15:28Z

test/unixfs-format.spec.js

@@ -79,37 +79,33 @@ describe('unixfs-format', () => {
    expect(data.blockSizes).to.not.deep.equal(unmarshalled.blockSizes)
  })

-  it('default mode for files', () => {
+  it('mode', () => {


Is there any way I can convince you to make the spec follow golang's fileMode() specification: golang/go#25422 (comment), or alternatively the less-rigorously defined POSIX st_mode struct?

Having already paid for transporting a 32bit value, it seems so odd to not populate the rest of the bits if we have them. In js-ipfs you can of course simply mask them off, the point is for the spec to allow it...

@achingbrain

Is there anything in the spec that prevents us filling the higher bits with subsequent PRs?

From my understanding all we've done so far is specify the equivalent of the file mode component of st_mode but the datatype can fit all of st_mode so there's nothing to stop us going further in the future.

@achingbrain yes you are correct. I crossed wires with not explicitly masking off future bits for when we do add them, I left a comment to the effect above

ribasushi · 2019-12-12T19:59:24Z

src/index.js

  const obj = new Data(types[decoded.Type], decoded.Data)
  obj.blockSizes = decoded.blocksizes

  if (decoded.mode) {
-    obj.mode = decoded.mode
+    obj.mode = decoded.mode.value


As per ipfs/specs#231, this needs to mask off with 07777

That's not in the spec yet, will do a further PR if it gets added.

However, let's be careful. If we get a file that has extra bits, we should round-trip those bits. Otherwise, it's hard to add features later.

hugomrdias · 2019-12-17T14:34:48Z

src/index.js

+const DEFAULT_FILE_MODE = parseInt('0644', 8)
+const DEFAULT_DIRECTORY_MODE = parseInt('0755', 8)
+
+function Data (arg1, arg2) {


can we be more descriptive in the name of the args ?

arg0, arg1?

hugomrdias · 2019-12-17T14:41:54Z

src/index.js

+    return new Data(arg1, arg2)
+  }
+
+  if (arg1 == null) {


do we really need to support null ? can't it be just

function Data (arg1 = { type: 'file }, arg2)

hugomrdias · 2019-12-17T14:49:47Z

src/index.js

+
+  return new Data({
+    type: types[decoded.Type],
+    data: decoded.hasData() ? decoded.Data : undefined,


Suggested change

data: decoded.hasData() ? decoded.Data : undefined,

data: decoded.getData(),

hugomrdias · 2019-12-17T14:50:02Z

src/index.js

+    type: types[decoded.Type],
+    data: decoded.hasData() ? decoded.Data : undefined,
+    blockSizes: decoded.blocksizes,
+    mode: decoded.hasMode() ? decoded.mode : undefined,


Suggested change

mode: decoded.hasMode() ? decoded.mode : undefined,

mode: decoded.getMode(),

I believe getMode returns 0 if unset. We want it to actually be unset.

Note: Actually, do we not want to return the default mode for the file type (then remove it on serialize)?

getMode does indeed return 0 if unset. We pass undefined here so we'll get 0755 for directories and 0644 for files.

The setting of defaults is done in the constructor so we don't have to duplicate it here as well.

humm the protons readme seems to suggest undefined maybe i read it wrong

hugomrdias · 2019-12-17T14:50:21Z

src/index.js

+    data: decoded.hasData() ? decoded.Data : undefined,
+    blockSizes: decoded.blocksizes,
+    mode: decoded.hasMode() ? decoded.mode : undefined,
+    mtime: decoded.hasMtime() ? new Date(decoded.mtime * 1000) : undefined


Suggested change

mtime: decoded.hasMtime() ? new Date(decoded.mtime * 1000) : undefined

mtime: decoded.getMtime()

Ditto above.

test/unixfs-format.spec.js

Stebalien · 2019-12-17T15:12:08Z

README.md

 }

 message Metadata {
-  optional string MimeType = 1;
+  required string MimeType = 1;


Oops, will remove.

Stebalien · 2019-12-17T15:18:38Z

src/index.js

+    type: types[decoded.Type],
+    data: decoded.hasData() ? decoded.Data : undefined,
+    blockSizes: decoded.blocksizes,
+    mode: decoded.hasMode() ? decoded.mode : undefined,


I believe getMode returns 0 if unset. We want it to actually be unset.

Note: Actually, do we not want to return the default mode for the file type (then remove it on serialize)?

Stebalien · 2019-12-17T15:18:44Z

src/index.js

+    data: decoded.hasData() ? decoded.Data : undefined,
+    blockSizes: decoded.blocksizes,
+    mode: decoded.hasMode() ? decoded.mode : undefined,
+    mtime: decoded.hasMtime() ? new Date(decoded.mtime * 1000) : undefined


Ditto above.

src/index.js

Co-Authored-By: Alan Shaw <alan.shaw@protocol.ai>

* perf: concurrent file import Adds two new options: `fileImportConcurrency` This controls the number of files that are imported concurrently. You may wish to set this high if you are importing lots of small files. `blockWriteConcurrency` This controls how many blocks from each file we write to disk at the same time. Setting this high when writing large files will significantly increase import speed, though having it high when `fileImportConcurrency` is also high can swamp the process. It also: 1. Flattens module options because validating deep objects was clunky and the separation of access to config sub objects within this module isn't very good 1. Replaces `superstruct` and `deep-extend` with `merge-options` which is better suited for merging options and is smaller 1. Replaces `async-iterator-*` modules with the more zeitgeisty `it-*` namespace Supersedes #38, sort of. No batching but atomicity guarantees are maintained and performance gains are broadly similar with the right tuning.

fix: allow mtime and mode to be optional

00e5ea0

Uses protobuf messages to enable having mode and mtime as actually optional values.

achingbrain mentioned this pull request Dec 12, 2019

docs: change mode and mtime to strings ipfs/specs#229

Closed

ribasushi reviewed Dec 12, 2019

View reviewed changes

achingbrain added 2 commits December 12, 2019 16:17

fix: values are required, containing types are not

3a86a0b

fix: unsaved file buffer

04ea7a1

achingbrain force-pushed the make-mtime-and-mode-optional branch from f6e99c9 to 04ea7a1 Compare December 12, 2019 16:21

achingbrain added 2 commits December 12, 2019 16:29

fix: use correct field index

397931e

refactor: call seconds seconds

1ee93a1

achingbrain mentioned this pull request Dec 12, 2019

docs: change mode and mtime to messages ipfs/specs#230

Closed

feat: return mtime as Date object

a6c4208

ribasushi reviewed Dec 12, 2019

View reviewed changes

fix: update protons to latest version

e232acf

achingbrain mentioned this pull request Dec 17, 2019

fix: support setting mode to 0 #37

Closed

achingbrain marked this pull request as ready for review December 17, 2019 10:55

achingbrain requested review from alanshaw, hugomrdias and Stebalien December 17, 2019 10:55

fix: remove boolean trap constructor and update readme

9517501

hugomrdias suggested changes Dec 17, 2019

View reviewed changes

Stebalien reviewed Dec 17, 2019

View reviewed changes

fix: address PR comments, refactor to modern class

45b0b30

hugomrdias approved these changes Dec 18, 2019

View reviewed changes

alanshaw approved these changes Dec 18, 2019

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

chore: pr comments

d93b32b

Co-Authored-By: Alan Shaw <alan.shaw@protocol.ai>

hugomrdias approved these changes Dec 18, 2019

View reviewed changes

achingbrain merged commit 2f02eb5 into master Dec 19, 2019

achingbrain deleted the make-mtime-and-mode-optional branch December 19, 2019 17:05

	data: decoded.hasData() ? decoded.Data : undefined,
	data: decoded.getData(),

	mode: decoded.hasMode() ? decoded.mode : undefined,
	mode: decoded.getMode(),

	mtime: decoded.hasMtime() ? new Date(decoded.mtime * 1000) : undefined
	mtime: decoded.getMtime()

fix: allow mtime and mode to be optional #38

fix: allow mtime and mode to be optional #38

Uh oh!

Conversation

achingbrain commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

achingbrain commented Dec 12, 2019 •

edited

Loading