~sircmpwn/aerc

maildir: Provide nicer error message on invalid url v1 PROPOSED

Tero Koskinen: 1
 maildir: Provide nicer error message on invalid url

 2 files changed, 39 insertions(+), 4 deletions(-)
Hi,

Thanks for comments.

On Wed, 22 Jul 2020 21:03:19 +0200
Reto <reto@labrat.space> wrote:
Next
I'll fix this.
Next
Hi,

On Fri, 24 Jul 2020 09:48:35 +0200
Reto <reto@labrat.space> wrote:
I sent the v2 version of the patch. It should contain the typo fix, have the extra
nil check removed, and logging simplified a little.

When accounts.conf is following:
```
[P1]
source   = maildir:///etc/acme
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P2]
source   = maildir://Maildir
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P3]
source   = maildir:///home/tkoskine/Mail
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P4]
source   = maildir:///home/tkoskine/mbox
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P5]
source   = maildir://Mail
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent

[P6]
source   = maildir:Mail
default  = INBOX
from     = Tero Koskinen <tero.koskinen@iki.fi>
copy-to  = Sent
```

log entries are:
```
~/src/cvs/aerc $ grep -v "(ui)" alog.txt   
2020/07/24 22:41:27 Starting up aerc
2020/07/24 22:41:27 Initializing PGP keyring
2020/07/24 22:41:27 Starting Unix server
2020/07/24 22:41:27 Failed to start Unix server: listen unix /home/tkoskine/.runtime/aerc.sock: bind: address already in use (non-fatal)
2020/07/24 22:41:27 configured base maildir: /home/tkoskine/Mail
2020/07/24 22:41:27 could not configure maildir: /etc/acme
2020/07/24 22:41:27 could not resolve maildir from URL 'maildir:Mail'
2020/07/24 22:41:27 open /etc/acme: permission denied
2020/07/24 22:41:27 could not resolve maildir from URL 'maildir://Maildir'
2020/07/24 22:41:27 could not resolve maildir from URL 'maildir://Mail'
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 could not configure maildir: /home/tkoskine/mbox
2020/07/24 22:41:27 Given maildir '/home/tkoskine/mbox' not a directory
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Listing mailboxes...
2020/07/24 22:41:27 Incorrect maildir directory
2020/07/24 22:41:27 Connected.
2020/07/24 22:41:27 opening INBOX
~/src/cvs/aerc $ 
```


Yours,
 Tero
Export patchset (mbox)
How do I use this?

Copy & paste the following snippet into your terminal to import this patchset into git:

curl -s https://lists.sr.ht/~sircmpwn/aerc/patches/11666/mbox | git am -3
Learn more about email & git

[PATCH] maildir: Provide nicer error message on invalid url Export this patch

If accounts.conf contains an invalid maildir url, return a nice
error instead of panicing.
Check a couple of different error cases to provide extra
information about the error to the user.
---
 worker/maildir/container.go | 26 +++++++++++++++++++++++---
 worker/maildir/worker.go    | 17 ++++++++++++++++-
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/worker/maildir/container.go b/worker/maildir/container.go
index cd9a447..170e917 100644
--- a/worker/maildir/container.go
+++ b/worker/maildir/container.go
@@ -1,6 +1,7 @@
package maildir

import (
	"errors"
	"fmt"
	"log"
	"os"
@@ -21,15 +22,34 @@ type Container struct {
}

// NewContainer creates a new container at the specified directory
// TODO: return an error if the provided directory is not accessible
func NewContainer(dir string, l *log.Logger) *Container {
	return &Container{dir: dir, uids: uidstore.NewStore(), log: l}
func NewContainer(dir string, l *log.Logger) (*Container, error) {
	if len(dir) == 0 {
		return nil, errors.New("No maildir specified")
	}
	f, err := os.Open(dir)
	if err != nil {
		if os.IsPermission(err) {
			return nil, fmt.Errorf("No permissions for '%s'", dir)
		} else {
			return nil, fmt.Errorf("Unable to open directory '%s'", dir)
		}
	}
	s, err := f.Stat()
	f.Close()
	if !s.IsDir() {
		return nil, fmt.Errorf("Given maildir '%s' not a directory", dir)
	}
	return &Container{dir: dir, uids: uidstore.NewStore(), log: l}, nil
}

// ListFolders returns a list of maildir folders in the container
func (c *Container) ListFolders() ([]string, error) {
	folders := []string{}
	err := filepath.Walk(c.dir, func(path string, info os.FileInfo, err error) error {
		if err != nil {
			return fmt.Errorf("Invalid path '%s': error: %v", path, err)

		}
		if !info.IsDir() {
			return nil
		}
diff --git a/worker/maildir/worker.go b/worker/maildir/worker.go
index ce548ff..d1ff3c2 100644
--- a/worker/maildir/worker.go
+++ b/worker/maildir/worker.go
@@ -1,6 +1,7 @@
package maildir

import (
	"errors"
	"fmt"
	"io"
	"net/url"
@@ -221,7 +222,15 @@ func (w *Worker) handleConfigure(msg *types.Configure) error {
		}
		dir = filepath.Join(home, u.Path)
	}
	w.c = NewContainer(dir, w.worker.Logger)
	if len(dir) == 0 {
		return fmt.Errorf("could not resolve maildir from URL '%s'", msg.Config.Source)
	}
	c, err := NewContainer(dir, w.worker.Logger)
	if err != nil {
		w.worker.Logger.Printf("could not configure maildir: %s", dir)
		return err
	}
	w.c = c
	w.worker.Logger.Printf("configured base maildir: %s", dir)
	return nil
}
@@ -231,6 +240,12 @@ func (w *Worker) handleConnect(msg *types.Connect) error {
}

func (w *Worker) handleListDirectories(msg *types.ListDirectories) error {
	// TODO If handleConfigure has returned error, w.c is nil.
	// It could be better if we skip directory listing completely
	// when configure fails.
	if w.c == nil {
		return errors.New("Incorrect maildir directory")
	}
	dirs, err := w.c.ListFolders()
	if err != nil {
		w.worker.Logger.Printf("error listing directories: %v", err)
-- 
2.27.0
Hi,
Thanks for the patch, some comments:
Please do the check len(dir) != 0 only once, here is fine then NewContainer is
never called with an empty string as input.

Cheers,
Reto