From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:863f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms12 with LMTPS id YIjBKGh7sWEmIQAAsNZ9tg (envelope-from ) for ; Thu, 09 Dec 2021 03:43:36 +0000 Received: from out1.migadu.com ([2001:41d0:2:863f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id oIW1JGh7sWFTHQAA1q6Kng (envelope-from ) for ; Thu, 09 Dec 2021 03:43:36 +0000 X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kyleam.com; s=key1; t=1639021416; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=9M9XFTQsDXGgFj8SWcekazfBg1VGuwsG8s9jaP2LEc0=; b=N71dqTAzM4j3fOG3ayLzeitnTu1i8WLhwUJGa/6865pQpUD1jp/0gpXzjrIoJSb71dm9bS XKPNoJxJuetbRgHJgEiKTsdVS3/z9wq9Q6CZS2wmekt9RCqDgONoyFNi2F3bXawj2RFAgL Ka9lkuh9NEBdWFfJQp7qQ9GoUiMWpRSq04mvZnrjdSbLxoV3++XdjwolfpZ1pEZ7lj6/bO wtVExeB/FYWGZeNthMFJIy4w9yHm5oesD1e3q2CWrDb2uck8WYxT7N2ByXbuD5yRFmLtov OkjUtTEQM25ufWEy/96Bt6wMzaxOq/0AVEDS3asWnGL1hF/JXS3RAqrdOxqSJA== From: Kyle Meyer To: sourcehut@relevant-information.com Cc: piem@inbox.kyleam.com Subject: Re: [PATCH 1/1] piem-notmuch-am-ready-mbox: Add option to edit patches before applying them In-Reply-To: <20211208160142.84085-2-sourcehut@relevant-information.com> References: <20211208160142.84085-1-sourcehut@relevant-information.com> <20211208160142.84085-2-sourcehut@relevant-information.com> Date: Wed, 08 Dec 2021 22:43:33 -0500 Message-ID: <87r1amfnoa.fsf@kyleam.com> MIME-Version: 1.0 Content-Type: text/plain X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: kyleam.com X-TUID: g08UvjH7TABU [ I left inline comments as I read, but there's a higher-level comment about an alternative design at the end, so you might prefer to look at that first. ] sourcehut@relevant-information.com writes: > From: The above will lead to Git repeating the email as the author name: sourcehut@relevant-information.com Are you okay with filling in a name here? (It of course doesn't have to be a real one.) Also, I think it'd be good to have some of the motivation you gave in your initial message here in the commit message. > --- > piem-notmuch.el | 40 +++++++++++++++++++++++++++++++++++----- > 1 file changed, 35 insertions(+), 5 deletions(-) > > diff --git a/piem-notmuch.el b/piem-notmuch.el > index 8b2a353..2ad4ace 100644 > --- a/piem-notmuch.el > +++ b/piem-notmuch.el > @@ -77,11 +77,35 @@ (defun piem-notmuch-mid-to-thread (mid) > "show" "--format=mbox" "--entire-thread=true" > (concat "id:" mid))))) > > +(defun piem-notmuch-show-view-mbox-message (arg) > + "View the mbox formatted version of the current message. > + > +Used for editing patches before applying them. > + > +With one universal prefix argument, view the entire thread instead. " Please drop the trailing space at the end of the docstring. > + (interactive "P") > + (let* ((id (notmuch-show-get-message-id)) > + (buf (get-buffer-create (concat "*notmuch-mbox-" id "*"))) I know this buffer name is following notmuch-show-view-raw-message (s/raw/mbox/), but, given the buffer's created by piem, I'd prefer to tack on a "piem-" in front of "notmuch-mbox". > + (inhibit-read-only t)) Even if this is a copy from the notmuch codebase, let's convert these tabs to spaces for consistency. > + (pop-to-buffer-same-window buf) > + (erase-buffer) > + (let ((coding-system-for-read 'no-conversion)) > + (if (equal '(4) arg) > + (call-process notmuch-command nil t nil "show" "--entire-thread=true" "--format=mbox" id) Please reflow this line to be under 80 chars. (Same comment applies to a line in piem-notmuch-am-ready-mbox.) > + (call-process notmuch-command nil t nil "show" "--format=mbox" id))) > + (goto-char (point-min)) > + (set-buffer-modified-p nil) > + (setq buffer-read-only nil) > + (view-buffer buf 'kill-buffer-if-not-modified))) Hmm, the goal is to tweak the patch, so I understand setting buffer-read-only to nil, but does that play well with View mode? I think using View pretty much assumes read-only operation, and, testing out a call to piem-notmuch-show-view-mbox-message, it looks like view-buffer will end up resetting buffer-read-only to t anyway. > + > + Please reduce this to a single space... > (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." > +message itself if it looks like a patch. If a corresponding ... and add two spaces after periods to match the Emacs project convention. > +*notmuch-mbox- buffer exists, use that instead of fetching from > +the notmuch database." > (when (derived-mode-p 'notmuch-show-mode) > (let* ((handle (piem-notmuch--with-current-message > (mm-dissect-buffer))) > @@ -90,10 +114,16 @@ (defun piem-notmuch-am-ready-mbox () > (if (= n-attachments 0) > (when (string-match-p piem-patch-subject-re > (notmuch-show-get-subject)) > - (let ((id (notmuch-show-get-message-id))) > - (lambda () > - (call-process notmuch-command nil t nil > - "show" "--format=mbox" id)))) > + (if-let ((buf (get-buffer (concat "*notmuch-mbox-" (notmuch-show-get-message-id) "*")))) > + (let ((str (with-current-buffer buf > + (buffer-substring-no-properties (point-min) > + (point-max))))) > + (lambda () > + (insert str))) > + (let ((id (notmuch-show-get-message-id))) > + (lambda () > + (call-process notmuch-command nil t nil > + "show" "--format=mbox" id))))) Okay, so stepping back, I believe the proposed workflow is * from a notmuch message, invoke piem-notmuch-show-view-mbox-message * make changes to that new buffer * switch back to the notmuch message buffer * call piem-am, and underneath piem-notmuch-am-ready-mbox will grab the intended mbox based on the message-ID-matched buffer name (Note: If the piem-notmuch-show-view-mbox-message caller used a prefix argument to dump the entire thread, they need to make sure to prune any non-patch messages in the second step above because piem-notmuch-am-ready-mbox will use the mbox as is, and it should return an am-ready mbox.) I wonder if we can redesign this a bit to more closely and transparently connect the "edit" and "am" parts. Something like: * from a notmuch message, invoke piem-notmuch-edit-message [*] (or whatever name) This commands puts the message (or perhaps thread) into a dedicated piem mode for editing, setting up any buffer-local variables piem needs for record-keeping/linking (e.g., what piem-inbox-coderepo returns in the notmuch message) and then ... * once the user is done editing, they can hit, say, 'C-c C-c' to call a wrapper command that feeds the mbox to piem-am (Perhaps there could also be a b4 variant.) What do you think? [*] And it might make sense to write the command in a way that it could eventually work with integration libraries too (e.g., Gnus), but tailoring things to notmuch initially is fine by me.