> On Tue, Jun 04 2013, Mark Walters <markwalters1009 at gmail.com> wrote: > >> This allows command line arguments for notmuch-search to be part of >> the query-string. > > Nice feature -- some comments below: >
Thanks for the review! replies 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. One easy fix would be to require any query starting with --something to be prefixed by "-- ". We could also just assume the query starts if we see a term not prefixed with -- or having a "--" term. What do you think? >> 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 at 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. I think this one is ok as the delete is outside the dolist. >> +(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]*) ? You are quite right (ie + would be better). > >> +(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))) These are both clear improvements. >> + (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. I think this would be better. I am not quite sure how to implement it, and I wasn't sure what notmuch-hello should do when displaying counts of an "illegal" saved search. What do you think? Best wishes Mark > > 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 at notmuchmail.org >> http://notmuchmail.org/mailman/listinfo/notmuch