Be more explicit about future changes to mode

[ribasushi]

No description provided.

[ribasushi]

The perm kerfuffle doesn't let me add reviewers: tagging @achingbrain @Stebalien

[achingbrain]

I'm not sure about the language - for a spec 'must' is very strong. If an implementation didn't mask the values we might say it had a bug but would it be was noncompliant? Maybe 'It is recommended that' would be better?

[ribasushi]

I used must to avoid the following scenario:

• Implementation takes the entire 32bit value
• Interprets not-yet defined bits in implementation-specific ways ( e.g. there might be a "this is executable" bit provided by the implementing language )
• Now we no longer can add an extra bit added in the future if it inadvertently ends up outright switching the semantics of the supposed-to-be-immutable CID. The implementer was "recommended" to do things, so they didn't do anything wrong by reading the entire value

[Stebalien]

I think must is a requirement. Otherwise, these bits will have host-specific meaning.

However, we should probably also note that implementations MUST NOT reject modes with these bits set and MUST round-trip them.

[Stebalien]

Isn't the mask 0777, not 07777?

[Stebalien]

(wrong button)

[ribasushi]

Isn't the mask 0777, not 07777?

The spec defines 4 bit-tripplets: https://github.com/ipfs/specs/blame/86c928d32ad180500576933ad3dd2f20d52ec4e4/UNIXFS.md#L86-L87
0777 would mask off sticky/suid/sgid.

[Stebalien]

Ah, yeah, fair enough. Package managers and the like probably need those bits.

[achingbrain]

@Stebalien when you say 'round trip' them, do you mean expose them to callers or just to make sure they are still written into the protobuf if the deserialized form of the UnixFS entry is mutated & persisted by the caller, or something else?

[Stebalien]

The latter. Modifying a file shouldn't remove the bits. However, the caller shouldn't see them.

[ribasushi]

@Stebalien actually now I am confused myself

Modifying a file shouldn't remove the bits. However, the caller shouldn't see them.

doesn't square with my proposed text of:

consumers MUST mask bits they do not expect, currently & 07777

Are we saying something like:

Transporters of such metadata (e.g. a copy() or edit() operation ) must preserve the entire uint32 value during a clone, without attempting to interpret it. On the other hand consumers of such metadata ( e.g. a filesystem or HTTP interface, or a copy() with umask ) MUST mask off bits they do not expect ( currently & 07777 ).

[Stebalien]

Are we saying something like:

That's what I was thinking. That is, if I modify/move a file, I shouldn't touch the mode bits.

On the other hand... users usually set the mode by:

1. Reading the current mode.
2. Setting the correct bit.
3. Writing it back.

This should preserve unknown bits. However, if the user straight-up sets the mode (0750), that should override the bits.

So... I wonder if we just want to return them and say "Bits not within the mask 07777 are reserved and should be ignored by the application". Basically, what we had.

At the end of the day, I think any of these solutions will work.

[achingbrain]

I think it's a case of caching the original mode in the deserialized form, exposing only the low bits we currently know about, then if serializing, write the mode out but use any bits we don't know about from the cached mode, up to the value of uint32. If that makes sense. I guess I'm assuming implementations encapsulate the protobuf serialization/deserialization in some way, similar to js-ipfs-unixfs.

Either way, this behaviour should be added to the spec so it's specific.

[Stebalien]

I'll leave it up to the two of you. I trust that, between the two of you, you'll make a reasonable choice.

[momack2]

To clarify - decisions should be owned by one person and have a clear deadline. I'm assuming Alex is the DRI for making the best decision possible with the available info (synthesized from Steve/Peter/etc). What is the deadline to make a call, Alex?

…

On Wed, Dec 18, 2019, 12:52 PM Steven Allen ***@***.***> wrote: I'll leave it up to the two of you. I trust that, between the two of you, you'll make a reasonable choice. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#231?email_source=notifications&email_token=AAEXAF66XZPTRVSOCR2GJG3QZIFGVA5CNFSM4J2CHNS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHF34HQ#issuecomment-567000606>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEXAF62E3H2DK45RMCE2ITQZIFGVANCNFSM4J2CHNSQ> .

[achingbrain]

I think we should be able to get this wrapped up by tomorrow, I think we're pretty close to a decision.

[achingbrain]

Are we saying something like: ... consumers of such metadata ... MUST mask off bits they do not expect

Assuming implementers encapsulate the protobuf serialization/deserialization I don't think we should then expect the consumers of the encapsulation to do the masking, it just makes it a leaky abstraction .

@ribasushi I propose we require the implementer of the spec to mask off and not try to interpret bits not defined in the spec, but also to persist the bits not in the spec when serializing.

This should allow older implementations to not inadvertently trample over newer features if/when higher bits are documented in subsequent updates.

What do you think?

[ribasushi]

@achingbrain this is now ready for a re-read. It is a bit more wordy than I'd like, but it's difficult to capture this without spelling it out.

I also noticed a couple issues while reading this, will address in additional PRs

[ribasushi]

@achingbrain applied both of your suggestions verbatim. Not sure what the exact protocol is here to merge things.

[Stebalien]

LGTM. @achingbrain ping me when you've given a signoff.

[achingbrain]

LGTM

[achingbrain]

Ping @Stebalien