~rjarry/aerc-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
8 3

[PATCH aerc v2 0/3] enable multi-file notmuch operations

Details
Message ID
<20240212182229.109865-5-me@jasoncarloscox.com>
DKIM signature
pass
Download raw message
Implement some possible strategies for handling file-based operations
(e.g., move, copy, delete) on notmuch messages that represent multiple
files. Users can set a default strategy via multi-file-strategy in
accounts.conf or on a per-command basis with the -m flag.

See the changes to aerc-notmuch(5) for more details about the
implemented strategies. There are certainly more that could be
implemented, but this series gets started with a few simple ones.

I know that there's been lots of talk lately of changing/unifying
notmuch and maildir. I have some thoughts on that, which I'll share
on the relevant threads soon, but whatever happens, I think we'll need
to handle multi-file operations at some point.

v1->v2: fix linting errors

Jason Cox (3):
  notmuch: ignore duplicate msg ID error on delete
  notmuch: simplify moving a message
  notmuch: add strategies for multi-file messages

 commands/msg/archive.go        |  26 +++-
 commands/msg/copy.go           |  29 +++-
 commands/msg/delete.go         |  23 ++-
 commands/msg/move.go           |  32 +++-
 commands/msg/recall.go         |   1 +
 commands/msg/reply.go          |   2 +-
 doc/aerc-notmuch.5.scd         |  24 ++-
 doc/aerc.1.scd                 |  23 ++-
 lib/msgstore.go                |  18 ++-
 worker/notmuch/lib/database.go |   2 +-
 worker/notmuch/message.go      | 171 ++++++++++++---------
 worker/notmuch/message_test.go | 264 +++++++++++++++++++++++++++++++++
 worker/notmuch/worker.go       |  62 ++++----
 worker/types/messages.go       |  13 +-
 worker/types/mfs.go            |  33 +++++
 15 files changed, 584 insertions(+), 139 deletions(-)
 create mode 100644 worker/notmuch/message_test.go
 create mode 100644 worker/types/mfs.go

-- 
2.43.1

[PATCH aerc v2 1/3] notmuch: ignore duplicate msg ID error on delete

Details
Message ID
<20240212182229.109865-6-me@jasoncarloscox.com>
In-Reply-To
<20240212182229.109865-5-me@jasoncarloscox.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +1 -1
According to the comments in notmuch.h, removing a file returns
NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID when the "filename was removed but
the message persists in the database with at least one other filename."

Based on the fact that the db.DeleteMessage() function is used in a
context that explicitly recognizes the possibility of multiple files and
only attempts to delete one of them, I'm fairly certain that the
existing behavior of ignoring all errors *except* the deplicate message
ID error is the result of a typo. (It also doesn't make sense to ignore
other errors, such as NOTMUCH_STATUS_XAPIAN_EXCEPTION.)

Cc: Tim Culverhouse <tim@timculverhouse.com>
Signed-off-by: Jason Cox <me@jasoncarloscox.com>
---

v1->v2: unchanged

 worker/notmuch/lib/database.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/worker/notmuch/lib/database.go b/worker/notmuch/lib/database.go
index be30cbc0..119c1749 100644
--- a/worker/notmuch/lib/database.go
+++ b/worker/notmuch/lib/database.go
@@ -230,7 +230,7 @@ func (db *DB) DeleteMessage(filename string) error {
		}
	}()
	err = db.db.RemoveFile(filename)
	if err != nil && errors.Is(err, notmuch.STATUS_DUPLICATE_MESSAGE_ID) {
	if err != nil && !errors.Is(err, notmuch.STATUS_DUPLICATE_MESSAGE_ID) {
		return err
	}
	return nil
-- 
2.43.1

[PATCH aerc v2 2/3] notmuch: simplify moving a message

Details
Message ID
<20240212182229.109865-7-me@jasoncarloscox.com>
In-Reply-To
<20240212182229.109865-5-me@jasoncarloscox.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +1 -10
Delete the existing file from the database after indexing the new one to
avoid the need to remember and re-apply tags.

Signed-off-by: Jason Cox <me@jasoncarloscox.com>
---

v1->v2: unchanged

 worker/notmuch/message.go | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/worker/notmuch/message.go b/worker/notmuch/message.go
index 849363f1..c26c84fc 100644
--- a/worker/notmuch/message.go
+++ b/worker/notmuch/message.go
@@ -237,19 +237,10 @@ func (m *Message) Move(srcDir, destDir maildir.Dir) error {
		return fmt.Errorf("no matching message file found in %s", string(srcDir))
	}

	tags, err := m.Tags()
	if err != nil {
		return err
	}

	// Remove encoded UID information from the key to prevent sync issues
	name := lib.StripUIDFromMessageFilename(filepath.Base(src))
	dest := filepath.Join(string(destDir), "cur", name)

	if err := m.db.DeleteMessage(src); err != nil {
		return err
	}

	if err := os.Rename(src, dest); err != nil {
		return err
	}
@@ -258,7 +249,7 @@ func (m *Message) Move(srcDir, destDir maildir.Dir) error {
		return err
	}

	if err := m.ModifyTags(tags, nil); err != nil {
	if err := m.db.DeleteMessage(src); err != nil {
		return err
	}

-- 
2.43.1

[PATCH aerc v2 3/3] notmuch: add strategies for multi-file messages

Details
Message ID
<20240212182229.109865-8-me@jasoncarloscox.com>
In-Reply-To
<20240212182229.109865-5-me@jasoncarloscox.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +586 -132
A single notmuch message can represent multiple files. As a result,
file-based operations like move, copy, and delete can be ambiguous. Add
a new account config option, multi-file-strategy, to tell aerc how to
handle these ambiguous cases. Also add options to relevant commands to
set the multi-file strategy on a per-invocation basis.

If no multi-file strategy is set, refuse to take file-based actions on
multi-file messages. This default behavior is mostly the same as aerc's
previous behavior, but a bit stricter in some cases which previously
tried to be smart about multi-file operations (e.g., move and delete).

Applying multi-file strategies to cross-account copy and move operations
is not implemented. These operations will proceed as they have in the
past -- aerc will copy/move a single file. However, for cross-account
move operations, aerc will refuse to delete multiple files to prevent
data loss as not all of the files are added to the destination account.

See the changes to aerc-notmuch(5) for details on the currently
supported multi-file strategies.

Changelog-added: Tell aerc how to handle file-based operations on
 multi-file notmuch messages with the account config option
 `multi-file-strategy` and the `-m` flag to `:archive`, `:copy`,
 `:delete`, and `:move`.
Signed-off-by: Jason Cox <me@jasoncarloscox.com>
---

v1->v2: fix linting errors

 commands/msg/archive.go        |  26 +++-
 commands/msg/copy.go           |  29 +++-
 commands/msg/delete.go         |  23 ++-
 commands/msg/move.go           |  32 +++-
 commands/msg/recall.go         |   1 +
 commands/msg/reply.go          |   2 +-
 doc/aerc-notmuch.5.scd         |  24 ++-
 doc/aerc.1.scd                 |  23 ++-
 lib/msgstore.go                |  18 ++-
 worker/notmuch/message.go      | 168 +++++++++++++--------
 worker/notmuch/message_test.go | 264 +++++++++++++++++++++++++++++++++
 worker/notmuch/worker.go       |  62 ++++----
 worker/types/messages.go       |  13 +-
 worker/types/mfs.go            |  33 +++++
 14 files changed, 586 insertions(+), 132 deletions(-)
 create mode 100644 worker/notmuch/message_test.go
 create mode 100644 worker/types/mfs.go

diff --git a/commands/msg/archive.go b/commands/msg/archive.go
index 49f375a8..bf960c0c 100644
--- a/commands/msg/archive.go
+++ b/commands/msg/archive.go
@@ -22,7 +22,19 @@ const (
var ARCHIVE_TYPES = []string{ARCHIVE_FLAT, ARCHIVE_YEAR, ARCHIVE_MONTH}

type Archive struct {
	Type string `opt:"type" action:"ParseArchiveType" metavar:"flat|year|month" complete:"CompleteType"`
	MultiFileStrategy *types.MultiFileStrategy `opt:"-m" action:"ParseMFS" complete:"CompleteMFS"`
	Type              string                   `opt:"type" action:"ParseArchiveType" metavar:"flat|year|month" complete:"CompleteType"`
}

func (a *Archive) ParseMFS(arg string) error {
	if arg != "" {
		mfs, ok := types.StrToStrategy[arg]
		if !ok {
			return fmt.Errorf("invalid multi-file strategy %s", arg)
		}
		a.MultiFileStrategy = &mfs
	}
	return nil
}

func (a *Archive) ParseArchiveType(arg string) error {
@@ -47,6 +59,10 @@ func (Archive) Aliases() []string {
	return []string{"archive"}
}

func (Archive) CompleteMFS(arg string) []string {
	return commands.FilterList(types.StrategyStrs(), arg, nil)
}

func (*Archive) CompleteType(arg string) []string {
	return commands.FilterList(ARCHIVE_TYPES, arg, nil)
}
@@ -57,11 +73,13 @@ func (a Archive) Execute(args []string) error {
	if err != nil {
		return err
	}
	err = archive(msgs, a.Type)
	err = archive(msgs, a.MultiFileStrategy, a.Type)
	return err
}

func archive(msgs []*models.MessageInfo, archiveType string) error {
func archive(msgs []*models.MessageInfo, mfs *types.MultiFileStrategy,
	archiveType string,
) error {
	h := newHelper()
	acct, err := h.account()
	if err != nil {
@@ -111,7 +129,7 @@ func archive(msgs []*models.MessageInfo, archiveType string) error {
	success := true

	for dir, uids := range uidMap {
		store.Move(uids, dir, true, func(
		store.Move(uids, dir, true, mfs, func(
			msg types.WorkerMessage,
		) {
			switch msg := msg.(type) {
diff --git a/commands/msg/copy.go b/commands/msg/copy.go
index ed963707..e8fc9e19 100644
--- a/commands/msg/copy.go
+++ b/commands/msg/copy.go
@@ -14,9 +14,10 @@ import (
)

type Copy struct {
	CreateFolders bool   `opt:"-p"`
	Account       string `opt:"-a" complete:"CompleteAccount"`
	Folder        string `opt:"folder" complete:"CompleteFolder"`
	CreateFolders     bool                     `opt:"-p"`
	Account           string                   `opt:"-a" complete:"CompleteAccount"`
	MultiFileStrategy *types.MultiFileStrategy `opt:"-m" action:"ParseMFS" complete:"CompleteMFS"`
	Folder            string                   `opt:"folder" complete:"CompleteFolder"`
}

func init() {
@@ -31,6 +32,17 @@ func (Copy) Aliases() []string {
	return []string{"cp", "copy"}
}

func (c *Copy) ParseMFS(arg string) error {
	if arg != "" {
		mfs, ok := types.StrToStrategy[arg]
		if !ok {
			return fmt.Errorf("invalid multi-file strategy %s", arg)
		}
		c.MultiFileStrategy = &mfs
	}
	return nil
}

func (*Copy) CompleteAccount(arg string) []string {
	return commands.FilterList(app.AccountNames(), arg, commands.QuoteSpace)
}
@@ -48,6 +60,10 @@ func (c *Copy) CompleteFolder(arg string) []string {
	return commands.FilterList(acct.Directories().List(), arg, nil)
}

func (Copy) CompleteMFS(arg string) []string {
	return commands.FilterList(types.StrategyStrs(), arg, nil)
}

func (c Copy) Execute(args []string) error {
	h := newHelper()
	uids, err := h.markedOrSelectedUids()
@@ -60,9 +76,10 @@ func (c Copy) Execute(args []string) error {
	}

	if len(c.Account) == 0 {
		store.Copy(uids, c.Folder, c.CreateFolders, func(msg types.WorkerMessage) {
			c.CallBack(msg, uids, store)
		})
		store.Copy(uids, c.Folder, c.CreateFolders, c.MultiFileStrategy,
			func(msg types.WorkerMessage) {
				c.CallBack(msg, uids, store)
			})
		return nil
	}

diff --git a/commands/msg/delete.go b/commands/msg/delete.go
index 19f44b43..63cfdfda 100644
--- a/commands/msg/delete.go
+++ b/commands/msg/delete.go
@@ -13,7 +13,9 @@ import (
	"git.sr.ht/~rjarry/aerc/worker/types"
)

type Delete struct{}
type Delete struct {
	MultiFileStrategy *types.MultiFileStrategy `opt:"-m" action:"ParseMFS" complete:"CompleteMFS"`
}

func init() {
	commands.Register(Delete{})
@@ -27,7 +29,22 @@ func (Delete) Aliases() []string {
	return []string{"delete", "delete-message"}
}

func (Delete) Execute(args []string) error {
func (d *Delete) ParseMFS(arg string) error {
	if arg != "" {
		mfs, ok := types.StrToStrategy[arg]
		if !ok {
			return fmt.Errorf("invalid multi-file strategy %s", arg)
		}
		d.MultiFileStrategy = &mfs
	}
	return nil
}

func (Delete) CompleteMFS(arg string) []string {
	return commands.FilterList(types.StrategyStrs(), arg, nil)
}

func (d Delete) Execute(args []string) error {
	h := newHelper()
	store, err := h.store()
	if err != nil {
@@ -46,7 +63,7 @@ func (Delete) Execute(args []string) error {
	marker.ClearVisualMark()
	// caution, can be nil
	next := findNextNonDeleted(uids, store)
	store.Delete(uids, func(msg types.WorkerMessage) {
	store.Delete(uids, d.MultiFileStrategy, func(msg types.WorkerMessage) {
		switch msg := msg.(type) {
		case *types.Done:
			var s string
diff --git a/commands/msg/move.go b/commands/msg/move.go
index 704ca6e6..c0c5d171 100644
--- a/commands/msg/move.go
+++ b/commands/msg/move.go
@@ -17,9 +17,10 @@ import (
)

type Move struct {
	CreateFolders bool   `opt:"-p"`
	Account       string `opt:"-a" complete:"CompleteAccount"`
	Folder        string `opt:"folder" complete:"CompleteFolder"`
	CreateFolders     bool                     `opt:"-p"`
	Account           string                   `opt:"-a" complete:"CompleteAccount"`
	MultiFileStrategy *types.MultiFileStrategy `opt:"-m" action:"ParseMFS" complete:"CompleteMFS"`
	Folder            string                   `opt:"folder" complete:"CompleteFolder"`
}

func init() {
@@ -34,6 +35,17 @@ func (Move) Aliases() []string {
	return []string{"mv", "move"}
}

func (m *Move) ParseMFS(arg string) error {
	if arg != "" {
		mfs, ok := types.StrToStrategy[arg]
		if !ok {
			return fmt.Errorf("invalid multi-file strategy %s", arg)
		}
		m.MultiFileStrategy = &mfs
	}
	return nil
}

func (*Move) CompleteAccount(arg string) []string {
	return commands.FilterList(app.AccountNames(), arg, commands.QuoteSpace)
}
@@ -51,6 +63,10 @@ func (m *Move) CompleteFolder(arg string) []string {
	return commands.FilterList(acct.Directories().List(), arg, nil)
}

func (Move) CompleteMFS(arg string) []string {
	return commands.FilterList(types.StrategyStrs(), arg, nil)
}

func (m Move) Execute(args []string) error {
	h := newHelper()
	acct, err := h.account()
@@ -71,9 +87,10 @@ func (m Move) Execute(args []string) error {
	marker.ClearVisualMark()

	if len(m.Account) == 0 {
		store.Move(uids, m.Folder, m.CreateFolders, func(msg types.WorkerMessage) {
			m.CallBack(msg, acct, uids, next, marker, false)
		})
		store.Move(uids, m.Folder, m.CreateFolders, m.MultiFileStrategy,
			func(msg types.WorkerMessage) {
				m.CallBack(msg, acct, uids, next, marker, false)
			})
		return nil
	}

@@ -158,7 +175,8 @@ func (m Move) Execute(args []string) error {
			}
		}
		if len(appended) > 0 {
			store.Delete(appended, func(msg types.WorkerMessage) {
			mfs := types.Refuse
			store.Delete(appended, &mfs, func(msg types.WorkerMessage) {
				m.CallBack(msg, acct, appended, next, marker, timeout)
			})
		}
diff --git a/commands/msg/recall.go b/commands/msg/recall.go
index e5cd2ed1..5acf6dff 100644
--- a/commands/msg/recall.go
+++ b/commands/msg/recall.go
@@ -75,6 +75,7 @@ func (r Recall) Execute(args []string) error {
			deleteMessage := func() {
				store.Delete(
					uids,
					nil,
					func(msg types.WorkerMessage) {
						switch msg := msg.(type) {
						case *types.Done:
diff --git a/commands/msg/reply.go b/commands/msg/reply.go
index ee31a37e..5a3e0d1a 100644
--- a/commands/msg/reply.go
+++ b/commands/msg/reply.go
@@ -179,7 +179,7 @@ func (r reply) Execute(args []string) error {
			switch {
			case c.Sent() && c.Archive() != "":
				store.Answered([]uint32{msg.Uid}, true, nil)
				err := archive([]*models.MessageInfo{msg}, c.Archive())
				err := archive([]*models.MessageInfo{msg}, nil, c.Archive())
				if err != nil {
					app.PushStatus("Archive failed", 10*time.Second)
				}
diff --git a/doc/aerc-notmuch.5.scd b/doc/aerc-notmuch.5.scd
index 6837f720..202f20fd 100644
--- a/doc/aerc-notmuch.5.scd
+++ b/doc/aerc-notmuch.5.scd
@@ -68,9 +68,8 @@ options are available:

	N.B.: aerc will still always show messages and not files (under notmuch,
	a single message can be represented by several files), which makes the
	semantics of certain commands as *move* ambiguous: for example, if you
	try to move a message represented by several files, aerc will not know
	what to do and thus refuse.
	semantics of certain commands as *move* ambiguous. Use *multi-file-strategy*
	to tell aerc how to resolve these ambiguities.

*maildir-account-path* = _<path>_
	Path to the maildir account relative to the *maildir-store*.
@@ -78,6 +77,25 @@ options are available:
	This could be used to achieve traditional maildir one tab per account
	behavior. The note on *maildir-store* also applies to this option.

*multi-file-stategy* = _<strategy>_
	Strategy for file operations (e.g., move, copy, delete) on messages that are
	backed by multiple files. Possible values:

	- *refuse* (default): Refuse to act.
	- *act-all*: Act on all files.
	- *act-one*: Act on one of the files, arbitrarily chosen, and ignore the
	rest.
	- *act-one-delete-rest*: Like *act-one*, but delete the remaining files.
	- *act-dir*: Act on all files within the current folder and ignore the rest.
	Note that this strategy only works within the maildir directories; in other
	directories, it behaves like *refuse*.
	- *act-dir-delete-rest*: Like *act-dir*, but delete the remaining files.

	Note that the strategy has no effect on cross-account operations. Copying a
	message across accounts will always copy a single file, arbitrarily chosen.
	Moving a message across accounts will always copy a single file, arbitrarily
	chosen, and refuse to delete multiple files from the source account.

# USAGE

Notmuch shows slightly different behavior than for example imap. Some commands
diff --git a/doc/aerc.1.scd b/doc/aerc.1.scd
index e3079090..042d0671 100644
--- a/doc/aerc.1.scd
+++ b/doc/aerc.1.scd
@@ -255,7 +255,7 @@ These commands work in any context.
These commands are valid in any context that has a selected message (e.g. the
message list, the message in the message viewer, etc).

*:archive* _<scheme>_
*:archive* [*-m* _<strategy>_] _<scheme>_
	Moves the selected message to the archive. The available schemes are:

	_flat_: No special structure, all messages in the archive directory
@@ -264,6 +264,9 @@ message list, the message in the message viewer, etc).

	_month_: Messages are stored in folders per year and subfolders per month

	The *-m* option sets the multi-file strategy. See *aerc-notmuch*(5) for more
	details.

*:accept* [*-e*|*-E*]
	Accepts an iCalendar meeting invitation.

@@ -278,8 +281,8 @@ message list, the message in the message viewer, etc).

	*-E*: Forces *[compose].edit-headers* = _false_ for this message only.

*:copy* [*-p*] [*-a* _<account>_] _<folder>_++
*:cp* [*-p*] [*-a* _<account>_] _<folder>_
*:copy* [*-p*] [*-a* _<account>_] [*-m* _<strategy>_] _<folder>_++
*:cp* [*-p*] [*-a* _<account>_] [*-m* _<strategy>_] _<folder>_
	Copies the selected message(s) to _<folder>_.

	*-p*: Create _<folder>_ if it does not exist.
@@ -287,6 +290,8 @@ message list, the message in the message viewer, etc).
	*-a*: Copy to _<folder>_ of _<account>_. If _<folder>_ does
	not exist, it will be created whether or not *-p* is used.

	*-m*: Set the multi-file strategy. See *aerc-notmuch*(5) for more details.

*:decline* [*-e*|*-E*]
	Declines an iCalendar meeting invitation.

@@ -294,10 +299,12 @@ message list, the message in the message viewer, etc).

	*-E*: Forces *[compose].edit-headers* = _false_ for this message only.

*:delete*++
*:delete-message*
*:delete* [*-m* _<strategy>_]++
*:delete-message* [*-m* _<strategy>_]
	Deletes the selected message.

	*-m*: Set the multi-file strategy. See *aerc-notmuch*(5) for more details.

*:envelope* [*-h*] [*-s* _<format-specifier>_]
	Opens the message envelope in a dialog popup.

@@ -341,8 +348,8 @@ message list, the message in the message viewer, etc).

	*-E*: Forces *[compose].edit-headers* = _false_ for this message only.

*:move* [*-p*] [*-a* _<account>_] _<folder>_++
*:mv* [*-p*] [*-a* _<account>_] _<folder>_
*:move* [*-p*] [*-a* _<account>_] [*-m* _<strategy>_] _<folder>_++
*:mv* [*-p*] [*-a* _<account>_] [*-m* _<strategy>_] _<folder>_
	Moves the selected message(s) to _<folder>_.

	*-p*: Create _<folder>_ if it does not exist.
@@ -350,6 +357,8 @@ message list, the message in the message viewer, etc).
	*-a*: Move to _<folder>_ of _<account>_. If _<folder>_ does
	not exist, it will be created whether or not *-p* is used.

	*-m*: Set the multi-file strategy. See *aerc-notmuch*(5) for more details.

*:patch* _<args ...>_
	Patch management sub-commands. See *aerc-patch*(7) for more details.

diff --git a/lib/msgstore.go b/lib/msgstore.go
index 33daa9ff..03ce3c74 100644
--- a/lib/msgstore.go
+++ b/lib/msgstore.go
@@ -572,14 +572,14 @@ func (store *MessageStore) doThreadFolding(uid uint32, hide bool, toggle bool) e
	return nil
}

func (store *MessageStore) Delete(uids []uint32,
func (store *MessageStore) Delete(uids []uint32, mfs *types.MultiFileStrategy,
	cb func(msg types.WorkerMessage),
) {
	for _, uid := range uids {
		store.Deleted[uid] = nil
	}

	store.worker.PostAction(&types.DeleteMessages{Uids: uids},
	store.worker.PostAction(&types.DeleteMessages{Uids: uids, MultiFileStrategy: mfs},
		func(msg types.WorkerMessage) {
			if _, ok := msg.(*types.Error); ok {
				store.revertDeleted(uids)
@@ -601,7 +601,7 @@ func (store *MessageStore) revertDeleted(uids []uint32) {
}

func (store *MessageStore) Copy(uids []uint32, dest string, createDest bool,
	cb func(msg types.WorkerMessage),
	mfs *types.MultiFileStrategy, cb func(msg types.WorkerMessage),
) {
	if createDest {
		store.worker.PostAction(&types.CreateDirectory{
@@ -611,8 +611,9 @@ func (store *MessageStore) Copy(uids []uint32, dest string, createDest bool,
	}

	store.worker.PostAction(&types.CopyMessages{
		Destination: dest,
		Uids:        uids,
		Destination:       dest,
		Uids:              uids,
		MultiFileStrategy: mfs,
	}, func(msg types.WorkerMessage) {
		if _, ok := msg.(*types.Done); ok {
			store.triggerMailAdded(dest)
@@ -622,7 +623,7 @@ func (store *MessageStore) Copy(uids []uint32, dest string, createDest bool,
}

func (store *MessageStore) Move(uids []uint32, dest string, createDest bool,
	cb func(msg types.WorkerMessage),
	mfs *types.MultiFileStrategy, cb func(msg types.WorkerMessage),
) {
	for _, uid := range uids {
		store.Deleted[uid] = nil
@@ -636,8 +637,9 @@ func (store *MessageStore) Move(uids []uint32, dest string, createDest bool,
	}

	store.worker.PostAction(&types.MoveMessages{
		Destination: dest,
		Uids:        uids,
		Destination:       dest,
		Uids:              uids,
		MultiFileStrategy: mfs,
	}, func(msg types.WorkerMessage) {
		switch msg.(type) {
		case *types.Error:
diff --git a/worker/notmuch/message.go b/worker/notmuch/message.go
index c26c84fc..822f3abd 100644
--- a/worker/notmuch/message.go
+++ b/worker/notmuch/message.go
@@ -17,6 +17,7 @@ import (
	"git.sr.ht/~rjarry/aerc/models"
	"git.sr.ht/~rjarry/aerc/worker/lib"
	notmuch "git.sr.ht/~rjarry/aerc/worker/notmuch/lib"
	"git.sr.ht/~rjarry/aerc/worker/types"
)

type Message struct {
@@ -173,87 +174,144 @@ func (m *Message) ModifyTags(add, remove []string) error {
	return m.db.MsgModifyTags(m.key, add, remove)
}

func (m *Message) Remove(dir maildir.Dir) error {
	filenames, err := m.db.MsgFilenames(m.key)
func (m *Message) Remove(curDir maildir.Dir, mfs types.MultiFileStrategy) error {
	rm, del, err := m.filenamesForStrategy(mfs, curDir)
	if err != nil {
		return err
	}
	for _, filename := range filenames {
		if dirContains(dir, filename) {
			err := m.db.DeleteMessage(filename)
			if err != nil {
				return err
			}

			if err := os.Remove(filename); err != nil {
				return err
			}

			return nil
		}
	}

	return fmt.Errorf("no matching message file found in %s", string(dir))
	rm = append(rm, del...)
	return m.deleteFiles(rm)
}

func (m *Message) Copy(target maildir.Dir) error {
	filename, err := m.Filename()
func (m *Message) Copy(curDir, destDir maildir.Dir, mfs types.MultiFileStrategy) error {
	cp, del, err := m.filenamesForStrategy(mfs, curDir)
	if err != nil {
		return err
	}

	source, key := parseFilename(filename)
	if key == "" {
		return fmt.Errorf("failed to parse message filename: %s", filename)
	}
	for _, filename := range cp {
		source, key := parseFilename(filename)
		if key == "" {
			return fmt.Errorf("failed to parse message filename: %s", filename)
		}

	newKey, err := source.Copy(target, key)
	if err != nil {
		return err
		newKey, err := source.Copy(destDir, key)
		if err != nil {
			return err
		}
		newFilename, err := destDir.Filename(newKey)
		if err != nil {
			return err
		}
		_, err = m.db.IndexFile(newFilename)
		if err != nil {
			return err
		}
	}
	newFilename, err := target.Filename(newKey)

	return m.deleteFiles(del)
}

func (m *Message) Move(curDir, destDir maildir.Dir, mfs types.MultiFileStrategy) error {
	move, del, err := m.filenamesForStrategy(mfs, curDir)
	if err != nil {
		return err
	}
	_, err = m.db.IndexFile(newFilename)
	return err
}

func (m *Message) Move(srcDir, destDir maildir.Dir) error {
	var src string
	for _, filename := range move {
		// Remove encoded UID information from the key to prevent sync issues
		name := lib.StripUIDFromMessageFilename(filepath.Base(filename))
		dest := filepath.Join(string(destDir), "cur", name)

	filenames, err := m.db.MsgFilenames(m.key)
	if err != nil {
		return err
		if err := os.Rename(filename, dest); err != nil {
			return err
		}

		if _, err = m.db.IndexFile(dest); err != nil {
			return err
		}

		if err := m.db.DeleteMessage(filename); err != nil {
			return err
		}
	}

	return m.deleteFiles(del)
}

func (m *Message) deleteFiles(filenames []string) error {
	for _, filename := range filenames {
		if dirContains(srcDir, filename) {
			src = filename
			break
		if err := os.Remove(filename); err != nil {
			return err
		}

		if err := m.db.DeleteMessage(filename); err != nil {
			return err
		}
	}

	if src == "" {
		return fmt.Errorf("no matching message file found in %s", string(srcDir))
	return nil
}

func (m *Message) filenamesForStrategy(strategy types.MultiFileStrategy,
	curDir maildir.Dir,
) (act, del []string, err error) {
	filenames, err := m.db.MsgFilenames(m.key)
	if err != nil {
		return nil, nil, err
	}
	return filterForStrategy(filenames, strategy, curDir)
}

	// Remove encoded UID information from the key to prevent sync issues
	name := lib.StripUIDFromMessageFilename(filepath.Base(src))
	dest := filepath.Join(string(destDir), "cur", name)
func filterForStrategy(filenames []string, strategy types.MultiFileStrategy,
	curDir maildir.Dir,
) (act, del []string, err error) {
	if curDir == "" &&
		(strategy == types.ActDir || strategy == types.ActDirDelRest) {
		strategy = types.Refuse
	}

	if err := os.Rename(src, dest); err != nil {
		return err
	if len(filenames) < 2 {
		return filenames, []string{}, nil
	}

	if _, err = m.db.IndexFile(dest); err != nil {
		return err
	act = []string{}
	rest := []string{}
	switch strategy {
	case types.Refuse:
		return nil, nil, fmt.Errorf("refusing to act on multiple files")
	case types.ActAll:
		act = filenames
	case types.ActOne:
		fallthrough
	case types.ActOneDelRest:
		act = filenames[:1]
		rest = filenames[1:]
	case types.ActDir:
		fallthrough
	case types.ActDirDelRest:
		for _, filename := range filenames {
			if filepath.Dir(filepath.Dir(filename)) == string(curDir) {
				act = append(act, filename)
			} else {
				rest = append(rest, filename)
			}
		}
	default:
		return nil, nil, fmt.Errorf("invalid multi-file strategy %v", strategy)
	}

	if err := m.db.DeleteMessage(src); err != nil {
		return err
	switch strategy {
	case types.ActOneDelRest:
		fallthrough
	case types.ActDirDelRest:
		del = rest
	default:
		del = []string{}
	}

	return nil
	return act, del, nil
}

func parseFilename(filename string) (maildir.Dir, string) {
@@ -270,13 +328,3 @@ func parseFilename(filename string) (maildir.Dir, string) {
	key := split[0]
	return maildir.Dir(dir), key
}

func dirContains(dir maildir.Dir, filename string) bool {
	for _, sub := range []string{"cur", "new"} {
		match, _ := filepath.Match(filepath.Join(string(dir), sub, "*"), filename)
		if match {
			return true
		}
	}
	return false
}
diff --git a/worker/notmuch/message_test.go b/worker/notmuch/message_test.go
new file mode 100644
index 00000000..51fcdb09
--- /dev/null
+++ b/worker/notmuch/message_test.go
@@ -0,0 +1,264 @@
//go:build notmuch
// +build notmuch

package notmuch

import (
	"testing"

	"git.sr.ht/~rjarry/aerc/worker/types"
	"github.com/emersion/go-maildir"
)

func TestFilterForStrategy(t *testing.T) {
	tests := []struct {
		filenames   []string
		strategy    types.MultiFileStrategy
		curDir      string
		expectedAct []string
		expectedDel []string
		expectedErr bool
	}{
		// if there's only one file, always act on it
		{
			filenames:   []string{"/h/j/m/A/cur/a.b.c:2,"},
			strategy:    types.Refuse,
			curDir:      "/h/j/m/B",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{},
		},
		{
			filenames:   []string{"/h/j/m/A/cur/a.b.c:2,"},
			strategy:    types.ActAll,
			curDir:      "/h/j/m/B",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{},
		},
		{
			filenames:   []string{"/h/j/m/A/cur/a.b.c:2,"},
			strategy:    types.ActOne,
			curDir:      "/h/j/m/B",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{},
		},
		{
			filenames:   []string{"/h/j/m/A/cur/a.b.c:2,"},
			strategy:    types.ActOneDelRest,
			curDir:      "/h/j/m/B",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{},
		},
		{
			filenames:   []string{"/h/j/m/A/cur/a.b.c:2,"},
			strategy:    types.ActDir,
			curDir:      "/h/j/m/B",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{},
		},
		{
			filenames:   []string{"/h/j/m/A/cur/a.b.c:2,"},
			strategy:    types.ActDirDelRest,
			curDir:      "/h/j/m/B",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{},
		},

		// follow strategy for multiple files
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy:    types.Refuse,
			curDir:      "/h/j/m/B",
			expectedErr: true,
		},
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy: types.ActAll,
			curDir:   "/h/j/m/B",
			expectedAct: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			expectedDel: []string{},
		},
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy:    types.ActOne,
			curDir:      "/h/j/m/B",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{},
		},
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy:    types.ActOneDelRest,
			curDir:      "/h/j/m/B",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
		},
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy: types.ActDir,
			curDir:   "/h/j/m/B",
			expectedAct: []string{
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
			},
			expectedDel: []string{},
		},
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy: types.ActDirDelRest,
			curDir:   "/h/j/m/B",
			expectedAct: []string{
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
			},
			expectedDel: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/C/new/d.e.f",
			},
		},

		// refuse to act on multiple files for ActDir and friends if
		// no current dir is provided
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy:    types.ActDir,
			curDir:      "",
			expectedErr: true,
		},
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy:    types.ActDirDelRest,
			curDir:      "",
			expectedErr: true,
		},

		// act on multiple files w/o current dir for other strategies
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy: types.ActAll,
			curDir:   "",
			expectedAct: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			expectedDel: []string{},
		},
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy:    types.ActOne,
			curDir:      "",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{},
		},
		{
			filenames: []string{
				"/h/j/m/A/cur/a.b.c:2,",
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
			strategy:    types.ActOneDelRest,
			curDir:      "",
			expectedAct: []string{"/h/j/m/A/cur/a.b.c:2,"},
			expectedDel: []string{
				"/h/j/m/B/new/b.c.d",
				"/h/j/m/B/cur/c.d.e:2,S",
				"/h/j/m/C/new/d.e.f",
			},
		},
	}

	for i, test := range tests {
		act, del, err := filterForStrategy(test.filenames, test.strategy,
			maildir.Dir(test.curDir))

		if test.expectedErr && err == nil {
			t.Errorf("[test %d] got nil, expected error", i)
		}

		if !test.expectedErr && err != nil {
			t.Errorf("[test %d] got %v, expected nil", i, err)
		}

		if !arrEq(act, test.expectedAct) {
			t.Errorf("[test %d] got %v, expected %v", i, act, test.expectedAct)
		}

		if !arrEq(del, test.expectedDel) {
			t.Errorf("[test %d] got %v, expected %v", i, del, test.expectedDel)
		}
	}
}

func arrEq(a, b []string) bool {
	if len(a) != len(b) {
		return false
	}

	for i := range a {
		if a[i] != b[i] {
			return false
		}
	}

	return true
}
diff --git a/worker/notmuch/worker.go b/worker/notmuch/worker.go
index ddbdfc02..98c20f2c 100644
--- a/worker/notmuch/worker.go
+++ b/worker/notmuch/worker.go
@@ -53,6 +53,7 @@ type worker struct {
	headers             []string
	headersExclude      []string
	state               uint64
	mfs                 types.MultiFileStrategy
}

// NewWorker creates a new notmuch worker with the provided worker.
@@ -240,6 +241,16 @@ func (w *worker) handleConfigure(msg *types.Configure) error {
	w.headers = msg.Config.Headers
	w.headersExclude = msg.Config.HeadersExclude

	mfs := msg.Config.Params["multi-file-strategy"]
	if mfs != "" {
		w.mfs, ok = types.StrToStrategy[mfs]
		if !ok {
			return fmt.Errorf("invalid multi-file strategy %s", mfs)
		}
	} else {
		w.mfs = types.Refuse
	}

	return nil
}

@@ -733,17 +744,12 @@ func (w *worker) handleDeleteMessages(msg *types.DeleteMessages) error {

	var deleted []uint32

	// With notmuch, two identical files can be referenced under
	// the same index key, even if they exist in two different
	// folders. So in order to remove the message from the right
	// maildir folder we need to pass a hint to Remove() so it
	// can purge the right file.
	folders, _ := w.store.FolderMap()
	path, ok := folders[w.currentQueryName]
	if !ok {
		w.err(msg, fmt.Errorf("Can only delete file from a maildir folder"))
		w.done(msg)
		return nil
	curDir := folders[w.currentQueryName]

	mfs := w.mfs
	if msg.MultiFileStrategy != nil {
		mfs = *msg.MultiFileStrategy
	}

	for _, uid := range msg.Uids {
@@ -753,7 +759,7 @@ func (w *worker) handleDeleteMessages(msg *types.DeleteMessages) error {
			w.err(msg, err)
			continue
		}
		if err := m.Remove(path); err != nil {
		if err := m.Remove(curDir, mfs); err != nil {
			w.w.Errorf("could not remove message: %v", err)
			w.err(msg, err)
			continue
@@ -782,13 +788,20 @@ func (w *worker) handleCopyMessages(msg *types.CopyMessages) error {
		return fmt.Errorf("Can only copy file to a maildir folder")
	}

	curDir := folders[w.currentQueryName]

	mfs := w.mfs
	if msg.MultiFileStrategy != nil {
		mfs = *msg.MultiFileStrategy
	}

	for _, uid := range msg.Uids {
		m, err := w.msgFromUid(uid)
		if err != nil {
			w.w.Errorf("could not get message: %v", err)
			return err
		}
		if err := m.Copy(dest); err != nil {
		if err := m.Copy(curDir, dest, mfs); err != nil {
			w.w.Errorf("could not copy message: %v", err)
			return err
		}
@@ -817,6 +830,13 @@ func (w *worker) handleMoveMessages(msg *types.MoveMessages) error {
		return fmt.Errorf("Can only move file to a maildir folder")
	}

	curDir := folders[w.currentQueryName]

	mfs := w.mfs
	if msg.MultiFileStrategy != nil {
		mfs = *msg.MultiFileStrategy
	}

	var err error
	for _, uid := range msg.Uids {
		m, err := w.msgFromUid(uid)
@@ -824,22 +844,8 @@ func (w *worker) handleMoveMessages(msg *types.MoveMessages) error {
			w.w.Errorf("could not get message: %v", err)
			break
		}
		filenames, err := m.db.MsgFilenames(m.key)
		if err != nil {
			return err
		}
		// In the future, it'd be nice if we could overload move with
		// the possibility to affect some or all of the files
		// corresponding to a message.
		if len(filenames) > 1 {
			return fmt.Errorf("Cannot move: message %d has multiple files", m.uid)
		}
		source, key := parseFilename(filenames[0])
		if key == "" {
			return fmt.Errorf("failed to parse message filename: %s", filenames[0])
		}
		if err := m.Move(source, dest); err != nil {
			w.w.Errorf("could not copy message: %v", err)
		if err := m.Move(curDir, dest, mfs); err != nil {
			w.w.Errorf("could not move message: %v", err)
			break
		}
		moved = append(moved, uid)
diff --git a/worker/types/messages.go b/worker/types/messages.go
index 35310b98..b05e4150 100644
--- a/worker/types/messages.go
+++ b/worker/types/messages.go
@@ -166,7 +166,8 @@ type FetchMessageFlags struct {

type DeleteMessages struct {
	Message
	Uids []uint32
	Uids              []uint32
	MultiFileStrategy *MultiFileStrategy
}

// Flag messages with different mail types
@@ -185,14 +186,16 @@ type AnsweredMessages struct {

type CopyMessages struct {
	Message
	Destination string
	Uids        []uint32
	Destination       string
	Uids              []uint32
	MultiFileStrategy *MultiFileStrategy
}

type MoveMessages struct {
	Message
	Destination string
	Uids        []uint32
	Destination       string
	Uids              []uint32
	MultiFileStrategy *MultiFileStrategy
}

type AppendMessage struct {
diff --git a/worker/types/mfs.go b/worker/types/mfs.go
new file mode 100644
index 00000000..071eda1d
--- /dev/null
+++ b/worker/types/mfs.go
@@ -0,0 +1,33 @@
package types

// MultiFileStrategy represents a strategy for taking file-based actions (e.g.,
// move, copy, delete) on messages that are represented by more than one file.
// These strategies are only used by the notmuch backend but are defined in this
// package to prevent import cycles.
type MultiFileStrategy uint

const (
	Refuse MultiFileStrategy = iota
	ActAll
	ActOne
	ActOneDelRest
	ActDir
	ActDirDelRest
)

var StrToStrategy = map[string]MultiFileStrategy{
	"refuse":              Refuse,
	"act-all":             ActAll,
	"act-one":             ActOne,
	"act-one-delete-rest": ActOneDelRest,
	"act-dir":             ActDir,
	"act-dir-delete-rest": ActDirDelRest,
}

func StrategyStrs() []string {
	strs := make([]string, len(StrToStrategy))
	for s := range StrToStrategy {
		strs = append(strs, s)
	}
	return strs
}
-- 
2.43.1

[aerc/patches] build success

builds.sr.ht <builds@sr.ht>
Details
Message ID
<CZ3B805QYALS.1EVDYY00OLPD7@fra01>
In-Reply-To
<20240212182229.109865-8-me@jasoncarloscox.com> (view parent)
DKIM signature
missing
Download raw message
aerc/patches: SUCCESS in 2m17s

[enable multi-file notmuch operations][0] v2 from [Jason Cox][1]

[0]: https://lists.sr.ht/~rjarry/aerc-devel/patches/49414
[1]: me@jasoncarloscox.com

✓ #1148293 SUCCESS aerc/patches/openbsd.yml     https://builds.sr.ht/~rjarry/job/1148293
✓ #1148292 SUCCESS aerc/patches/alpine-edge.yml https://builds.sr.ht/~rjarry/job/1148292

Re: [PATCH aerc v2 3/3] notmuch: add strategies for multi-file messages

Details
Message ID
<CZCPCD9ABDKO.2W9ZWSPDIU5OW@sindominio.net>
In-Reply-To
<20240212182229.109865-8-me@jasoncarloscox.com> (view parent)
DKIM signature
pass
Download raw message
Hi, Jason,

This patch is really nice. I've been wishing for it for a long time.
I did some testing (although not exhaustive: 4 commands and 6 strategies
is a lot). 

I'm on a notmuch+maildir setup. I usually check the number of files by adding
a msglist column like `column-copies = {{len .Filenames}}` which is deceiving
because it does not always updates upon changes (that sucks, actually, more on
that below).

Some comments follow:

First, I noticed that the competion pop-up for the strategy names (that shows once you write `-m `) is taller than it should: it seems that there are 12 lines instead of 6, being the first 6 lines blank.

On 12/02/2024, 19:21, Jason Cox wrote:
>  	N.B.: aerc will still always show messages and not files (under
>  	notmuch, a single message can be represented by several files),
Minor linguistic nitpick: maybe it's easier for the reader if we agree upon
a single way to say this: so far I've seen "a single message is represented by
several files", "several files are represented by a message" and "a message is
backed by several files". I have a slight preference to the second option.

> +*multi-file-stategy* = _<strategy>_
> +	Strategy for file operations (e.g., move, copy, delete) on messages
> that are
> +	backed by multiple files. Possible values:
> +
> +	- *refuse* (default): Refuse to act.

Right now, if I `:delete -m refuse` a multi-file message, I get "1 message
deleted", although apparently no change happens.

Similarly, if I `:move -m refuse Folder` or `:copy -m refuse Folder` I get "1
message {moved|copied} to Folder" and the message becomes dimmed but never
disappears. Same thing happens with `:archive -m refuse flat` too. Upon
restart, messages deleted/moved/archived are still there.

Maybe we should refuse in a more explicit way. 

Also, while we're at it, maybe we could have a more informative message along the lines of "1 message (2 files) moved to Folder" when the -m option is specified.

> +	- *act-all*: Act on all files.

Tested :delete, :move, :copy, :archive, they work allright. 

> +	- *act-dir*: Act on all files within the current folder and ignore the rest.
> +	Note that this strategy only works within the maildir directories; in other
> +	directories, it behaves like *refuse*.

Tested :delete, no problem. What do you mean by "other directories"?

> +	- *act-one-delete-rest*: Like *act-one*, but delete the remaining files.

Tested :move, it worked.

> +	- *act-one*: Act on one of the files, arbitrarily chosen, and ignore the
> +	rest.

Tested :copy, :move and I couldn't make it work. It seems to do its thing but
afaics the message is not copied/moved. The counters are not updated either
after a restart. :( Tested :archive and it moved all of the files to the
Archive folder!

After some testing with this strategy, I'm coming to the conclusion that
I can't trust aerc's message listing after these operations: it's like notmuch
needed an update (`notmuch new --no-hooks` in cli doesn't help either). Restarting aerc usually works. 

Unfortunately, I must give up now, I'll try again sometime soon(ish).

> +	- *act-dir-delete-rest*: Like *act-dir*, but delete the remaining files.

Haven't tried.

Re: [PATCH aerc v2 3/3] notmuch: add strategies for multi-file messages

Details
Message ID
<CZCQIMFR3JDW.TIGAXAQPCCN5@jasoncarloscox.com>
In-Reply-To
<CZCPCD9ABDKO.2W9ZWSPDIU5OW@sindominio.net> (view parent)
DKIM signature
pass
Download raw message
On Fri Feb 23, 2024 at 2:23 PM EST, inwit wrote:
> This patch is really nice. I've been wishing for it for a long time.
> I did some testing (although not exhaustive: 4 commands and 6 strategies
> is a lot). 

Thank you!

> I'm on a notmuch+maildir setup. I usually check the number of files by adding
> a msglist column like `column-copies = {{len .Filenames}}` which is deceiving
> because it does not always updates upon changes (that sucks, actually, more on
> that below).

I noticed that problem as well -- annoying.

> First, I noticed that the competion pop-up for the strategy names (that shows once you write `-m `) is taller than it should: it seems that there are 12 lines instead of 6, being the first 6 lines blank.

Right you are. I'll fix that.

> On 12/02/2024, 19:21, Jason Cox wrote:
> >  	N.B.: aerc will still always show messages and not files (under
> >  	notmuch, a single message can be represented by several files),
> Minor linguistic nitpick: maybe it's easier for the reader if we agree upon
> a single way to say this: so far I've seen "a single message is represented by
> several files", "several files are represented by a message" and "a message is
> backed by several files". I have a slight preference to the second option.

Yeah, it's sort of a confusing topic. I'll see if I can clean it up
soon.

> > +*multi-file-stategy* = _<strategy>_
> > +	Strategy for file operations (e.g., move, copy, delete) on messages
> > that are
> > +	backed by multiple files. Possible values:
> > +
> > +	- *refuse* (default): Refuse to act.
>
> Right now, if I `:delete -m refuse` a multi-file message, I get "1 message
> deleted", although apparently no change happens.
>
> Similarly, if I `:move -m refuse Folder` or `:copy -m refuse Folder` I get "1
> message {moved|copied} to Folder" and the message becomes dimmed but never
> disappears. Same thing happens with `:archive -m refuse flat` too. Upon
> restart, messages deleted/moved/archived are still there.
>
> Maybe we should refuse in a more explicit way. 

These are basically pre-existing issues. The different commands all
handle failure differently, largely in confusing ways. I started trying
to fix it when I made these patches but eventually gave up. Maybe I
should give it another shot though since this multi-file stuff makes the
existing confusing behavior even more confusing.

> > +	- *act-dir*: Act on all files within the current folder and ignore the rest.
> > +	Note that this strategy only works within the maildir directories; in other
> > +	directories, it behaves like *refuse*.
>
> Tested :delete, no problem. What do you mean by "other directories"?

I'm referring to the virtual query directories, e.g. from the query map
or dynamically created with :cf. Maybe calling them folders would help
make that more clear, or I could just explain it more in the docs.

> > +	- *act-one*: Act on one of the files, arbitrarily chosen, and ignore the
> > +	rest.
>
> Tested :copy, :move and I couldn't make it work. It seems to do its thing but
> afaics the message is not copied/moved. The counters are not updated either
> after a restart. :( Tested :archive and it moved all of the files to the
> Archive folder!

Huh, I just tested it again with :copy and it worked fine.

> After some testing with this strategy, I'm coming to the conclusion that
> I can't trust aerc's message listing after these operations: it's like notmuch
> needed an update (`notmuch new --no-hooks` in cli doesn't help either). Restarting aerc usually works. 

Yeah, unfortunately some parts of the UI don't update when they should.
I'm fairly certain that the actual operations are all making the
required changes in the notmuch DB, but aerc doesn't always seem to
catch the updates. I took these as existing issues that I didn't spend
much time on, but maybe I should take a look as well. It would certainly
make testing better.

> Unfortunately, I must give up now, I'll try again sometime soon(ish).

Thanks again for testing!

Re: [PATCH aerc v2 3/3] notmuch: add strategies for multi-file messages

Details
Message ID
<CZEDPGO29RLM.3GHF2SVYO1IL2@sindominio.net>
In-Reply-To
<CZCQIMFR3JDW.TIGAXAQPCCN5@jasoncarloscox.com> (view parent)
DKIM signature
pass
Download raw message
On 23/02/2024, 21:18, Jason Cox wrote:
> > Maybe we should refuse in a more explicit way. 
>
> These are basically pre-existing issues. The different commands all
> handle failure differently, largely in confusing ways. I started trying
> to fix it when I made these patches but eventually gave up. Maybe I
> should give it another shot though since this multi-file stuff makes the
> existing confusing behavior even more confusing.

Yeah, that's what I expected, about the pre-existing issues. It'd be
nice to make the commands homogeneous.

> > Tested :delete, no problem. What do you mean by "other directories"?
>
> I'm referring to the virtual query directories, e.g. from the query map
> or dynamically created with :cf. Maybe calling them folders would help
> make that more clear, or I could just explain it more in the docs.

Understood. We need a common language here too. How about "folders" when
the query is equal to `path:dir` or `folder:dir`, "queries" for those
stored on the query map and "dynamic queries" for the rest?

> > > +	- *act-one*: Act on one of the files, arbitrarily chosen, and ignore the
> > > +	rest.
> >
> > Tested :copy, :move and I couldn't make it work. It seems to do its thing but
> > afaics the message is not copied/moved. The counters are not updated either
> > after a restart. :( Tested :archive and it moved all of the files to the
> > Archive folder!
>
> Huh, I just tested it again with :copy and it worked fine.

Indeed, with :copy it seems to work, sorry. However, with :move it
doesn't in my setup. I'm now checking through `--output=files` in the
cli.

> Yeah, unfortunately some parts of the UI don't update when they should.
> I'm fairly certain that the actual operations are all making the
> required changes in the notmuch DB, but aerc doesn't always seem to
> catch the updates. I took these as existing issues that I didn't spend
> much time on, but maybe I should take a look as well. It would certainly
> make testing better.

Since my recent test with :move --act-one fails to show on notmuch cli
too, I'd take this hypothesis with a pinch of salt. I mean, it surely is
happening something like that, but it seems it's not the only thing
happening.

All in all, I wonder if this patch is just too big and it'd be easier to
test if it was split? I'm absolutely not a coder, so this is just a shot
in the dark.

Re: [PATCH aerc v2 3/3] notmuch: add strategies for multi-file messages

Details
Message ID
<CZI3K65LWI35.3RJWEUXO0A6HY@jasoncarloscox.com>
In-Reply-To
<CZEDPGO29RLM.3GHF2SVYO1IL2@sindominio.net> (view parent)
DKIM signature
pass
Download raw message
On Sun Feb 25, 2024 at 1:41 PM EST, inwit wrote:
> All in all, I wonder if this patch is just too big and it'd be easier to
> test if it was split? I'm absolutely not a coder, so this is just a shot
> in the dark.

I don't think it's too big, but I do think that some of the pre-existing
issues probably need to be ironed out first. Testing these patches is
tricky when the result messages and UI updates can't be trusted.

I'm not sure when/if I'll get around to those pre-existing issues. I'm
currently using a comfy tag-based workflow, so I rarely end up using
this multi-file stuff anyway. If anyone else wants to pick up on it in
the meantime, I'd be happy to help out where needed, though.
Reply to thread Export thread (mbox)