bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

[Luis Henriques]

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

Hi!

[Resending as I don't see message in the list after a few hours.]

I'd like to have git-format-patch diffs to be properly identified when I'm
using Gnus to read mailing-lists.  It mostly works fine, *if* the
(inlined) patches include a signature at the end ('--').  If the signature
is missing then the patch isn't identified as such.

Since all the other diff formats in mm-uu-type-alist don't have the
'end-point' I thought it would be fine to also remove it from the
'git-format-patch'.

The issue I'm trying to fix can be easily seen in Gnus by comparing two
emails with the following message-ids from the emacs-devel@gnu.org
mailing-list:

  87v81dmhxi.fsf@orpheu.olymp
  20240702155100.2150717-1-brennan@umanwizard.com

(These emails can be accessed by entering the Gnus group, hitting 'j'
(gnus-summary-goto-article) and yanking the above message-ids.)

Cheers,
-- 
Luís


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Ensure-that-git-diffs-without-signature-are-properly.patch --]
[-- Type: text/x-patch, Size: 795 bytes --]

From fb9a1413655837607b2ed91d11d5cb2e3ba99415 Mon Sep 17 00:00:00 2001
From: Luis Henriques <henrix@camandro.org>
Date: Thu, 11 Jul 2024 10:02:04 +0100
Subject: [PATCH] Ensure that git diffs without signature (--) are properly
 identified

* lisp/gnus/mm-uu.el (mm-uu-type-alist): Remove 'end-point' from
git-format-patch diffs so that diffs without signature can be identified.
---
 lisp/gnus/mm-uu.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/gnus/mm-uu.el b/lisp/gnus/mm-uu.el
index 3c7e3cbdf1af..f5d553bd0892 100644
--- a/lisp/gnus/mm-uu.el
+++ b/lisp/gnus/mm-uu.el
@@ -173,7 +173,7 @@ mm-uu-type-alist
      ,#'mm-uu-diff-test)
     (git-format-patch
      "^diff --git "
-     "^-- "
+     nil
      ,#'mm-uu-diff-extract
      nil
      ,#'mm-uu-diff-test)

bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

[Robert Pluim]

>>>>> On Thu, 11 Jul 2024 13:20:32 +0100, Luis Henriques <henrix@camandro.org> said:

    Luis> Hi!
    Luis> [Resending as I don't see message in the list after a few hours.]

I see both those messages. There is moderation for unsubscribed users,
so sometimes there is lag.

    Luis> I'd like to have git-format-patch diffs to be properly identified when I'm
    Luis> using Gnus to read mailing-lists.  It mostly works fine, *if* the
    Luis> (inlined) patches include a signature at the end ('--').  If the signature
    Luis> is missing then the patch isn't identified as such.

    Luis> Since all the other diff formats in mm-uu-type-alist don't have the
    Luis> 'end-point' I thought it would be fine to also remove it from the
    Luis> 'git-format-patch'.

git-format-patch only produces patches like that if you pass it
'--no-signature', I think.

    Luis> The issue I'm trying to fix can be easily seen in Gnus by comparing two
    Luis> emails with the following message-ids from the emacs-devel@gnu.org
    Luis> mailing-list:

    Luis>   87v81dmhxi.fsf@orpheu.olymp

That one actually looks like just 'git diff' rather than 'git format-patch'

Iʼm trying to work out the benefit here compared to the status quo vs
the risk of breaking something. If Gnus doesnʼt identify such messages
as containing patches, you donʼt get the in-article buttons, but you
can still pipe the message to 'git apply'.

Also, how does this work for messages containing multiple patches? Is
detection of just the start of each patch enough?

Maybe adding a new detection method would be better?

Robert
-- 

bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

[Luis Henriques]

Hi Robert!

(First of all, thank you for your review!)

On Thu, Jul 11 2024, Robert Pluim wrote:

>>>>>> On Thu, 11 Jul 2024 13:20:32 +0100, Luis Henriques <henrix@camandro.org> said:
>
>     Luis> Hi!
>     Luis> [Resending as I don't see message in the list after a few hours.]
>
> I see both those messages. There is moderation for unsubscribed users,
> so sometimes there is lag.

Yeah, sorry.  I saw both hitting the list pretty much at the same time.  I
guess I was just too eager on getting it there.

>
>     Luis> I'd like to have git-format-patch diffs to be properly identified when I'm
>     Luis> using Gnus to read mailing-lists.  It mostly works fine, *if* the
>     Luis> (inlined) patches include a signature at the end ('--').  If the signature
>     Luis> is missing then the patch isn't identified as such.
>
>     Luis> Since all the other diff formats in mm-uu-type-alist don't have the
>     Luis> 'end-point' I thought it would be fine to also remove it from the
>     Luis> 'git-format-patch'.
>
> git-format-patch only produces patches like that if you pass it
> '--no-signature', I think.

Or you may just set 'format.signature' to an empty string in your config,
which is what I have been using almost since day one.  This will prevent
git from leaking it's version.

>     Luis> The issue I'm trying to fix can be easily seen in Gnus by comparing two
>     Luis> emails with the following message-ids from the emacs-devel@gnu.org
>     Luis> mailing-list:
>
>     Luis>   87v81dmhxi.fsf@orpheu.olymp
>
> That one actually looks like just 'git diff' rather than 'git format-patch'

I didn't go check, but if I had to guess, 'git format-patch' actually uses
'git diff' for generating the diff (with stats) and adds a signature at
the end (if configured to do so).

Anyway, I always send patches without git signature, generated with 'git
format-patch' and (most of the time) sent with 'git send-email'.  And
those are never identified as patches.

> Iʼm trying to work out the benefit here compared to the status quo vs
> the risk of breaking something. If Gnus doesnʼt identify such messages
> as containing patches, you donʼt get the in-article buttons, but you
> can still pipe the message to 'git apply'.

Right, the only benefit is just the extra eye-candy stuff.

> Also, how does this work for messages containing multiple patches? Is
> detection of just the start of each patch enough?

Do you have an example where this happens?  I don't think I ever saw an
email with two inlined patches.  But obviously, with this patch applied,
everything from the "^diff --git " up to the end of the email will be a
diff.  Just like everything after "^=== modified file " or "^Index: " will
be a diff.

> Maybe adding a new detection method would be better?

The problem I see with that is that this new detection method will
necessarily overlap with the 'git-format-patch' in mm-uu-type-alist.
Won't it simply shadow it, and will always be used?

Cheers,
-- 
Luís

bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

[Kévin Le Gouguec]

Luis Henriques <henrix@camandro.org> writes:

>> Also, how does this work for messages containing multiple patches? Is
>> detection of just the start of each patch enough?
>
> Do you have an example where this happens?  I don't think I ever saw an
> email with two inlined patches.  But obviously, with this patch applied,
> everything from the "^diff --git " up to the end of the email will be a
> diff.  Just like everything after "^=== modified file " or "^Index: " will
> be a diff.

FWIW, I've had this uncommitted kludge in my config for some time now:

  @@ -403,6 +431,9 @@

   ;; TODO: handle inline diffs in Gnus articles; maybe tweaking
   ;; mm-uu-type-alist?
  +(with-eval-after-load 'mm-uu
  +  (setf (nth 1 (alist-get 'git-format-patch mm-uu-type-alist))
  +        "^-- \\|^$"))

Past Me wasn't considerate enough to add a Message-ID demonstrating
where this fixes problems.  An earlier version of that config featured:

  ;; Gerrit does not include trailing "-- ".
  (with-eval-after-load 'mm-uu
    (setf (nth 1 (alist-get 'git-format-patch mm-uu-type-alist))
          "^-- \\|\\'"))

Past Me eventually ditched this for being, citing the commit message,
"too flaky".  Again, did not bother to add a reference to a problematic
sample 🙄

I think the logic behind "^-- \\|^$" is that ^$ ought to find the end of
a "very-inlined patch" (i.e. not "attached", not even as an inlined
part; think 'C-x v d', 'C-x h' 'M-w', 'C-x o', 'C-y') because even empty
context lines have a leading space…

… but that's just guesswork.  Thought it'd be worth mentioning, but feel
free to ignore the noise if not helpful.

bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

[Juri Linkov]

>   +(with-eval-after-load 'mm-uu
>   +  (setf (nth 1 (alist-get 'git-format-patch mm-uu-type-alist))
>   +        "^-- \\|^$"))

Indeed, this is the right thing to do.  In the past
few days I have seen more patches without attachments
that makes hard to read them without fontification.
So such change could help to greatly improve readability.
However, often there is also text after the patch,
so nil for 'end-point' doesn't look right.  So better
to end the patch with an empty line, because properly
formatted patches have no empty lines in them.

diff --git a/lisp/gnus/mm-uu.el b/lisp/gnus/mm-uu.el
index 3c7e3cbdf1a..26b2c03a3dc 100644
--- a/lisp/gnus/mm-uu.el
+++ b/lisp/gnus/mm-uu.el
@@ -173,7 +173,7 @@ mm-uu-type-alist
      ,#'mm-uu-diff-test)
     (git-format-patch
      "^diff --git "
-     "^-- "
+     "^$"
      ,#'mm-uu-diff-extract
      nil
      ,#'mm-uu-diff-test)

+ PS: the patch above should end before this line.
- I tested it on a few posts, and everything looks correct.

bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

[Robert Pluim]

>>>>> On Fri, 12 Jul 2024 09:51:16 +0300, Juri Linkov <juri@linkov.net> said:

    >> +(with-eval-after-load 'mm-uu
    >> +  (setf (nth 1 (alist-get 'git-format-patch mm-uu-type-alist))
    >> +        "^-- \\|^$"))

    Juri> Indeed, this is the right thing to do.  In the past
    Juri> few days I have seen more patches without attachments
    Juri> that makes hard to read them without fontification.
    Juri> So such change could help to greatly improve readability.
    Juri> However, often there is also text after the patch,
    Juri> so nil for 'end-point' doesn't look right.  So better
    Juri> to end the patch with an empty line, because properly
    Juri> formatted patches have no empty lines in them.

    Juri> diff --git a/lisp/gnus/mm-uu.el b/lisp/gnus/mm-uu.el
    Juri> index 3c7e3cbdf1a..26b2c03a3dc 100644
    Juri> --- a/lisp/gnus/mm-uu.el
    Juri> +++ b/lisp/gnus/mm-uu.el
    Juri> @@ -173,7 +173,7 @@ mm-uu-type-alist
    Juri>       ,#'mm-uu-diff-test)
    Juri>      (git-format-patch
    Juri>       "^diff --git "
    Juri> -     "^-- "
    Juri> +     "^$"
    Juri>       ,#'mm-uu-diff-extract
    Juri>       nil
    Juri>       ,#'mm-uu-diff-test)

    Juri> + PS: the patch above should end before this line.
    Juri> - I tested it on a few posts, and everything looks correct.

It works on your message, at least 😀

I think that would be a safe enough change.


Robert
-- 

bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

[Luís Henriques]

On Fri, Jul 12, 2024 at 09:51:16AM +0300, Juri Linkov wrote:
> >   +(with-eval-after-load 'mm-uu
> >   +  (setf (nth 1 (alist-get 'git-format-patch mm-uu-type-alist))
> >   +        "^-- \\|^$"))
> 
> Indeed, this is the right thing to do.  In the past
> few days I have seen more patches without attachments
> that makes hard to read them without fontification.
> So such change could help to greatly improve readability.
> However, often there is also text after the patch,
> so nil for 'end-point' doesn't look right.  So better
> to end the patch with an empty line, because properly
> formatted patches have no empty lines in them.
> 
> diff --git a/lisp/gnus/mm-uu.el b/lisp/gnus/mm-uu.el
> index 3c7e3cbdf1a..26b2c03a3dc 100644
> --- a/lisp/gnus/mm-uu.el
> +++ b/lisp/gnus/mm-uu.el
> @@ -173,7 +173,7 @@ mm-uu-type-alist
>       ,#'mm-uu-diff-test)
>      (git-format-patch
>       "^diff --git "
> -     "^-- "
> +     "^$"
>       ,#'mm-uu-diff-extract
>       nil
>       ,#'mm-uu-diff-test)
> 
> + PS: the patch above should end before this line.
> - I tested it on a few posts, and everything looks correct.

Ah! Nice, this indeed looks better than my proposal.  Thanks!  Hopefully
this will be acceptable for merging.

Cheers,
--
Luis

bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

[Juri Linkov]

close 72059 31.0.50
thanks

>> >   +(with-eval-after-load 'mm-uu
>> >   +  (setf (nth 1 (alist-get 'git-format-patch mm-uu-type-alist))
>> >   +        "^-- \\|^$"))
>> 
>> Indeed, this is the right thing to do.  In the past
>> few days I have seen more patches without attachments
>> that makes hard to read them without fontification.
>> So such change could help to greatly improve readability.
>> However, often there is also text after the patch,
>> so nil for 'end-point' doesn't look right.  So better
>> to end the patch with an empty line, because properly
>> formatted patches have no empty lines in them.
>> 
>> diff --git a/lisp/gnus/mm-uu.el b/lisp/gnus/mm-uu.el
>> index 3c7e3cbdf1a..26b2c03a3dc 100644
>> --- a/lisp/gnus/mm-uu.el
>> +++ b/lisp/gnus/mm-uu.el
>> @@ -173,7 +173,7 @@ mm-uu-type-alist
>>       ,#'mm-uu-diff-test)
>>      (git-format-patch
>>       "^diff --git "
>> -     "^-- "
>> +     "^$"
>>       ,#'mm-uu-diff-extract
>>       nil
>>       ,#'mm-uu-diff-test)
>> 
>> + PS: the patch above should end before this line.
>> - I tested it on a few posts, and everything looks correct.
>
> Ah! Nice, this indeed looks better than my proposal.  Thanks!  Hopefully
> this will be acceptable for merging.

Thanks for the request!  So now this is pushed to master.

bug#72059: [PATCH] Ensure that git diffs without signature (--) are properly identified

[Eli Zaretskii]

merge 72059 72058
thanks

> From: Luis Henriques <henrix@camandro.org>
> Date: Thu, 11 Jul 2024 13:20:32 +0100
> 
> [Resending as I don't see message in the list after a few hours.]

Merging the two identical bug reports.