diff options
Diffstat (limited to 'assets/hooks')
| -rwxr-xr-x | assets/hooks/post-new | 125 | ||||
| -rwxr-xr-x | assets/hooks/test_post_new.py | 92 |
2 files changed, 201 insertions, 16 deletions
diff --git a/assets/hooks/post-new b/assets/hooks/post-new index 428ec29..fca31a1 100755 --- a/assets/hooks/post-new +++ b/assets/hooks/post-new @@ -73,6 +73,109 @@ def log(message): print(f"post-new: {message}", file=sys.stderr) +def _quote(value): + """Escape a Message-ID for a double-quoted notmuch term.""" + return '"' + value.replace("\\", "\\\\").replace('"', '\\"') + '"' + + +def sent_only(query, folders): + """The ids matching `query` whose files are ALL inside `folders`. + + None on failure, so the caller leaves `tag:new` in place rather than + stripping on a half-read answer. + + **Every file has to be in a sent folder, not merely one of them.** notmuch + deduplicates by Message-ID, so mail the user sends to another of their own + accounts is ONE message with two files: the sender's Sent copy and the + recipient's Inbox copy. The old query matched on a path and the tag applied + to the message, so matching the Sent copy stripped `inbox` from the copy + that had genuinely arrived, and the mail was missing from the account that + received it (item 166). + + This cannot be expressed as a query, which is why it is a loop. Measured + against a two-file message: `not path:"Inbox/**"` does NOT exclude it, and + `count --output=files` on a path query reports every file of every matching + message rather than the files that matched. Both read as if they worked and + are wrong for the same reason, that a notmuch term is a predicate over a + MESSAGE and the distinction being drawn here is between its FILES. + """ + root = mail_root() + if root is None: + log("no mail root; leaving sent mail alone") + return None + + prefixes = [root / folder for folder in folders] + + matched = search(query, "messages") + if matched is None: + return None + + # ponytail: one `notmuch search` per matched message. N is the sent mail + # in tag:new, so an ordinary sync is a handful and a first-run reindex is + # the whole corpus. Batch by parsing --format=json once if that ever + # matters; it does not at this size, and the loop is the readable form. + ids = [] + for message_id in matched: + paths = search(f"id:{_quote(message_id)}", "files") + if paths is None: + return None + if all(any(_within(Path(path), prefix) for prefix in prefixes) + for path in paths): + ids.append(message_id) + return ids + + +def _within(path, prefix): + """Whether `path` is inside `prefix`, compared as paths. + + Not `startswith`: `<root>/Sent-old/cur/1` starts with `<root>/Sent` and is + a different folder. Same trap as the attachment-save path check in the + application. + """ + try: + path.relative_to(prefix) + except ValueError: + return False + return True + + +def mail_root(): + """The Maildir root, or None. + + `database.mail_root`, not `database.path`: notmuch can hold the Xapian + index somewhere else entirely, and this user's does. Under that layout + `database.path` is the INDEX directory and no message file is inside it. + """ + result = subprocess.run(["notmuch", "config", "get", "database.mail_root"], + capture_output=True, text=True) + if result.returncode != 0: + return None + value = result.stdout.strip() + return Path(value) if value else None + + +def search(query, output): + """`notmuch search --output=<output>` as a list, or None on failure. + + An id comes back bare here, without the `id:` prefix, because + `--output=messages` prints `id:<value>` and the prefix is stripped. + """ + result = subprocess.run( + ["notmuch", "search", f"--output={output}", "--", query], + capture_output=True, text=True) + if result.returncode != 0: + log(f"notmuch search failed: {result.stderr.strip()}") + return None + + values = [] + for line in result.stdout.splitlines(): + line = line.strip() + if not line: + continue + values.append(line[3:] if line.startswith("id:") else line) + return values + + def strip_inbox_from_sent(run): """Take `inbox` off mail the user SENT, and nothing else. @@ -105,20 +208,20 @@ def strip_inbox_from_sent(run): query = f"{SCOPE} and ({qtmaildirconf.sent_query(folders)})" - # Counted BEFORE the tag, because the tag is what makes the count zero. - # A `notmuch tag` that matches nothing SUCCEEDS, so the old log line said - # "applied" whether it stripped four messages or none, and item 164 is - # exactly the case where that distinction is the whole question: a draft - # kept `inbox` on a pass whose log claimed the carve-out had run. The - # count is the only thing that separates "the tag ran and something - # re-added inbox afterwards" from "the message was never in scope". - matched = count(query) - - if not run(["-inbox"], query): + ids = sent_only(query, folders) + if ids is None: return False + for message_id in ids: + if not run(["-inbox"], f"id:{_quote(message_id)}"): + return False + + # A `notmuch tag` matching nothing SUCCEEDS, so the old log line said + # "applied" whether it stripped four messages or none, and item 164 is + # exactly the case where that distinction is the whole question: a draft + # that kept `inbox` on a pass whose log claimed the carve-out had run. log(f"sent-folder carve-out applied over {len(folders)} folder(s), " - f"{matched} message(s)") + f"{len(ids)} message(s)") return True diff --git a/assets/hooks/test_post_new.py b/assets/hooks/test_post_new.py index a0228aa..07728a6 100755 --- a/assets/hooks/test_post_new.py +++ b/assets/hooks/test_post_new.py @@ -38,6 +38,16 @@ from pathlib import Path HOOK = Path(__file__).resolve().parent / "post-new" +def files(env, query): + """How many FILES notmuch holds for the messages matching a query, which + is not the message count: a message sent to another of the user's own + accounts is one message with two files.""" + result = subprocess.run( + ["notmuch", "search", "--output=files", "--", query], + env=env, capture_output=True, text=True, check=True) + return len([line for line in result.stdout.splitlines() if line]) + + def make_message(maildir, name, sender, subject): path = maildir / "new" / name path.write_text( @@ -202,7 +212,7 @@ def test_a_protected_removal_is_skipped_whole_and_the_run_continues(): assert count(env, "tag:new") == 0 -def setup_accounts(tmp, sent_config=True): +def setup_accounts(tmp, sent_config=True, split_index=False): """A maildir laid out as qtmaildir configures it: two accounts, each with an Inbox and a Sent folder, one message in each. @@ -232,10 +242,23 @@ def setup_accounts(tmp, sent_config=True): "you@example.org", "something else sent") config = Path(tmp) / "notmuch-config" - config.write_text( - f"[database]\npath={root}\n\n" - f"[new]\ntags=new;unread;inbox\n\n" - f"[user]\nname=Test\nprimary_email=you@example.org\n") + if split_index: + # The index somewhere else entirely, which is how the developer's own + # machine runs: `database.path` is then the INDEX directory and no + # message file is inside it. Anything reading that key as the mail + # root resolves every path wrongly, and the ordinary layout above + # cannot show it because both keys return the same string. + index = Path(tmp) / "index" + index.mkdir() + config.write_text( + f"[database]\npath={index}\nmail_root={root}\n\n" + f"[new]\ntags=new;unread;inbox\n\n" + f"[user]\nname=Test\nprimary_email=you@example.org\n") + else: + config.write_text( + f"[database]\npath={root}\n\n" + f"[new]\ntags=new;unread;inbox\n\n" + f"[user]\nname=Test\nprimary_email=you@example.org\n") env = dict(os.environ) env["NOTMUCH_CONFIG"] = str(config) @@ -284,6 +307,65 @@ def test_sent_mail_does_not_keep_the_inbox_tag(): assert count(env, 'tag:inbox and path:"acct-two/Inbox/**"') == 1 +def test_a_message_that_was_sent_AND_received_keeps_inbox(): + """Item 166. notmuch deduplicates by Message-ID, so mail the user sends + to their own other account is ONE message with TWO files: the sender's + Sent copy and the recipient's Inbox copy. + + The carve-out matches on a file's path but tags the MESSAGE, so matching + the Sent copy stripped `inbox` from the Inbox copy as well and the mail + vanished from the account that genuinely received it. The predicate has + to hold for every file, not for any file. + + The identical `name` is what makes this one message: make_message() + derives the Message-Id from it. + """ + with tempfile.TemporaryDirectory() as tmp: + env = setup_accounts(tmp) + root = Path(tmp) / "Mail" + make_message(root / "acct-one/Sent", "self-addressed", + "you@example.org", "to my other account") + make_message(root / "acct-two/Inbox", "self-addressed", + "you@example.org", "to my other account") + subprocess.run(["notmuch", "new"], env=env, capture_output=True, + check=True) + # One message, two files: the premise of the whole defect. + assert count(env, "id:self-addressed@example.org") == 1 + assert files(env, "id:self-addressed@example.org") == 2 + + write_rules(env, []) + result = subprocess.run([str(HOOK)], env=env, capture_output=True, + text=True) + assert result.returncode == 0, result.stderr + + # It arrived, so it keeps `inbox`... + assert count(env, "tag:inbox and id:self-addressed@example.org") == 1 + # ...and the ordinary sent message, whose only file is in a sent + # folder, still loses it. Without this half the fix could simply be + # "never strip anything". + assert count(env, "tag:inbox and id:sent-one@example.org") == 0 + + +def test_the_carve_out_works_with_the_index_split_from_the_mail(): + """notmuch can hold the Xapian index outside the Maildir, and this user's + does. `database.path` is then the index directory, so a carve-out reading + that key as the mail root builds prefixes no message file is under, finds + every file "outside" the sent folders, and silently stops stripping + anything. + + The ordinary fixture cannot catch it: with the index inside the mail root + both keys return the same string and either one passes. + """ + with tempfile.TemporaryDirectory() as tmp: + env = setup_accounts(tmp, split_index=True) + write_rules(env, []) + result = subprocess.run([str(HOOK)], env=env, capture_output=True, + text=True) + assert result.returncode == 0, result.stderr + assert count(env, 'tag:inbox and path:"acct-one/Sent/**"') == 0 + assert count(env, 'tag:inbox and path:"acct-one/Inbox/**"') == 1 + + def test_sent_mail_keeps_every_other_tag(): """Only `inbox` is stripped. `unread` in particular must survive: maildir.synchronize_flags is true, so removing it rewrites Maildir |