From: Tomi Ollila <tomi.ollila@iki.fi>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH] emacs: search: allow command line args as part of query
In-Reply-To: <1370359319-2140-1-git-send-email-markwalters1009@gmail.com>
References: <1370292776-24535-1-git-send-email-markwalters1009@gmail.com>
	<1370359319-2140-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.15.2+172~g0a61aef (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-unknown-linux-gnu)
Date: Tue, 04 Jun 2013 22:40:54 +0300
Message-ID: <m28v2p7jg9.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Tue, Jun 04 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> This allows command line arguments for notmuch-search to be part of
> the query-string. 

Nice feature -- some comments below:

> The string must be of the form
> [:blank:]*--cli-arguments -- query. I hope this doesn't clash with

One problem requiring trailing '--' noticed after reviewed -- if one
forgets that the whole string is going to be query string.

> xapian: I believe that queries shouldn't start with a "-".
> The cli-arguments must be arguments in a whitelist of arguments: this
> adds a slight maintenance burden but means we don't have to deal with
> users who passed "--format=text" or other incompatible options.
>
> Correctly parsed example queries are
> --sort=oldest-first -- tag:inbox
> --exclude=false -- from:fred
>
> Some options (currently only sort-order) we parse in emacs, the rest
> we just pass to the cli. In light testing it seems to work.
>
> A full custom parser would be nicer but at least here we are only parsing
> the non-query part of a string which is relatively simple: indeed we
> already do that in the c code.
>
> We could just implement the option for sort-order, but I thought for
> interface consistency making all the sensible options (sort-order
> exclude limit and offset) work was worth the extra hassle.
> ---
>
> This is a slight change to
> 1370292776-24535-1-git-send-email-markwalters1009@gmail.com The change
> is that we add a whitelist of allowed cli options; other options are
> removed and the user is warned (but the query is not aborted).
>
> One other tiny change is that a query starting with "[[:blank:]]*-- "
> is allowed: everything after the -- is part of the real qeury so if
> any strange query is accidentally being misparsed the user can prefix
> with "-- " and it will give the current behaviour.
>
> Best wishes
>
> Mark
>
>
>  emacs/notmuch-hello.el |    5 +++--
>  emacs/notmuch-lib.el   |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  emacs/notmuch.el       |   36 +++++++++++++++++++++++++-----------
>  3 files changed, 72 insertions(+), 13 deletions(-)
>
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index c1c6f4b..bcc1843 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -383,10 +383,11 @@ options will be handled as specified for
>  `notmuch-hello-insert-searches'."
>    (with-temp-buffer
>      (dolist (elem query-alist nil)
> -      (let ((count-query (if (consp (cdr elem))
> +      (let* ((full-count-query (if (consp (cdr elem))
>  			     ;; do we have a different query for the message count?
>  			     (third elem)
> -			   (cdr elem))))
> +			   (cdr elem)))
> +	     (count-query (car (notmuch-parse-query full-count-query))))
>  	(insert
>  	 (notmuch-hello-filtered-query count-query
>  				       (or (plist-get options :filter-count)
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 28f78e0..65c489e 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -189,6 +189,50 @@ user-friendly queries."
>    "Return a query that matches the message with id ID."
>    (concat "id:" (notmuch-escape-boolean-term id)))
>  
> +(defun notmuch-search-parse-sort-order (args oldest-first)
> +  (dolist (arg args nil)
> +    (when (equal arg "--sort=oldest-first")
> +      (setq oldest-first t))
> +    (when (equal arg "--sort=newest-first")
> +      (setq oldest-first nil)))
> +  (setq args (delete "--sort=oldest-first" args))
> +  (setq args (delete "--sort=newest-first" args))
> +  (cons oldest-first args))

If one gave --sort=oldest-first --sort=newest-first  oldest-first is
chosen instead of newest-first as both are removed from args -- that
should be pretty easy to fix by putting deletes inside whens.

> +(defvar notmuch-parse-option-whitelist
> +  '("^--sort=oldest-first$"
> +    "^--sort=newest-first$"
> +    "^--exclude=true$"
> +    "^--exclude=false$"
> +    "^--exclude=flag$"
> +    "^--limit=[0-9]*$"
> +    "^--offset=[0-9]*$"
> +    "^--$"))

is zero numbers of numbers for --limit & --offset good ([0-9]*) ?

> +(defun notmuch-parse-in-whitelist-p (arg)
> +  (let ((allowed nil))
> +    (dolist (opt notmuch-parse-option-whitelist nil)
> +      (setq allowed (or allowed (string-match-p opt arg))))
> +    allowed))
> +
> +(defun notmuch-parse-query (query)
> +  "Parse a query into a search and cli arguments
> +
> +Returns a list consisting of query followed by the cli-args (as a
> +list). If the string does not have cli-args then this will be nil."
> +
> +  (if (or (string-match "^[[:blank:]]*--.*? -- " query)
> +	  (string-match "^[[:blank:]]*-- " query))
> +      (let ((actual-query (substring query (match-end 0)))
> +	    (args (split-string (match-string 0 query) " " t)))

Should the whitespace matching in locations above be consistent:
like "^[[:blank:]]*--.*?[[:blank:]]--[[:blank:]]". Blank matches
space & tab (according to http://www.gnu.org/software/emacs/manual/html_node/elisp/Char-Classes.html#Char-Classes )

Hmm, learned a bit:

(split-string "  foo   bar  ") -> ("foo" "bar")
(split-string "  foo   bar  " " ") -> ("" "" "foo" "" "" "bar" "" "")
(split-string "  foo   bar  " " " t) -> ("foo" "bar")
(split-string "  foo   bar  " " +") -> ("" "foo" "bar" "")
(split-string "  foo   bar  " " +" t) -> ("foo" "bar")

-> ... (args (split-string (match-string 0 query) "[[:blank:]]" t)))

> +	(message "Parsing query")
> +	(dolist (arg args nil)
> +	  (unless (notmuch-parse-in-whitelist-p arg)
> +	    (setq args (delete arg args))
> +	    (message "Removing unknown option %s" arg)))

And finally, I'd suggest it is an error to encounter unknown
option and in that case operation is aborted.


Tomi


> +	(cons actual-query args))
> +    ;; no cli arguments
> +    (list query)))
>  ;;
>  
>  (defun notmuch-common-do-stash (text)
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 7994d74..6a4052e 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -255,6 +255,7 @@ For a mouse binding, return nil."
>    (notmuch-common-do-stash (notmuch-search-find-thread-id)))
>  
>  (defvar notmuch-search-query-string)
> +(defvar notmuch-search-query-args)
>  (defvar notmuch-search-target-thread)
>  (defvar notmuch-search-target-line)
>  (defvar notmuch-search-continuation)
> @@ -409,6 +410,7 @@ Complete list of currently available key bindings:
>    (interactive)
>    (kill-all-local-variables)
>    (make-local-variable 'notmuch-search-query-string)
> +  (make-local-variable 'notmuch-search-query-args)
>    (make-local-variable 'notmuch-search-oldest-first)
>    (make-local-variable 'notmuch-search-target-thread)
>    (make-local-variable 'notmuch-search-target-line)
> @@ -897,7 +899,7 @@ PROMPT is the string to prompt with."
>  			      'notmuch-search-history nil nil)))))
>  
>  ;;;###autoload
> -(defun notmuch-search (&optional query oldest-first target-thread target-line continuation)
> +(defun notmuch-search (&optional query oldest-first target-thread target-line continuation cli-args)
>    "Run \"notmuch search\" with the given `query' and display results.
>  
>  If `query' is nil, it is read interactively from the minibuffer.
> @@ -909,13 +911,20 @@ Other optional parameters are used as follows:
>    target-line: The line number to move to if the target thread does not
>                 appear in the search results."
>    (interactive)
> -  (let* ((query (or query (notmuch-read-query "Notmuch search: ")))
> +  (let* ((full-query (or query (notmuch-read-query "Notmuch search: ")))
> +	 (parsed-query (notmuch-parse-query full-query))
> +	 (query (car parsed-query))
> +	 (cli-args (or cli-args (cdr parsed-query)))
> +	 (combined-order-query (notmuch-search-parse-sort-order cli-args oldest-first))
> +	 (oldest-first (car combined-order-query))
> +	 (cli-args (cdr combined-order-query))
>  	 (buffer (get-buffer-create (notmuch-search-buffer-title query))))
>      (switch-to-buffer buffer)
>      (notmuch-search-mode)
>      ;; Don't track undo information for this buffer
>      (set 'buffer-undo-list t)
>      (set 'notmuch-search-query-string query)
> +    (set 'notmuch-search-query-args cli-args)
>      (set 'notmuch-search-oldest-first oldest-first)
>      (set 'notmuch-search-target-thread target-thread)
>      (set 'notmuch-search-target-line target-line)
> @@ -928,13 +937,13 @@ Other optional parameters are used as follows:
>        (erase-buffer)
>        (goto-char (point-min))
>        (save-excursion
> -	(let ((proc (notmuch-start-notmuch
> +	(let ((proc (apply #'notmuch-start-notmuch
>  		     "notmuch-search" buffer #'notmuch-search-process-sentinel
>  		     "search" "--format=sexp" "--format-version=1"
> -		     (if oldest-first
> +		     (if notmuch-search-oldest-first
>  			 "--sort=oldest-first"
>  		       "--sort=newest-first")
> -		     query))
> +		     (append cli-args (list query))))
>  	      ;; Use a scratch buffer to accumulate partial output.
>  	      ;; This buffer will be killed by the sentinel, which
>  	      ;; should be called no matter how the process dies.
> @@ -957,9 +966,10 @@ same relative position within the new buffer."
>  	(oldest-first notmuch-search-oldest-first)
>  	(target-thread (notmuch-search-find-thread-id 'bare))
>  	(query notmuch-search-query-string)
> -	(continuation notmuch-search-continuation))
> +	(continuation notmuch-search-continuation)
> +	(cli-args notmuch-search-query-args))
>      (notmuch-kill-this-buffer)
> -    (notmuch-search query oldest-first target-thread target-line continuation)
> +    (notmuch-search query oldest-first target-thread target-line continuation cli-args)
>      (goto-char (point-min))))
>  
>  (defcustom notmuch-poll-script nil
> @@ -1024,18 +1034,22 @@ search."
>    (set 'notmuch-search-oldest-first (not notmuch-search-oldest-first))
>    (notmuch-search-refresh-view))
>  
> -(defun notmuch-search-filter (query)
> +(defun notmuch-search-filter (full-query)
>    "Filter the current search results based on an additional query string.
>  
>  Runs a new search matching only messages that match both the
>  current search results AND the additional query string provided."
>    (interactive (list (notmuch-read-query "Filter search: ")))
> -  (let ((grouped-query (if (string-match-p notmuch-search-disjunctive-regexp query)
> +  (let* ((parsed-query (notmuch-parse-query full-query))
> +	 (query (car parsed-query))
> +	 (grouped-query (if (string-match-p notmuch-search-disjunctive-regexp query)
>  			   (concat "( " query " )")
> -			 query)))
> +			 query))
> +	 (extra-cli-args (cdr parsed-query))
> +	 (cli-args (append notmuch-search-query-args extra-cli-args)))
>      (notmuch-search (if (string= notmuch-search-query-string "*")
>  			grouped-query
> -		      (concat notmuch-search-query-string " and " grouped-query)) notmuch-search-oldest-first)))
> +		      (concat notmuch-search-query-string " and " grouped-query)) notmuch-search-oldest-first nil nil nil cli-args)))
>  
>  (defun notmuch-search-filter-by-tag (tag)
>    "Filter the current search results based on a single tag.
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch