[BUG] Patch order is not respected for sequence of patches like [PATCH X/Y]

[BUG] Patch order is not respected for sequence of patches like [PATCH X/Y]

[Ihor Radchenko]

Hi,

I just tried to use piem for patches in
https://orgmode.org/list/87v8s8n1bm.fsf@heagren.com

The patches are supposed to be applied in specific order 1/2 and then
2/2. However, the patches in the email are attached in the opposite
order.

piem-am on the above email disregards the patch order and simply applies
the patches as they appear in the email attachments, which is not what
the patch author intended.

I consider the current behavior a bug.

Best,
Ihor

[PATCH] piem-am: Order attached patches by file name prefix

[Kyle Meyer]

Ihor Radchenko writes:

> I just tried to use piem for patches in
> https://orgmode.org/list/87v8s8n1bm.fsf@heagren.com
>
> The patches are supposed to be applied in specific order 1/2 and then
> 2/2. However, the patches in the email are attached in the opposite
> order.
>
> piem-am on the above email disregards the patch order and simply applies
> the patches as they appear in the email attachments, which is not what
> the patch author intended.
>
> I consider the current behavior a bug.

Okay, though it's at least an intended bug :)  It is not particularly
user-facing, but piem is documented as taking the attachments in order
(although reading the wording now, it's probably a bit ambiguous).  The
sender attached the patches in the wrong order, forcing those reading it
and applying it manually to work from bottom to top.

Regardless of what you call it, I don't mind piem trying to be more
clever/helpful in these scenarios.  How about something like this?
(Tested on <87v8s8n1bm.fsf@heagren.com> from Notmuch and Gnus.)

-- >8 --
Subject: [PATCH] piem-am: Order attached patches by file name prefix

When extracting patches from attachments, piem-gnus-am-ready-mbox and
piem-notmuch-am-ready-mbox construct the mbox messages in the same
order as the attachments.  This depends on the sender attaching the
patches in ascending order.

Be a bit more helpful in situations where the sender attaches the
patches out of order by reordering patches according to the NNNN-
prefix that git-format-patch adds to the start of the patch file name.
This approach won't be able to reliably sort patches that aren't
generated by git-format-patch, but that's outside of any use case that
piem is intended to support.

Suggested-by: Ihor Radchenko <yantar92@gmail.com>
Link: https://inbox.kyleam.com/piem/87mtdj9dzt.fsf@localhost
---
 piem-gnus.el    | 13 ++++++++++---
 piem-notmuch.el | 15 +++++++++++----
 piem.el         | 26 +++++++++++++++-----------
 3 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/piem-gnus.el b/piem-gnus.el
index c62a2e6e..4011fa2d 100644
--- a/piem-gnus.el
+++ b/piem-gnus.el
@@ -91,17 +91,24 @@ (defun piem-gnus-mid-to-thread (mid)
 
 (defun piem-gnus-am-ready-mbox ()
   "Return a function that inserts an am-ready mbox.
+
 If the buffer has any MIME parts that look like a patch, use
-those parts' contents (in order) as the mbox.  Otherwise, use the
-message itself if it looks like a patch."
+those parts' contents as the mbox, ordering the patches based on
+the number at the start of the file name.  If none of the file
+names start with a number, retain the original order of the
+attachments.
+
+If no MIME parts look like a patch, use the message itself if it
+looks like a patch."
   (when (derived-mode-p 'gnus-article-mode 'gnus-summary-mode)
     (cond
      (gnus-article-mime-handles
       (when-let ((patches (delq nil (mapcar #'piem-am-extract-attached-patch
                                             gnus-article-mime-handles))))
+        (setq patches (sort patches (lambda (x y) (< (car x) (car y)))))
         (cons (lambda ()
                 (dolist (patch patches)
-                  (insert patch)))
+                  (insert (cdr patch))))
               "mbox")))
      (gnus-article-buffer
       (when-let ((patch (with-current-buffer gnus-article-buffer
diff --git a/piem-notmuch.el b/piem-notmuch.el
index 41f2793a..493b7249 100644
--- a/piem-notmuch.el
+++ b/piem-notmuch.el
@@ -85,9 +85,15 @@ (defun piem-notmuch-mid-to-thread (mid)
 
 (defun piem-notmuch-am-ready-mbox ()
   "Return a function that inserts an am-ready mbox.
+
 If the buffer has any MIME parts that look like a patch, use
-those parts' contents (in order) as the mbox.  Otherwise, use the
-message itself if it looks like a patch."
+those parts' contents as the mbox, ordering the patches based on
+the number at the start of the file name.  If none of the file
+names start with a number, retain the original order of the
+attachments.
+
+If no MIME parts look like a patch, use the message itself if it
+looks like a patch."
   (when (derived-mode-p 'notmuch-show-mode)
     (let* ((handle (piem-notmuch--with-current-message
                      (mm-dissect-buffer)))
@@ -106,10 +112,11 @@ (defun piem-notmuch-am-ready-mbox ()
              (push patch patches)))
          handle)
         (when patches
-          (setq patches (nreverse patches))
+          (setq patches (sort (nreverse patches)
+                              (lambda (x y) (< (car x) (car y)))))
           (cons (lambda ()
                   (dolist (patch patches)
-                    (insert patch)))
+                    (insert (cdr patch))))
                 "mbox"))))))
 
 (defun piem-notmuch-extract-patch-am-ready-mbox ()
diff --git a/piem.el b/piem.el
index 47944e40..63c358a2 100644
--- a/piem.el
+++ b/piem.el
@@ -817,17 +817,21 @@ (defun piem-inject-thread-into-maildir (mid &optional inbox message-only)
 ;;;; Patch handling
 
 (defun piem-am-extract-attached-patch (handle)
-  "Return content for HANDLE if it looks like a patch."
-  (and (listp handle)
-       (let ((type (mm-handle-media-type handle))
-             (filename (mm-handle-filename handle)))
-         (or (member type '("text/x-diff" "text/x-patch"))
-             (and filename
-                  (equal type "text/plain")
-                  (string-suffix-p ".patch" filename t))))
-       (with-temp-buffer
-         (mm-display-inline handle)
-         (buffer-substring-no-properties (point-min) (point-max)))))
+  "Get the content for HANDLE if it looks like a patch.
+The return value is of the form (N . CONTENT), where N is the
+number at the start of the file name."
+  (when (listp handle)
+    (let ((type (mm-handle-media-type handle))
+          (filename (mm-handle-filename handle)))
+      (and (or (member type '("text/x-diff" "text/x-patch"))
+               (and filename
+                    (equal type "text/plain")
+                    (string-suffix-p ".patch" filename t)))
+           (with-temp-buffer
+             (mm-display-inline handle)
+             (cons
+              (string-to-number filename)
+              (buffer-substring-no-properties (point-min) (point-max))))))))
 
 (defun piem-extract-mbox-info (&optional buffer)
   "Collect information from message in BUFFER.

base-commit: 67411fadab680b01d9027a3f4833ef8892377874
-- 
2.36.1

Re: [PATCH] piem-am: Order attached patches by file name prefix

[Ihor Radchenko]

Kyle Meyer <kyle@kyleam.com> writes:

> Regardless of what you call it, I don't mind piem trying to be more
> clever/helpful in these scenarios.  How about something like this?
> (Tested on <87v8s8n1bm.fsf@heagren.com> from Notmuch and Gnus.)

LGTM!
I only have some minor comments on the code.

>  (defun piem-gnus-am-ready-mbox ()
>    "Return a function that inserts an am-ready mbox.
> +
>  If the buffer has any MIME parts that look like a patch, use
> -those parts' contents (in order) as the mbox.  Otherwise, use the
> -message itself if it looks like a patch."
> +those parts' contents as the mbox, ordering the patches based on
> +the number at the start of the file name.  If none of the file
> +names start with a number, retain the original order of the
> +attachments.
> ...
>  (defun piem-notmuch-am-ready-mbox ()
>    "Return a function that inserts an am-ready mbox.
> +
>  If the buffer has any MIME parts that look like a patch, use
> -those parts' contents (in order) as the mbox.  Otherwise, use the
> -message itself if it looks like a patch."
> +those parts' contents as the mbox, ordering the patches based on
> +the number at the start of the file name.  If none of the file
> +names start with a number, retain the original order of the
> +attachments.

There is certainly some code duplication going on here (;

> +  (when (listp handle)
> +    (let ((type (mm-handle-media-type handle))
> +          (filename (mm-handle-filename handle)))
> +      (and (or (member type '("text/x-diff" "text/x-patch"))
> +               (and filename
> +                    (equal type "text/plain")
> +                    (string-suffix-p ".patch" filename t)))
> +           (with-temp-buffer
> +             (mm-display-inline handle)
> +             (cons
> +              (string-to-number filename)
> +              (buffer-substring-no-properties (point-min) (point-max))))))))

Why using (and ...)? It feels like (when ...) is more appropriate in
this context.

Best,
Ihor

Re: [PATCH] piem-am: Order attached patches by file name prefix

[Kyle Meyer]

Ihor Radchenko writes:

> I only have some minor comments on the code.

Thanks for the feedback.

>>  (defun piem-gnus-am-ready-mbox ()
>>    "Return a function that inserts an am-ready mbox.
>> +
>>  If the buffer has any MIME parts that look like a patch, use
>> -those parts' contents (in order) as the mbox.  Otherwise, use the
>> -message itself if it looks like a patch."
>> +those parts' contents as the mbox, ordering the patches based on
>> +the number at the start of the file name.  If none of the file
>> +names start with a number, retain the original order of the
>> +attachments.
>> ...
>>  (defun piem-notmuch-am-ready-mbox ()
>>    "Return a function that inserts an am-ready mbox.
>> +
>>  If the buffer has any MIME parts that look like a patch, use
>> -those parts' contents (in order) as the mbox.  Otherwise, use the
>> -message itself if it looks like a patch."
>> +those parts' contents as the mbox, ordering the patches based on
>> +the number at the start of the file name.  If none of the file
>> +names start with a number, retain the original order of the
>> +attachments.
>
> There is certainly some code duplication going on here (;

For the two repeated blocks of the docstrings, I'm okay with a bit of
repeated documentation in this situation.  Another option would be to
put something about the attachment order in the docstring of
piem-am-ready-mbox-functions, but that moves it farther from the
relevant context.  (Or to just not document it at all.)

If you're commenting more generally on the _code_ of
piem-gnus-am-ready-mbox and piem-notmuch-am-ready-mbox, I'm open to
suggestions for extracting out any remaining shared bits, but my hunch
is that most directions would obscure things.

>> +  (when (listp handle)
>> +    (let ((type (mm-handle-media-type handle))
>> +          (filename (mm-handle-filename handle)))
>> +      (and (or (member type '("text/x-diff" "text/x-patch"))
>> +               (and filename
>> +                    (equal type "text/plain")
>> +                    (string-suffix-p ".patch" filename t)))
>> +           (with-temp-buffer
>> +             (mm-display-inline handle)
>> +             (cons
>> +              (string-to-number filename)
>> +              (buffer-substring-no-properties (point-min) (point-max))))))))
>
> Why using (and ...)? It feels like (when ...) is more appropriate in
> this context.

I like the (very soft and fuzzy) style guideline of using `and` to
signal the return value is the primary thing of interest and `when` to
signal the code is executed for side effects.  I'm not sure who I first
saw describe this convention [*], when I started to share the
preference, or how consistent I am in how I apply it.

Due to indentation, nesting, and/or grouping, I think there are cases
where it's more readable to use `when`, such as the top-level `when`
above.  Perhaps on a different night I would have felt the same about
the `and` you mentioned and flipped it to a `when`.

Is there a particular reason you prefer `when` in the above case?


[*] I'm not sure how widely this preference is shared among Elisp
    coders.  Perhaps it mostly comes from elsewhere, like Common Lisp
    (<https://www.cs.cmu.edu/Groups/AI/html/cltl/clm/node84.html>):

      As a matter of style, `when` is normally used to conditionally
      produce some side effects, and the value of the `when` form is
      normally not used. If the value is relevant, then it may be
      stylistically more appropriate to use `and` or `if`.

    I recall seeing Nicolas state it several times in reviews on the Org
    list.  Some examples:

    - https://list.orgmode.org/?q=f%3Anicolas+AND+nq%3Awhen+ADJ+nq%3Aside
    - https://list.orgmode.org/87d23sdtod.fsf@nicolasgoaziou.fr/

Re: [PATCH] piem-am: Order attached patches by file name prefix

[Ihor Radchenko]

Kyle Meyer <kyle@kyleam.com> writes:

> Ihor Radchenko writes:
>
>> There is certainly some code duplication going on here (;
>
> For the two repeated blocks of the docstrings, I'm okay with a bit of
> repeated documentation in this situation.  Another option would be to
> put something about the attachment order in the docstring of
> piem-am-ready-mbox-functions, but that moves it farther from the
> relevant context.  (Or to just not document it at all.)
>
> If you're commenting more generally on the _code_ of
> piem-gnus-am-ready-mbox and piem-notmuch-am-ready-mbox, I'm open to
> suggestions for extracting out any remaining shared bits, but my hunch
> is that most directions would obscure things.

I referred to both code and the docstring.
I'd define a separate function that inserts am-ready mbox:

(defun piem-am-ready-mbox-generic (patches format)
 "Return a function that inserts PATCHES as an am-ready mbox.
FORMAT is --patch-format value passed to `git am'.  See
`piem-am-ready-mbox-functions'.

The PATCHES will be sorted by number before insertion."
 (cons
  (lambda ()
   (setq patches (sort patches (lambda (x y) (< (car x) (car y)))))
   (dolist (patch patches) (insert patch)))
  format))

and then just call it from piem-notmuch-am-ready-mbox and
piem-gnus-am-ready-mbox.

>> Why using (and ...)? It feels like (when ...) is more appropriate in
>> this context.
>
> I like the (very soft and fuzzy) style guideline of using `and` to
> signal the return value is the primary thing of interest and `when` to
> signal the code is executed for side effects.  I'm not sure who I first
> saw describe this convention [*], when I started to share the
> preference, or how consistent I am in how I apply it.
>
> Due to indentation, nesting, and/or grouping, I think there are cases
> where it's more readable to use `when`, such as the top-level `when`
> above.  Perhaps on a different night I would have felt the same about
> the `and` you mentioned and flipped it to a `when`.
>
> Is there a particular reason you prefer `when` in the above case?
>
>
> [*] I'm not sure how widely this preference is shared among Elisp
>     coders.  Perhaps it mostly comes from elsewhere, like Common Lisp
>     (<https://www.cs.cmu.edu/Groups/AI/html/cltl/clm/node84.html>):
>
>       As a matter of style, `when` is normally used to conditionally
>       produce some side effects, and the value of the `when` form is
>       normally not used. If the value is relevant, then it may be
>       stylistically more appropriate to use `and` or `if`.
>
>     I recall seeing Nicolas state it several times in reviews on the Org
>     list.  Some examples:
>
>     - https://list.orgmode.org/?q=f%3Anicolas+AND+nq%3Awhen+ADJ+nq%3Aside
>     - https://list.orgmode.org/87d23sdtod.fsf@nicolasgoaziou.fr/

I do agree that using side-effects inside `and' is confusing and `when'
should be preferred.

However, we are talking about an opposite situation here.
No side effects and we intend to provide a return value.
The cites style convention does not necessarily apply in reverse except
that `when' should not ideally be used.

When choosing between `and' and `if' I personally also dislike using
(and condition1 condition2 ... return-value)

I find
(if (and condition1 condition2 ...)
 return-value
 nil)
more clear because it signifies that nil is an intended return value.

Of course, this is a very minor stylistic comment - something I "feel"
right from Elisp experience. I am not even a professional developer for
what it's worth.

Best,
Ihor

Re: [PATCH] piem-am: Order attached patches by file name prefix

[Kyle Meyer]

Ihor Radchenko writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
>> If you're commenting more generally on the _code_ of
>> piem-gnus-am-ready-mbox and piem-notmuch-am-ready-mbox, I'm open to
>> suggestions for extracting out any remaining shared bits, but my hunch
>> is that most directions would obscure things.
>
> I referred to both code and the docstring.
> I'd define a separate function that inserts am-ready mbox:

As you can probably guess from my last sentence, I don't think
maintaining that extra level of indirection in piem.el buys enough to be
worth the decrease in readability.  But that's of course a judgment
call.

> I do agree that using side-effects inside `and' is confusing and `when'
> should be preferred.
>
> However, we are talking about an opposite situation here.
> No side effects and we intend to provide a return value.
> The cites style convention does not necessarily apply in reverse except
> that `when' should not ideally be used.

I'm not claiming it's symmetric: I mentioned cases where I prefer `and
-> when`, but you're right that that doesn't go in the other direction.
So I agree with what you say here, as far as I can see.

> When choosing between `and' and `if' I personally also dislike using
> (and condition1 condition2 ... return-value)
> [...]

My tastes don't align with yours here.

> Of course, this is a very minor stylistic comment - [...]

Of course.  Thanks for taking the time to expand on your opinion.

Pushed (6776cacf).