code review 5416060: io: new FileInfo, FileMode types + update tree

os: new FileInfo, FileMode types + update tree

[rsc, 2011-11-21 19:12:04]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-11-21 19:36:23]

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go
File src/pkg/io/fileinfo.go (right):

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode13
src/pkg/io/fileinfo.go:13: // describe the same file.  For example, on Unix this
see line 24

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode15
src/pkg/io/fileinfo.go:15: // underlying structures are identical.
s/.$/;/ on other\
systems the decision may be based on the path names.

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode16
src/pkg/io/fileinfo.go:16: // Same must return false if other is an unexpected
"unexpected"?
do you mean

Same must return false if the dynamic type of other is different
from that of the receiver.

?

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode24
src/pkg/io/fileinfo.go:24: // to another portably.  Not all bits apply to all
systems.
since i get dinged, so will you. one space after sentence-ending period.

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode29
src/pkg/io/fileinfo.go:29: // The bottom nine bits are the standard Unix
rwxrwxrwx permissions.
top and bottom are fine and can stay if you like.
i'd use most- and least-significant to avoid endianness questions.

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode77
src/pkg/io/fileinfo.go:77: func (m FileMode) Perm() FileMode {
doc comments

[r2, 2011-11-21 19:37:39]

part of me feels io is the wrong place for this, that putting it here expands
the purview of the io package. this is not a 'basic interface to i/o primitives'
but a wrapper interface for file systems.

[rsc, 2011-11-21 19:42:11]

On Mon, Nov 21, 2011 at 14:37, Rob 'Commander' Pike <r@google.com> wrote:
> part of me feels io is the wrong place for this, that putting it here
> expands the purview of the io package. this is not a 'basic interface
> to i/o primitives' but a wrapper interface for file systems.

i was unsure about that.  it is easy to keep in os too.
at one point i thought that having it in io would let
syscall implement it by having access to io.FileMode,
but that is not true (the interface mentions time and
syscall cannot import time).

i will make a note to move it back into os.

[gri, 2011-11-21 19:47:54]

FYI.

I don't mind this being in package io for a start. Perhaps theres' a better
choice eventually.

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go
File src/pkg/io/fileinfo.go (right):

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode9
src/pkg/io/fileinfo.go:9: Mode() FileMode     // file mode bits
There could be an Impl() FileMode or some such, returning a mask defining which
FileMode bits are implemented by a specific FileInfo (describing the
capability). It would be a constant for a concrete FileInfo implementation.

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode33
src/pkg/io/fileinfo.go:33: ModeDir       FileMode = 1 << (32 - 1 - iota) // d:
is a directory
the parens are not needed

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode73
src/pkg/io/fileinfo.go:73: func (m FileMode) IsDir() bool {
given that fi.Mode().IsDir() is called so often, I think IsDir() should just be
on FileInfo directly.

[r2, 2011-11-21 19:56:14]

On Nov 21, 2011, at 11:42 AM, Russ Cox wrote:

> On Mon, Nov 21, 2011 at 14:37, Rob 'Commander' Pike <r@google.com> wrote:
>> part of me feels io is the wrong place for this, that putting it here
>> expands the purview of the io package. this is not a 'basic interface
>> to i/o primitives' but a wrapper interface for file systems.
> 
> i was unsure about that.  it is easy to keep in os too.
> at one point i thought that having it in io would let
> syscall implement it by having access to io.FileMode,
> but that is not true (the interface mentions time and
> syscall cannot import time).
> 
> i will make a note to move it back into os.

os is good

[r, 2011-11-21 19:57:25]

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go
File src/pkg/io/fileinfo.go (right):

http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode33
src/pkg/io/fileinfo.go:33: ModeDir       FileMode = 1 << (32 - 1 - iota) // d:
is a directory
On 2011/11/21 19:47:54, gri wrote:
> the parens are not needed

yes they are

[rsc, 2011-11-21 20:01:44]

On Mon, Nov 21, 2011 at 14:57,  <r@golang.org> wrote:
> yes they are

gri has clearly been doing too much c programming.

[rsc, 2011-11-21 20:12:46]

On Mon, Nov 21, 2011 at 14:36,  <r@golang.org> wrote:
> since i get dinged, so will you. one space after sentence-ending period.

who is complaining about this?
it's definitely not me, and i've written a ton
of comments with this spacing in the tree.

[r2, 2011-11-21 20:13:44]

On Nov 21, 2011, at 12:12 PM, Russ Cox wrote:

> On Mon, Nov 21, 2011 at 14:36,  <r@golang.org> wrote:
>> since i get dinged, so will you. one space after sentence-ending period.
> 
> who is complaining about this?
> it's definitely not me, and i've written a ton
> of comments with this spacing in the tree.

i don't like to name names, but bradfitz. ok, i like to name names.

-rob

[rsc, 2011-11-21 20:18:45]

On Mon, Nov 21, 2011 at 14:47,  <gri@golang.org> wrote:
>
http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode9
> src/pkg/io/fileinfo.go:9: Mode() FileMode     // file mode bits
> There could be an Impl() FileMode or some such, returning a mask
> defining which FileMode bits are implemented by a specific FileInfo
> (describing the capability). It would be a constant for a concrete
> FileInfo implementation.

I don't think it's important enough to force every implementation
to write one of these methods and then force every caller to
use it.  Everything just gets more complicated if we do that.

>
http://codereview.appspot.com/5416060/diff/2001/src/pkg/io/fileinfo.go#newcode73
> src/pkg/io/fileinfo.go:73: func (m FileMode) IsDir() bool {
> given that fi.Mode().IsDir() is called so often, I think IsDir() should
> just be on FileInfo directly.

It does get called often, but making it a separate method
forces every implementation to write one (and they're already
writing Mode) and to keep it consistent with Mode.  And then
you know about Mode.

Russ

[bradfitz, 2011-11-21 20:19:03]

On Mon, Nov 21, 2011 at 3:13 PM, Rob 'Commander' Pike <r@google.com> wrote:

>
> On Nov 21, 2011, at 12:12 PM, Russ Cox wrote:
>
> > On Mon, Nov 21, 2011 at 14:36,  <r@golang.org> wrote:
> >> since i get dinged, so will you. one space after sentence-ending period.
> >
> > who is complaining about this?
> > it's definitely not me, and i've written a ton
> > of comments with this spacing in the tree.
>
> i don't like to name names, but bradfitz. ok, i like to name names.
>

Actually Nigel.

According to wikipedia, two spaces after sentences are okay in fixed-width
fonts, but unacceptable in variable-width fonts.

;; Per nigeltao: use one space after full stop in comments.
;; Only enforce this in go-mode.
(defun nigelize ()
 (interactive)
 (when (eq major-mode 'go-mode) (query-replace ".  " ". " nil (point-min)
(point-max))))
(eval-after-load 'go-mode
 #'(add-hook 'before-save-hook #'nigelize))

[rsc, 2011-11-21 20:21:41]

It seems overly pedantic, even to me.

I am not going to complain if some people use
one space and some people use two spaces,
and I'm also not going to reformat the thousands
of lines of comments that use two spaces in the
tree.

Russ

[rsc, 2011-11-21 20:30:45]

PTAL

Moved back into os.

[iant, 2011-11-21 20:51:32]

FYI

Have you just not gotten around to the five other os/stat_XXX.go files?

http://codereview.appspot.com/5416060/diff/5006/src/cmd/godoc/dirtrees.go
File src/cmd/godoc/dirtrees.go (right):

http://codereview.appspot.com/5416060/diff/5006/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:43: return fi.Mode().IsDir() && len(name) > 0 &&
There seems to be a lot of s/IsDirectory()/Mode().IsDir()/.  Should we just keep
IsDirectory as a convenience method?

http://codereview.appspot.com/5416060/diff/5006/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

http://codereview.appspot.com/5416060/diff/5006/src/pkg/os/file_unix.go#newco...
src/pkg/os/file_unix.go:140: // further FileInf
Something went wrong at the end of this comment.

http://codereview.appspot.com/5416060/diff/5006/src/pkg/os/file_unix.go#newco...
src/pkg/os/file_unix.go:149: // nil error. If it encounters an error before the
end of the
This comment does not appear to correctly describe the function's behaviour
either before or after this CL.

http://codereview.appspot.com/5416060/diff/5006/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5416060/diff/5006/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:258: func itoa(x int) string {
This seems unrelated to this CL.

[rsc, 2011-11-21 20:52:49]

On Mon, Nov 21, 2011 at 15:51,  <iant@golang.org> wrote:
> Have you just not gotten around to the five other os/stat_XXX.go files?

Yes, I was waiting for comments to settle down before
taking the time to make os work everywhere.

Russ

[rsc, 2011-11-21 20:54:33]

On Mon, Nov 21, 2011 at 15:51,  <iant@golang.org> wrote:
> There seems to be a lot of s/IsDirectory()/Mode().IsDir()/.  Should we
> just keep IsDirectory as a convenience method?

I think it's inconvenient mainly because it is changing,
not because it is worse.  I'd want to shorten IsDirectory
to IsDir anyway, so it would still all change.  There are
other implementations of this interface, and I'd like to
keep it as trivial as possible to implement, so I was
trying to keep the number of required methods low.

Russ

[gri, 2011-11-21 21:02:57]

oops - don't know what I was thinking (not much, i guess) - on to my
3rd coffee today
- gri

On Mon, Nov 21, 2011 at 12:01 PM, Russ Cox <rsc@golang.org> wrote:
> gri has clearly been doing too much c programming.

[iant2, 2011-11-21 22:38:06]

Russ Cox <rsc@golang.org> writes:

> On Mon, Nov 21, 2011 at 15:51,  <iant@golang.org> wrote:
>> There seems to be a lot of s/IsDirectory()/Mode().IsDir()/.  Should we
>> just keep IsDirectory as a convenience method?
>
> I think it's inconvenient mainly because it is changing,
> not because it is worse.  I'd want to shorten IsDirectory
> to IsDir anyway, so it would still all change.  There are
> other implementations of this interface, and I'd like to
> keep it as trivial as possible to implement, so I was
> trying to keep the number of required methods low.

Well, we could get the method for free by making the mode an embedded
field in the various implementations.

It's not a big deal.  It's just that asking whether a FileInfo is a
directory is clearly a common operation, and writing s.IsDir() is both
easier to remember and more logical than s.Mode().IsDir().

Ian

[gri, 2011-11-21 22:56:33]

I'm all for the shortening from IsDirectory() to IsDir(). But in
addition to iant's comments, it seems that there a many more clients
(call sites) than implementations, and that we should keep the call
sites simple and worry about ease of implementation after that.
- gri

On Mon, Nov 21, 2011 at 12:54 PM, Russ Cox <rsc@golang.org> wrote:
> On Mon, Nov 21, 2011 at 15:51,  <iant@golang.org> wrote:
>> There seems to be a lot of s/IsDirectory()/Mode().IsDir()/.  Should we
>> just keep IsDirectory as a convenience method?
>
> I think it's inconvenient mainly because it is changing,
> not because it is worse.  I'd want to shorten IsDirectory
> to IsDir anyway, so it would still all change.  There are
> other implementations of this interface, and I'd like to
> keep it as trivial as possible to implement, so I was
> trying to keep the number of required methods low.
>
> Russ
>

[nigeltao, 2011-11-21 23:27:55]

On 22 November 2011 07:19, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Mon, Nov 21, 2011 at 3:13 PM, Rob 'Commander' Pike <r@google.com> wrote:
>> On Nov 21, 2011, at 12:12 PM, Russ Cox wrote:
>> > On Mon, Nov 21, 2011 at 14:36,  <r@golang.org> wrote:
>> >> since i get dinged, so will you. one space after sentence-ending
>> >> period.
>> >
>> > who is complaining about this?
>> > it's definitely not me, and i've written a ton
>> > of comments with this spacing in the tree.
>>
>> i don't like to name names, but bradfitz. ok, i like to name names.
>
> Actually Nigel.
> According to wikipedia, two spaces after sentences are okay in fixed-width
> fonts, but unacceptable in variable-width fonts.

Actually, my rule is keep the 1-space or 2-space style consistent with
the rest of the file. A 2-spacing colleague kept submitting patches to
a file I originally wrote that started as (and remained) 1-space, and
wrote that emacs script to do so. But, if a file is already in 2-space
style, then I'm happy to leave it (and any new code in it) in 2-space
style.

[r2, 2011-11-22 01:20:50]

Modes aren't directories. That's what's bugging me. Files are. I don't have a
good answer.

-rob

[gri, 2011-11-22 01:28:08]

Seems like another indication that the IsDir() should be a method of FileInfo.
- gri

On Mon, Nov 21, 2011 at 5:20 PM, Rob 'Commander' Pike <r@google.com> wrote:
> Modes aren't directories. That's what's bugging me. Files are. I don't have a
good answer.
>
> -rob
>
>

[r2, 2011-11-22 01:53:30]

On Nov 21, 2011, at 5:28 PM, Robert Griesemer wrote:

> Seems like another indication that the IsDir() should be a method of FileInfo.

maybe. i was expressing my discomfort rather than proposing a solution.

-rob

[r2, 2011-11-22 01:59:55]

Since by far the most common use of an FI  is to ask whether the file is
directory, that should be easy to do. I firmly believe that the common case
should be the easy case.

I understand the arguments about splitting the definition out, so I propose:

1) Leave IsDir() in FileMode.
2) Add IsDir() to FileInfo.
3) Document that the definition of FileInfo.IsDir is that it is exactly the same
as FileInfo.Mode().IsDir and also document why.

-rob

[rsc, 2011-11-28 18:53:21]

PTAL

> I understand the arguments about splitting the definition out, so I propose:
>
> 1) Leave IsDir() in FileMode.
> 2) Add IsDir() to FileInfo.
> 3) Document that the definition of FileInfo.IsDir is that it is exactly the
same as FileInfo.Mode().IsDir and also document why.

Did these.

[niemeyer, 2011-11-28 19:07:14]

We should change archive/zip's FileHeader.Mode function to return a FileMode in
a follow up CL, and probably something similar in archive/tar.

[r, 2011-11-28 19:08:56]

http://codereview.appspot.com/5416060/diff/5006/src/pkg/time/zoneinfo_plan9.go
File src/pkg/time/zoneinfo_plan9.go (right):

http://codereview.appspot.com/5416060/diff/5006/src/pkg/time/zoneinfo_plan9.g...
src/pkg/time/zoneinfo_plan9.go:12: //)
d?

[r, 2011-11-28 19:13:34]

other than the one name, LGTM

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/types.go
File src/pkg/os/types.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/types.go#newcode29
src/pkg/os/types.go:29: Same(other FileInfo) bool
this name doesn't read well in practice.  it's an adjective that's used as a
verb. normally this would be Equals but that conveys the wrong sense. 
IsSameFileAs is  ideal except for ridiculous verbosity. i propose SameAs

if this.SameAs(other) { .... }

[r, 2011-11-28 19:15:33]

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/types.go
File src/pkg/os/types.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/types.go#newcode21
src/pkg/os/types.go:21: IsDir() bool        // abbreviation for IsDir()
abbreviation for Mode().IsDir()

[rsc, 2011-11-28 19:15:59]

On Mon, Nov 28, 2011 at 14:15,  <r@golang.org> wrote:
> // abbreviation for IsDir()
> abbreviation for Mode().IsDir()

the perils of global search and replace

[rsc, 2011-11-28 19:52:36]

I agree about Same being too vague, but SameAs
sounds almost as vague.  How about SameFile?
That gets a little more concrete.  I am also looking
into whether SameFile needs to be in the interface
or can be an 'optional' method.

Russ

[r2, 2011-11-28 20:45:23]

On Nov 28, 2011, at 11:52 AM, Russ Cox wrote:

> I agree about Same being too vague, but SameAs
> sounds almost as vague.  How about SameFile?
> That gets a little more concrete.  I am also looking
> into whether SameFile needs to be in the interface
> or can be an 'optional' method.

SameFile is fine, certainly an improvement over Same.

-rob

[gri, 2011-11-28 21:06:24]

http://codereview.appspot.com/5416060/diff/12001/misc/dashboard/builder/main.go
File misc/dashboard/builder/main.go (right):

http://codereview.appspot.com/5416060/diff/12001/misc/dashboard/builder/main....
misc/dashboard/builder/main.go:413: return err == nil && s.Mode().IsDir()
just s.IsDir()

http://codereview.appspot.com/5416060/diff/12001/misc/dashboard/builder/main....
misc/dashboard/builder/main.go:418: return err == nil && !s.Mode().IsDir()
ditto

http://codereview.appspot.com/5416060/diff/12001/misc/dashboard/builder/packa...
File misc/dashboard/builder/package.go (right):

http://codereview.appspot.com/5416060/diff/12001/misc/dashboard/builder/packa...
misc/dashboard/builder/package.go:85: return !fi.Mode().IsDir() && // exclude
directories
just !fi.IsDir()

http://codereview.appspot.com/5416060/diff/12001/src/cmd/godoc/httpzip.go
File src/cmd/godoc/httpzip.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/cmd/godoc/httpzip.go#new...
src/cmd/godoc/httpzip.go:96: mode = 0
remove and have comment instead? (// no need to set mode)

http://codereview.appspot.com/5416060/diff/12001/src/cmd/godoc/httpzip.go#new...
src/cmd/godoc/httpzip.go:169: // no size or mtime for directories
given this comment, having the next two lines is odd - add respective comments
next to the values

http://codereview.appspot.com/5416060/diff/12001/src/cmd/godoc/zip.go
File src/cmd/godoc/zip.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/cmd/godoc/zip.go#newcode58
src/cmd/godoc/zip.go:58: return os.ModeDir | 0555
choice of constants could use a comment

http://codereview.appspot.com/5416060/diff/12001/src/cmd/gofix/main.go
File src/cmd/gofix/main.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/cmd/gofix/main.go#newcod...
src/cmd/gofix/main.go:236: return !mode.IsDir() && !strings.HasPrefix(name, ".")
&& strings.HasSuffix(name, ".go")
just f.IsDir() and get rid of mode

http://codereview.appspot.com/5416060/diff/12001/src/cmd/gofmt/gofmt.go
File src/cmd/gofmt/gofmt.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/cmd/gofmt/gofmt.go#newco...
src/cmd/gofmt/gofmt.go:87: return !mode.IsDir() && !strings.HasPrefix(name, ".")
&& strings.HasSuffix(name, ".go")
just f.IsDir() and get rid of mode

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/export_test.go
File src/pkg/os/export_test.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/export_test.go#ne...
src/pkg/os/export_test.go:9: var TimespecToTime = timespecToTime
not sure why this file is in the CL

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/file_unix.go#newc...
src/pkg/os/file_unix.go:140: // further FileInf
incomplete sentence

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/getwd.go
File src/pkg/os/getwd.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/getwd.go#newcode34
src/pkg/os/getwd.go:34: return pwd, nil
just return ?

http://codereview.appspot.com/5416060/diff/12001/src/pkg/os/getwd.go#newcode91
src/pkg/os/getwd.go:91: return pwd, nil
just return?

http://codereview.appspot.com/5416060/diff/12001/src/pkg/time/zoneinfo_plan9.go
File src/pkg/time/zoneinfo_plan9.go (right):

http://codereview.appspot.com/5416060/diff/12001/src/pkg/time/zoneinfo_plan9....
src/pkg/time/zoneinfo_plan9.go:9: //import (
not sure why this is here

[rsc, 2011-11-30 16:33:40]

On Mon, Nov 21, 2011 at 15:51,  <iant@golang.org> wrote:
>
http://codereview.appspot.com/5416060/diff/5006/src/pkg/os/file_unix.go#newco...
> src/pkg/os/file_unix.go:149: // nil error. If it encounters an error
> before the end of the
> This comment does not appear to correctly describe the function's
> behaviour either before or after this CL.

This is for a different CL, then, but I am not sure what you
are referring to.  It looks like if the function hits an unexpected
(non-EOF) error it returns the current value of names.

>
http://codereview.appspot.com/5416060/diff/5006/src/pkg/time/format.go#newcod...
> src/pkg/time/format.go:258: func itoa(x int) string {
> This seems unrelated to this CL.

Yes, this is my codereview plugin not working right.
It is showing pieces of the time CL.  Sorry.

Russ

[rsc, 2011-11-30 17:04:19]

*** Submitted as http://code.google.com/p/go/source/detail?r=7cdeacb20ef6 ***

os: new FileInfo, FileMode types + update tree

R=golang-dev, r, r, gri, bradfitz, iant, iant, nigeltao, n13m3y3r
CC=golang-dev
http://codereview.appspot.com/5416060