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.

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

os: ambiguous documentation of type FileMode #25422

Open
mib-kd743naq opened this issue May 16, 2018 · 16 comments
Open

os: ambiguous documentation of type FileMode #25422

mib-kd743naq opened this issue May 16, 2018 · 16 comments

Comments

@mib-kd743naq
Copy link

@mib-kd743naq mib-kd743naq commented May 16, 2018

What version of Go are you using (go version)?

N/A

Does this issue reproduce with the latest release?

Yes ( latest document online )

What operating system and processor architecture are you using (go env)?

N/A

What did you do?

Read documentation of the FileMode bitfields

What did you expect to see?

Either

  • ...The values of the lowest 9 bits should be considered part of the public API...
    OR
  • ...The values of any of the 32 bits already defined in the const listing below should be considered part of the public API...

What did you see instead?

  • ...The values of these bits should be considered part of the public API...

The documentation as currently written is clearly open to interpretation. Further discussion: https://botbot.me/freenode/go-nuts/2018-05-16/?msg=100120706&page=5

@ianlancetaylor ianlancetaylor changed the title Ambiguous documentation of type FileMode os: ambiguous documentation of type FileMode May 16, 2018
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone May 16, 2018
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 16, 2018

I guess the text could be read to be ambiguous. When the comment says "these bits should be considered part of the public API" it refers to all the bits, not just the nine least-significant bits.

@mib-kd743naq

This comment has been minimized.

Copy link
Author

@mib-kd743naq mib-kd743naq commented May 16, 2018

I guess the text could be read to be ambiguous ... it refers to all the bits, not just the nine least-significant bits.

Note that @GinjaNinja32 read the documentation exactly the opposite way, so a fix is clearly in order ;)

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented May 16, 2018

In context, I would interpret it to mean:
“The numeric values of the single-bit FileMode constants should be considered part of the public API […].”

@vikramcse

This comment has been minimized.

Copy link

@vikramcse vikramcse commented Aug 4, 2018

@bcmills @ianlancetaylor

The defined file mode bits are the most significant bits of the FileMode. The nine least-significant bits are the standard Unix rwxrwxrwx permissions. The numeric values of the single-bit FileMode constants should be considered part of the public API and may be used in wire protocols or disk representations: they must not be changed, although new bits might be added.

Should I raise CL with above comment?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 5, 2018

I think we should separate the non-single-bit constants into a separate const block and adjust the comment as needed to make clear which ones can not change.

In truth changing any of the constant values would break Go 1 compatibility but it does make sense to clarify that the single-bit flags especially can't be changed.

@Windsooon

This comment has been minimized.

Copy link

@Windsooon Windsooon commented May 27, 2019

How about

The nine least-significant bits are the standard Unix rwxrwxrwx permissions (permissions bits). Permissions bits should be considered part of the public API and may be used in wire protocols or disk representations which must not be changed, although new bits might be added.

@mib-kd743naq

This comment has been minimized.

Copy link
Author

@mib-kd743naq mib-kd743naq commented Jun 25, 2019

@bcmills @ianlancetaylor this ticket is over a year old. Could we bump it a bit? Thanks!

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 25, 2019

Want to send in a pull request?

@Windsooon

This comment has been minimized.

Copy link

@Windsooon Windsooon commented Jun 27, 2019

I can send a PR is we are ok with my suggestion.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 27, 2019

Sounds reasonable, thanks.

@mib-kd743naq

This comment has been minimized.

Copy link
Author

@mib-kd743naq mib-kd743naq commented Jun 27, 2019

@Windsooon if you are referring literally to the text in #25422 (comment), it is insufficient to address the original reason I opened this PR. It is clear that the low 9 bits are "set in stone". The original text and also your suggested text do not answer the question:

  • Is there an explicit promise that the POSIX-expressing bits at positions 32 ( is directory ), 28 ( is symlink ), 27 ( is block/char device node ), 26 ( is pipe ), 25 ( is socket ), 24 ( has setuid-perm ), 23 ( has setgid-perm ), 22 ( what type of device node when bit 27 is set ), 22 ( has sticky-perm ) are guaranteed to "be correct" for all systems golang currently builds on?

( afaik golang does not work on systems without at least rudimentary POSIX-ish support )

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 27, 2019

@mib-kd743naq The os package is responsible for adjusting the underlying file information to use the exported mode bits.

@mib-kd743naq

This comment has been minimized.

Copy link
Author

@mib-kd743naq mib-kd743naq commented Jun 28, 2019

The os package is responsible for adjusting the underlying file information

I gathered as much ;) I simply wanted to point out again that a fix which only discusses "the low 9 bits" is not sufficient. I haven't tried to send a PR myself as CLA's are hard :(

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 22, 2019

Change https://golang.org/cl/191313 mentions this issue: os: fix ambiguous documentation of type FileMode

@mib-kd743naq

This comment has been minimized.

Copy link
Author

@mib-kd743naq mib-kd743naq commented Aug 22, 2019

@bcmills @ianlancetaylor I am really confused... Based on your comments #25422 (comment) and #25422 (comment) all currently defined bits out of the 32 are stable and can be relied upon never changing ( specifically bits 32, 28, 27/22, 26, 25, 24, 21 )

Yet commit https://go-review.googlesource.com/c/go/+/191313/1/src/os/types.go explicitly changes the text to read:

The values of the lowest 9 bits should be considered ...

So which one is it? Any defined out of the 32, or just the lowest 9?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 23, 2019

You're right, I'm confused too. The text in #25422 (comment) looks more or less correct. The text in #25422 (comment) looks misleading. I was reading the latter comment out of context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.