From: Jani Nikula <jani@nikula.org>
To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH] emacs: add stash support for git send-email command line
In-Reply-To: <m2h9yn6zf1.fsf@dme.org>
References: <1414525138-838-1-git-send-email-jani@nikula.org>
	<m2h9yn6zf1.fsf@dme.org>
User-Agent: Notmuch/0.18.2+148~g4214adf (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-pc-linux-gnu)
Date: Wed, 29 Oct 2014 22:45:05 +0200
Message-ID: <87y4ryzju6.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Wed, 29 Oct 2014, David Edmondson <dme@dme.org> wrote:
> On Tue, Oct 28 2014, Jani Nikula wrote:
>> Stash From/To/Cc as --to/--to/--cc, respectively, and Message-Id as
>> --in-reply-to, suitable for pasting to git send-email command line.
>> ---
>>  emacs/notmuch-show.el | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index a9974826e824..328c93ba0584 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -1274,6 +1274,7 @@ reset based on the original query."
>>      (define-key map "t" 'notmuch-show-stash-to)
>>      (define-key map "l" 'notmuch-show-stash-mlarchive-link)
>>      (define-key map "L" 'notmuch-show-stash-mlarchive-link-and-go)
>> +    (define-key map "G" 'notmuch-show-stash-git-send-email)
>>      (define-key map "?" 'notmuch-subkeymap-help)
>>      map)
>>    "Submap for stash commands")
>> @@ -2125,6 +2126,30 @@ the user (see `notmuch-show-stash-mlarchive-link-alist')."
>>    (notmuch-show-stash-mlarchive-link mla)
>>    (browse-url (current-kill 0 t)))
>>  
>> +(defun notmuch-show-stash-git-helper (addresses prefix)
>> +  "Escape, trim, and add PREFIX to each address in list of ADDRESSES."
>> +  (when addresses
>> +    (mapconcat (lambda (x)
>> +		 (concat prefix "\""
>> +			 ;; escape double-quotes
>> +			 (replace-regexp-in-string
>> +			  "\"" "\\\\\""
>> +			  ;; trim leading and trailing spaces
>> +			  (replace-regexp-in-string
>> +			   "\\(^ *\\| *$\\)" ""
>> +			   x)) "\" "))
>> +	       addresses "")))
>
> This doesn't seem quite right. You leave a spurious trailing
> space. Maybe that's because you need it in the following function to
> separate the from/to/cc elements? That kind of interaction between the
> two functions is icky.

Agreed, thanks for pointing it out.

> There's no need to test `addresses' at the head of the list - mapconcat
> is fine with nil.
>
> How about:
>
> (defun notmuch-show-stash-git-helper (addresses prefix)
>   "Escape, trim, and add PREFIX to each address in list of ADDRESSES."
>   (mapconcat (lambda (x)
> 	       (concat prefix "\""
> 		       ;; escape double-quotes
> 		       (replace-regexp-in-string
> 			"\"" "\\\\\""
> 			;; trim leading and trailing spaces
> 			(replace-regexp-in-string
> 			 "\\(^ *\\| *$\\)" ""
> 			 x)) "\""))
> 	     addresses " "))
>
> This would remove the trailing space, so...
>
>> +
>> +(defun notmuch-show-stash-git-send-email ()
>> +  "Copy From/To/Cc/Message-Id of current message to kill-ring in a form suitable for pasting to git send-email command line."
>> +  (interactive)
>> +  (notmuch-common-do-stash
>> +   (concat
>> +    (notmuch-show-stash-git-helper (message-tokenize-header (notmuch-show-get-from)) "--to=")
>> +    (notmuch-show-stash-git-helper (message-tokenize-header (notmuch-show-get-to)) "--to=")
>> +    (notmuch-show-stash-git-helper (message-tokenize-header (notmuch-show-get-cc)) "--cc=")
>> +    (concat "--in-reply-to=\"" (notmuch-show-get-message-id t) "\""))))
>
> ...this would have to use something like:
>
>   (mapconcat 'identity (list
> 			(notmuch-show-stash-git-helper (message-tokenize-header (notmuch-show-get-from)) "--to=")
> 			(notmuch-show-stash-git-helper (message-tokenize-header (notmuch-show-get-to)) "--to=")
> 			(notmuch-show-stash-git-helper (message-tokenize-header (notmuch-show-get-cc)) "--cc=")
> 			(concat "--in-reply-to=\"" (notmuch-show-get-message-id t) "\""))
> 	     "")
>
> to separate the chunks (untested).

The last "" has to be " " to separate the elements, but this brings
another small wrinkle: if one of the headers is missing, typically Cc:,
it will be nil in the list, and mapconcat adds spaces both sides of
that, i.e. double space. Any ideas how to fix that?

> Could you avoid the double-quote quoting by using single quotes inside
> the strings?

The addresses may contain both single quotes and double quotes, so
escaping either of them seems like the only option.

> Do the leading and trailing spaces really matter?

Does aesthetically displeasing count? Because message-tokenize-header
splits using "," but headers typically have ", " between addresses, the
end result will practically always have --to=" user@example.com" without
the trimming.

Thanks for the helpful review; I'm not much of a lisp coder...

BR,
Jani.


>
> (Domo: Look, I managed to write 'separate', twice!)
>
>> +
>>  ;; Interactive part functions and their helpers
>>  
>>  (defun notmuch-show-generate-part-buffer (message-id nth)
>> -- 
>> 2.1.1
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch