From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <kyle@kyleam.com>
Received: from mp0 ([2001:41d0:2:aacc::])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	by ms12 with LMTPS
	id QEDXORonvGEUKgAAsNZ9tg
	(envelope-from <kyle@kyleam.com>)
	for <larch@inbox.kyleam.com>; Fri, 17 Dec 2021 05:58:50 +0000
Received: from out2.migadu.com ([2001:41d0:2:aacc::])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	by mp0 with LMTPS
	id eHEONRonvGEgeAAA1q6Kng
	(envelope-from <kyle@kyleam.com>)
	for <larch@inbox.kyleam.com>; Fri, 17 Dec 2021 05:58:50 +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=1639720730;
	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=e5gChUzKDT+MuqqC3IoEDcjh9ktRMghw6dVtr+aoO1k=;
	b=zeGJiKrIh5em31N7Pjjzc2UAw/xnSmgvxT6KYWKSUwPkPZLxCYdAVeGeTpqJbKXUZ5jDz3
	KoAYJFFefccmHbxY7c4qd4dHU4H+QvSC9ILq5njOVlpFBG/Q7WZQkIyH0HBFz81qQ8lEA+
	73M7o6N7dE2jQ5VAdCb4Au70tH/Gd9nrXwpmpWht00Tq1zbWG5XSuUl/wBmLk64AM/tERq
	bsCEevpNrUBDh3Kd6mfCa46hH9vplw8hvHu6SB2m3IaeFnIpe6Wa/Y4DejIip92VEeJa05
	54+mOF7xT6oqkW2VwCaF2GB9zKRLWrDv81IJj3q9zVGjt4jRps8/Veh9gqSkXw==
From: Kyle Meyer <kyle@kyleam.com>
To: sourcehut@relevant-information.com
Cc: piem@inbox.kyleam.com
Subject: Re: [PATCH] Add ability to edit patches before applying them
In-Reply-To: <20211216200504.31619-1-sourcehut@relevant-information.com>
References: <87lf0q6ckn.fsf@kyleam.com>
 <20211216200504.31619-1-sourcehut@relevant-information.com>
Date: Fri, 17 Dec 2021 00:58:49 -0500
Message-ID: <878rwjwz52.fsf@kyleam.com>
MIME-Version: 1.0
Content-Type: text/plain
X-Migadu-Flow: FLOW_OUT
X-Migadu-Auth-User: kyleam.com
X-TUID: i1OPo3ZwFZZ6

sourcehut@relevant-information.com writes:

> From: Leo <sourcehut@relevant-information.com>
>
> Sometimes it is necessary to edit patches before applying them.  These changes
> allows you to do that by providing a new command `piem-edit` that
> shows the buffer that is prepared by `piem-am-ready-mbox` to the user.
> ---
> Alright, here is the patch!

Thanks, I don't have time tonight to play around with this, but this is
looking good.  Here are some comments from a quick read.

>  piem.el | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/piem.el b/piem.el
> index afe6b7a..668f282 100644
> --- a/piem.el
> +++ b/piem.el
> @@ -900,6 +900,40 @@ (defun piem-am-read-worktree (coderepo branch)
>            (concat (file-name-nondirectory fname) "-"
>                    (replace-regexp-in-string "/" "-" branch))))))
>  
> +(defvar-local piem-edit-patch--coderepo nil)
> +
> +(defun piem-edit ()
> +  "Edit an am-ready mbox before feeding it to `git am'."
> +  (interactive)
> +  (pcase-let ((`(,mbox . ,_)

The ignored return value above should be passed as the format argument
to piem-am.

> +               (or (piem-am-ready-mbox)

I like the idea of using piem-am-ready-mbox, but it generates a hidden
buffer name (due to the leading space), which is inconvenient/confusing
for a user-facing buffer.  Perhaps piem-am-ready-mbox should gain an
optional argument to allow the buffer name to be specified and a
dedicated name should be used for this buffer ("*piem-edit-patch*"?).

> +                   (user-error
> +                    "Could not find am-ready mbox for current buffer")))
> +              (coderepo (piem-inbox-coderepo)))
> +    (progn

superfluous progn

> +      (switch-to-buffer mbox)

I'd prefer pop-to-buffer unless there's a specific reason to go with
switch-to-buffer.

> +      (piem-edit-patch-mode)
> +      (setq piem-edit-patch--coderepo coderepo))))
> +
> +(defun piem-edit-patch-am ()
> +  "Apply the patch that is currently edited."
> +  (interactive)
> +  (piem-am (current-buffer)
> +       "mbox"
> +       (piem-extract-mbox-info (current-buffer))
> +       piem-edit-patch--coderepo))

piem-edit-patch-am should probably take care of killing the buffer
(piem-am only does that for interactive calls).

> +(defvar piem-edit-patch-mode-map
> +  (let ((map (make-sparse-keymap)))
> +    (define-key map (kbd "C-c C-c") #'piem-edit-patch-am)
> +    map)
> +  "Keymap for editing patches with piem.")
> +
> +(define-derived-mode piem-edit-patch-mode text-mode  "piem-edit-patch"

extra space after text-mode

> +  "Major mode for editing patches with piem."
> +  :group 'piem
> +  :after-hook (buffer-enable-undo))

Hmm, I don't see a reason for after-hook here versus the more standard
call in BODY.

And very much a nitpick, but I'd like if this code came after piem-am.