address-book-cmd: be more lenient with errors

Instead of failing completely when address-book-cmd returns an invalid
line, ignore the line and report a warning in the logs.

Do not wait for 100 "valid" addresses before bailing out as the command
could only output garbage in large quantities. The issue of the command
not printing any new line characters still exists. We could address this
but I think it would be overkill.

Reported-by: Bence Ferdinandy <bence@ferdinandy.com>
Signed-off-by: Robin Jarry <robin@jarry.cc>
Tested-by: Bence Ferdinandy <bence@ferdinandy.com>

CHANGELOG.md

@@ -12,6 +12,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   `:rmdir`, `:archive` and the `copy-to` option.
 - Display messages from bottom to top with `reverse-msglist-order=true` in
   `aerc.conf`.
+- Display threads from bottom to top with `reverse-thread-order=true` in
+  `aerc.conf`.
 
 ### Fixed
 

completer/completer.go

@@ -10,7 +10,9 @@ import (
 	"os/exec"
 	"regexp"
 	"strings"
+	"syscall"
 
+	"git.sr.ht/~rjarry/aerc/logging"
 	"github.com/google/shlex"
 )
 
@@ -71,6 +73,10 @@ func isAddressHeader(h string) bool {
 	return false
 }
 
+const maxCompletionLines = 100
+
+var tooManyLines = fmt.Errorf("returned more than %d lines", maxCompletionLines)
+
 // completeAddress uses the configured address book completion command to fetch
 // completions for the specified string, returning a slice of completions and
 // a prefix to be prepended to the selected completion, or an error.
@@ -88,6 +94,8 @@ func (c *Completer) completeAddress(s string) ([]string, string, error) {
 	if err != nil {
 		return nil, "", fmt.Errorf("stderr: %w", err)
 	}
+	// reset the process group id to allow killing all its children
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
 	if err := cmd.Start(); err != nil {
 		return nil, "", fmt.Errorf("cmd start: %w", err)
 	}
@@ -99,6 +107,12 @@ func (c *Completer) completeAddress(s string) ([]string, string, error) {
 
 	completions, err := readCompletions(stdout)
 	if err != nil {
+		// make sure to kill the process *and* all its children
+		//nolint:errcheck // who cares?
+		syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
+		logging.Warnf("command %s killed: %s", cmd, err)
+	}
+	if err != nil && !errors.Is(err, tooManyLines) {
 		buf, _ := io.ReadAll(stderr)
 		msg := strings.TrimSpace(string(buf))
 		if msg != "" {
@@ -148,27 +162,38 @@ func (c *Completer) getAddressCmd(s string) (*exec.Cmd, error) {
 func readCompletions(r io.Reader) ([]string, error) {
 	buf := bufio.NewReader(r)
 	completions := []string{}
-	for {
+	for i := 0; i < maxCompletionLines; i++ {
 		line, err := buf.ReadString('\n')
 		if errors.Is(err, io.EOF) {
 			return completions, nil
 		} else if err != nil {
 			return nil, err
 		}
+		if strings.TrimSpace(line) == "" {
+			// skip empty lines
+			continue
+		}
 		parts := strings.SplitN(line, "\t", 3)
 		addr, err := mail.ParseAddress(strings.TrimSpace(parts[0]))
 		if err != nil {
-			return nil, err
+			logging.Warnf(
+				"line %d: %#v: could not parse address: %v",
+				line, err)
+			continue
 		}
 		if len(parts) > 1 {
 			addr.Name = strings.TrimSpace(parts[1])
 		}
 		decoded, err := decodeMIME(addr.String())
 		if err != nil {
-			return nil, fmt.Errorf("could not decode MIME string: %w", err)
+			logging.Warnf(
+				"line %d: %#v: could not decode MIME string: %v",
+				i+1, line, err)
+			continue
 		}
 		completions = append(completions, decoded)
 	}
+	return completions, tooManyLines
 }
 
 func decodeMIME(s string) (string, error) {

config/aerc.conf

@@ -194,6 +194,14 @@ completion-popovers=true
 # Default: false
 #reverse-msglist-order = false
 
+# Reverse display of the mesage threads. Default order is the the intial
+# message is on the top with all the replies being displayed below. The
+# reverse option will put the initial message at the bottom with the
+# replies on top.
+#
+# Default: false
+#reverse-thread-order=false
+
 #[ui:account=foo]
 #
 # Enable a threaded view of messages. If this is not supported by the backend

config/config.go

@@ -80,6 +80,7 @@ type UIConfig struct {
 	BorderCharHorizontal rune `ini:"-"`
 
 	ReverseOrder       bool `ini:"reverse-msglist-order"`
+	ReverseThreadOrder bool `ini:"reverse-thread-order"`
 }
 
 type ContextType int

doc/aerc-config.5.scd

@@ -335,6 +335,14 @@ These options are configured in the *[ui]* section of aerc.conf.
 
 	Default: false
 
+*reverse-thread-order*
+	Reverse display of the mesage threads. Default order is the the intial
+	message is on the top with all the replies being displayed below. The
+	reverse option will put the initial message at the bottom with the
+	replies on top.
+
+	Default: false
+
 *threading-enabled*
 	Enable a threaded view of messages. If this is not supported by the
 	backend (IMAP server or notmuch), threads will be built by the client.

lib/iterator/index.go

@@ -21,6 +21,9 @@ func FixBounds(i, lower, upper int) int {
 // WrapBounds will wrap the index i around its upper- or lower-bound if
 // out-of-bound
 func WrapBounds(i, lower, upper int) int {
+	if upper <= 0 {
+		return lower
+	}
 	switch {
 	case i > upper:
 		i = lower + (i-upper-1)%upper

lib/msgstore.go

@@ -40,6 +40,7 @@ type MessageStore struct {
 	sortCriteria []*types.SortCriterion
 
 	threadedView       bool
+	reverseThreadOrder bool
 	buildThreads       bool
 	builder            *ThreadBuilder
 
@@ -74,7 +75,7 @@ func NewMessageStore(worker *types.Worker,
 	dirInfo *models.DirectoryInfo,
 	defaultSortCriteria []*types.SortCriterion,
 	thread bool, clientThreads bool, clientThreadsDelay time.Duration,
-	reverseOrder bool,
+	reverseOrder bool, reverseThreadOrder bool,
 	triggerNewEmail func(*models.MessageInfo),
 	triggerDirectoryChange func(),
 ) *MessageStore {
@@ -93,6 +94,7 @@ func NewMessageStore(worker *types.Worker,
 
 		threadedView:       thread,
 		buildThreads:       clientThreads,
+		reverseThreadOrder: reverseThreadOrder,
 
 		filter:       []string{"filter"},
 		sortCriteria: defaultSortCriteria,
@@ -231,7 +233,7 @@ func (store *MessageStore) Update(msg types.WorkerMessage) {
 		newMap := make(map[uint32]*models.MessageInfo)
 
 		builder := NewThreadBuilder(store.iterFactory)
-		builder.RebuildUids(msg.Threads)
+		builder.RebuildUids(msg.Threads, store.reverseThreadOrder)
 		store.uids = builder.Uids()
 		store.threads = msg.Threads
 
@@ -364,6 +366,14 @@ func (store *MessageStore) update(threads bool) {
 	}
 }
 
+func (store *MessageStore) SetReverseThreadOrder(reverse bool) {
+	store.reverseThreadOrder = reverse
+}
+
+func (store *MessageStore) ReverseThreadOrder() bool {
+	return store.reverseThreadOrder
+}
+
 func (store *MessageStore) SetThreadedView(thread bool) {
 	store.threadedView = thread
 	if store.buildThreads {
@@ -424,7 +434,7 @@ func (store *MessageStore) runThreadBuilderNow() {
 		}
 	}
 	// build new threads
-	th := store.builder.Threads(store.uids)
+	th := store.builder.Threads(store.uids, store.reverseThreadOrder)
 
 	// save local threads to the message store variable and
 	// run callback if defined (callback should reposition cursor)
@@ -593,29 +603,20 @@ func (store *MessageStore) NextPrev(delta int) {
 	if len(uids) == 0 {
 		return
 	}
+
 	iter := store.iterFactory.NewIterator(uids)
 
-	uid := store.SelectedUid()
-
-	newIdx := store.FindIndexByUid(uid)
+	newIdx := store.FindIndexByUid(store.SelectedUid())
 	if newIdx < 0 {
 		store.Select(uids[iter.StartIndex()])
 		return
 	}
-
-	low, high := iter.EndIndex(), iter.StartIndex()
-	sign := -1
-	if high < low {
-		low, high = high, low
-		sign = 1
-	}
-	newIdx += sign * delta
-	if newIdx >= len(uids) {
-		newIdx = high
-	} else if newIdx < 0 {
-		newIdx = low
-	}
-
+	newIdx = iterator.MoveIndex(
+		newIdx,
+		delta,
+		iter,
+		iterator.FixBounds,
+	)
 	store.Select(uids[newIdx])
 
 	if store.BuildThreads() && store.ThreadedView() {
@@ -631,15 +632,7 @@ func (store *MessageStore) NextPrev(delta int) {
 	if store.marker != nil {
 		store.marker.UpdateVisualMark()
 	}
-
-	nextResultIndex := len(store.results) - store.resultIndex - 2*delta
-	if nextResultIndex < 0 || nextResultIndex >= len(store.results) {
-		return
-	}
-	nextResultUid := store.results[nextResultIndex]
-	if nextResultUid == store.SelectedUid() {
-		store.resultIndex += delta
-	}
+	store.updateResults()
 }
 
 func (store *MessageStore) Next() {
@@ -690,18 +683,35 @@ func (store *MessageStore) ApplyClear() {
 	store.Sort(nil, nil)
 }
 
+func (store *MessageStore) updateResults() {
+	if len(store.results) == 0 || store.resultIndex < 0 {
+		return
+	}
+	uid := store.SelectedUid()
+	for i, u := range store.results {
+		if uid == u {
+			store.resultIndex = i
+			break
+		}
+	}
+}
+
 func (store *MessageStore) nextPrevResult(delta int) {
 	if len(store.results) == 0 {
 		return
 	}
-	store.resultIndex += delta
-	if store.resultIndex >= len(store.results) {
-		store.resultIndex = 0
-	}
+	iter := store.iterFactory.NewIterator(store.results)
 	if store.resultIndex < 0 {
-		store.resultIndex = len(store.results) - 1
+		store.resultIndex = iter.StartIndex()
+	} else {
+		store.resultIndex = iterator.MoveIndex(
+			store.resultIndex,
+			delta,
+			iter,
+			iterator.WrapBounds,
+		)
 	}
-	store.Select(store.results[len(store.results)-store.resultIndex-1])
+	store.Select(store.results[store.resultIndex])
 	store.update(false)
 }
 

lib/threadbuilder.go

@@ -55,7 +55,7 @@ func (builder *ThreadBuilder) Update(msg *models.MessageInfo) {
 }
 
 // Threads returns a slice of threads for the given list of uids
-func (builder *ThreadBuilder) Threads(uids []uint32) []*types.Thread {
+func (builder *ThreadBuilder) Threads(uids []uint32, inverse bool) []*types.Thread {
 	builder.Lock()
 	defer builder.Unlock()
 
@@ -67,7 +67,7 @@ func (builder *ThreadBuilder) Threads(uids []uint32) []*types.Thread {
 	builder.sortThreads(threads, uids)
 
 	// rebuild uids from threads
-	builder.RebuildUids(threads)
+	builder.RebuildUids(threads, inverse)
 
 	elapsed := time.Since(start)
 	logging.Infof("%d threads created in %s", len(threads), elapsed)
@@ -155,15 +155,23 @@ func (builder *ThreadBuilder) sortThreads(threads []*types.Thread, orderedUids [
 }
 
 // RebuildUids rebuilds the uids from the given slice of threads
-func (builder *ThreadBuilder) RebuildUids(threads []*types.Thread) {
+func (builder *ThreadBuilder) RebuildUids(threads []*types.Thread, inverse bool) {
 	uids := make([]uint32, 0, len(threads))
 	iterT := builder.iterFactory.NewIterator(threads)
 	for iterT.Next() {
+		var threaduids []uint32
 		_ = iterT.Value().(*types.Thread).Walk(
 			func(t *types.Thread, level int, currentErr error) error {
-				uids = append(uids, t.Uid)
+				threaduids = append(threaduids, t.Uid)
 				return nil
 			})
+		if inverse {
+			for j := len(threaduids) - 1; j >= 0; j-- {
+				uids = append(uids, threaduids[j])
+			}
+		} else {
+			uids = append(uids, threaduids...)
+		}
 	}
 	result := make([]uint32, 0, len(uids))
 	iterU := builder.iterFactory.NewIterator(uids)

widgets/account.go

@@ -286,6 +286,7 @@ func (acct *AccountView) onMessage(msg types.WorkerMessage) {
 				acct.dirlist.UiConfig(name).ForceClientThreads,
 				acct.dirlist.UiConfig(name).ClientThreadsDelay,
 				acct.dirlist.UiConfig(name).ReverseOrder,
+				acct.dirlist.UiConfig(name).ReverseThreadOrder,
 				func(msg *models.MessageInfo) {
 					acct.conf.Triggers.ExecNewEmail(acct.acct,
 						acct.conf, msg)

widgets/dirtree.go

@@ -249,7 +249,7 @@ func (dt *DirectoryTree) countVisible(list []*types.Thread) (n int) {
 
 func (dt *DirectoryTree) displayText(node *types.Thread) string {
 	elems := strings.Split(dt.treeDirs[getAnyUid(node)], dt.pathSeparator)
-	return fmt.Sprintf("%s%s%s", threadPrefix(node), getFlag(node), elems[countLevels(node)])
+	return fmt.Sprintf("%s%s%s", threadPrefix(node, false), getFlag(node), elems[countLevels(node)])
 }
 
 func (dt *DirectoryTree) getDirectory(node *types.Thread) string {

widgets/msglist.go

@@ -12,6 +12,7 @@ import (
 	"git.sr.ht/~rjarry/aerc/config"
 	"git.sr.ht/~rjarry/aerc/lib"
 	"git.sr.ht/~rjarry/aerc/lib/format"
+	"git.sr.ht/~rjarry/aerc/lib/iterator"
 	"git.sr.ht/~rjarry/aerc/lib/ui"
 	"git.sr.ht/~rjarry/aerc/logging"
 	"git.sr.ht/~rjarry/aerc/models"
@@ -88,57 +89,61 @@ func (ml *MessageList) Draw(ctx *ui.Context) {
 		row          int = 0
 	)
 
-	if store.ThreadedView() {
-		iter := store.ThreadsIterator()
-		var i int = 0
-
-		for iter.Next() {
-			thread := iter.Value().(*types.Thread)
-			var lastSubject string
-			err := thread.Walk(func(t *types.Thread, _ int, currentErr error) error {
-				if currentErr != nil {
-					return currentErr
+	createBaseCtx := func(uid uint32, row int) format.Ctx {
+		return format.Ctx{
+			FromAddress: acct.acct.From,
+			AccountName: acct.Name(),
+			MsgInfo:     store.Messages[uid],
+			MsgNum:      row,
+			MsgIsMarked: store.Marker().IsMarked(uid),
 		}
+	}
+
+	if store.ThreadedView() {
+		var (
+			lastSubject string
+			prevThread  *types.Thread
+			i           int = 0
+		)
+		factory := iterator.NewFactory(!store.ReverseThreadOrder())
+	threadLoop:
+		for iter := store.ThreadsIterator(); iter.Next(); {
+			var cur []*types.Thread
+			err := iter.Value().(*types.Thread).Walk(
+				func(t *types.Thread, _ int, _ error,
+				) error {
 					if t.Hidden || t.Deleted {
 						return nil
 					}
-				if i < ml.Scroll() {
-					i++
-					return nil
-				}
-				i++
-				msg := store.Messages[t.Uid]
-				var prefix string
-				var subject string
-				var normalizedSubject string
-				if msg != nil {
-					prefix = threadPrefix(t)
-					if msg.Envelope != nil {
-						subject = msg.Envelope.Subject
-						normalizedSubject, _ = sortthread.GetBaseSubject(subject)
-					}
-				}
-				fmtCtx := format.Ctx{
-					FromAddress:       acct.acct.From,
-					AccountName:       acct.Name(),
-					MsgInfo:           msg,
-					MsgNum:            row,
-					MsgIsMarked:       store.Marker().IsMarked(t.Uid),
-					ThreadPrefix:      prefix,
-					ThreadSameSubject: normalizedSubject == lastSubject,
-				}
-				if ml.drawRow(textWidth, ctx, t.Uid, row, &needsHeaders, fmtCtx) {
-					return types.ErrSkipThread
-				}
-				lastSubject = normalizedSubject
-				row++
+					cur = append(cur, t)
 					return nil
 				})
 			if err != nil {
-				logging.Warnf("failed to walk threads: %v", err)
+				logging.Errorf("thread walk: %v", err)
+			}
+			for curIter := factory.NewIterator(cur); curIter.Next(); {
+				if i < ml.Scroll() {
+					i++
+					continue
+				}
+				if thread := curIter.Value().(*types.Thread); thread != nil {
+					fmtCtx := createBaseCtx(thread.Uid, row)
+					fmtCtx.ThreadPrefix = threadPrefix(thread,
+						store.ReverseThreadOrder())
+					if fmtCtx.MsgInfo != nil && fmtCtx.MsgInfo.Envelope != nil {
+						baseSubject, _ := sortthread.GetBaseSubject(
+							fmtCtx.MsgInfo.Envelope.Subject)
+						fmtCtx.ThreadSameSubject = baseSubject == lastSubject &&
+							sameParent(thread, prevThread) &&
+							!isParent(thread)
+						lastSubject = baseSubject
+						prevThread = thread
+					}
+					if ml.drawRow(textWidth, ctx, thread.Uid, row, &needsHeaders, fmtCtx) {
+						break threadLoop
+					}
+					row += 1
 				}
-			if row >= ctx.Height() {
-				break
 			}
 		}
 	} else {
@@ -148,15 +153,7 @@ func (ml *MessageList) Draw(ctx *ui.Context) {
 				continue
 			}
 			uid := iter.Value().(uint32)
-
-			msg := store.Messages[uid]
-			fmtCtx := format.Ctx{
-				FromAddress: acct.acct.From,
-				AccountName: acct.Name(),
-				MsgInfo:     msg,
-				MsgNum:      row,
-				MsgIsMarked: store.Marker().IsMarked(uid),
-			}
+			fmtCtx := createBaseCtx(uid, row)
 			if ml.drawRow(textWidth, ctx, uid, row, &needsHeaders, fmtCtx) {
 				break
 			}
@@ -429,15 +426,19 @@ func (ml *MessageList) drawEmptyMessage(ctx *ui.Context) {
 		uiConfig.GetStyle(config.STYLE_MSGLIST_DEFAULT), "%s", msg)
 }
 
-func threadPrefix(t *types.Thread) string {
+func threadPrefix(t *types.Thread, reverse bool) string {
 	var arrow string
 	if t.Parent != nil {
 		if t.NextSibling != nil {
 			arrow = "├─>"
+		} else {
+			if reverse {
+				arrow = "┌─>"
 			} else {
 				arrow = "└─>"
 			}
 		}
+	}
 	var prefix []string
 	for n := t; n.Parent != nil; n = n.Parent {
 		if n.Parent.NextSibling != nil {
@@ -458,3 +459,11 @@ func threadPrefix(t *types.Thread) string {
 	ps := strings.Join(prefix, "")
 	return fmt.Sprintf("%v%v", ps, arrow)
 }
+
+func sameParent(left, right *types.Thread) bool {
+	return left.Root() == right.Root()
+}
+
+func isParent(t *types.Thread) bool {
+	return t == t.Root()
+}