Issue 171700045: code review 171700045: x/net/webdav: add memfs, an initial implementation of a...
|
Created:
9 years, 5 months ago by nmvc Modified:
9 years, 4 months ago Reviewers:
dave CC:
dave_cheney.net, nigeltao, gobot, golang-codereviews Visibility:
Public. |
Descriptionx/net/webdav: add memfs, an initial implementation of an in-memory filesystem for webdav
Patch Set 1 #Patch Set 2 : diff -r 870131cdf8c5 https://code.google.com/p/go.net #Patch Set 3 : diff -r 870131cdf8c5 https://code.google.com/p/go.net #
Total comments: 8
Patch Set 4 : diff -r 870131cdf8c5 https://code.google.com/p/go.net #
Total comments: 2
Patch Set 5 : diff -r 870131cdf8c5 https://code.google.com/p/go.net #Patch Set 6 : diff -r 870131cdf8c5 https://code.google.com/p/go.net #
Total comments: 39
Patch Set 7 : diff -r adc59880b724 https://code.google.com/p/go.net #Patch Set 8 : diff -r adc59880b724 https://code.google.com/p/go.net #Patch Set 9 : diff -r adc59880b724 https://code.google.com/p/go.net #Patch Set 10 : diff -r adc59880b724 https://code.google.com/p/go.net #
Total comments: 28
Patch Set 11 : diff -r adc59880b724 https://code.google.com/p/go.net #
MessagesTotal messages: 17
Hello dave@cheney.net, nigeltao@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.net
Sign in to reply to this message.
This is my first CL using this tool (and first time using hg). Pointers to anything silly I've done are appreciated :) Also, this is mostly to see if the approach taken for my implementation is acceptable, will then add some base tests for the walk routine.
Sign in to reply to this message.
Very nice. https://codereview.appspot.com/171700045/diff/40001/webdav/file.go File webdav/file.go (right): https://codereview.appspot.com/171700045/diff/40001/webdav/file.go#newcode117 webdav/file.go:117: root *node nit: maybe make root a node value, not a *node. https://codereview.appspot.com/171700045/diff/40001/webdav/file.go#newcode135 webdav/file.go:135: func (y *memfs) walk(op, fullname string, f func(dir *node, frag string, final bool) error) error { It's traditional to name the receiver after the type, so m, or f, or fs maybe, but not y. https://codereview.appspot.com/171700045/diff/40001/webdav/file.go#newcode294 webdav/file.go:294: mu sync.Mutex please put a newline above mu, the goal is to group the things protected by the mutex from those that are not. https://codereview.appspot.com/171700045/diff/40001/webdav/file.go#newcode332 webdav/file.go:332: // mutex is held, and thus the per-node mutex is free to be unheld here. unheld ?
Sign in to reply to this message.
https://codereview.appspot.com/171700045/diff/40001/webdav/file.go File webdav/file.go (right): https://codereview.appspot.com/171700045/diff/40001/webdav/file.go#newcode117 webdav/file.go:117: root *node On 2014/11/14 04:30:35, dfc wrote: > nit: maybe make root a node value, not a *node. Done. https://codereview.appspot.com/171700045/diff/40001/webdav/file.go#newcode135 webdav/file.go:135: func (y *memfs) walk(op, fullname string, f func(dir *node, frag string, final bool) error) error { On 2014/11/14 04:30:35, dfc wrote: > It's traditional to name the receiver after the type, so m, or f, or fs maybe, > but not y. Done. https://codereview.appspot.com/171700045/diff/40001/webdav/file.go#newcode294 webdav/file.go:294: mu sync.Mutex On 2014/11/14 04:30:35, dfc wrote: > please put a newline above mu, the goal is to group the things protected by the > mutex from those that are not. Done. https://codereview.appspot.com/171700045/diff/40001/webdav/file.go#newcode332 webdav/file.go:332: // mutex is held, and thus the per-node mutex is free to be unheld here. On 2014/11/14 04:30:35, dfc wrote: > unheld ? I'm not completely sure how to phrase what I wish here, but basically I wish to say the memfs that contains this node, has it's "mu" held as a condition of calling this method - as it is the mutex protecting structural/hierarchical changes.
Sign in to reply to this message.
https://codereview.appspot.com/171700045/diff/60001/webdav/file.go File webdav/file.go (right): https://codereview.appspot.com/171700045/diff/60001/webdav/file.go#newcode334 webdav/file.go:334: func (n *node) childSnapshot() []os.FileInfo { Maybe just delete the last part of the sentence. Or something like // childSnapshot generates a snapshot of all children, it is assumed that the caller holds the node's parent filesystem mutex.
Sign in to reply to this message.
https://codereview.appspot.com/171700045/diff/60001/webdav/file.go File webdav/file.go (right): https://codereview.appspot.com/171700045/diff/60001/webdav/file.go#newcode334 webdav/file.go:334: func (n *node) childSnapshot() []os.FileInfo { On 2014/11/14 04:40:40, dfc wrote: > Maybe just delete the last part of the sentence. Or something like > > // childSnapshot generates a snapshot of all children, it is assumed that the > caller holds the node's parent filesystem mutex. Done.
Sign in to reply to this message.
R=nigeltao@golang.org (assigned by dave@cheney.net)
Sign in to reply to this message.
LGTM, but please wait for Nigel On Fri, Nov 14, 2014 at 3:47 PM, <gobot@golang.org> wrote: > R=nigeltao@golang.org (assigned by dave@cheney.net) > > https://codereview.appspot.com/171700045/
Sign in to reply to this message.
Will do, also intend to take a pass at some unit tests (specifically for walking) On Fri Nov 14 2014 at 3:47:46 PM Dave Cheney <dave@cheney.net> wrote: > LGTM, but please wait for Nigel > > On Fri, Nov 14, 2014 at 3:47 PM, <gobot@golang.org> wrote: > > R=nigeltao@golang.org (assigned by dave@cheney.net) > > > > https://codereview.appspot.com/171700045/ >
Sign in to reply to this message.
https://codereview.appspot.com/171700045/diff/100001/webdav/file.go File webdav/file.go (right): https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode94 webdav/file.go:94: // NewMemFS returns a new in-memory FileSystem implementation. s/ implementation// https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode105 webdav/file.go:105: // A memfs implements FileSystem, storing all metadata and actual file data in-memory, I know that Go doesn't have a strict line limit, but personally, for top-level doc comments (but not for other comments, or for code in general), I still like to cap lines at 80 chars. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode106 webdav/file.go:106: // no limits on filesystem size are used, so it is not recommend this be utilized s/recommend/recommended/ and s/utilized/used/ https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode112 webdav/file.go:112: // As a by-product of this concurrent reads and writes to a file are not possible. Add a comma after "this", and consider /As a by-product/Because/ https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode115 webdav/file.go:115: type memfs struct { I would s/memfs/memFS/, since I also have a memLS brewing, and I think that using caps would emphasize the difference (and is also consistent with NewMemFS). https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode135 webdav/file.go:135: func (fs *memfs) walk(op, fullname string, f func(dir *node, frag string, final bool) error) error { It might be easier if walk returned (*node, error) instead of (error). https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode140 webdav/file.go:140: // Clean the path I don't think this comment adds anything. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode160 webdav/file.go:160: for len(remaining) > 0 && remaining[0] == '/' { I don't think this is necessary: it's "i+1" on the line above, and the path.Clean call above should have removed any double-slashes. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode179 webdav/file.go:179: Err: errors.New("no such directory"), I think you want: Err: os.ErrNotExist, so that the error returned works with os.IsNotExist. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode186 webdav/file.go:186: Err: errors.New("not a directory"), Similarly, I think that this should be os.ErrInvalid. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode200 webdav/file.go:200: return errors.New("empty file name") I'd use os.ErrInvalid. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode247 webdav/file.go:247: children: n.childSnapshot(), If childSnapshot is only called from the one place, here, you might as well inline the code here. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode280 webdav/file.go:280: return nil You could move the if n == nil { return os.ErrNotExist } return nil check from a few lines down to here. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode291 webdav/file.go:291: // A node represents a single entry in the in-memory filesystem, also implements os.FileInfo Add a full stop, and I'd s/, also/and also/ https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode292 webdav/file.go:292: type node struct { I'd s/node/memFSNode/, since I have a memLSNode type brewing. Similarly for s/type file/type memFSFile/ https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode296 webdav/file.go:296: modtime time.Time s/modtime/modTime/ would be consistent with the ModTime method. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode372 webdav/file.go:372: if count < 0 { s/</<=/ as per http://golang.org/pkg/os/#File.Readdir Yeah, we (the Go team) probably goofed on this one, but it's part of the Go 1 API now. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode373 webdav/file.go:373: return f.children, nil I think this should be if count <= 0 { old := f.pos f.pos = len(f.children) return f.children[old:], nil } You will need to hold f.n.mu while doing this, and after all that, you could probably clean up the Readdir code. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode384 webdav/file.go:384: return f.children[:count], nil The slice bounds should be f.pos-count:f.pos, not 0:count. How about: if !f.n.IsDir() { return nil, os.ErrInvalid } f.n.mu.Lock() defer f.n.mu.Unlock() old := f.pos if old >= len(f.children) { return nil, io.EOF } if count > 0 { f.pos += count if f.pos > len(f.children) { f.pos = len(f.children) } } else { f.pos = len(f.children) } return f.children[old:f.pos], nil
Sign in to reply to this message.
https://codereview.appspot.com/171700045/diff/100001/webdav/file.go File webdav/file.go (right): https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode94 webdav/file.go:94: // NewMemFS returns a new in-memory FileSystem implementation. On 2014/11/14 06:53:44, nigeltao wrote: > s/ implementation// Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode105 webdav/file.go:105: // A memfs implements FileSystem, storing all metadata and actual file data in-memory, On 2014/11/14 06:53:43, nigeltao wrote: > I know that Go doesn't have a strict line limit, but personally, for top-level > doc comments (but not for other comments, or for code in general), I still like > to cap lines at 80 chars. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode106 webdav/file.go:106: // no limits on filesystem size are used, so it is not recommend this be utilized On 2014/11/14 06:53:44, nigeltao wrote: > s/recommend/recommended/ and s/utilized/used/ Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode112 webdav/file.go:112: // As a by-product of this concurrent reads and writes to a file are not possible. On 2014/11/14 06:53:44, nigeltao wrote: > Add a comma after "this", and consider /As a by-product/Because/ Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode115 webdav/file.go:115: type memfs struct { On 2014/11/14 06:53:44, nigeltao wrote: > I would s/memfs/memFS/, since I also have a memLS brewing, and I think that > using caps would emphasize the difference (and is also consistent with > NewMemFS). Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode115 webdav/file.go:115: type memfs struct { On 2014/11/14 06:53:44, nigeltao wrote: > I would s/memfs/memFS/, since I also have a memLS brewing, and I think that > using caps would emphasize the difference (and is also consistent with > NewMemFS). Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode135 webdav/file.go:135: func (fs *memfs) walk(op, fullname string, f func(dir *node, frag string, final bool) error) error { On 2014/11/14 06:53:44, nigeltao wrote: > It might be easier if walk returned (*node, error) instead of (error). I tried this, don't think it works that well. - Mkdir gets longer as it needs to discard the node - OpenFile uses a value constructed from the file under the mutex, not the node. - RemoveAll doesn't care for the node. - Stat gets slightly shorter. So it simplifies Stat, the rest are the same or a bit worse. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode140 webdav/file.go:140: // Clean the path On 2014/11/14 06:53:44, nigeltao wrote: > I don't think this comment adds anything. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode160 webdav/file.go:160: for len(remaining) > 0 && remaining[0] == '/' { On 2014/11/14 06:53:44, nigeltao wrote: > I don't think this is necessary: it's "i+1" on the line above, and the > path.Clean call above should have removed any double-slashes. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode179 webdav/file.go:179: Err: errors.New("no such directory"), On 2014/11/14 06:53:44, nigeltao wrote: > I think you want: > Err: os.ErrNotExist, > so that the error returned works with os.IsNotExist. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode186 webdav/file.go:186: Err: errors.New("not a directory"), On 2014/11/14 06:53:44, nigeltao wrote: > Similarly, I think that this should be os.ErrInvalid. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode200 webdav/file.go:200: return errors.New("empty file name") On 2014/11/14 06:53:44, nigeltao wrote: > I'd use os.ErrInvalid. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode247 webdav/file.go:247: children: n.childSnapshot(), On 2014/11/14 06:53:44, nigeltao wrote: > If childSnapshot is only called from the one place, here, you might as well > inline the code here. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode280 webdav/file.go:280: return nil On 2014/11/14 06:53:44, nigeltao wrote: > You could move the > > if n == nil { > return os.ErrNotExist > } > return nil > > check from a few lines down to here. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode291 webdav/file.go:291: // A node represents a single entry in the in-memory filesystem, also implements os.FileInfo On 2014/11/14 06:53:43, nigeltao wrote: > Add a full stop, and I'd s/, also/and also/ Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode292 webdav/file.go:292: type node struct { On 2014/11/14 06:53:43, nigeltao wrote: > I'd s/node/memFSNode/, since I have a memLSNode type brewing. Similarly for > s/type file/type memFSFile/ Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode296 webdav/file.go:296: modtime time.Time On 2014/11/14 06:53:44, nigeltao wrote: > s/modtime/modTime/ would be consistent with the ModTime method. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode372 webdav/file.go:372: if count < 0 { On 2014/11/14 06:53:44, nigeltao wrote: > s/</<=/ as per http://golang.org/pkg/os/#File.Readdir > > Yeah, we (the Go team) probably goofed on this one, but it's part of the Go 1 > API now. Done. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode373 webdav/file.go:373: return f.children, nil On 2014/11/14 06:53:43, nigeltao wrote: > I think this should be > > if count <= 0 { > old := f.pos > f.pos = len(f.children) > return f.children[old:], nil > } > > You will need to hold f.n.mu while doing this, and after all that, you could > probably clean up the Readdir code. Acknowledged. https://codereview.appspot.com/171700045/diff/100001/webdav/file.go#newcode384 webdav/file.go:384: return f.children[:count], nil On 2014/11/14 06:53:44, nigeltao wrote: > The slice bounds should be f.pos-count:f.pos, not 0:count. How about: > > if !f.n.IsDir() { > return nil, os.ErrInvalid > } > f.n.mu.Lock() > defer f.n.mu.Unlock() > old := f.pos > if old >= len(f.children) { > return nil, io.EOF > } > if count > 0 { > f.pos += count > if f.pos > len(f.children) { > f.pos = len(f.children) > } > } else { > f.pos = len(f.children) > } > return f.children[old:f.pos], nil Done, with slight change to the <= case to handle if a non-unbounded read had already occurred.
Sign in to reply to this message.
I have updated the walk method, as after running the litmus tests noted that /a/ and /a should be treated identically, so my behaviour of adding a suffix / [and thus a final walk to ""] was a mistake.
Sign in to reply to this message.
ping. what is needed to get this CL across the line ? On Tue, Nov 18, 2014 at 3:44 PM, <nmvc@google.com> wrote: > I have updated the walk method, as after running the litmus tests noted > that /a/ and /a should be treated identically, so my behaviour of adding > a suffix / [and thus a final walk to ""] was a mistake. > > https://codereview.appspot.com/171700045/
Sign in to reply to this message.
Nigel's just out for a bit, waiting for him to get off leave. On Wed Dec 03 2014 at 10:28:24 AM Dave Cheney <dave@cheney.net> wrote: > ping. what is needed to get this CL across the line ? > > On Tue, Nov 18, 2014 at 3:44 PM, <nmvc@google.com> wrote: > > I have updated the walk method, as after running the litmus tests noted > > that /a/ and /a should be treated identically, so my behaviour of adding > > a suffix / [and thus a final walk to ""] was a mistake. > > > > https://codereview.appspot.com/171700045/ >
Sign in to reply to this message.
https://codereview.appspot.com/171700045/diff/180001/webdav/file.go File webdav/file.go (right): https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode105 webdav/file.go:105: // in-memory, no limits on filesystem size are used, so it is not recommended s/, no/. No/ https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode108 webdav/file.go:108: // Concurrent access is permitted, all filesystem structure changes are It's odd how you say "Concurrent access is permitted... concurrent reads and writes to a file are not possible." You could change the second part to "concurrent reads and writes to a file are serialized." But I'd just write: // Concurrent access is permitted. The tree structure is protected by a mutex, // and each node's contents and metadata are protected by a per-node mutex. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode120 webdav/file.go:120: // walk walks the directory tree for the fullname, calling f at each step. If f Does the fullname have to start with "/"? I think it's worth documenting either way, and if it's OK not to, test cases for that would be great. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode140 webdav/file.go:140: // starts at y.root What is "y"? Also, the sentence is missing a full stop. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode153 webdav/file.go:153: if err := f(dir, frag, final); err != nil { You could simplify things if all the f closures below always start with: if !final { return nil } https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode333 webdav/file.go:333: // A file is an implementation of File over a plain byte array. s/array/slice/ but also, the file struct doesn't really hold the []byte, the memFSNode does. How about: // A memFile is a File implementation for a memFSNode. It is a per-file (not // per-node) read/write position, and if the node is a directory, a snapshot of // that node's children. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode334 webdav/file.go:334: type file struct { I'd s/file/memFile/ https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode337 webdav/file.go:337: // changes to pos are guarded by n.mu I'd make this comment a complete English sentence: // Changes to pos are guarded by n.mu. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode375 webdav/file.go:375: f.pos = len(f.children) - old No, this really should be f.pos = len(f.children) right? https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode396 webdav/file.go:396: return int64(f.pos), os.ErrInvalid return 0, os.ErrInvalid unless there's a reason to return a non-zero first value even in the error case. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode406 webdav/file.go:406: func (f *file) Write(p []byte) (n int, err error) { You don't need to name the return values. Also, I think it's worth trying to use append, which grows the buffer exponentially: func (f *file) Write(p []byte) (int, error) { lenp := len(p) f.n.mu.Lock() defer f.n.mu.Unlock() if f.pos < len(f.n.data) { n := copy(f.n.data[f.pos:], p) f.pos += n p = p[n:] } if f.pos == len(f.n.data) { f.n.data = append(f.n.data, p) f.pos = len(f.n.data) } f.n.modTime = time.Now() return lenp, nil } } https://codereview.appspot.com/171700045/diff/180001/webdav/file_test.go File webdav/file_test.go (right): https://codereview.appspot.com/171700045/diff/180001/webdav/file_test.go#newc... webdav/file_test.go:171: err := fs.Mkdir(d, 0666) You could roll this into the next line: if err := fs.Mkdir(d, 0666); err != nil { https://codereview.appspot.com/171700045/diff/180001/webdav/file_test.go#newc... webdav/file_test.go:173: t.Errorf("dir=%q, could not create: %v", d, err) If a test fails, it's worth knowing which test case failed. I'd say: t.Errorf("tc.dir=%q: mkdir %q: %v", tc.dir, d, err) https://codereview.appspot.com/171700045/diff/180001/webdav/file_test.go#newc... webdav/file_test.go:193: t.Errorf("%v", err) t.Errorf("tc.dir=%q: %v", tc.dir, err)
Sign in to reply to this message.
https://codereview.appspot.com/171700045/diff/180001/webdav/file.go File webdav/file.go (right): https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode105 webdav/file.go:105: // in-memory, no limits on filesystem size are used, so it is not recommended On 2014/12/05 03:39:00, nigeltao wrote: > s/, no/. No/ Done. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode108 webdav/file.go:108: // Concurrent access is permitted, all filesystem structure changes are On 2014/12/05 03:39:00, nigeltao wrote: > It's odd how you say "Concurrent access is permitted... concurrent reads and > writes to a file are not possible." You could change the second part to > "concurrent reads and writes to a file are serialized." > > But I'd just write: > > // Concurrent access is permitted. The tree structure is protected by a mutex, > // and each node's contents and metadata are protected by a per-node mutex. Done. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode140 webdav/file.go:140: // starts at y.root On 2014/12/05 03:39:00, nigeltao wrote: > What is "y"? Also, the sentence is missing a full stop. 'fs' was originally 'y', updated. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode153 webdav/file.go:153: if err := f(dir, frag, final); err != nil { On 2014/12/05 03:38:59, nigeltao wrote: > You could simplify things if all the f closures below always start with: > > if !final { return nil } I anticipate that propfind will be a case where it is not only on leaf nodes, so prefer this flexibility for now I think? https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode333 webdav/file.go:333: // A file is an implementation of File over a plain byte array. On 2014/12/05 03:38:59, nigeltao wrote: > s/array/slice/ but also, the file struct doesn't really hold the []byte, the > memFSNode does. How about: > > // A memFile is a File implementation for a memFSNode. It is a per-file (not > // per-node) read/write position, and if the node is a directory, a snapshot of > // that node's children. Done. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode334 webdav/file.go:334: type file struct { On 2014/12/05 03:39:00, nigeltao wrote: > I'd s/file/memFile/ Done. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode337 webdav/file.go:337: // changes to pos are guarded by n.mu On 2014/12/05 03:39:00, nigeltao wrote: > I'd make this comment a complete English sentence: > > // Changes to pos are guarded by n.mu. Done. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode375 webdav/file.go:375: f.pos = len(f.children) - old On 2014/12/05 03:39:00, nigeltao wrote: > No, this really should be > f.pos = len(f.children) > right? To be honest, I wasn't sure. Re-reading the documentation suggests that you're correct, it always returns all the FileInfo. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode375 webdav/file.go:375: f.pos = len(f.children) - old On 2014/12/05 03:39:00, nigeltao wrote: > No, this really should be > f.pos = len(f.children) > right? Done. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode396 webdav/file.go:396: return int64(f.pos), os.ErrInvalid On 2014/12/05 03:38:59, nigeltao wrote: > return 0, os.ErrInvalid > unless there's a reason to return a non-zero first value even in the error case. Reading the definition of os.File.Seek: "It returns the new offset and an error, if any." I took that to mean it should always return an offset? Changed - but unsure. https://codereview.appspot.com/171700045/diff/180001/webdav/file.go#newcode406 webdav/file.go:406: func (f *file) Write(p []byte) (n int, err error) { On 2014/12/05 03:39:00, nigeltao wrote: > You don't need to name the return values. > > Also, I think it's worth trying to use append, which grows the buffer > exponentially: > > func (f *file) Write(p []byte) (int, error) { > lenp := len(p) > f.n.mu.Lock() > defer f.n.mu.Unlock() > if f.pos < len(f.n.data) { > n := copy(f.n.data[f.pos:], p) > f.pos += n > p = p[n:] > } > if f.pos == len(f.n.data) { > f.n.data = append(f.n.data, p) > f.pos = len(f.n.data) > } > f.n.modTime = time.Now() > return lenp, nil > } > } Write permits the creation of holes; i.e. a write can occur after the end of the file, not solely at the end of the file. Given that, do you think it still makes sense to use append? It would be the addition of an f.pos > len(f.n.data) clause to your above example. https://codereview.appspot.com/171700045/diff/180001/webdav/file_test.go File webdav/file_test.go (right): https://codereview.appspot.com/171700045/diff/180001/webdav/file_test.go#newc... webdav/file_test.go:171: err := fs.Mkdir(d, 0666) On 2014/12/05 03:39:00, nigeltao wrote: > You could roll this into the next line: > > if err := fs.Mkdir(d, 0666); err != nil { Done. https://codereview.appspot.com/171700045/diff/180001/webdav/file_test.go#newc... webdav/file_test.go:173: t.Errorf("dir=%q, could not create: %v", d, err) On 2014/12/05 03:39:00, nigeltao wrote: > If a test fails, it's worth knowing which test case failed. I'd say: > > t.Errorf("tc.dir=%q: mkdir %q: %v", tc.dir, d, err) Done. https://codereview.appspot.com/171700045/diff/180001/webdav/file_test.go#newc... webdav/file_test.go:193: t.Errorf("%v", err) On 2014/12/05 03:39:00, nigeltao wrote: > t.Errorf("tc.dir=%q: %v", tc.dir, err) Done.
Sign in to reply to this message.
R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/171700045 is best) in the description in your new CL. Thanks very much.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
