[PATCH 1/2] test: add emacs show mode test for toggling display of multipart/alternative
Jameson Graef Rollins writes:
> +diff OUTPUT.{text,html} >OUTPUT.diff
> +cat +7,9c7,10
> +< [ text/html (not shown) ]
> +< [ text/plain ]
> +< This is the text/plain part of a multipart/alternative.
> +---
> +> [ text/html ]
> +> This is the text/html part of a multipart/alternative.
> +>
> +> [ text/plain (not shown) ]
> +EOF
Hmm, old-school diff. I guess this (instead of unified diff) is so that
if the diff isn't like this, then the diff of the diffs are clearly
readable? :)
git am complains at me because this patch introduces trailing whitespace
(to mimic, of course, the output of diff which will have trailing
whitespace as well). I would love for you to write this file using some
other technique which would not require putting trailing whitespace in
the test file.
Additionally the patch is stale, perhaps because my patch that moved some tests
to emacs-show was just pushed too.
Ethan
[PATCH v2 3/3] test: conform to content length, encoding fields
Peter Wang writes:
> Update tests to expect content-length and content-transfer-encoding
> fields in show --format=json output, for leaf parts with omitted body
> content.
These three patches all look fine to me, except for the following
problem.
> diff --git a/test/json b/test/json
> index ac8fa8e..8ce2e8a 100755
> --- a/test/json
> +++ b/test/json
> @@ -45,7 +45,7 @@ emacs_deliver_message \
> (insert \"Message-ID: <$id>\n\")"
> output=$(notmuch show --format=json "id:$id")
> filename=$(notmuch search --output=files "id:$id")
> -test_expect_equal_json "$output" "[[[{\"id\": \"$id\", \"match\": true,
> \"excluded\": false, \"filename\": \"$filename\", \"timestamp\": 946728000,
> \"date_relative\": \"2000-01-01\", \"tags\": [\"inbox\"], \"headers\":
> {\"Subject\": \"$subject\", \"From\": \"Notmuch Test Suite notmuchmail.org>\", \"To\": \"test_suite at notmuchmail.org\", \"Date\":
> \"Sat, 01 Jan 2000 12:00:00 +\"}, \"body\": [{\"id\": 1,
> \"content-type\": \"multipart/mixed\", \"content\": [{\"id\": 2,
> \"content-type\": \"text/plain\", \"content\": \"This is a test message with
> inline attachment with a filename\"}, {\"id\": 3, \"content-type\":
> \"application/octet-stream\", \"filename\": \"README\"}]}]}, ["
> +test_expect_equal_json "$output" "[[[{\"id\": \"$id\", \"match\": true,
> \"excluded\": false, \"filename\": \"$filename\", \"timestamp\": 946728000,
> \"date_relative\": \"2000-01-01\", \"tags\": [\"inbox\"], \"headers\":
> {\"Subject\": \"$subject\", \"From\": \"Notmuch Test Suite notmuchmail.org>\", \"To\": \"test_suite at notmuchmail.org\", \"Date\":
> \"Sat, 01 Jan 2000 12:00:00 +\"}, \"body\": [{\"id\": 1,
> \"content-type\": \"multipart/mixed\", \"content\": [{\"id\": 2,
> \"content-type\": \"text/plain\", \"content\": \"This is a test message with
> inline attachment with a filename\"}, {\"id\": 3, \"content-type\":
> \"application/octet-stream\", \"content-length\": 12392,
> \"content-transfer-encoding\": \"base64\", \"filename\": \"README\"}]}]},
> ["
This test fails for me. You're encoding the content-length of
test/README. test/README certainly hasn't changed in the last six months
so that seems like a reasonable thing to do... except then why is it
12392 on your machine, and 12380 on mine? I don't object to using
test/README as a simple file to test with, but then you certainly
shouldn't hard-code its length. You could pipe test/README through
base64 and then through wc -c to get an accurate length, but for my
machine a newline gets appended by base64 I think, and it gives me
12381.
I'm tagging this patch moreinfo but you would have my +1 if you fix
this.
Ethan
[PATCH 1/4] show: indicate length of omitted body content (json)
Peter Wang writes: > If a leaf part's body content is omitted, return the content length in > --format=json output. This information may be used by the consumer, > e.g. to decide whether to download a large attachment over a slow link. It looks like this patch series was thoroughly reviewed and then obsoleted by the series in id:"1344428872-12374-1-git-send-email-novalazy at gmail.com". I'm therefore marking it notmuch::obsolete, and will review the other patch set. Ethan
[PATCH v2 (Draft)] emacs: split async json parser into utility function
Mark Walters writes: > Split out the json parser into a utility function. > --- > > Most of this patch is code movement: but I don't see how to arrange the > patch to show that. Hi! This looks like a straightforward patch and if it will make notmuch-pick more efficient, I'm in favor. I tagged this patch moreinfo because David Bremner's suggestions that you expand on the docstrings for notmuch-json-parser and notmuch-json-state are good ideas. I'd also propose that you split this patch into two patches -- one that pulls out the variables and performs the renames, and the other which breaks out the code into its own function. This should make the code movement more obvious. I haven't started full-time work yet so if you would like me to do this, I can ;) Based on David Bremner's feedback that it might be a good idea to have a commit message that explains exactly what is code movement and isn't, here's my proposal for a commit message. Split out the json parser into a utility function. This patch breaks out a chunk of notmuch-search-process-filter, with the following changes: - notmuch-search-json-parser becomes notmuch-json-parser. - notmuch-search-parser-state becomes notmuch-json-state. We also rearrange the json-error case but are careful to always call error-function in the results buffer. Ethan
[PATCH] emacs: functions to import sender or recipient into BBDB
Daniel Bergey writes: > From a show buffer, bbdb/notmuch-snarf-from imports the sender into > bbdb. bbdb/notmuch-snarf-to attempts to import all recipients. BBDB > displays a buffer with each contact; C-g displays the next contact, or > returns to the notmuch-show buffer. > > This is my first notmuch patch. Comments very welcome. Hi! > emacs/notmuch-show.el | 28 I don't think this belongs in notmuch-show. My first inclination is that this should go into a new file contrib/notmuch-bbdb.el (assuming there's no other notmuch-bbdb integration stuff floating around). > 1 file changed, 28 insertions(+) > > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el > index 6335d45..3bc1da0 100644 > --- a/emacs/notmuch-show.el > +++ b/emacs/notmuch-show.el > @@ -1895,6 +1895,34 @@ the user (see > `notmuch-show-stash-mlarchive-link-alist')." > (button-get button :notmuch-filename) > (button-get button :notmuch-content-type))) > > +;; bbdb interaction functions, awaiting user keybindings > + > +(defun bbdb/snarf-between-commas () > + ; What about names written "Surname, First M" ? Most comments in emacslisp start with two semicolons. I do think more sophisticated parsing is necessary. If you're lucky, somebody else already has a library to parse email addresses in this form. > + (goto-char (point-min)) I'm not crazy about this. It's probably fine for something limited to bbdb users (especially since bbdb-snarf uses a very similar technique), but I think the better approach here is to just take a region and go from region-beginning and region-end. > + (let ((comma (point))) > +(while (search-forward "," nil "end") The third argument of search-forward is NOERROR. I don't understand what the value "end" means. The help says "Optional third argument, if t..." > + (bbdb-snarf-region comma (point)) > + (setq comma (point))) > +(bbdb-snarf-region comma (point)) ; last entry > + )) Doesn't this cause snarf the comma into any of those entries? It seems like point starts before the first entry but then goes before each comma. Obviously this wouldn't be here if it didn't work. I thought bbdb-snarf handled this kind of thing, but it doesn't. Could you explain this? Ethan
[PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t
Ethan Glasser-Camp writes: > It looks like you have better wording for patch 4/8 so I'd like to see > you resend it. > > I'm marking patches 3, 4, and 7 as moreinfo. Please resubmit! It turns out that patch 4 already has a v2 in the thread, but I didn't see it due to some kind of selective blindness. It would be nice if nmbug had grouped it as part of the same patch series. I'm marking the old patch 4/8 obsolete. Only patch 3 and 7 remain. Ethan
[PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
Mark Walters writes:
> Some messages are sent as multipart/alternative but the alternatives
> contain different information. This allows the user to cycle which
> part to view. By default this is bound to 'W'.
> ---
>
> This version at least uses the notmuch escaping for message-id which
> makes me a bit happier: it probably doesn't have any nasty security
> flaws. I do still feel that the lisp is a bit ugly though.
For what it's worth, I don't feel that this code is horrible. It seems
like there remain design decisions to be made about how notmuch show
"ought" to handle multipart/alternatives, but I can at least comment on
this code.
First, the use of a plist looks fine to me because most of the time it's
going to have length 0. At most it will have one entry per message -- a
few hundred. So I'm not worried about efficiency concerns.
> (defcustom notmuch-show-stash-mlarchive-link-alist
>'(("Gmane" . "http://mid.gmane.org/;)
> ("MARC" . "http://marc.info/?i=;)
> @@ -536,9 +540,19 @@ message at DEPTH in the current thread."
>
> (defun notmuch-show-insert-part-multipart/alternative (msg part content-type
> nth depth declared-type)
>(notmuch-show-insert-part-header nth declared-type content-type nil)
> - (let ((chosen-type (car (notmuch-multipart/alternative-choose
> (notmuch-show-multipart/*-to-list part
> - (inner-parts (plist-get part :content))
> - (start (point)))
> + (let* ((chosen-nth (or (lax-plist-get
> notmuch-show-message-multipart/alternative-display-part
> + (notmuch-id-to-query (plist-get msg
> :id))) 0))
> + (chosen-type (nth chosen-nth
> + (notmuch-multipart/alternative-choose
> (notmuch-show-multipart/*-to-list part
> + (inner-parts (plist-get part :content))
> + (start (point)))
Changing let to let* makes me the slightest bit uneasy, although I'm not
sure I could explain why.
It would be nice if you could wrap the manipulation of
notmuch-show-message-multipart/alternative-display-part in functions,
ideally with names that are shorter than the variable they
manipulate. Specifically, I think the definition of chosen-nth (which is
almost repeated below) could be its own function, something like
(notmuch-show-message-current-multipart msg), which could take a msg-id
or a plist and do the plist-get and id-to-query that you do here.
> +;; If we have run out of possible content-types restart from the
> beginning
> +(unless chosen-type
> + (setq chosen-type (car (notmuch-multipart/alternative-choose
> (notmuch-show-multipart/*-to-list part
> + (setq notmuch-show-message-multipart/alternative-display-part
> + (lax-plist-put
> notmuch-show-message-multipart/alternative-display-part
> +(notmuch-id-to-query (plist-get msg :id)) 0)))
> +
> ;; This inserts all parts of the chosen type rather than just one,
> ;; but it's not clear that this is the wrong thing to do - which
> ;; should be chosen if there are more than one that match?
> @@ -942,6 +956,16 @@ message at DEPTH in the current thread."
>"Not processing cryptographic MIME parts."))
>(notmuch-show-refresh-view))
>
> +(defun notmuch-show-cycle-message-multipart ()
> + "Cycle which part to display of a multipart messageToggle the display of
> non-matching messages."
This docstring is broken.
> + (interactive)
> + (let* ((msg-id (notmuch-show-get-message-id))
> + (next-part (1+ (or (lax-plist-get
> notmuch-show-message-multipart/alternative-display-part msg-id) 0
> +(setq notmuch-show-message-multipart/alternative-display-part
> + (lax-plist-put notmuch-show-message-multipart/alternative-display-part
> msg-id next-part))
> +(message "Cycling multipart/alternative for current message")
> +(notmuch-show-refresh-view)))
Maybe move the reset-and-go-back-to-zero behavior to this function
instead of in the display function. This opens you up to weird
situations if one of the multipart/alternatives should disappear from a
message or if some other function should change the alternative on
display for a given message, but both of these seem unlikely to me..
Ethan
[PATCH] Add NEWS item for multipart/alternative toggle
Users who relied on notmuch-show-all-multipart/alternative-parts might need to know that it is now buffer-local. Signed-off-by: Ethan Glasser-Camp --- Hi! I'm trying to figure out the status of this patch series, which seems to have fallen through the cracks. It looks like Jani's solution exists and works, though it isn't perfect. I would like to get Jani's solution applied until someone who is sufficiently motivated decides to work on fancy cycle-multipart-message things. It seems like the last hurdle is that Mark would like Jani to put the buffer-localization of notmuch-show-all-multipart/alternative-parts in NEWS. Here's that. However, it also seems like there was a disagreement about design direction -- should multiple alternative parts be toggled so you can see them all at once, or cycled so you can see one at a time? See id:"87d32yadl5.fsf at qmul.ac.uk". I'm for the code that works now which is Jani's patch. NEWS | 11 +++ 1 file changed, 11 insertions(+) diff --git a/NEWS b/NEWS index 2b50ba3..fc9b50a 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,14 @@ +Emacs Interface +--- + +Display of multipart messages + + Displaying multipart/alternative parts has been made a toggle, the + same way as indentation and message decryption. + notmuch-show-all-multipart/alternative-parts is now buffer-local, so + if you have set it using setq, you will have to update that to use + setq-default. + Notmuch 0.14 (2012-08-20) = -- 1.7.9.5
[PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t
On Wed, 17 Oct 2012, Adrien Bustany wrote: > The code of the patches in unchanged, but the formatting issues are now > hopefully fixed. Hi Adrien, please check at what version flush and reopen have been introduced to xapian. If they are new-ish (I don't know, didn't have the time to check), please add appropriate #ifdefs. [1] lays the groundwork for this. We'll also need to decide what is the minimum xapian version required in general, i.e. features earlier than that don't need conditional compilation. BR, Jani. [1] id:"1350487737-32058-2-git-send-email-bgamari.foss at gmail.com"
[PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t
Peter Wang writes:
> Add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t so that it can
> cover all four values of search --exclude in the cli.
This series looks good to me. It's a nice clean up and a nice new
feature. Patches all apply.
However, I'm getting test failures like:
FAIL Search, exclude "deleted" messages from message search --exclude=false
--- excludes.3.expected 2012-10-19 04:45:06.900518377 +
+++ excludes.3.output 2012-10-19 04:45:06.900518377 +
@@ -1,2 +1,2 @@
-id:msg-001 at notmuch-test-suite
id:msg-002 at notmuch-test-suite
+id:msg-001 at notmuch-test-suite
FAIL Search, don't exclude "deleted" messages when --exclude=flag specified
--- excludes.7.expected 2012-10-19 04:45:07.004518378 +
+++ excludes.7.output 2012-10-19 04:45:07.004518378 +
@@ -1,2 +1,2 @@
-thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox
unread)
thread:XXX 2001-01-05 [1/2] Notmuch Test Suite; Not deleted reply
(deleted inbox unread)
+thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox
unread)
FAIL Search, don't exclude "deleted" messages from search if not configured
--- excludes.8.expected 2012-10-19 04:45:07.028518377 +
+++ excludes.8.output 2012-10-19 04:45:07.028518377 +
@@ -1,2 +1,2 @@
-thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox
unread)
thread:XXX 2001-01-05 [2/2] Notmuch Test Suite; Deleted (deleted
inbox unread)
+thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox
unread)
In other words, threads and messages are coming up out of order. I'm not
sure of the right way to fix this. If you would like me to try sticking
"| sort" here and there in the tests I will do so. I'm not sure if the
test suite is guaranteed to scan messages in a certain order.
Mark Walters wrote in
id:"1340198947-29370-5-git-send-email-novalazy at gmail.com" that he
thought patch 1/8 seemed more intrusive than he liked. Maybe I just have
a higher standard for "intrusive" than he does ;) but I thought it was
fine.
It looks like you have better wording for patch 4/8 so I'd like to see
you resend it.
> - if (query->omit_excluded != NOTMUCH_EXCLUDE_FALSE)
> + if (query->omit_excluded == NOTMUCH_EXCLUDE_TRUE ||
> + query->omit_excluded == NOTMUCH_EXCLUDE_ALL)
> + {
> final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,
>final_query, exclude_query);
> - else {
> + } else {
"House style" is to not put braces around one-line then-clauses. This is
the only place where you did that.
I'm marking patches 3, 4, and 7 as moreinfo. Please resubmit!
Ethan
[PATCH 3/7] go: Allow notmuch objects to be garbage collected
Adrien Bustany writes: > This makes notmuch appropriately free the underlying notmuch C objects > when garbage collecting their Go wrappers. To make sure we don't break > the underlying links between objects (for example, a notmuch_messages_t > being GC'ed before a notmuch_message_t belonging to it), we add for each > wraper struct a pointer to the owner object (Go objects with a reference > pointing to them don't get garbage collected). Hi Adrien! This whole series is marked moreinfo, but I don't think that's just. It looks like there were some unresolved issues about reference tracking and garbage collection, and some suggestions to use the C values of enums instead of regenerating them with iota, but there's definitely valid code that I assume would be useful if anyone ever wanted to write in Go ;). Are you figuring to clean this series up? This comment should s/wraper/wrapper/. Ethan
Notmuch scripts (again), now with more usenet
ccx at webprojekty.cz writes: > Hello, for quite some time my set of scripts just lied in my repo and > waited for polish before release. So tonight I finally managed to update > the docs, remove old stuff, rewrite some unfortunate things etc. > > One notable addition is slrn2maildir script which can convert NNTP > spool, eg. gmane mailing lists or blogs, as fetched by slrnpull to > maildir format. This way you can follow plethora of mailing lists > without subscribing, any blog that publishes full atom/rss feed or > usenet newsgroup. > > For details see the readme: > http://webprojekty.cz/ccx/loggerhead/zmuch/view/head:/README > or check out the code: > bzr branch http://webprojekty.cz/ccx/bzr/zmuch > > I hope it's now in the form acceptable for inclusion to contrib. Hi! Sorry about the delay, but I'm going through the patch queue now and it seems like this branch is just completely gone. I get 502 Bad Gateway errors when I follow the first link. Did it move or is there a problem with your site? Ethan
[PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t
Adrien Bustany writes: > The code of the patches in unchanged, but the formatting issues are now > hopefully fixed. These look fine to me, and they're pretty trivial. Ethan
Re: [PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t
On Wed, 17 Oct 2012, Adrien Bustany adr...@bustany.org wrote: The code of the patches in unchanged, but the formatting issues are now hopefully fixed. Hi Adrien, please check at what version flush and reopen have been introduced to xapian. If they are new-ish (I don't know, didn't have the time to check), please add appropriate #ifdefs. [1] lays the groundwork for this. We'll also need to decide what is the minimum xapian version required in general, i.e. features earlier than that don't need conditional compilation. BR, Jani. [1] id:1350487737-32058-2-git-send-email-bgamari.f...@gmail.com ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] Add NEWS item for multipart/alternative toggle
Users who relied on notmuch-show-all-multipart/alternative-parts might need to know that it is now buffer-local. Signed-off-by: Ethan Glasser-Camp et...@betacantrips.com --- Hi! I'm trying to figure out the status of this patch series, which seems to have fallen through the cracks. It looks like Jani's solution exists and works, though it isn't perfect. I would like to get Jani's solution applied until someone who is sufficiently motivated decides to work on fancy cycle-multipart-message things. It seems like the last hurdle is that Mark would like Jani to put the buffer-localization of notmuch-show-all-multipart/alternative-parts in NEWS. Here's that. However, it also seems like there was a disagreement about design direction -- should multiple alternative parts be toggled so you can see them all at once, or cycled so you can see one at a time? See id:87d32yadl5@qmul.ac.uk. I'm for the code that works now which is Jani's patch. NEWS | 11 +++ 1 file changed, 11 insertions(+) diff --git a/NEWS b/NEWS index 2b50ba3..fc9b50a 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,14 @@ +Emacs Interface +--- + +Display of multipart messages + + Displaying multipart/alternative parts has been made a toggle, the + same way as indentation and message decryption. + notmuch-show-all-multipart/alternative-parts is now buffer-local, so + if you have set it using setq, you will have to update that to use + setq-default. + Notmuch 0.14 (2012-08-20) = -- 1.7.9.5 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
Mark Walters markwalters1...@gmail.com writes: Some messages are sent as multipart/alternative but the alternatives contain different information. This allows the user to cycle which part to view. By default this is bound to 'W'. --- This version at least uses the notmuch escaping for message-id which makes me a bit happier: it probably doesn't have any nasty security flaws. I do still feel that the lisp is a bit ugly though. For what it's worth, I don't feel that this code is horrible. It seems like there remain design decisions to be made about how notmuch show ought to handle multipart/alternatives, but I can at least comment on this code. First, the use of a plist looks fine to me because most of the time it's going to have length 0. At most it will have one entry per message -- a few hundred. So I'm not worried about efficiency concerns. (defcustom notmuch-show-stash-mlarchive-link-alist '((Gmane . http://mid.gmane.org/;) (MARC . http://marc.info/?i=;) @@ -536,9 +540,19 @@ message at DEPTH in the current thread. (defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type) (notmuch-show-insert-part-header nth declared-type content-type nil) - (let ((chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part - (inner-parts (plist-get part :content)) - (start (point))) + (let* ((chosen-nth (or (lax-plist-get notmuch-show-message-multipart/alternative-display-part + (notmuch-id-to-query (plist-get msg :id))) 0)) + (chosen-type (nth chosen-nth + (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part + (inner-parts (plist-get part :content)) + (start (point))) Changing let to let* makes me the slightest bit uneasy, although I'm not sure I could explain why. It would be nice if you could wrap the manipulation of notmuch-show-message-multipart/alternative-display-part in functions, ideally with names that are shorter than the variable they manipulate. Specifically, I think the definition of chosen-nth (which is almost repeated below) could be its own function, something like (notmuch-show-message-current-multipart msg), which could take a msg-id or a plist and do the plist-get and id-to-query that you do here. +;; If we have run out of possible content-types restart from the beginning +(unless chosen-type + (setq chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part + (setq notmuch-show-message-multipart/alternative-display-part + (lax-plist-put notmuch-show-message-multipart/alternative-display-part +(notmuch-id-to-query (plist-get msg :id)) 0))) + ;; This inserts all parts of the chosen type rather than just one, ;; but it's not clear that this is the wrong thing to do - which ;; should be chosen if there are more than one that match? @@ -942,6 +956,16 @@ message at DEPTH in the current thread. Not processing cryptographic MIME parts.)) (notmuch-show-refresh-view)) +(defun notmuch-show-cycle-message-multipart () + Cycle which part to display of a multipart messageToggle the display of non-matching messages. This docstring is broken. + (interactive) + (let* ((msg-id (notmuch-show-get-message-id)) + (next-part (1+ (or (lax-plist-get notmuch-show-message-multipart/alternative-display-part msg-id) 0 +(setq notmuch-show-message-multipart/alternative-display-part + (lax-plist-put notmuch-show-message-multipart/alternative-display-part msg-id next-part)) +(message Cycling multipart/alternative for current message) +(notmuch-show-refresh-view))) Maybe move the reset-and-go-back-to-zero behavior to this function instead of in the display function. This opens you up to weird situations if one of the multipart/alternatives should disappear from a message or if some other function should change the alternative on display for a given message, but both of these seem unlikely to me.. Ethan ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t
Ethan Glasser-Camp ethan.glasser.c...@gmail.com writes: It looks like you have better wording for patch 4/8 so I'd like to see you resend it. I'm marking patches 3, 4, and 7 as moreinfo. Please resubmit! It turns out that patch 4 already has a v2 in the thread, but I didn't see it due to some kind of selective blindness. It would be nice if nmbug had grouped it as part of the same patch series. I'm marking the old patch 4/8 obsolete. Only patch 3 and 7 remain. Ethan ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] emacs: functions to import sender or recipient into BBDB
Daniel Bergey ber...@alum.mit.edu writes: From a show buffer, bbdb/notmuch-snarf-from imports the sender into bbdb. bbdb/notmuch-snarf-to attempts to import all recipients. BBDB displays a buffer with each contact; C-g displays the next contact, or returns to the notmuch-show buffer. This is my first notmuch patch. Comments very welcome. Hi! emacs/notmuch-show.el | 28 I don't think this belongs in notmuch-show. My first inclination is that this should go into a new file contrib/notmuch-bbdb.el (assuming there's no other notmuch-bbdb integration stuff floating around). 1 file changed, 28 insertions(+) diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el index 6335d45..3bc1da0 100644 --- a/emacs/notmuch-show.el +++ b/emacs/notmuch-show.el @@ -1895,6 +1895,34 @@ the user (see `notmuch-show-stash-mlarchive-link-alist'). (button-get button :notmuch-filename) (button-get button :notmuch-content-type))) +;; bbdb interaction functions, awaiting user keybindings + +(defun bbdb/snarf-between-commas () + ; What about names written Surname, First M u...@server.tld? Most comments in emacslisp start with two semicolons. I do think more sophisticated parsing is necessary. If you're lucky, somebody else already has a library to parse email addresses in this form. + (goto-char (point-min)) I'm not crazy about this. It's probably fine for something limited to bbdb users (especially since bbdb-snarf uses a very similar technique), but I think the better approach here is to just take a region and go from region-beginning and region-end. + (let ((comma (point))) +(while (search-forward , nil end) The third argument of search-forward is NOERROR. I don't understand what the value end means. The help says Optional third argument, if t... + (bbdb-snarf-region comma (point)) + (setq comma (point))) +(bbdb-snarf-region comma (point)) ; last entry + )) Doesn't this cause snarf the comma into any of those entries? It seems like point starts before the first entry but then goes before each comma. Obviously this wouldn't be here if it didn't work. I thought bbdb-snarf handled this kind of thing, but it doesn't. Could you explain this? Ethan ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 (Draft)] emacs: split async json parser into utility function
Mark Walters markwalters1...@gmail.com writes: Split out the json parser into a utility function. --- Most of this patch is code movement: but I don't see how to arrange the patch to show that. Hi! This looks like a straightforward patch and if it will make notmuch-pick more efficient, I'm in favor. I tagged this patch moreinfo because David Bremner's suggestions that you expand on the docstrings for notmuch-json-parser and notmuch-json-state are good ideas. I'd also propose that you split this patch into two patches -- one that pulls out the variables and performs the renames, and the other which breaks out the code into its own function. This should make the code movement more obvious. I haven't started full-time work yet so if you would like me to do this, I can ;) Based on David Bremner's feedback that it might be a good idea to have a commit message that explains exactly what is code movement and isn't, here's my proposal for a commit message. Split out the json parser into a utility function. This patch breaks out a chunk of notmuch-search-process-filter, with the following changes: - notmuch-search-json-parser becomes notmuch-json-parser. - notmuch-search-parser-state becomes notmuch-json-state. We also rearrange the json-error case but are careful to always call error-function in the results buffer. Ethan ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 1/4] show: indicate length of omitted body content (json)
Peter Wang noval...@gmail.com writes: If a leaf part's body content is omitted, return the content length in --format=json output. This information may be used by the consumer, e.g. to decide whether to download a large attachment over a slow link. It looks like this patch series was thoroughly reviewed and then obsoleted by the series in id:1344428872-12374-1-git-send-email-noval...@gmail.com. I'm therefore marking it notmuch::obsolete, and will review the other patch set. Ethan ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 3/3] test: conform to content length, encoding fields
Peter Wang noval...@gmail.com writes:
Update tests to expect content-length and content-transfer-encoding
fields in show --format=json output, for leaf parts with omitted body
content.
These three patches all look fine to me, except for the following
problem.
diff --git a/test/json b/test/json
index ac8fa8e..8ce2e8a 100755
--- a/test/json
+++ b/test/json
@@ -45,7 +45,7 @@ emacs_deliver_message \
(insert \Message-ID: $id\n\)
output=$(notmuch show --format=json id:$id)
filename=$(notmuch search --output=files id:$id)
-test_expect_equal_json $output [[[{\id\: \$id\, \match\: true,
\excluded\: false, \filename\: \$filename\, \timestamp\: 946728000,
\date_relative\: \2000-01-01\, \tags\: [\inbox\], \headers\:
{\Subject\: \$subject\, \From\: \Notmuch Test Suite
test_su...@notmuchmail.org\, \To\: \test_su...@notmuchmail.org\,
\Date\: \Sat, 01 Jan 2000 12:00:00 +\}, \body\: [{\id\: 1,
\content-type\: \multipart/mixed\, \content\: [{\id\: 2,
\content-type\: \text/plain\, \content\: \This is a test message with
inline attachment with a filename\}, {\id\: 3, \content-type\:
\application/octet-stream\, \filename\: \README\}]}]}, [
+test_expect_equal_json $output [[[{\id\: \$id\, \match\: true,
\excluded\: false, \filename\: \$filename\, \timestamp\: 946728000,
\date_relative\: \2000-01-01\, \tags\: [\inbox\], \headers\:
{\Subject\: \$subject\, \From\: \Notmuch Test Suite
test_su...@notmuchmail.org\, \To\: \test_su...@notmuchmail.org\,
\Date\: \Sat, 01 Jan 2000 12:00:00 +\}, \body\: [{\id\: 1,
\content-type\: \multipart/mixed\, \content\: [{\id\: 2,
\content-type\: \text/plain\, \content\: \This is a test message with
inline attachment with a filename\}, {\id\: 3, \content-type\:
\application/octet-stream\, \content-length\: 12392,
\content-transfer-encoding\: \base64\, \filename\: \README\}]}]},
[
This test fails for me. You're encoding the content-length of
test/README. test/README certainly hasn't changed in the last six months
so that seems like a reasonable thing to do... except then why is it
12392 on your machine, and 12380 on mine? I don't object to using
test/README as a simple file to test with, but then you certainly
shouldn't hard-code its length. You could pipe test/README through
base64 and then through wc -c to get an accurate length, but for my
machine a newline gets appended by base64 I think, and it gives me
12381.
I'm tagging this patch moreinfo but you would have my +1 if you fix
this.
Ethan
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 1/2] test: add emacs show mode test for toggling display of multipart/alternative
Jameson Graef Rollins jroll...@finestructure.net writes:
+diff OUTPUT.{text,html} OUTPUT.diff
+cat EOF EXPECTED.diff
+7,9c7,10
+ [ text/html (not shown) ]
+ [ text/plain ]
+ This is the text/plain part of a multipart/alternative.
+---
+ [ text/html ]
+ This is the text/html part of a multipart/alternative.
+
+ [ text/plain (not shown) ]
+EOF
Hmm, old-school diff. I guess this (instead of unified diff) is so that
if the diff isn't like this, then the diff of the diffs are clearly
readable? :)
git am complains at me because this patch introduces trailing whitespace
(to mimic, of course, the output of diff which will have trailing
whitespace as well). I would love for you to write this file using some
other technique which would not require putting trailing whitespace in
the test file.
Additionally the patch is stale, perhaps because my patch that moved some tests
to emacs-show was just pushed too.
Ethan
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: random corpus generator, v3
da...@tethera.net writes:
This obsoletes the series at:
id:134431-4301-1-git-send-email-brem...@debian.org
Changes since v2:
- clean up new test-binaries and objects
- remove the set -o pipefail leftover from debugging. Possibly this
makes sense as a global setting, but in a seperate patch.
- add hex-escape to test/basic
- rebase against updated master.
Hi! This looks pretty good to me and I am for improving the test
infrastructure.
Some minor problems:
- Patch 2 doesn't apply; neither do patches 4 or 5, presumably due to changes
that weren't made due to patch 2.
- Commit message discipline: the subject line of patch 4 ends in a
period. Seperate is spelled by most people as separate, though I
would encourage you to buck the trend if you are so inclined.
- In patch 4:
+if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
+ _notmuch_message_add_term (message, type, mail);
+} else {
+ return NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
+}
Why not switch the branches? That is, check for private_status !=
NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND and return immediately?
- In patch 5:
+for (count = 0; count num_messages; count++) {
+ int j;
+ int num_tags = random () % (max_tags + 1);
+ int this_mid_len = random () % message_id_len + 1;
This looks odd. I'm pretty sure it's correct, but my brain keeps saying,
Why are there no parentheses on (message_id_len + 1)? Maybe just a
comment that message ids must be at least one character long, or the
ranges of values necessary for both of these variables.
Ethan
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
