Add ConversionError field to Composer.Part to track multipart conversion
errors.
Check for conversion errors in :send, block sending if the errors are
found.
Signed-off-by: Vitaly Ovchinnikov <v@ovch.ru>
---
The patch helps with the configurations like this:
binds.conf:
[compose::review]
y = :multipart text/html<Enter>:send<Enter>
aerc.conf:
[multipart-converters]
text/html = /path/to/some/script.sh
/path/to/some/script.sh:
#!/bin/sh
exit 10 # falls for some reason
Without the patch, when you press `y` aerc runs `:multipart` command and
although it gets an error from the converter script, the error is
ignored and then the `:send` command actually sends a broken message.
The patch stores conversion errors for the message parts and checks for
them in `:send`. If the error is found, the sending is blocked and the
user gets an error message.
There is no way to skip this like missing attachment or empty subject.
This is done intentionally, as this is a problem of different level to
me. The user needs to update or delete the problem part before actually
sending a message.
app/compose.go | 29 ++++++++++++++++++++++++++---commands/compose/send.go | 5 +++++lib/attachment.go | 7 ++++---
3 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/app/compose.go b/app/compose.go
index 469f3c36..eae77e65 100644
--- a/app/compose.go+++ b/app/compose.go
@@ -1012,6 +1012,22 @@ func (c *Composer) ShouldWarnSubject() bool {
return len(subject) == 0
}
+func (c *Composer) CheckForMultipartErrors() error {+ problems := []string{}+ for _, p := range c.textParts {+ if p.ConversionError != nil {+ text := fmt.Sprintf("%s: %s", p.MimeType, p.ConversionError.Error())+ problems = append(problems, text)+ }+ }++ if len(problems) == 0 {+ return nil+ }++ return fmt.Errorf("multipart conversion error: %s", strings.Join(problems, "; "))+}+func writeMsgImpl(c *Composer, header *mail.Header, writer io.Writer) error {
mimeParams := map[string]string{"Charset": "UTF-8"}
if config.Compose.FormatFlowed {
@@ -1761,16 +1777,23 @@ func newReviewMessage(composer *Composer, err error) *reviewMessage {
}
func (c *Composer) updateMultipart(p *lib.Part) error {
+ // conversion errors handling+ p.ConversionError = nil+ setError := func(e error) error {+ p.ConversionError = e+ return e+ }+ command, found := config.Converters[p.MimeType]
if !found {
// unreachable
- return fmt.Errorf("no command defined for mime/type")+ return setError(fmt.Errorf("no command defined for mime/type")) }
// reset part body to avoid it leaving outdated if the command fails
p.Data = nil
body, err := c.GetBody()
if err != nil {
- return errors.Wrap(err, "GetBody")+ return setError(errors.Wrap(err, "GetBody")) }
cmd := exec.Command("sh", "-c", command)
cmd.Stdin = body
@@ -1786,7 +1809,7 @@ func (c *Composer) updateMultipart(p *lib.Part) error {
stderr = fmt.Sprintf(": %.30s", stderr)
}
}
- return fmt.Errorf("%s: %w%s", command, err, stderr)+ return setError(fmt.Errorf("%s: %w%s", command, err, stderr)) }
p.Data = out
return nil
diff --git a/commands/compose/send.go b/commands/compose/send.go
index 2e7590a0..fd39b48c 100644
--- a/commands/compose/send.go+++ b/commands/compose/send.go
@@ -64,6 +64,11 @@ func (s Send) Execute(args []string) error {
}
composer, _ := tab.Content.(*app.Composer)
+ err := composer.CheckForMultipartErrors()+ if err != nil {+ return err+ }+ config := composer.Config()
if s.CopyTo == "" {
diff --git a/lib/attachment.go b/lib/attachment.go
index aa3cd108..6f29da55 100644
--- a/lib/attachment.go+++ b/lib/attachment.go
@@ -16,9 +16,10 @@ import (
)
type Part struct {
- MimeType string- Params map[string]string- Data []byte+ MimeType string+ Params map[string]string+ Data []byte+ ConversionError error}
func NewPart(mimetype string, params map[string]string, body io.Reader,
--
2.43.0
Vitaly Ovchinnikov <v@ovch.ru> wrote:
> Add ConversionError field to Composer.Part to track multipart conversion> errors.>> Check for conversion errors in :send, block sending if the errors are> found.>> Signed-off-by: Vitaly Ovchinnikov <v@ovch.ru>> --->> The patch helps with the configurations like this:>> binds.conf:> [compose::review]> y = :multipart text/html<Enter>:send<Enter>>> aerc.conf:> [multipart-converters]> text/html = /path/to/some/script.sh>> /path/to/some/script.sh:> #!/bin/sh> exit 10 # falls for some reason>> Without the patch, when you press `y` aerc runs `:multipart` command and> although it gets an error from the converter script, the error is> ignored and then the `:send` command actually sends a broken message.>> The patch stores conversion errors for the message parts and checks for> them in `:send`. If the error is found, the sending is blocked and the> user gets an error message.>> There is no way to skip this like missing attachment or empty subject.> This is done intentionally, as this is a problem of different level to> me. The user needs to update or delete the problem part before actually> sending a message.
Acked-by: Robin Jarry <robin@jarry.cc>
Applied, thanks.
To git@git.sr.ht:~rjarry/aerc
4cbd116f0348..65332c233880 master -> master
Unfortunately, this patch breaks my workflow: I used to :accept calendar invitations and, in the review screen, I used to get an error in red saying "text/calendar error: no command defined for mime/type", but still I was able to send the message (and thus accept the invitation).
After this patch, I can't :accept invitations anymore. :(
On Sat 13 Apr 2024 20:15:54, inwit wrote:
> Unfortunately, this patch breaks my workflow: I used to :accept calendar invitations and, in the review screen, I used to get an error in red saying "text/calendar error: no command defined for mime/type", but still I was able to send the message (and thus accept the invitation).>> After this patch, I can't :accept invitations anymore. :(
A send-now option could probably solve that :)
--
Moritz Poldrack
https://moritz.sh> List was current at time of printing.
> After this patch, I can't :accept invitations anymore. :(
Could you send me such an invitation privately along with some
instructions on how to reproduce the problem? I never used :accept, so
not sure what to do.
Overall, we could probably make this behavior optional this or that way,
yet I am not sure if this is the right thing... I'd personally wait for
Robin's decision here.
-- Vitaly
On Sat Apr 13, 2024 at 20:51, Vitaly Ovchinnikov <v@ovch.ru> wrote:
> > After this patch, I can't :accept invitations anymore. :(>> Could you send me such an invitation privately along with some> instructions on how to reproduce the problem? I never used :accept, so> not sure what to do.>> Overall, we could probably make this behavior optional this or that way,> yet I am not sure if this is the right thing... I'd personally wait for> Robin's decision here.>> -- Vitaly
I think this is an earlier bug, that your patch just highlighted.
--
+36305425054
bence.ferdinandy.com
Bence Ferdinandy, Apr 13, 2024 at 20:55:
>> On Sat Apr 13, 2024 at 20:51, Vitaly Ovchinnikov <v@ovch.ru> wrote:> > > After this patch, I can't :accept invitations anymore. :(> >> > Could you send me such an invitation privately along with some> > instructions on how to reproduce the problem? I never used :accept, so> > not sure what to do.> >> > Overall, we could probably make this behavior optional this or that way,> > yet I am not sure if this is the right thing... I'd personally wait for> > Robin's decision here.> >> > -- Vitaly>> I think this is an earlier bug, that your patch just highlighted.
Yes, I agree with this. I also saw that issue with :accept already. I'd
say that it has been like that for a long time.
The fix should be pretty simple.
On Sat Apr 13, 2024 at 21:00, Robin Jarry <robin@jarry.cc> wrote:
> Bence Ferdinandy, Apr 13, 2024 at 20:55:> >> > On Sat Apr 13, 2024 at 20:51, Vitaly Ovchinnikov <v@ovch.ru> wrote:> > > > After this patch, I can't :accept invitations anymore. :(> > >> > > Could you send me such an invitation privately along with some> > > instructions on how to reproduce the problem? I never used :accept, so> > > not sure what to do.> > >> > > Overall, we could probably make this behavior optional this or that way,> > > yet I am not sure if this is the right thing... I'd personally wait for> > > Robin's decision here.> > >> > > -- Vitaly> >> > I think this is an earlier bug, that your patch just highlighted.>> Yes, I agree with this. I also saw that issue with :accept already. I'd > say that it has been like that for a long time.>> The fix should be pretty simple.
The culprit is here:
https://git.sr.ht/~rjarry/aerc/tree/65332c233880c9609ea39af22ee677e98f4cba1b/item/app/compose.go?view-source#L1779
updateMultipart sets a conversion error on text/calendar
Vitaly's patch is just looking at this error already set.
Not sure why the error is set though.
--
+36305425054
bence.ferdinandy.com
On Sat Apr 13, 2024 at 20:20, Moritz Poldrack <moritz@poldrack.dev> wrote:
> On Sat 13 Apr 2024 20:15:54, inwit wrote:> > Unfortunately, this patch breaks my workflow: I used to :accept calendar invitations and, in the review screen, I used to get an error in red saying "text/calendar error: no command defined for mime/type", but still I was able to send the message (and thus accept the invitation).> >> > After this patch, I can't :accept invitations anymore. :(>> A send-now option could probably solve that :)
I tried doing a quick and dirty but got into an import cycle when trying to
just call :send from code :(
--
+36305425054
bence.ferdinandy.com