notmuch.el, needs without-restriction to properly save draft

notmuch.el, needs without-restriction to properly save draft

[Marc Fargas]

Hi there,

I'm not sure if this is the appropiate mailing list for bugs regarding
notmuch.el (Notmuch in Emacs). Also, I'm not subscribed so please keep
me in CC!

I recently noticed that when saving drafts in notmuch I would loose all
the headers that are not normally visible on the compose
buffer. References, In-Reply-To and the likes.

By default, when when composing a Reply all those cool and nice headers
are narrowed out of the buffer. They show up on sending because
"message-send" will sort the headers breaking the restriction.

After digging, it seems that "(insert-buffer-substring buf)", in
"with-temporary-notmuch-message-buffer" which is used by
"notmuch-draft-save" is not seeing the buffer contents beyond the
buffer's current restriction.

Thus, "insert-buffer-substring buf" should be wrapped inside a
"without-restriction".

Doing this makes the saved draft keep all headers:

  (defun unrestrict-notmuch (orig-fn &rest args)
    (without-restriction
      (apply orig-fn args)))

  (advice-add #'notmuch-draft-postpone :around #'unrestrict-notmuch)

Note that when resuming the draft later on all headers will be visible,
still working out how to restore the narrow restriction there.

marc

Re: notmuch.el, needs without-restriction to properly save draft

[Carl Worth]

On Wed, Mar 13 2024, Marc Fargas wrote:
> Hi there,

Hi, Marc!

> I'm not sure if this is the appropiate mailing list for bugs regarding
> notmuch.el (Notmuch in Emacs). Also, I'm not subscribed so please keep
> me in CC!

Yes, this is a fine place to report things

> I recently noticed that when saving drafts in notmuch I would loose all
> the headers that are not normally visible on the compose
> buffer. References, In-Reply-To and the likes.

Thanks for the bug report, as well as the fix you provided. I haven't
run it, but it looks totally reasonable.

> Note that when resuming the draft later on all headers will be visible,
> still working out how to restore the narrow restriction there.

That seems worth chasing down, but shouldn't prevent the acceptance of
your fix here which seems like only progress in the right direction.

Could you put your fix together in the form of a git-appliable patch?
Such as by applying it to the notmuch source, running `git commit` and
then `git format-patch HEAD~` or similar.

Thanks again,

-Carl

[PATCH] Use `without-restriction` in `with-temporary-notmuch-message-buffer`

[Marc Fargas]

This ensures that the temporary copy of the current message-mode
buffer is whole and not limited by a current restriction.

An example of such restriction is the default one established by
message-mode when composing a reply, that hides the References,
In-Reply-To and similar headers.
---
 emacs/notmuch-maildir-fcc.el | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch-maildir-fcc.el b/emacs/notmuch-maildir-fcc.el
index 5102078..fdced98 100644
--- a/emacs/notmuch-maildir-fcc.el
+++ b/emacs/notmuch-maildir-fcc.el
@@ -148,11 +148,12 @@ Otherwise set it according to `notmuch-fcc-dirs'."
   `(let ((case-fold-search t)
 	 (buf (current-buffer))
 	 (mml-externalize-attachments message-fcc-externalize-attachments))
-     (with-current-buffer (get-buffer-create " *message temp*")
-       (message-clone-locals buf) ;; for message-encoded-mail-cache
-       (erase-buffer)
-       (insert-buffer-substring buf)
-       ,@body)))
+     (without-restriction
+       (with-current-buffer (get-buffer-create " *message temp*")
+	(message-clone-locals buf) ;; for message-encoded-mail-cache
+	(erase-buffer)
+	(insert-buffer-substring buf)
+	,@body))))
 
 (defun notmuch-maildir-setup-message-for-saving ()
   "Setup message for saving.
-- 
2.43.2

Re: [PATCH] Use `without-restriction` in `with-temporary-notmuch-message-buffer`

[Marc Fargas]

Hi,

I hope my previous e-mails was properly sent.

I have been testing the patch and.. does it work? The code looks proper
to me but testing on my computer it is not working properly, as if the
`without-restrictions` was being ignored.

I think I do not understand `defmacro` properly to see where the problem
is. Please someone test the patch just to make sure its not a local
problem of my setup.

(Just Reply to an email, postpone; see if the saved draft is kept in the
thread with its headers or becomes an orphan in drafts).

Thanks,
marc

El jue. 14 de mar. 2024, Marc escribió:
> This ensures that the temporary copy of the current message-mode
> buffer is whole and not limited by a current restriction.
>
> An example of such restriction is the default one established by
> message-mode when composing a reply, that hides the References,
> In-Reply-To and similar headers.\r

Re: [PATCH] Use `without-restriction` in `with-temporary-notmuch-message-buffer`

[David Bremner]

Marc Fargas <telenieko@telenieko.com> writes:


> I hope my previous e-mails was properly sent.
>

Yes, it worked fine.

> I have been testing the patch and.. does it work? The code looks proper
> to me but testing on my computer it is not working properly, as if the
> `without-restrictions` was being ignored.

It seems to work here _but_ I did not notice the problem with postponed
replies being orphaned before so maybe it is an issue of your local
setup? 

To test this idea you can try ./devel/try-emacs-mua -q

Equally likely is that I have not understood the problem yet.

>
> (Just Reply to an email, postpone; see if the saved draft is kept in the
> thread with its headers or becomes an orphan in drafts).
>

Although it is significantly more work, it would be very helpful if you
could come up with an automated test in the style of those in
T630-emacs-draft.sh. There is one that checks for an internal header, so
perhaps it could be modified to check for In-Reply-To?

David

Re: notmuch.el, needs without-restriction to properly save draft

[Marc Fargas]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

Hello,

El mié. 13 de mar. 2024, Carl escribió:
> (...)
> Could you put your fix together in the form of a git-appliable patch?
> Such as by applying it to the notmuch source, running `git commit` and
> then `git format-patch HEAD~` or similar.

Please disregard the previous patch, consider the one attached
here. Hope I did the "format-patch" thing properly.

I had to move the `without-restriction` call to the top of the
`with-temporary-notmuch-message-buffer` defmacro. It does work properly
now.

marc


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1799 bytes --]

From 48efd572e6b8e6d214fc8b4e7b28f001fc13345a Mon Sep 17 00:00:00 2001
From: Marc Fargas <telenieko@telenieko.com>
Date: Thu, 14 Mar 2024 15:56:49 +0100
Subject: [PATCH] Use `without-restriction` in
 `with-temporary-notmuch-message-buffer`

This ensures that the temporary copy of the current message-mode
buffer is whole and not limited by a current restriction.

An example of such restriction is the default one established by
message-mode when composing a reply, that hides the References,
In-Reply-To and similar headers.
---
 emacs/notmuch-maildir-fcc.el | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-maildir-fcc.el b/emacs/notmuch-maildir-fcc.el
index 5102078..cf50e85 100644
--- a/emacs/notmuch-maildir-fcc.el
+++ b/emacs/notmuch-maildir-fcc.el
@@ -145,14 +145,15 @@ Otherwise set it according to `notmuch-fcc-dirs'."
 
 (defmacro with-temporary-notmuch-message-buffer (&rest body)
   "Set-up a temporary copy of the current message-mode buffer."
-  `(let ((case-fold-search t)
-	 (buf (current-buffer))
-	 (mml-externalize-attachments message-fcc-externalize-attachments))
-     (with-current-buffer (get-buffer-create " *message temp*")
-       (message-clone-locals buf) ;; for message-encoded-mail-cache
-       (erase-buffer)
-       (insert-buffer-substring buf)
-       ,@body)))
+  `(without-restriction
+     (let ((case-fold-search t)
+	   (buf (current-buffer))
+	   (mml-externalize-attachments message-fcc-externalize-attachments))
+       (with-current-buffer (get-buffer-create " *message temp*")
+	 (message-clone-locals buf) ;; for message-encoded-mail-cache
+	 (erase-buffer)
+	 (insert-buffer-substring buf)
+	 ,@body))))
 
 (defun notmuch-maildir-setup-message-for-saving ()
   "Setup message for saving.
-- 
2.44.0


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: notmuch.el, needs without-restriction to properly save draft

[Marc Fargas]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

Hello,

El mié. 13 de mar. 2024, Carl escribió:
> (...)
> Could you put your fix together in the form of a git-appliable patch?
> Such as by applying it to the notmuch source, running `git commit` and
> then `git format-patch HEAD~` or similar.

Please disregard the previous patch, consider the one attached
here. Hope I did the "format-patch" thing properly.

I had to move the `without-restriction` call to the top of the
`with-temporary-notmuch-message-buffer` defmacro. It does work properly
now.

marc

PS: Resent while properly attaching the patch file (I hope, and
apologies).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-without-restriction-in-with-temporary-notmuch-me.patch --]
[-- Type: text/x-patch, Size: 1799 bytes --]

From 48efd572e6b8e6d214fc8b4e7b28f001fc13345a Mon Sep 17 00:00:00 2001
From: Marc Fargas <telenieko@telenieko.com>
Date: Thu, 14 Mar 2024 15:56:49 +0100
Subject: [PATCH] Use `without-restriction` in
 `with-temporary-notmuch-message-buffer`

This ensures that the temporary copy of the current message-mode
buffer is whole and not limited by a current restriction.

An example of such restriction is the default one established by
message-mode when composing a reply, that hides the References,
In-Reply-To and similar headers.
---
 emacs/notmuch-maildir-fcc.el | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-maildir-fcc.el b/emacs/notmuch-maildir-fcc.el
index 5102078..cf50e85 100644
--- a/emacs/notmuch-maildir-fcc.el
+++ b/emacs/notmuch-maildir-fcc.el
@@ -145,14 +145,15 @@ Otherwise set it according to `notmuch-fcc-dirs'."
 
 (defmacro with-temporary-notmuch-message-buffer (&rest body)
   "Set-up a temporary copy of the current message-mode buffer."
-  `(let ((case-fold-search t)
-	 (buf (current-buffer))
-	 (mml-externalize-attachments message-fcc-externalize-attachments))
-     (with-current-buffer (get-buffer-create " *message temp*")
-       (message-clone-locals buf) ;; for message-encoded-mail-cache
-       (erase-buffer)
-       (insert-buffer-substring buf)
-       ,@body)))
+  `(without-restriction
+     (let ((case-fold-search t)
+	   (buf (current-buffer))
+	   (mml-externalize-attachments message-fcc-externalize-attachments))
+       (with-current-buffer (get-buffer-create " *message temp*")
+	 (message-clone-locals buf) ;; for message-encoded-mail-cache
+	 (erase-buffer)
+	 (insert-buffer-substring buf)
+	 ,@body))))
 
 (defun notmuch-maildir-setup-message-for-saving ()
   "Setup message for saving.
-- 
2.44.0


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: notmuch.el, needs without-restriction to properly save draft

[Marc Fargas]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

Hi again,

El dom. 24 de mar. 2024, Marc escribió:
> El mié. 13 de mar. 2024, Carl escribió:
>> (...)
>> Could you put your fix together in the form of a git-appliable patch?
>> Such as by applying it to the notmuch source, running `git commit` and
>> then `git format-patch HEAD~` or similar.
>
> Please disregard the previous patch, consider the one attached
> here. Hope I did the "format-patch" thing properly.
>
> I had to move the `without-restriction` call to the top of the
> `with-temporary-notmuch-message-buffer` defmacro. It does work properly
> now.

I just saw on the archives that David Bremner replied to my original
patch, I have no idea why that email didn't reach me :(

David: 
> Although it is significantly more work, it would be very helpful if
> you could come up with an automated test in the style of those in
> T630-emacs-draft.sh. There is one that checks for an internal header,
> so perhaps it could be modified to check for In-Reply-To?

I am attaching a new patch that includes an additional test on
T630-emacs-draft.sh.

On the test only `References` is at the top (hence hidden by emacs), not
`In-Reply-To`. I guess that does rely on some configuration.

In any case, the test reproduces the problem (lost headers on draft
save), and the patch fixes it!

Hope all is good!! :D

marc


[-- Attachment #2: 0001-Use-without-restriction-in-with-temporary-notmuch-me.patch --]
[-- Type: text/x-patch, Size: 3435 bytes --]

From 77e8a775571458962ff11af549d1a0428a17435e Mon Sep 17 00:00:00 2001
From: Marc Fargas <telenieko@telenieko.com>
Date: Thu, 14 Mar 2024 15:56:49 +0100
Subject: [PATCH] Use `without-restriction` in
 `with-temporary-notmuch-message-buffer`

This ensures that the temporary copy of the current message-mode
buffer is whole and not limited by a current restriction.

An example of such restriction is the default one established by
message-mode when composing a reply, that hides the References,
In-Reply-To and similar headers.
---
 emacs/notmuch-maildir-fcc.el | 17 +++++++++--------
 test/T630-emacs-draft.sh     | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-maildir-fcc.el b/emacs/notmuch-maildir-fcc.el
index 5102078..cf50e85 100644
--- a/emacs/notmuch-maildir-fcc.el
+++ b/emacs/notmuch-maildir-fcc.el
@@ -145,14 +145,15 @@ Otherwise set it according to `notmuch-fcc-dirs'."
 
 (defmacro with-temporary-notmuch-message-buffer (&rest body)
   "Set-up a temporary copy of the current message-mode buffer."
-  `(let ((case-fold-search t)
-	 (buf (current-buffer))
-	 (mml-externalize-attachments message-fcc-externalize-attachments))
-     (with-current-buffer (get-buffer-create " *message temp*")
-       (message-clone-locals buf) ;; for message-encoded-mail-cache
-       (erase-buffer)
-       (insert-buffer-substring buf)
-       ,@body)))
+  `(without-restriction
+     (let ((case-fold-search t)
+	   (buf (current-buffer))
+	   (mml-externalize-attachments message-fcc-externalize-attachments))
+       (with-current-buffer (get-buffer-create " *message temp*")
+	 (message-clone-locals buf) ;; for message-encoded-mail-cache
+	 (erase-buffer)
+	 (insert-buffer-substring buf)
+	 ,@body))))
 
 (defun notmuch-maildir-setup-message-for-saving ()
   "Setup message for saving.
diff --git a/test/T630-emacs-draft.sh b/test/T630-emacs-draft.sh
index c443f41..1fad58a 100755
--- a/test/T630-emacs-draft.sh
+++ b/test/T630-emacs-draft.sh
@@ -71,4 +71,36 @@ Fcc: MAIL_DIR/sent
 <#secure method=pgpmime mode=sign>
 EOF
 test_expect_equal_file EXPECTED OUTPUT.clean
+
+add_email_corpus attachment
+test_begin_subtest "Saving a draft keeps hidden headers"
+test_emacs '(notmuch-mua-new-reply "id:874llc2bkp.fsf@curie.anarc.at")
+            (message-goto-subject)
+            (delete-line)
+            (insert "Subject: draft-test-reply\n")
+	    (test-output "DRAFT")
+	    (notmuch-draft-postpone)
+	    (notmuch-show "subject:draft-test-reply")
+	    (notmuch-show-resume-message)
+	    (test-output)'
+notmuch_dir_sanitize OUTPUT > OUTPUT.clean
+
+cat <<EOF > EXPECTED
+References: <87d10042pu.fsf@curie.anarc.at> <87woy8vx7i.fsf@tesseract.cs.unb.ca> <87a7v42bv9.fsf@curie.anarc.at> <874llc2bkp.fsf@curie.anarc.at>
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: Antoine Beaupré <anarcat@orangeseeds.org>
+Subject: draft-test-reply
+In-Reply-To: <874llc2bkp.fsf@curie.anarc.at>
+Fcc: MAIL_DIR/sent
+--text follows this line--
+Antoine Beaupré <anarcat@orangeseeds.org> writes:
+
+> And obviously I forget the frigging attachment.
+>
+>
+> PS: don't we have a "you forgot to actually attach the damn file" plugin
+> when we detect the word "attachment" and there's no attach? :p
+EOF
+test_expect_equal_file EXPECTED OUTPUT.clean
+
 test_done
-- 
2.44.0


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: notmuch.el, needs without-restriction to properly save draft

[Marc Fargas]

Hi all,

El dom. 24 de mar. 2024, Marc escribió:
> I am attaching a new patch that includes an additional test on
> T630-emacs-draft.sh.
>
> On the test only `References` is at the top (hence hidden by emacs), not
> `In-Reply-To`. I guess that does rely on some configuration.
>
> In any case, the test reproduces the problem (lost headers on draft
> save), and the patch fixes it!

Following up on this, has anyone had a chance to review the proposed
patch (the one including tests)?

Let me know if there's anything else I can do to get the fix into
notmuch!

Marc\r