Please take a look.


https://codereview.appspot.com/36540043/diff/60001/utils/tailer/tailer.go
File utils/tailer/tailer.go (right):

https://codereview.appspot.com/36540043/diff/60001/utils/tailer/tailer.go#newcode131
utils/tailer/tailer.go:131: buffer := make([]byte, t.bufsize)
On 2013/12/11 18:27:41, rog wrote:
> If we've got t.bufsize, "buf" would seem logical as a name.
> Not that it matters much though.

Ack, but in the other direction. ;)

https://codereview.appspot.com/36540043/diff/60001/utils/tailer/tailer.go#newcode211
utils/tailer/tailer.go:211: line = append(line, slice...)
On 2013/12/11 18:27:41, rog wrote:
> Unfortunately I think this is wrong, as we may be appending to the
internal
> bufio.Reader buffer, which may be overwritten by the ReadSlice call.

> How about separating the first-slice case from the appending logic?

> Something like this, perhaps?

> // readLine reads the next valid line from the reader, even if it is
> // larger than the reader buffer.
> func (t *Tailer) readLine() ([]byte, error) {
> 	for {
> 		slice, err := t.reader.ReadSlice(delimiter)
> 		if err == nil {
> 			if t.isValid(slice) {
> 				return slice, nil
> 			}
> 			continue
> 		}
> 		line := append([]byte(nil), slice...)
> 		for err == bufio.ErrBufferFull {
> 			slice, err = t.reader.ReadSlice(delimiter)
> 			line = append(line, slice...)
> 		}
> 		switch err {
> 		case nil:
> 			if t.isValid(line) {
> 				return line, nil
> 			}
> 		case io.EOF:
> 			// EOF without delimiter, step back.
> 			t.readSeeker.Seek(-int64(len(line)), os.SEEK_CUR)
> 			return nil, err
> 		default:
> 			return nil, err
> 		}
> 	}
> }

Yeah, that's better, thx.

https://codereview.appspot.com/36540043/diff/60001/utils/tailer/tailer_test.go
File utils/tailer/tailer_test.go (right):

https://codereview.appspot.com/36540043/diff/60001/utils/tailer/tailer_test.go#newcode30
utils/tailer/tailer_test.go:30: buffer := bytes.NewBuffer(nil)
On 2013/12/11 18:27:41, rog wrote:
> I'm afraid the way you're using this is racy (and in all the other
tests too)
> because you're reading from the buffer (with assertCollected)
concurrently with
> the tailer writing to it.

> You could use io.Pipe instead of bytes.Buffer to avoid the problem.

> BTW whenever you've got goroutine-based code, it's worth running go
test -race
> to check this kind of stuff.

Done.

https://codereview.appspot.com/36540043/diff/60001/utils/tailer/tailer_test.go#newcode47
utils/tailer/tailer_test.go:47: func (tailerSuite)
TestLaggedTermination(c *gc.C) {
On 2013/12/11 18:27:41, rog wrote:
> I see 10 of these tests are almost identical.
> This suggests to me that a table-based test might work well here, and
make it
> easier to see what's actually being tested.

> Then we could easily add quite a few more tests (for example, I'd like
to see
> tests with blank lines in various places, and probably some more too).

Done.

https://codereview.appspot.com/36540043/diff/60001/utils/tailer/tailer_test.go#newcode264
utils/tailer/tailer_test.go:264: disturber := func(lines []string) {
On 2013/12/11 18:27:41, rog wrote:
> This should probably be defined just before it's used.

Now different approach with table driven tests.

https://codereview.appspot.com/36540043/diff/60001/utils/tailer/tailer_test.go#newcode331
utils/tailer/tailer_test.go:331: sigc <- struct{}{}
On 2013/12/11 18:27:41, rog wrote:
> What's the point of this?
> We know the data has been written when this function returns.

Ouch, yes.

https://codereview.appspot.com/36540043/