Take care of more TODO flagged code

- Improve TODO comments, mention related issues
- Export ErrNotWritable, move it to datastore.go
- Improve logging of corrupt mailbox GOB file

inbucket.go

@@ -179,7 +179,6 @@ func signalProcessor(c <-chan os.Signal) {
 				log.Infof("Recieved SIGHUP, cycling logfile")
 				closeLogFile()
 				// There is nothing we can do if the log open fails
-				// TODO We could panic, but that would be lame?
 				_ = openLogFile()
 			} else {
 				log.Infof("Ignoring SIGHUP, logfile not configured")

pop3d/listener.go

@@ -23,7 +23,10 @@ type Server struct {
 
 // New creates a new Server struct
 func New() *Server {
-	// TODO is two filestores better/worse than sharing w/ smtpd?
+	// Get a new instance of the the FileDataStore - the locking and counting
+	// mechanisms are both global variables in the smtpd package.  If that
+	// changes in the future, this should be modified to use the same DataStore
+	// instance.
 	ds := smtpd.DefaultFileDataStore()
 	cfg := config.GetPOP3Config()
 	return &Server{domain: cfg.Domain, dataStore: ds, maxIdleSeconds: cfg.MaxIdleSeconds,

smtpd/datastore.go

@@ -9,8 +9,13 @@ import (
 	"github.com/jhillyerd/go.enmime"
 )
 
-// ErrNotExist indicates the requested message does not exist
+var (
-var ErrNotExist = errors.New("Message does not exist")
+	// ErrNotExist indicates the requested message does not exist
+	ErrNotExist = errors.New("Message does not exist")
+
+	// ErrNotWritable indicates the message is closed; no longer writable
+	ErrNotWritable = errors.New("Message not writable")
+)
 
 // DataStore is an interface to get Mailboxes stored in Inbucket
 type DataStore interface {

smtpd/filestore.go

@@ -3,7 +3,6 @@ package smtpd
 import (
 	"bufio"
 	"encoding/gob"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -28,9 +27,6 @@ var (
 	// million index files
 	indexLock = new(sync.RWMutex)
 
-	// TODO Consider moving this to the Message interface
-	errNotWritable = errors.New("Message not writable")
-
 	// countChannel is filled with a sequential numbers (0000..9999), which are
 	// used by generateID() to generate unique message IDs.  It's global
 	// because we only want one regardless of the number of DataStore objects
@@ -222,17 +218,15 @@ func (mb *FileMailbox) readIndex() error {
 	// Decode gob data
 	dec := gob.NewDecoder(bufio.NewReader(file))
 	for {
-		// TODO Detect EOF
 		msg := new(FileMessage)
 		if err = dec.Decode(msg); err != nil {
 			if err == io.EOF {
 				// It's OK to get an EOF here
 				break
 			}
-			return fmt.Errorf("While decoding message: %v", err)
+			return fmt.Errorf("Corrupt mailbox %q: %v", mb.indexPath, err)
 		}
 		msg.mailbox = mb
-		log.Tracef("Found: %v", msg)
 		mb.messages = append(mb.messages, msg)
 	}
 
@@ -339,8 +333,7 @@ func (m *FileMessage) ID() string {
 	return m.Fid
 }
 
-// Date returns the date of the Message
+// Date returns the date/time this Message was received by Inbucket
-// TODO Is this the create timestamp, or from the Date header?
 func (m *FileMessage) Date() time.Time {
 	return m.Fdate
 }
@@ -443,7 +436,7 @@ func (m *FileMessage) ReadRaw() (raw *string, err error) {
 func (m *FileMessage) Append(data []byte) error {
 	// Prevent Appending to a pre-existing Message
 	if !m.writable {
-		return errNotWritable
+		return ErrNotWritable
 	}
 	// Open file for writing if we haven't yet
 	if m.writer == nil {

smtpd/handler.go

@@ -413,8 +413,11 @@ func (ss *Session) dataHandler() {
 				for _, m := range messages {
 					if m != nil {
 						if err := m.Close(); err != nil {
+							// This logic should be updated to report failures
+							// writing the initial message file to the client
+							// after we implement a single-store system (issue
+							// #23)
 							ss.logError("Error: %v while writing message", err)
-							// TODO Report to client?
 						}
 						expReceivedTotal.Add(1)
 					}
@@ -437,7 +440,7 @@ func (ss *Session) dataHandler() {
 			ss.send("552 Maximum message size exceeded")
 			ss.logWarn("Max message size exceeded while in DATA")
 			ss.reset()
-			// TODO: Should really cleanup the crap on filesystem...
+			// Should really cleanup the crap on filesystem (after issue #23)
 			return
 		}
 		// Append to message objects
@@ -448,7 +451,7 @@ func (ss *Session) dataHandler() {
 						ss.logError("Failed to append to mailbox %v: %v", mailboxes[i], err)
 						ss.send("554 Something went wrong")
 						ss.reset()
-						// TODO: Should really cleanup the crap on filesystem...
+						// Should really cleanup the crap on filesystem (after issue #23)
 						return
 					}
 				}

webui/mailbox_controller.go

@@ -251,7 +251,7 @@ func MailboxHTML(w http.ResponseWriter, req *http.Request, ctx *httpd.Context) (
 		"ctx":     ctx,
 		"name":    name,
 		"message": message,
-		// TODO: It is not really safe to render, need to sanitize.
+		// TODO It is not really safe to render, need to sanitize, issue #5
 		"body": template.HTML(mime.HTML),
 	})
 }