From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH] emacs: help: bugfix
In-Reply-To: <1384076682-16872-1-git-send-email-markwalters1009@gmail.com>
References: <1383932419-12533-1-git-send-email-markwalters1009@gmail.com>
	<1384076682-16872-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.16+154~g96c0ce2 (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Tue, 12 Nov 2013 13:47:36 -0500
Message-ID: <87ppq54fuf.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Sun, 10 Nov 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Hi
>
> David found a bug in the this remap/help series. He has a global
> keybinding of "C-c s" for notmuch-search and this causes help in
> tree-mode to hang.
>
> I have mostly diagnosed this: the problem comes that all the construct
> help routines are inside a string-match/replace-match pair. Somewhere
> in these routines the match-data is being stomped on (but I have to
> admit I am not sure where).
>
> In any case putting the construct help routines inside a
> save-match-data seems to fix it.
>
> This version is a bit ugly: I am not sure of the best way to deal with
> the save-match-data macro. (I think it is best to have it round
> everything that happens between finding the match and replacing the
> match to avoid anything similar in future).
>
> This applies on top of the parent series.
>
> Any comments gratefully received!
>
> Best wishes
>
> Mark
>
> ---
>  emacs/notmuch-lib.el |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 7b8acb3..e98e073 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -305,13 +305,16 @@ prefix argument.  PREFIX and TAIL are used internally."
>  (defun notmuch-substitute-command-keys (doc)
>    "Like `substitute-command-keys' but with documentation, not function names."
>    (let ((beg 0))
> -    (while (string-match "\\\\{\\([^}[:space:]]*\\)}" doc beg)
> -      (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
> -	     (keymap (symbol-value (intern keymap-name)))
> -	     (ua-keys (where-is-internal 'universal-argument keymap t))
> -	     (desc-alist (notmuch-describe-keymap keymap ua-keys keymap))
> -	     (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
> -	     (desc (mapconcat #'identity desc-list "\n")))
> +    (while (string-match "\\\\{\\([^}[:space:]]*\\)}" doc beg) ;; matches \{not-space}
> +      (let ((desc
> +	     (save-match-data
> +	       (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
> +		      (keymap (symbol-value (intern keymap-name)))
> +		      (ua-keys (where-is-internal 'universal-argument keymap t))
> +		      (desc-alist (notmuch-describe-keymap keymap ua-keys keymap))
> +		      (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
> +		      (desc (mapconcat #'identity desc-list "\n")))
> +		 desc))))

Oof, what an annoying place to lose the match data.  My one suggestion
would be to put (mapconcat #'identity desc-list "\n") as the body of the
inner let, rather than binding `desc' and then immediately evaluating to
`desc'.  That would remove some redundancy, and it's already clear from
the outer let that the result of the mapconcat is named `desc'.

>  	(setq doc (replace-match desc 1 1 doc)))
>        (setq beg (match-end 0)))
>      doc))
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch