| Message ID | 20230501225158.18612-3-arunisaac@systemreboot.net |
|---|---|
| State | New |
| Headers | show |
| Series | Cc all issue participants | expand |
Hi Arun, Arun Isaac <arunisaac@systemreboot.net> writes: > * mumi/client.scm: Import (srfi srfi-1). > (reply-email-headers): New function. > (send-email): Call reply-email-headers. > * tests/client.scm ("send patches to existing issue", "send single > patch to existing issue"): Stub reply-email-headers. > ("send patch to existing issue and Cc other participants"): New test. Great series! > --- > mumi/client.scm | 35 +++++++++++++++++++++++++++++++---- > tests/client.scm | 24 ++++++++++++++++++++++++ > 2 files changed, 55 insertions(+), 4 deletions(-) > > diff --git a/mumi/client.scm b/mumi/client.scm > index 2750836..f0e4321 100644 > --- a/mumi/client.scm > +++ b/mumi/client.scm > @@ -18,6 +18,7 @@ > > (define-module (mumi client) > #:use-module (rnrs io ports) > + #:use-module (srfi srfi-1) > #:use-module (srfi srfi-19) > #:use-module (srfi srfi-26) > #:use-module (srfi srfi-43) > @@ -236,15 +237,41 @@ OPTIONS. Return the message ID of the first email sent." > (display (get-string-all port)) > message-id))))) > > +(define (reply-email-headers issue-number) > + "Return an association list of email headers when replying to > +ISSUE-NUMBER." > + (let ((messages > + (assoc-ref > + (assoc-ref > + (graphql-http-get (graphql-endpoint) > + `(document > + (query (#(issue #:number ,issue-number) > + (messages (from name address) > + date))))) > + "issue") > + "messages"))) > + ;; When sending email to an issue, we Cc all issue participants. > + ;; TODO: Also add an In-Reply-To header. > + `((cc . ,(delete-duplicates > + (map (lambda (message) > + (let ((from (assoc-ref message "from"))) > + (string-append (assoc-ref from "name") > + " <" (assoc-ref from "address") ">"))) > + (vector->list messages))))))) > + > (define (send-email patches) > "Send PATCHES via email." > (if (current-issue-number) > ;; If an issue is current, send patches to that issue's email > ;; address. > - (git-send-email (string-append (number->string (current-issue-number)) > - "@" > - (client-config 'debbugs-host)) > - patches) > + (let ((issue-number (current-issue-number))) > + (git-send-email (string-append (number->string issue-number) > + "@" > + (client-config 'debbugs-host)) > + patches > + (map (cut string-append "--cc=" <>) > + (assq-ref (reply-email-headers issue-number) > + 'cc)))) I was thinking looking at this, with X-Debbugs-Cc headers now being added automatically by Git for members of a team, there could be duplication between X-Debbugs-Cc and the participannts retrieved from the messages above. To ensure participants do not receive duplicate replies, it'd be probably best to stick to using X-Debbugs-Cc with all interactions with Debbugs; this way a duplicate header is (hopefully) ignored by Debbugs itself. Does that make sense?
Hi Maxim, > I was thinking looking at this, with X-Debbugs-Cc headers now being > added automatically by Git for members of a team, there could be > duplication between X-Debbugs-Cc and the participannts retrieved from > the messages above. > > To ensure participants do not receive duplicate replies, it'd be > probably best to stick to using X-Debbugs-Cc with all interactions with > Debbugs; this way a duplicate header is (hopefully) ignored by Debbugs > itself. > > Does that make sense? Yes, that makes sense! I will send a new patchset. According to https://www.debian.org/Bugs/Reporting#xcc , if we want to send copies to multiple addresses, we should put them in as a comma-separated list in the same X-Debbugs-Cc header. teams.scm seems to be putting in multiple X-Debbugs-Cc headers. Maybe we should change that? I see mumi as being more widely usable than just for Guix. For example, I wish to use it for skribilo, another Guile project that uses the Debbugs issue tracker. So, in the future, if and when applicable, we might want to move some of teams.scm's functionality into `mumi send-email'. Regards, Arun
> Yes, that makes sense! I will send a new patchset. According to > https://www.debian.org/Bugs/Reporting#xcc , if we want to send copies to > multiple addresses, we should put them in as a comma-separated list in > the same X-Debbugs-Cc header. teams.scm seems to be putting in multiple > X-Debbugs-Cc headers. Maybe we should change that? I just realized that using comma-separated lists calls for the functionality of teams.scm to be moved into `mumi send-email'. If teams.scm and `mumi send-email' remain separate, they will each produce a separate comma-separated list of X-Debbugs-Cc addresses, and we will be back to square one with multiple X-Debbugs-Cc headers. WDYT? What is the best way forward?
Hi Arun, Arun Isaac <arunisaac@systemreboot.net> writes: >> Yes, that makes sense! I will send a new patchset. According to >> https://www.debian.org/Bugs/Reporting#xcc , if we want to send copies to >> multiple addresses, we should put them in as a comma-separated list in >> the same X-Debbugs-Cc header. teams.scm seems to be putting in multiple >> X-Debbugs-Cc headers. Maybe we should change that? > > I just realized that using comma-separated lists calls for the > functionality of teams.scm to be moved into `mumi send-email'. If > teams.scm and `mumi send-email' remain separate, they will each produce > a separate comma-separated list of X-Debbugs-Cc addresses, and we will > be back to square one with multiple X-Debbugs-Cc headers. > > WDYT? What is the best way forward? If you meant that it's mumi instead of git that should call etc/teams.scm, it makes sense. I'm not sure the functionality of teams.scm proper should be moved wholesale into mumi, as it's useful outside of mumi (for plain git users, say). Mumi could invoke etc/teams.scm to produce the list of team members for the changes involved, add any missing collaborators retrieved from the message data to the set (avoiding duplicates), then format the 'X-Debbugs-CC' header with comma-separated values. It should then invoke git with the '--no-header-cmd' option to avoid teams.scm being called again. Note that our teams.scm script currently generate distinct X-Debbugs-CC header for each participant. The Debbugs copy used by GNU is the one hosted at [0], which doesn't mention the comma-separated requirement/suggestion, so I think that's currently OK, although I'm not too confident in my reading of the 'process' Perl script [1]. At any rate it'd be easy to adjust in teams.scm. The email specification mentions that some special fields such as To and Cc should be separated by commas, but says nothing about custom fields [2], so in my opinion both forms should be supported by Debbugs (and probably are, although it's hard to say without trying). [0] https://gitlab.com/npostavs/debbugs/-/tree/gnu-reconstruction?ref_type=heads [1] https://gitlab.com/npostavs/debbugs/-/blob/gnu-reconstruction/scripts/process [2] https://datatracker.ietf.org/doc/html/rfc5322#section-3.6
Hi Maxim, > If you meant that it's mumi instead of git that should call > etc/teams.scm, it makes sense. I'm not sure the functionality of > teams.scm proper should be moved wholesale into mumi, as it's useful > outside of mumi (for plain git users, say). I see your point about keeping things working for plain git users. Makes sense. > Mumi could invoke etc/teams.scm to produce the list of team members for > the changes involved, add any missing collaborators retrieved from the > message data to the set (avoiding duplicates), then format the > 'X-Debbugs-CC' header with comma-separated values. It should then > invoke git with the '--no-header-cmd' option to avoid teams.scm being > called again. mumi invoking etc/teams.scm is not so nice since that means coupling mumi to the specific repository layout of guix. This reduces its generality to other projects, say skribilo. Maybe, let's just keep multiple X-Debbugs-Cc headers for now. Let mumi and teams.scm be unaware of each other for now. We can revisit the question of coupling them later. > Note that our teams.scm script currently generate distinct X-Debbugs-CC > header for each participant. The Debbugs copy used by GNU is the one > hosted at [0], which doesn't mention the comma-separated > requirement/suggestion, so I think that's currently OK, although I'm not > too confident in my reading of the 'process' Perl script [1]. At any > rate it'd be easy to adjust in teams.scm. > > The email specification mentions that some special fields such as To and > Cc should be separated by commas, but says nothing about custom fields [2], > so in my opinion both forms should be supported by Debbugs (and probably > are, although it's hard to say without trying). Fair enough. Let's keep multiple X-Debbugs-Cc headers and hope for the best! :-) A new patchset follows. The only difference between this patchset and the previous one is that we set X-Debbugs-Cc headers instead of Cc headers. Regards, Arun
Hi Arun, Arun Isaac <arunisaac@systemreboot.net> writes: > Hi Maxim, > >> If you meant that it's mumi instead of git that should call >> etc/teams.scm, it makes sense. I'm not sure the functionality of >> teams.scm proper should be moved wholesale into mumi, as it's useful >> outside of mumi (for plain git users, say). > > I see your point about keeping things working for plain git users. Makes > sense. > >> Mumi could invoke etc/teams.scm to produce the list of team members for >> the changes involved, add any missing collaborators retrieved from the >> message data to the set (avoiding duplicates), then format the >> 'X-Debbugs-CC' header with comma-separated values. It should then >> invoke git with the '--no-header-cmd' option to avoid teams.scm being >> called again. > > mumi invoking etc/teams.scm is not so nice since that means coupling > mumi to the specific repository layout of guix. This reduces its > generality to other projects, say skribilo. Perhaps it could use the value of `git config sendemail.headerCmd` to figure out which script already produces X-Debbugs-CC headers, if any. > Maybe, let's just keep multiple X-Debbugs-Cc headers for now. Let mumi > and teams.scm be unaware of each other for now. We can revisit the > question of coupling them later. OK, sounds good to me.
> Perhaps it could use the value of `git config sendemail.headerCmd` to > figure out which script already produces X-Debbugs-CC headers, if any. That's a good idea! We can revisit this later. >> Maybe, let's just keep multiple X-Debbugs-Cc headers for now. Let mumi >> and teams.scm be unaware of each other for now. We can revisit the >> question of coupling them later. > > OK, sounds good to me. Pushed, thanks!
diff --git a/mumi/client.scm b/mumi/client.scm index 2750836..f0e4321 100644 --- a/mumi/client.scm +++ b/mumi/client.scm @@ -18,6 +18,7 @@ (define-module (mumi client) #:use-module (rnrs io ports) + #:use-module (srfi srfi-1) #:use-module (srfi srfi-19) #:use-module (srfi srfi-26) #:use-module (srfi srfi-43) @@ -236,15 +237,41 @@ OPTIONS. Return the message ID of the first email sent." (display (get-string-all port)) message-id))))) +(define (reply-email-headers issue-number) + "Return an association list of email headers when replying to +ISSUE-NUMBER." + (let ((messages + (assoc-ref + (assoc-ref + (graphql-http-get (graphql-endpoint) + `(document + (query (#(issue #:number ,issue-number) + (messages (from name address) + date))))) + "issue") + "messages"))) + ;; When sending email to an issue, we Cc all issue participants. + ;; TODO: Also add an In-Reply-To header. + `((cc . ,(delete-duplicates + (map (lambda (message) + (let ((from (assoc-ref message "from"))) + (string-append (assoc-ref from "name") + " <" (assoc-ref from "address") ">"))) + (vector->list messages))))))) + (define (send-email patches) "Send PATCHES via email." (if (current-issue-number) ;; If an issue is current, send patches to that issue's email ;; address. - (git-send-email (string-append (number->string (current-issue-number)) - "@" - (client-config 'debbugs-host)) - patches) + (let ((issue-number (current-issue-number))) + (git-send-email (string-append (number->string issue-number) + "@" + (client-config 'debbugs-host)) + patches + (map (cut string-append "--cc=" <>) + (assq-ref (reply-email-headers issue-number) + 'cc)))) (match patches ;; If it's a single patch, send it to the patch email address ;; and be done with it diff --git a/tests/client.scm b/tests/client.scm index 94c8c5d..f0ff34e 100644 --- a/tests/client.scm +++ b/tests/client.scm @@ -90,6 +90,8 @@ called with." (lambda () (with-variables (list (cons (var@@ (mumi client) current-issue-number) (const 12345)) + (cons (var@@ (mumi client) reply-email-headers) + (const '((cc)))) client-config-stub do-not-poll-server-for-issue-number) (cut (@@ (mumi client) send-email) @@ -116,6 +118,28 @@ called with." (lambda () (with-variables (list (cons (var@@ (mumi client) current-issue-number) (const 12345)) + (cons (var@@ (mumi client) reply-email-headers) + (const '((cc)))) + client-config-stub + do-not-poll-server-for-issue-number) + (cut (@@ (mumi client) send-email) + (list "foo.patch"))))))) + +(test-equal "send patch to existing issue and Cc other participants" + '(("git" "send-email" + "--to=12345@example.com" + "--cc=John Doe <jdoe@machine.example>" + "--cc=Mary Smith <mary@example.net>" + "foo.patch")) + (map (match-lambda + ((command _) command)) + (trace-calls (var@@ (mumi client) call-with-input-pipe) + (lambda () + (with-variables (list (cons (var@@ (mumi client) current-issue-number) + (const 12345)) + (cons (var@@ (mumi client) reply-email-headers) + (const `((cc "John Doe <jdoe@machine.example>" + "Mary Smith <mary@example.net>")))) client-config-stub do-not-poll-server-for-issue-number) (cut (@@ (mumi client) send-email)
* mumi/client.scm: Import (srfi srfi-1). (reply-email-headers): New function. (send-email): Call reply-email-headers. * tests/client.scm ("send patches to existing issue", "send single patch to existing issue"): Stub reply-email-headers. ("send patch to existing issue and Cc other participants"): New test. --- mumi/client.scm | 35 +++++++++++++++++++++++++++++++---- tests/client.scm | 24 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-)