bug#63378: A single X-Debbugs-CC header must be used

bug#63378: A single X-Debbugs-CC header must be used

[Maxim Cournoyer]

Hi,

After some tests, it appears that a single X-Debbugs-CC header must be
used, otherwise the last one is the one that prevails.  This matches my
reading of the 'process' script of the GNU Debbugs instance [0], and
thus must conform to the same email rules outlined in RFC5322 for the To
or CC fields (only one such header must be used); multiple values can be
separated by a comma. [1]

--8<---------------cut here---------------start------------->8---
my %header;

for my $hdr (@headerlines) {
    $hdr = decode_rfc1522($hdr);
    $_ = $hdr;
    s/\n\s/ /g;
    &finish if m/^x-loop: (\S+)$/i && $1 eq "$gMaintainerEmail";
    my $ins = !m/^subject:/i && !m/^reply-to:/i && !m/^return-path:/i
           && !m/^From / && !m/^X-Debbugs-/i && !m/^cc:/i && !m/^to:/i;
    $fwd .= $hdr."\n" if $ins;
    # print DEBUG ">$_<\n";
    if (s/^(\S+):\s*//) {
	my $v = lc $1;
	print DEBUG ">$v=$_<\n";
        ## There may be multiple To: or Cc: headers (see bug#5996).
        if ( ($v eq 'to' || $v eq 'cc') &&
	     defined $header{$v} && length($header{$v}) ) {
            $header{$v} = $header{$v} . ', ' . $_ if length($_);
        } else {
            $header{$v} = $_;
        }
    } else {
	print DEBUG "!>$_<\n";
    }
}
$header{'message-id'} = '' if not defined $header{'message-id'};
--8<---------------cut here---------------end--------------->8---

Only 'to' or 'cc' multiple headers are coalesced into one; otherwise the
$header specific key (for a given header) is overridden to the last
value encountered at line '$header{$v} = $_;', IIUC.

Our teams.scm script should be adjusted to produce a single X-Debbugs-CC
header with comma-separated values.

[0]  https://gitlab.com/npostavs/debbugs/-/blob/gnu-reconstruction/scripts/process#L171
[1]  https://datatracker.ietf.org/doc/rfc5322/, 3.6 Field Definitions

-- 
Thanks,
Maxim

bug#63378: [PATCH] teams: Fix script to produce a single X-Debbugs-Cc header.

[Maxim Cournoyer]

Fixes <https://issues.guix.gnu.org/63378>.

* etc/teams.scm.in (cc): Adjust format pattern.
(team->members, member->string): New procedures.
(list-members): Refactor in terms of the above procedures.
(main): Adjust the output of the 'cc-members-header-cmd' and
'cc-mentors-header-cmd' actions.
---
 etc/teams.scm.in | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/etc/teams.scm.in b/etc/teams.scm.in
index dec175f630..50bfbca22e 100644
--- a/etc/teams.scm.in
+++ b/etc/teams.scm.in
@@ -605,23 +605,27 @@ (define (cc . teams)
   "Return arguments for `git send-email' to notify the members of the given
 TEAMS when a patch is received by Debbugs."
   (format #true
-          "~{--add-header=\"X-Debbugs-Cc: ~a\"~^ ~}"
+          "--add-header=\"X-Debbugs-Cc: ~{~a~^,~}\""
           (map person-email
                (delete-duplicates (append-map team-members teams) equal?))))
 
+(define (team->members team)
+  "Return the list of members in TEAM."
+  (sort (team-members team)
+        (lambda (m1 m2)
+          (string<? (person-name m1) (person-name m2)))))
+
+(define (member->string member)
+  "Return the 'email <name>' string representation of MEMBER."
+  (format #false "~a <~a>" (person-name member) (person-email member)))
+
 (define* (list-members team #:optional port (prefix ""))
   "Print the members of the given TEAM."
   (define port* (or port (current-output-port)))
   (for-each
    (lambda (member)
-     (format port*
-             "~a~a <~a>~%"
-             prefix
-             (person-name member)
-             (person-email member)))
-   (sort
-    (team-members team)
-    (lambda (m1 m2) (string<? (person-name m1) (person-name m2))))))
+     (format port* "~a~a~%" prefix (member->string member)))
+   (team->members team)))
 
 (define (list-teams)
   "Print all teams, their scope and their members."
@@ -715,13 +719,14 @@ (define (main . args)
      (apply cc (find-team-by-scope
                 (diff-revisions rev-start rev-end))))
     (("cc-members-header-cmd" patch-file)
-     (for-each (lambda (team-name)
-                 (list-members (find-team team-name) (current-output-port)
-                               "X-Debbugs-Cc: "))
-               (patch->teams patch-file)))
+     (format #true "X-Debbugs-Cc: ~{~a~^,~}"
+             (append-map (compose (cut map member->string <>)
+                                  team->members
+                                  find-team)
+                         (patch->teams patch-file))))
     (("cc-mentors-header-cmd" patch-file)
-     (list-members (find-team "mentors") (current-output-port)
-                   "X-Debbugs-Cc: "))
+     (format #true "X-Debbugs-Cc: ~{~a~^,~}"
+             (map member->string (team->members (find-team "mentors")))))
     (("get-maintainer" patch-file)
      (apply main "list-members" (patch->teams patch-file)))
     (("list-teams" . args)

base-commit: 4228d3c358669f5d15f01d3ba466f3356ecb6546
-- 
2.39.2

bug#63378: A single X-Debbugs-CC header must be used

[Maxim Cournoyer]

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Fixes <https://issues.guix.gnu.org/63378>.
>
> * etc/teams.scm.in (cc): Adjust format pattern.
> (team->members, member->string): New procedures.
> (list-members): Refactor in terms of the above procedures.
> (main): Adjust the output of the 'cc-members-header-cmd' and
> 'cc-mentors-header-cmd' actions.

Paul from Debian has posted a merge request to the upstream Debbugs [0]
that would make it possible to use multiple X-Debbugs-CC.

Now, I don't think the GNU Debbugs instance is kept up-to-date with
Debbugs upstream, having its own set of changes, so I think the patch
here should still be applied in the meantime.

[0]  https://salsa.debian.org/debbugs-team/debbugs/-/merge_requests/18/diffs

-- 
Thanks,
Maxim

bug#63378: A single X-Debbugs-CC header must be used

[Arun Isaac]

> Now, I don't think the GNU Debbugs instance is kept up-to-date with
> Debbugs upstream, having its own set of changes, so I think the patch
> here should still be applied in the meantime.

I agree. And, even otherwise, it is nice for X-Debbugs-Cc to mirror the
Cc header and use a comma-separated list. So, I'm all in favour of this
patch. Specific comments about the patch itself follow in a separate
email.

I will work on a similar patch for `mumi send-email' and send it
sometime later this week. As discussed earlier, I will make `mumi
send-email' invoke `git config sendemail.headerCmd' to find out about
etc/teams.scm.

bug#63378: [PATCH] teams: Fix script to produce a single X-Debbugs-Cc header.

[Arun Isaac]

Hi Maxim,

When a patch relates to no teams, I get the empty header output
"X-Debbugs-Cc: ". For such patches, it would be nicer to not add any
header instead of adding an empty header.

> +(define (team->members team)
> +  "Return the list of members in TEAM."
> +  (sort (team-members team)
> +        (lambda (m1 m2)
> +          (string<? (person-name m1) (person-name m2)))))
> +
> +(define (member->string member)
> +  "Return the 'email <name>' string representation of MEMBER."
> +  (format #false "~a <~a>" (person-name member) (person-email
> member)))

When person name contains a comma, it should be escaped by surrounding
in double quotes. Else, the comma would interfere with the
comma-separated list that we are constructing for
X-Debbugs-Cc. Admittedly, none of our current team members have commas
in their person names. But, some people like to have a name like
"LastName, FirstName". We should protect against that edge case in the
interest of futureproofing.

> +     (format #true "X-Debbugs-Cc: ~{~a~^,~}"
> +             (append-map (compose (cut map member->string <>)
> +                                  team->members
> +                                  find-team)
> +                         (patch->teams patch-file))))

A very nitpicky suggestion: Maybe, separate multiple persons by ", "
instead of just ",". ", " looks slightly better cosmetically. Likewise
in other places where this occurs.

Thanks for all the hard work! :-)

Regards,
Arun

bug#63378: A single X-Debbugs-CC header must be used

[Maxim Cournoyer]

Hi Arun,

Arun Isaac <arunisaac@systemreboot.net> writes:

>> Now, I don't think the GNU Debbugs instance is kept up-to-date with
>> Debbugs upstream, having its own set of changes, so I think the patch
>> here should still be applied in the meantime.
>
> I agree. And, even otherwise, it is nice for X-Debbugs-Cc to mirror the
> Cc header and use a comma-separated list. So, I'm all in favour of this
> patch. Specific comments about the patch itself follow in a separate
> email.
>
> I will work on a similar patch for `mumi send-email' and send it
> sometime later this week. As discussed earlier, I will make `mumi
> send-email' invoke `git config sendemail.headerCmd' to find out about
> etc/teams.scm.

Sounds good, and thanks for your efforts pushing toward better teams
tooling!  I'll now address your comments.

-- 
Thanks,
Maxim

bug#63378: [PATCH v2] teams: Fix script to produce a single X-Debbugs-Cc header.

[Maxim Cournoyer]

Fixes <https://issues.guix.gnu.org/63378>.

* etc/teams.scm.in (cc): Adjust format pattern.
(sort-members, member->string): New procedures.
(list-members): Refactor in terms of the above procedures.
(main): Adjust the output of the 'cc-members-header-cmd' and
'cc-mentors-header-cmd' actions.
---
 etc/teams.scm.in | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/etc/teams.scm.in b/etc/teams.scm.in
index d21f1ff67b..d81ce984f6 100644
--- a/etc/teams.scm.in
+++ b/etc/teams.scm.in
@@ -605,24 +605,28 @@ (define (find-team-by-scope files)
 (define (cc . teams)
   "Return arguments for `git send-email' to notify the members of the given
 TEAMS when a patch is received by Debbugs."
-  (format #true
-          "~{--add-header=\"X-Debbugs-Cc: ~a\"~^ ~}"
-          (map person-email
-               (delete-duplicates (append-map team-members teams) equal?))))
+  (let ((members (append-map team-members teams)))
+    (unless (null? members)
+      (format #true "--add-header=\"X-Debbugs-Cc: ~{~a~^, ~}\""
+              (map person-email (sort-members members))))))
+
+(define (sort-members members)
+  "Deduplicate and sort MEMBERS alphabetically by their name."
+  (sort (delete-duplicates members equal?)
+        (lambda (m1 m2)
+          (string<? (person-name m1) (person-name m2)))))
+
+(define (member->string member)
+  "Return the 'email <name>' string representation of MEMBER."
+  (format #false "\"~a\" <~a>" (person-name member) (person-email member)))
 
 (define* (list-members team #:optional port (prefix ""))
   "Print the members of the given TEAM."
   (define port* (or port (current-output-port)))
   (for-each
    (lambda (member)
-     (format port*
-             "~a~a <~a>~%"
-             prefix
-             (person-name member)
-             (person-email member)))
-   (sort
-    (team-members team)
-    (lambda (m1 m2) (string<? (person-name m1) (person-name m2))))))
+     (format port* "~a~a~%" prefix (member->string member)))
+   (sort-members (team-members team))))
 
 (define (list-teams)
   "Print all teams, their scope and their members."
@@ -716,13 +720,15 @@ (define (main . args)
      (apply cc (find-team-by-scope
                 (diff-revisions rev-start rev-end))))
     (("cc-members-header-cmd" patch-file)
-     (for-each (lambda (team-name)
-                 (list-members (find-team team-name) (current-output-port)
-                               "X-Debbugs-Cc: "))
-               (patch->teams patch-file)))
+     (let* ((teams (map find-team (patch->teams patch-file)))
+            (members (sort-members (append-map team-members teams))))
+       (unless (null? members)
+         (format #true "X-Debbugs-Cc: ~{~a~^, ~}"
+                 (map member->string members)))))
     (("cc-mentors-header-cmd" patch-file)
-     (list-members (find-team "mentors") (current-output-port)
-                   "X-Debbugs-Cc: "))
+     (format #true "X-Debbugs-Cc: ~{~a~^, ~}"
+             (map member->string
+                  (sort-members (team-members (find-team "mentors"))))))
     (("get-maintainer" patch-file)
      (apply main "list-members" (patch->teams patch-file)))
     (("list-teams" . args)

base-commit: 8f92dfd9ae7ac491ab7fb4b425799a8c909708a8
-- 
2.39.2

bug#63378: [PATCH] teams: Fix script to produce a single X-Debbugs-Cc header.

[Maxim Cournoyer]

Hi Arun,

Arun Isaac <arunisaac@systemreboot.net> writes:

> Hi Maxim,
>
> When a patch relates to no teams, I get the empty header output
> "X-Debbugs-Cc: ". For such patches, it would be nicer to not add any
> header instead of adding an empty header.

Good catch!  Fixed.

>> +(define (team->members team)
>> +  "Return the list of members in TEAM."
>> +  (sort (team-members team)
>> +        (lambda (m1 m2)
>> +          (string<? (person-name m1) (person-name m2)))))
>> +
>> +(define (member->string member)
>> +  "Return the 'email <name>' string representation of MEMBER."
>> +  (format #false "~a <~a>" (person-name member) (person-email
>> member)))
>
> When person name contains a comma, it should be escaped by surrounding
> in double quotes. Else, the comma would interfere with the
> comma-separated list that we are constructing for
> X-Debbugs-Cc. Admittedly, none of our current team members have commas
> in their person names. But, some people like to have a name like
> "LastName, FirstName". We should protect against that edge case in the
> interest of futureproofing.

OK!  I opted for simplicity and double-quoted all the names.

>> +     (format #true "X-Debbugs-Cc: ~{~a~^,~}"
>> +             (append-map (compose (cut map member->string <>)
>> +                                  team->members
>> +                                  find-team)
>> +                         (patch->teams patch-file))))
>
> A very nitpicky suggestion: Maybe, separate multiple persons by ", "
> instead of just ",". ", " looks slightly better cosmetically. Likewise
> in other places where this occurs.

Good idea; I've learnt this was valid email syntax just today :-).

Below is the diff of my rework:

--8<---------------cut here---------------start------------->8---
1 file changed, 18 insertions(+), 17 deletions(-)
etc/teams.scm.in | 35 ++++++++++++++++++-----------------

modified   etc/teams.scm.in
@@ -605,20 +605,20 @@ (define (find-team-by-scope files)
 (define (cc . teams)
   "Return arguments for `git send-email' to notify the members of the given
 TEAMS when a patch is received by Debbugs."
-  (format #true
-          "--add-header=\"X-Debbugs-Cc: ~{~a~^,~}\""
-          (map person-email
-               (delete-duplicates (append-map team-members teams) equal?))))
-
-(define (team->members team)
-  "Return the list of members in TEAM."
-  (sort (team-members team)
+  (let ((members (append-map team-members teams)))
+    (unless (null? members)
+      (format #true "--add-header=\"X-Debbugs-Cc: ~{~a~^, ~}\""
+              (map person-email (sort-members members))))))
+
+(define (sort-members members)
+  "Deduplicate and sort MEMBERS alphabetically by their name."
+  (sort (delete-duplicates members equal?)
         (lambda (m1 m2)
           (string<? (person-name m1) (person-name m2)))))
 
 (define (member->string member)
   "Return the 'email <name>' string representation of MEMBER."
-  (format #false "~a <~a>" (person-name member) (person-email member)))
+  (format #false "\"~a\" <~a>" (person-name member) (person-email member)))
 
 (define* (list-members team #:optional port (prefix ""))
   "Print the members of the given TEAM."
@@ -626,7 +626,7 @@ (define* (list-members team #:optional port (prefix ""))
   (for-each
    (lambda (member)
      (format port* "~a~a~%" prefix (member->string member)))
-   (team->members team)))
+   (sort-members (team-members team))))
 
 (define (list-teams)
   "Print all teams, their scope and their members."
@@ -720,14 +720,15 @@ (define (main . args)
      (apply cc (find-team-by-scope
                 (diff-revisions rev-start rev-end))))
     (("cc-members-header-cmd" patch-file)
-     (format #true "X-Debbugs-Cc: ~{~a~^,~}"
-             (append-map (compose (cut map member->string <>)
-                                  team->members
-                                  find-team)
-                         (patch->teams patch-file))))
+     (let* ((teams (map find-team (patch->teams patch-file)))
+            (members (sort-members (append-map team-members teams))))
+       (unless (null? members)
+         (format #true "X-Debbugs-Cc: ~{~a~^, ~}"
+                 (map member->string members)))))
     (("cc-mentors-header-cmd" patch-file)
-     (format #true "X-Debbugs-Cc: ~{~a~^,~}"
-             (map member->string (team->members (find-team "mentors")))))
+     (format #true "X-Debbugs-Cc: ~{~a~^, ~}"
+             (map member->string
+                  (sort-members (team-members (find-team "mentors"))))))
     (("get-maintainer" patch-file)
      (apply main "list-members" (patch->teams patch-file)))
     (("list-teams" . args)
--8<---------------cut here---------------end--------------->8---

A proper v2 patch has been sent.

-- 
Thanks,
Maxim

bug#63378: [PATCH] teams: Fix script to produce a single X-Debbugs-Cc header.

[Arun Isaac]

Hi Maxim,

Thank you for the updated patch! :-) It LGTM. Please push.

> OK!  I opted for simplicity and double-quoted all the names.

Fair enough. Though a check is only one condition away! ;-)

(if (string-contains? (person-name member) ",")
    (string-append "\"" (person-name member) "\"")
    (person-name member))

Regards,
Arun

bug#63378: [PATCH] teams: Fix script to produce a single X-Debbugs-Cc header.

[Maxim Cournoyer]

Hi Arun,

Arun Isaac <arunisaac@systemreboot.net> writes:

> Hi Maxim,
>
> Thank you for the updated patch! :-) It LGTM. Please push.
>
>> OK!  I opted for simplicity and double-quoted all the names.
>
> Fair enough. Though a check is only one condition away! ;-)
>
> (if (string-contains? (person-name member) ",")
>     (string-append "\"" (person-name member) "\"")
>     (person-name member))

It's string-contains without ?, apparently.  Let's try this (and save a
few bytes per submission :-)):

--8<---------------cut here---------------start------------->8---
modified   etc/teams.scm.in
@@ -618,7 +618,11 @@ (define (sort-members members)

 (define (member->string member)
   "Return the 'email <name>' string representation of MEMBER."
-  (format #false "\"~a\" <~a>" (person-name member) (person-email member)))
+  (let* ((name (person-name member))
+         (quoted-name/maybe (if (string-contains name ",")
+                                (string-append "\"" name "\"")
+                                name)))
+    (format #false "~a <~a>" quoted-name/maybe (person-email member))))

 (define* (list-members team #:optional port (prefix ""))
   "Print the members of the given TEAM."
--8<---------------cut here---------------end--------------->8---

The change is now installed; thanks for the review!

-- 
Thanks,
Maxim

bug#63378: [PATCH] teams: Fix script to produce a single X-Debbugs-Cc header.

[Arun Isaac]

> It's string-contains without ?, apparently.

Ah, yes. That one always trips me up.

> The change is now installed; thanks for the review!

Thank you! :-)