Skip to content

fix: allow mtime and mode to be optional #38

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

Merged
merged 10 commits into from
Dec 19, 2019
Merged

fix: allow mtime and mode to be optional #38

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
00e5ea0
fix: allow mtime and mode to be optional
achingbrain Dec 12, 2019
3a86a0b
fix: values are required, containing types are not
achingbrain Dec 12, 2019
04ea7a1
fix: unsaved file buffer
achingbrain Dec 12, 2019
397931e
fix: use correct field index
achingbrain Dec 12, 2019
1ee93a1
refactor: call seconds seconds
achingbrain Dec 12, 2019
a6c4208
feat: return mtime as Date object
achingbrain Dec 12, 2019
e232acf
fix: update protons to latest version
achingbrain Dec 17, 2019
9517501
fix: remove boolean trap constructor and update readme
achingbrain Dec 17, 2019
45b0b30
fix: address PR comments, refactor to modern class
achingbrain Dec 18, 2019
d93b32b
chore: pr comments
achingbrain Dec 18, 2019
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
40 changes: 22 additions & 18 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,6 @@ function Data (type, data) {
this.data = data
this.blockSizes = []

if (this.type === 'file') {
this.mode = parseInt('0644', 8)
}

if (this.type === 'directory' || this.type === 'hamt-sharded-directory') {
this.mode = parseInt('0755', 8)
}

this.addBlockSize = (size) => {
this.blockSizes.push(size)
}
Expand Down Expand Up @@ -92,12 +84,22 @@ function Data (type, data) {
blockSizes = undefined
}

if ((this.type === 'directory' || this.type === 'hamt-sharded-directory') && this.mode === parseInt('0755', 8)) {
delete this.mode
let mode

if (this.mode != null && this.mode >= 0) {
mode = {
value: this.mode
}
}

if (this.type === 'file' && this.mode === parseInt('0644', 8)) {
delete this.mode
let mtime

if (this.mtime != null && this.mtime >= 0) {
mtime = {
value: [
this.mtime
]
}
}

return unixfsData.encode({
Expand All @@ -107,27 +109,29 @@ function Data (type, data) {
blocksizes: blockSizes,
hashType: this.hashType,
fanout: this.fanout,
mode: this.mode,
mtime: this.mtime
mode: mode,
mtime: mtime
})
}
}

// decode from protobuf https://github.com/ipfs/go-ipfs/blob/master/unixfs/format.go#L24
Data.unmarshal = (marsheled) => {
const decoded = unixfsData.decode(marsheled)

if (!decoded.Data) {
decoded.Data = undefined
}

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

if (decoded.mode) {
obj.mode = decoded.mode
if (decoded.mode != null) {
obj.mode = decoded.mode.value
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

if (decoded.mtime) {
obj.mtime = decoded.mtime
if (decoded.mtime != null) {
obj.mtime = decoded.mtime.value[0]
}

return obj
Expand Down
12 changes: 10 additions & 2 deletions src/unixfs.proto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,18 @@ module.exports = `message Data {
repeated uint64 blocksizes = 4;
optional uint64 hashType = 5;
optional uint64 fanout = 6;
optional uint32 mode = 7;
optional int64 mtime = 8;
optional Mode mode = 7;
optional Mtime mtime = 8;
}

message Metadata {
optional string MimeType = 1;
}

message Mode {
optional uint32 value = 1;
}

message Mtime {
repeated int64 value = 1;
}`
63 changes: 40 additions & 23 deletions test/unixfs-format.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,37 +79,33 @@ describe('unixfs-format', () => {
expect(data.blockSizes).to.not.deep.equal(unmarshalled.blockSizes)
})

it('default mode for files', () => {
it('mode', () => {
Copy link

Choose a reason for hiding this comment

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

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...

Copy link

Choose a reason for hiding this comment

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

@achingbrain

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

@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

const mode = parseInt('0555', 8)
const data = new UnixFS('file')
expect(data.mode).to.equal(parseInt('0644', 8))
const marshalled = data.marshal()
const unmarshalled = UnixFS.unmarshal(marshalled)
expect(unmarshalled.mode).to.equal(parseInt('0644', 8))
})
data.mode = mode

it('default mode for directories', () => {
const data = new UnixFS('directory')
expect(data.mode).to.equal(parseInt('0755', 8))
const marshalled = data.marshal()
const unmarshalled = UnixFS.unmarshal(marshalled)
expect(unmarshalled.mode).to.equal(parseInt('0755', 8))
expect(UnixFS.unmarshal(data.marshal())).to.have.property('mode', mode)
})

it('default mode for hamt-sharded-directories', () => {
const data = new UnixFS('hamt-sharded-directory')
expect(data.mode).to.equal(parseInt('0755', 8))
const marshalled = data.marshal()
const unmarshalled = UnixFS.unmarshal(marshalled)
expect(unmarshalled.mode).to.equal(parseInt('0755', 8))
it('removes mode', () => {
const mode = parseInt('0555', 8)
const data = new UnixFS('file')
data.mode = mode

const unmarshalled = UnixFS.unmarshal(data.marshal())
expect(unmarshalled).to.have.property('mode', mode)

delete unmarshalled.mode

expect(UnixFS.unmarshal(unmarshalled.marshal())).to.not.have.property('mode')
})

it('mode', () => {
const mode = parseInt('0555', 8)
it('sets mode to 0', () => {
const mode = 0
const data = new UnixFS('file')
data.mode = mode
const marshalled = data.marshal()
const unmarshalled = UnixFS.unmarshal(marshalled)
expect(unmarshalled.mode).to.equal(mode)

expect(UnixFS.unmarshal(data.marshal())).to.have.property('mode', mode)
})

it('mtime', () => {
Expand All @@ -121,6 +117,27 @@ describe('unixfs-format', () => {
expect(unmarshalled.mtime).to.equal(mtime)
})

it('removes mtime', () => {
const mtime = parseInt(Date.now() / 1000)
const data = new UnixFS('file')
data.mtime = mtime

const unmarshalled = UnixFS.unmarshal(data.marshal())
expect(unmarshalled).to.have.property('mtime', mtime)

delete unmarshalled.mtime

expect(UnixFS.unmarshal(unmarshalled.marshal())).to.not.have.property('mtime')
})

it('sets mtime to 0', () => {
const mtime = 0
const data = new UnixFS('file')
data.mtime = mtime

expect(UnixFS.unmarshal(data.marshal())).to.have.property('mtime', mtime)
})

// figuring out what is this metadata for https://github.com/ipfs/js-ipfs-data-importing/issues/3#issuecomment-182336526
it.skip('metadata', () => {})

Expand Down