[PATCH 2/2] emacs: fix off-by-one error in notmuch-hello column alignment
Expected results for few tests are fixed, the relevant test is unmarked broken. --- emacs/notmuch-hello.el |2 +- test/emacs-hello |1 - .../notmuch-hello-new-section |2 +- .../notmuch-hello-section-with-empty |2 +- 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el index e9caade..28f39f1 100644 --- a/emacs/notmuch-hello.el +++ b/emacs/notmuch-hello.el @@ -461,7 +461,7 @@ Such a list can be computed with `notmuch-hello-query-counts'." ;; just insert `(- widest (length name))' spaces - the ;; column separator is included in the button if ;; `(equal widest (length name)'. - (widget-insert (make-string (max 1 + (widget-insert (make-string (max 0 (- widest (length name))) ? ) (setq count (1+ count)) diff --git a/test/emacs-hello b/test/emacs-hello index 9e5d045..be66ba4 100755 --- a/test/emacs-hello +++ b/test/emacs-hello @@ -45,7 +45,6 @@ test_emacs "(let ((notmuch-hello-sections test_expect_equal_file OUTPUT $EXPECTED/notmuch-hello-section-counts test_begin_subtest "Column alignment for tag/queries with long names" -test_subtest_known_broken tag=a-very-long-tag # length carefully calculated for 80 characters window width notmuch tag +$tag '*' test_emacs '(notmuch-hello) diff --git a/test/emacs.expected-output/notmuch-hello-new-section b/test/emacs.expected-output/notmuch-hello-new-section index c64d712..6a339aa 100644 --- a/test/emacs.expected-output/notmuch-hello-new-section +++ b/test/emacs.expected-output/notmuch-hello-new-section @@ -1,4 +1,4 @@ Test: [hide] - 52 inbox + 52 inbox diff --git a/test/emacs.expected-output/notmuch-hello-section-with-empty b/test/emacs.expected-output/notmuch-hello-section-with-empty index 8209fed..dc2568d 100644 --- a/test/emacs.expected-output/notmuch-hello-section-with-empty +++ b/test/emacs.expected-output/notmuch-hello-section-with-empty @@ -1,4 +1,4 @@ Test-with-empty: [hide] - 52 inbox + 52 inbox -- 1.7.9.1 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH 1/2] test: add broken test for long names in Emacs notmuch-hello view
Currently, the column alignment in Emacs notmuch-hello is broken for tags/queries with long names. --- test/emacs-hello |9 + .../emacs.expected-output/notmuch-hello-long-names | 18 ++ 2 files changed, 27 insertions(+), 0 deletions(-) create mode 100644 test/emacs.expected-output/notmuch-hello-long-names diff --git a/test/emacs-hello b/test/emacs-hello index b235e3a..9e5d045 100755 --- a/test/emacs-hello +++ b/test/emacs-hello @@ -44,4 +44,13 @@ test_emacs "(let ((notmuch-hello-sections (test-output))" test_expect_equal_file OUTPUT $EXPECTED/notmuch-hello-section-counts +test_begin_subtest "Column alignment for tag/queries with long names" +test_subtest_known_broken +tag=a-very-long-tag # length carefully calculated for 80 characters window width +notmuch tag +$tag '*' +test_emacs '(notmuch-hello) +(test-output)' +notmuch tag -$tag '*' +test_expect_equal_file OUTPUT $EXPECTED/notmuch-hello-long-names + test_done diff --git a/test/emacs.expected-output/notmuch-hello-long-names b/test/emacs.expected-output/notmuch-hello-long-names new file mode 100644 index 000..be6d2c5 --- /dev/null +++ b/test/emacs.expected-output/notmuch-hello-long-names @@ -0,0 +1,18 @@ + Welcome to notmuch. You have 52 messages. + +Saved searches: [edit] + + 52 inbox 52 unread + +Search: . + +All tags: [hide] + + 52 a-very-long-tag 52 inbox 52 unread + 4 attachment 7 signed + +Type a search query and hit RET to view matching threads. + Edit saved searches with the `edit' button. + Hit RET or click on a saved search or tag name to view matching threads. + `=' to refresh this screen. `s' to search messages. `q' to quit. + Customize this page. -- 1.7.9.1 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH 2/2] cli: make --entire-thread option notmuch-show.c INT_OR_BOOLEAN
Make the --entire-thread option notmuch-show.c
NOTMUCH_OPT_INT_OR_BOOLEAN. In particular this means a caller can turn
off entire-thread (by specifying --entire-thread=0) when format=json.
(This was not previously possible.)
---
notmuch-show.c | 10 --
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/notmuch-show.c b/notmuch-show.c
index 05d51b2..f0c640f 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -985,6 +985,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused
(char *argv[]))
const notmuch_show_format_t *format = &format_text;
notmuch_show_params_t params = { .part = -1 };
int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
+int entire_thread = -1;
notmuch_bool_t verify = FALSE;
notmuch_bool_t no_exclude = FALSE;
@@ -996,7 +997,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused
(char *argv[]))
{ "raw", NOTMUCH_FORMAT_RAW },
{ 0, 0 } } },
{ NOTMUCH_OPT_INT, ¶ms.part, "part", 'p', 0 },
- { NOTMUCH_OPT_BOOLEAN, ¶ms.entire_thread, "entire-thread", 't', 0 },
+ { NOTMUCH_OPT_INT_OR_BOOLEAN, &entire_thread, "entire-thread", 't', 0 },
{ NOTMUCH_OPT_BOOLEAN, ¶ms.decrypt, "decrypt", 'd', 0 },
{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
{ NOTMUCH_OPT_BOOLEAN, &no_exclude, "no-exclude", 'n', 0 },
@@ -1020,7 +1021,8 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
switch (format_sel) {
case NOTMUCH_FORMAT_JSON:
format = &format_json;
- params.entire_thread = TRUE;
+ if (entire_thread == -1)
+ entire_thread = 1;
break;
case NOTMUCH_FORMAT_TEXT:
format = &format_text;
@@ -1042,6 +1044,10 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
params.raw = TRUE;
break;
}
+/* Set default to not entire_thread; JSON case dealt with above */
+if (entire_thread == -1)
+ entire_thread = 0;
+params.entire_thread = notmuch_int_to_boolean (entire_thread);
if (params.decrypt || verify) {
#ifdef GMIME_ATLEAST_26
--
1.7.9.1
[PATCH 1/2] cli: Parsing. Add option NOTMUCH_OPT_INT_OR_BOOLEAN
Allow the option NOTMUCH_OPT_INT_OR_BOOLEAN for command line parsing
which accepts --verbose=3 and --verbose with the latter setting
verbose to 1. It also allows --verbose=0 so (with a little caller
support) user can turn off boolean options.
This means that extra options can be added to the command line
programs in a backwards compatible manner.
---
command-line-arguments.c | 29 +++--
command-line-arguments.h |3 +++
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/command-line-arguments.c b/command-line-arguments.c
index e711414..99e13c6 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -4,6 +4,22 @@
#include "error_util.h"
#include "command-line-arguments.h"
+/* A helper for parsing an int to a boolean */
+notmuch_bool_t
+notmuch_int_to_boolean (int i)
+{
+switch (i) {
+ case 1:
+ return TRUE;
+ case 0:
+ return FALSE;
+ default:
+ INTERNAL_ERROR ("Non-boolean value %d", i);
+ /* UNREACHED */
+ return FALSE;
+ }
+}
+
/*
Search the array of keywords for a given argument, assigning the
output variable to the corresponding value. Return FALSE if nothing
@@ -72,6 +88,7 @@ parse_option (const char *arg,
if (try->name && strncmp (arg, try->name, strlen (try->name)) == 0) {
char next = arg[strlen (try->name)];
const char *value= arg+strlen(try->name)+1;
+ enum notmuch_opt_type opt_type = try->opt_type;
char *endptr;
@@ -79,7 +96,14 @@ parse_option (const char *arg,
* delimiter, and a non-zero length value
*/
- if (try->opt_type != NOTMUCH_OPT_BOOLEAN) {
+ if (opt_type == NOTMUCH_OPT_INT_OR_BOOLEAN) {
+ if (next != 0)
+ opt_type = NOTMUCH_OPT_INT;
+ else
+ opt_type = NOTMUCH_OPT_BOOLEAN;
+ }
+
+ if (opt_type != NOTMUCH_OPT_BOOLEAN) {
if (next != '=' && next != ':') return FALSE;
if (value[0] == 0) return FALSE;
} else {
@@ -89,7 +113,7 @@ parse_option (const char *arg,
if (try->output_var == NULL)
INTERNAL_ERROR ("output pointer NULL for option %s", try->name);
- switch (try->opt_type) {
+ switch (opt_type) {
case NOTMUCH_OPT_KEYWORD:
return _process_keyword_arg (try, value);
break;
@@ -107,6 +131,7 @@ parse_option (const char *arg,
break;
case NOTMUCH_OPT_POSITION:
case NOTMUCH_OPT_END:
+ case NOTMUCH_OPT_INT_OR_BOOLEAN: /* should be dealt with above */
default:
INTERNAL_ERROR ("unknown or unhandled option type %d",
try->opt_type);
/*UNREACHED*/
diff --git a/command-line-arguments.h b/command-line-arguments.h
index de1734a..a2fc545 100644
--- a/command-line-arguments.h
+++ b/command-line-arguments.h
@@ -6,6 +6,7 @@
enum notmuch_opt_type {
NOTMUCH_OPT_END = 0,
NOTMUCH_OPT_BOOLEAN, /* --verbose */
+NOTMUCH_OPT_INT_OR_BOOLEAN,/* --verbose or --verbose=1 */
NOTMUCH_OPT_INT, /* --frob=8 */
NOTMUCH_OPT_KEYWORD, /* --format=raw|json|text */
NOTMUCH_OPT_STRING,/* --file=/tmp/gnarf.txt */
@@ -76,5 +77,7 @@ parse_position_arg (const char *arg,
int position_arg_index,
const notmuch_opt_desc_t* options);
+notmuch_bool_t
+notmuch_int_to_boolean (int i);
#endif
--
1.7.9.1
[PATCH 0/2] cli: Parsing. Add option NOTMUCH_OPT_INT_OR_BOOLEAN
The first patch adds a new command line parsing option NOTMUCH_OPT_INT_OR_BOOLEAN for command line parsing which accepts --verbose=3 and --verbose with the latter setting verbose to 1. It also allows --verbose=0 so (with a little caller support) the user can turn off boolean options. It also means that extra options can be added to the command line programs in a backwards compatible manner (e.g. if --verbose already exists we could add --verbose=2). The second patch uses this to make the --entire-thread option to notmuch-show a NOTMUCH_OPT_INT_OR_BOOLEAN. In particular this allows the caller to disable --entire-thread (with --entire-thread=0) when format=json. Best wishes Mark Mark Walters (2): cli: Parsing. Add option NOTMUCH_OPT_INT_OR_BOOLEAN cli: make --entire-thread option notmuch-show.c INT_OR_BOOLEAN command-line-arguments.c | 29 +++-- command-line-arguments.h |3 +++ notmuch-show.c | 10 -- 3 files changed, 38 insertions(+), 4 deletions(-) -- 1.7.9.1
Obsolete Patches
Hi There was a comment on irc about the needs review queue on nmbug status being large. I think there may be several patches on there that have been superseded or otherwise made obsolete. This makes choosing what to review harder. My guess is that it is very much easier for the author to know this than for David (B). Hence I would like to suggest that authors say when their patches should be removed from the queue. I have several patches in this category (in each case I just list the first patch of the series) id:"1328375350-10352-2-git-send-email-markwalters1009 at gmail.com" (superseded) id:"1328388317-20161-2-git-send-email-markwalters1009 at gmail.com" (Dmitry suggested a better solution) id:"1329296619-7463-2-git-send-email-markwalters1009 at gmail.com" (a later version has been pushed) id:"1329900529-16295-1-git-send-email-markwalters1009 at gmail.com" (various problems so should be removed; perhaps marked wip) id:"1330539189-16593-2-git-send-email-markwalters1009 at gmail.com" (as much as is relevant has been pushed: it's the revert exclude for 0.12 series) Best wishes Mark
Re: [PATCH 0/2] cli: Parsing. Add option NOTMUCH_OPT_INT_OR_BOOLEAN
Hi On Fri, 09 Mar 2012 02:48:35 +0200, Jani Nikula wrote: > On Thu, 8 Mar 2012 22:15:42 +, Mark Walters > wrote: > > The first patch adds a new command line parsing option > > NOTMUCH_OPT_INT_OR_BOOLEAN for command line parsing which accepts > > --verbose=3 and --verbose with the latter setting verbose to 1. It > > also allows --verbose=0 so (with a little caller support) the user can > > turn off boolean options. > > > > It also means that extra options can be added to the command line > > programs in a backwards compatible manner (e.g. if --verbose already > > exists we could add --verbose=2). > > > > The second patch uses this to make the --entire-thread option to > > notmuch-show a NOTMUCH_OPT_INT_OR_BOOLEAN. In particular this allows > > the caller to disable --entire-thread (with --entire-thread=0) when > > format=json. > > I'm afraid I find both of the patches a bit hacky. The first because > it's more about optional arguments to options than int-or-bool. The > second because it's more about detecting whether the user has provided > an option than int-or-bool. Yes to both of these (although I don't think of it has hacky). The second of these is the important consideration. > For booleans, I think the notmuch style would be to allow --foo=true and > --foo=false in addition to just --foo (which implies true) so you could > also disable booleans. I think this would be fairly simple to implement, > and would require no changes to option parser users. > > With --entire-thread the bigger problem is detecting whether the option > was specified by the user or not. It would be great if the option parser > could provide this information, but it might take a while to get > there... In the mean time, I think I would rather see a well commented > local hack here initializing the notmuch_bool_t params.entire_thread to > -1, and changing it to TRUE or FALSE if not already done so by > parse_arguments(). I think these need to be considered together. There are three important possibilities for a boolean option foo: 1) foo not mentioned 2) --foo=false and 3) --foo=true (--foo as an alias) but the parser only has a boolean to store this in. We could overload the boolean as you suggest (it is really an int so should be safe) but that does seem hacky. That was why I decided to make the parser set an int rather than a boolean. Since there are not very many OPT_BOOLEANs currently in the code I think my decision to allow general ints to be returned is a needless extension, but I do think we wish to allow the 3 states of UNSET, FALSE and TRUE. Otherwise we limit any future boolean options to always have the same default regardless of other settings. If people are happy with setting a notmuch_bool_t to -1 (for unset) then that is definitely the simplest option. Or if people think that default boolean options should not depend on other options then we can just add this hack for --entire-thread. Best wishes Mark ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v2 0/8] Rewrite mbox and raw show formats
On Tue, Mar 6, 2012 at 11:48, Austin Clements wrote: > v2 makes the new raw format support interior, as well as root and leaf > parts. > > Patches 1, 2, and 8 are new but simple. ?Patches 3 through 6 have not > changed. ?Patch 7 has obviously changed quite a bit. The whole series looks good to me.
Re: [PATCH v2 0/8] Rewrite mbox and raw show formats
On Tue, Mar 6, 2012 at 11:48, Austin Clements wrote: > v2 makes the new raw format support interior, as well as root and leaf > parts. > > Patches 1, 2, and 8 are new but simple. Patches 3 through 6 have not > changed. Patch 7 has obviously changed quite a bit. The whole series looks good to me. ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] mime_node_open: check if the file is in mbox format, and inform gmime.
From: David Bremner
It seems that it has always been an error to try to parse an mbox
format file with gmime without calling g_mime_parser_set_scan_from.
This change reads the first 5 bytes of the file, and if they are "From ",
declares the file to be an mbox.
---
This patch seems to fix the problem for me. I don't think the
performance impact should be too bad, but I didn't really test it.
mime-node.c | 21 +
1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index a95bdab..8939147 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -72,6 +72,8 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
mime_node_context_t *mctx;
mime_node_t *root;
notmuch_status_t status;
+char from_buf[6]; /* From space NULL */
+int is_mbox = 0;
root = talloc_zero (ctx, mime_node_t);
if (root == NULL) {
@@ -96,6 +98,23 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
goto DONE;
}
+if (fread (from_buf, 1, 5, mctx->file) != 5) {
+ fprintf (stderr, "Failed to read 5 bytes from %s: %s\n",
+filename, strerror (errno));
+ status = NOTMUCH_STATUS_FILE_ERROR;
+ goto DONE;
+}
+from_buf[5] = '\0';
+
+if (fseek (mctx->file, 0L, SEEK_SET)) {
+ fprintf (stderr, "Failed to rewind %s: %s\n",
+filename, strerror (errno));
+ status = NOTMUCH_STATUS_FILE_ERROR;
+ goto DONE;
+}
+
+is_mbox = (strcmp (from_buf, "From ") == 0);
+
mctx->stream = g_mime_stream_file_new (mctx->file);
if (!mctx->stream) {
fprintf (stderr, "Out of memory.\n");
@@ -111,7 +130,9 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
goto DONE;
}
+g_mime_parser_set_scan_from (mctx->parser, is_mbox);
mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
if (!mctx->mime_message) {
fprintf (stderr, "Failed to parse %s\n", filename);
status = NOTMUCH_STATUS_FILE_ERROR;
--
1.7.9.1
Re: [PATCH 0/2] cli: Parsing. Add option NOTMUCH_OPT_INT_OR_BOOLEAN
On Thu, 8 Mar 2012 22:15:42 +, Mark Walters wrote: > The first patch adds a new command line parsing option > NOTMUCH_OPT_INT_OR_BOOLEAN for command line parsing which accepts > --verbose=3 and --verbose with the latter setting verbose to 1. It > also allows --verbose=0 so (with a little caller support) the user can > turn off boolean options. > > It also means that extra options can be added to the command line > programs in a backwards compatible manner (e.g. if --verbose already > exists we could add --verbose=2). > > The second patch uses this to make the --entire-thread option to > notmuch-show a NOTMUCH_OPT_INT_OR_BOOLEAN. In particular this allows > the caller to disable --entire-thread (with --entire-thread=0) when > format=json. I'm afraid I find both of the patches a bit hacky. The first because it's more about optional arguments to options than int-or-bool. The second because it's more about detecting whether the user has provided an option than int-or-bool. For booleans, I think the notmuch style would be to allow --foo=true and --foo=false in addition to just --foo (which implies true) so you could also disable booleans. I think this would be fairly simple to implement, and would require no changes to option parser users. With --entire-thread the bigger problem is detecting whether the option was specified by the user or not. It would be great if the option parser could provide this information, but it might take a while to get there... In the mean time, I think I would rather see a well commented local hack here initializing the notmuch_bool_t params.entire_thread to -1, and changing it to TRUE or FALSE if not already done so by parse_arguments(). BR, Jani. > > Best wishes > > Mark > > Mark Walters (2): > cli: Parsing. Add option NOTMUCH_OPT_INT_OR_BOOLEAN > cli: make --entire-thread option notmuch-show.c INT_OR_BOOLEAN > > command-line-arguments.c | 29 +++-- > command-line-arguments.h |3 +++ > notmuch-show.c | 10 -- > 3 files changed, 38 insertions(+), 4 deletions(-) > > -- > 1.7.9.1 > > ___ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Parsing regression with gmime-2.6?
On 03/08/2012 04:32 PM, Jameson Graef Rollins wrote: > I actually agree with this position. mbox files are not proper email > messages, so if gmime does not explicitly support parsing them then we > really can't expect it to parse them. i think the point here is that gmime used to support parsing them and now it doesn't. So if you've got a pre-existing notmuch index (built from when gmime *did* parse them) and you're now running notmuch linked against a gmime that *doesn't* parse them, then notmuch search will produce references to messages that notmuch show can't produce output for. While i appreciate wanting to be sure that we're working with well-formed files, I don't have a good answer for what the right thing to do is, if gmime upstream agrees with your assessment of the situation. yuck, --dkg
Parsing regression with gmime-2.6?
On Tue, 06 Mar 2012 18:04:50 -0400, David Bremner wrote: > There seems to be something weird going on with gmime-2.6; maybe we > didn't catch some api change? This does seem to be a regression in gmime 2.6. I've reported the bug upstream, along with a simplified (non-notmuch) demonstration: https://bugzilla.gnome.org/show_bug.cgi?id=671680 We'll see what gmime's upstream has to say about it. As a devil's advocate, i could argue that a message in a maildir that starts with a "From " line isn't a proper e-mail message in the first place, and therefore gmime 2.6 is being more rigorously correct about what it accepts. In particular, if a user were to place a multi-message mbox file in their notmuch message store, i think that notmuch linked against 2.4 would happily index only the first message of it, and the rest of the message would be "hidden", whereas gmime 2.6 allows us to detect these failures and avoid indexing them directly. That said, i understand that this is probably not an entirely rare situation, and i lean toward the idea that gmime 2.4's behavior was actually the Right Thing. Also, I haven't been able to find any explicit documentation to indicate that the behavior change was a deliberate one. I'm happy to relay any helpful or constructive suggestions to the gmime upstream ticket, if folks don't want to deal with bugzilla directly. --dkg -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 965 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120308/191964c6/attachment.pgp>
[PATCH 2/2] cli: make --entire-thread option notmuch-show.c INT_OR_BOOLEAN
Make the --entire-thread option notmuch-show.c
NOTMUCH_OPT_INT_OR_BOOLEAN. In particular this means a caller can turn
off entire-thread (by specifying --entire-thread=0) when format=json.
(This was not previously possible.)
---
notmuch-show.c | 10 --
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/notmuch-show.c b/notmuch-show.c
index 05d51b2..f0c640f 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -985,6 +985,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused
(char *argv[]))
const notmuch_show_format_t *format = &format_text;
notmuch_show_params_t params = { .part = -1 };
int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
+int entire_thread = -1;
notmuch_bool_t verify = FALSE;
notmuch_bool_t no_exclude = FALSE;
@@ -996,7 +997,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused
(char *argv[]))
{ "raw", NOTMUCH_FORMAT_RAW },
{ 0, 0 } } },
{ NOTMUCH_OPT_INT, ¶ms.part, "part", 'p', 0 },
- { NOTMUCH_OPT_BOOLEAN, ¶ms.entire_thread, "entire-thread", 't', 0 },
+ { NOTMUCH_OPT_INT_OR_BOOLEAN, &entire_thread, "entire-thread", 't', 0 },
{ NOTMUCH_OPT_BOOLEAN, ¶ms.decrypt, "decrypt", 'd', 0 },
{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
{ NOTMUCH_OPT_BOOLEAN, &no_exclude, "no-exclude", 'n', 0 },
@@ -1020,7 +1021,8 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
switch (format_sel) {
case NOTMUCH_FORMAT_JSON:
format = &format_json;
- params.entire_thread = TRUE;
+ if (entire_thread == -1)
+ entire_thread = 1;
break;
case NOTMUCH_FORMAT_TEXT:
format = &format_text;
@@ -1042,6 +1044,10 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
params.raw = TRUE;
break;
}
+/* Set default to not entire_thread; JSON case dealt with above */
+if (entire_thread == -1)
+ entire_thread = 0;
+params.entire_thread = notmuch_int_to_boolean (entire_thread);
if (params.decrypt || verify) {
#ifdef GMIME_ATLEAST_26
--
1.7.9.1
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH 1/2] cli: Parsing. Add option NOTMUCH_OPT_INT_OR_BOOLEAN
Allow the option NOTMUCH_OPT_INT_OR_BOOLEAN for command line parsing
which accepts --verbose=3 and --verbose with the latter setting
verbose to 1. It also allows --verbose=0 so (with a little caller
support) user can turn off boolean options.
This means that extra options can be added to the command line
programs in a backwards compatible manner.
---
command-line-arguments.c | 29 +++--
command-line-arguments.h |3 +++
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/command-line-arguments.c b/command-line-arguments.c
index e711414..99e13c6 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -4,6 +4,22 @@
#include "error_util.h"
#include "command-line-arguments.h"
+/* A helper for parsing an int to a boolean */
+notmuch_bool_t
+notmuch_int_to_boolean (int i)
+{
+switch (i) {
+ case 1:
+ return TRUE;
+ case 0:
+ return FALSE;
+ default:
+ INTERNAL_ERROR ("Non-boolean value %d", i);
+ /* UNREACHED */
+ return FALSE;
+ }
+}
+
/*
Search the array of keywords for a given argument, assigning the
output variable to the corresponding value. Return FALSE if nothing
@@ -72,6 +88,7 @@ parse_option (const char *arg,
if (try->name && strncmp (arg, try->name, strlen (try->name)) == 0) {
char next = arg[strlen (try->name)];
const char *value= arg+strlen(try->name)+1;
+ enum notmuch_opt_type opt_type = try->opt_type;
char *endptr;
@@ -79,7 +96,14 @@ parse_option (const char *arg,
* delimiter, and a non-zero length value
*/
- if (try->opt_type != NOTMUCH_OPT_BOOLEAN) {
+ if (opt_type == NOTMUCH_OPT_INT_OR_BOOLEAN) {
+ if (next != 0)
+ opt_type = NOTMUCH_OPT_INT;
+ else
+ opt_type = NOTMUCH_OPT_BOOLEAN;
+ }
+
+ if (opt_type != NOTMUCH_OPT_BOOLEAN) {
if (next != '=' && next != ':') return FALSE;
if (value[0] == 0) return FALSE;
} else {
@@ -89,7 +113,7 @@ parse_option (const char *arg,
if (try->output_var == NULL)
INTERNAL_ERROR ("output pointer NULL for option %s", try->name);
- switch (try->opt_type) {
+ switch (opt_type) {
case NOTMUCH_OPT_KEYWORD:
return _process_keyword_arg (try, value);
break;
@@ -107,6 +131,7 @@ parse_option (const char *arg,
break;
case NOTMUCH_OPT_POSITION:
case NOTMUCH_OPT_END:
+ case NOTMUCH_OPT_INT_OR_BOOLEAN: /* should be dealt with above */
default:
INTERNAL_ERROR ("unknown or unhandled option type %d",
try->opt_type);
/*UNREACHED*/
diff --git a/command-line-arguments.h b/command-line-arguments.h
index de1734a..a2fc545 100644
--- a/command-line-arguments.h
+++ b/command-line-arguments.h
@@ -6,6 +6,7 @@
enum notmuch_opt_type {
NOTMUCH_OPT_END = 0,
NOTMUCH_OPT_BOOLEAN, /* --verbose */
+NOTMUCH_OPT_INT_OR_BOOLEAN,/* --verbose or --verbose=1 */
NOTMUCH_OPT_INT, /* --frob=8 */
NOTMUCH_OPT_KEYWORD, /* --format=raw|json|text */
NOTMUCH_OPT_STRING,/* --file=/tmp/gnarf.txt */
@@ -76,5 +77,7 @@ parse_position_arg (const char *arg,
int position_arg_index,
const notmuch_opt_desc_t* options);
+notmuch_bool_t
+notmuch_int_to_boolean (int i);
#endif
--
1.7.9.1
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH 0/2] cli: Parsing. Add option NOTMUCH_OPT_INT_OR_BOOLEAN
The first patch adds a new command line parsing option NOTMUCH_OPT_INT_OR_BOOLEAN for command line parsing which accepts --verbose=3 and --verbose with the latter setting verbose to 1. It also allows --verbose=0 so (with a little caller support) the user can turn off boolean options. It also means that extra options can be added to the command line programs in a backwards compatible manner (e.g. if --verbose already exists we could add --verbose=2). The second patch uses this to make the --entire-thread option to notmuch-show a NOTMUCH_OPT_INT_OR_BOOLEAN. In particular this allows the caller to disable --entire-thread (with --entire-thread=0) when format=json. Best wishes Mark Mark Walters (2): cli: Parsing. Add option NOTMUCH_OPT_INT_OR_BOOLEAN cli: make --entire-thread option notmuch-show.c INT_OR_BOOLEAN command-line-arguments.c | 29 +++-- command-line-arguments.h |3 +++ notmuch-show.c | 10 -- 3 files changed, 38 insertions(+), 4 deletions(-) -- 1.7.9.1 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[WIP PATCH] debugging gmime-2.6 fail.
On Thu, 8 Mar 2012 11:35:35 -0400, David Bremner wrote: > I ran out of time for the moment, but the following patch gets me down from > 4196 failures on the notmuch mailing list to 3422. That patch is of course complete nonsense. Attached is another silly patch, which at least demonstrates the problem. With the attached patch, notmuch (+ gmime-2.6) only fails for non-mbox messages. So I guess we need a way to detect if a file is mbox before parsing? or a way to get gmime to be less strict here? -- next part -- A non-text attachment was scrubbed... Name: 0001-WIP-debugging-gmime-2.6-problems.patch Type: text/x-diff Size: 847 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120308/98bc975a/attachment.patch>
Re: [PATCH] mime_node_open: check if the file is in mbox format, and inform gmime.
On Thu, 8 Mar 2012 17:48:15 -0400, David Bremner wrote: > From: David Bremner > > It seems that it has always been an error to try to parse an mbox > format file with gmime without calling g_mime_parser_set_scan_from. > > This change reads the first 5 bytes of the file, and if they are "From ", > declares the file to be an mbox. > --- > > This patch seems to fix the problem for me. I don't think the > performance impact should be too bad, but I didn't really test it. As I've stated previously in this thread, I think this behavior is a mistake. This will not result in a proper parsing of an mbox file, and improper or incomplete parsing of mbox files will lead to bad/confusing behavior. We should either completely support mbox files or not support them. Partial support is, imho, a recipe for disaster. We don't currently support them, and it would take a lot of extra work to do so completely. I don't see any harm in just telling users to convert their mbox files into proper message files. jamie. pgpjh9jZkZzp1.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] mime_node_open: check if the file is in mbox format, and inform gmime.
On Thu, 8 Mar 2012 17:48:15 -0400, David Bremner wrote: > From: David Bremner > > It seems that it has always been an error to try to parse an mbox > format file with gmime without calling g_mime_parser_set_scan_from. > > This change reads the first 5 bytes of the file, and if they are "From ", > declares the file to be an mbox. > --- > > This patch seems to fix the problem for me. I don't think the > performance impact should be too bad, but I didn't really test it. As I've stated previously in this thread, I think this behavior is a mistake. This will not result in a proper parsing of an mbox file, and improper or incomplete parsing of mbox files will lead to bad/confusing behavior. We should either completely support mbox files or not support them. Partial support is, imho, a recipe for disaster. We don't currently support them, and it would take a lot of extra work to do so completely. I don't see any harm in just telling users to convert their mbox files into proper message files. jamie. -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 835 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120308/99046603/attachment.pgp>
Re: [PATCH v6 02/10] reply: Factor out reply creation
On Tue, 21 Feb 2012 23:46:31 -0700, Adam Wolfe Gordon
wrote:
> Factor out the creation of a reply message based on an original
> message so it can be shared by different reply formats.
> ---
> notmuch-reply.c | 101 +++---
> 1 files changed, 58 insertions(+), 43 deletions(-)
>
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 6b244e6..8e56245 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -505,6 +505,61 @@ guess_from_received_header (notmuch_config_t *config,
> notmuch_message_t *message
> return NULL;
> }
>
> +static GMimeMessage *
> +create_reply_message(void *ctx,
> + notmuch_config_t *config,
> + notmuch_message_t *message,
> + notmuch_bool_t reply_all)
> +{
> +const char *subject, *from_addr = NULL;
> +const char *in_reply_to, *orig_references, *references;
> +
> +/* The 1 means we want headers in a "pretty" order. */
> +GMimeMessage *reply = g_mime_message_new (1);
> +if (reply == NULL) {
> + fprintf (stderr, "Out of memory\n");
> + return NULL;
> +}
> +
> +subject = notmuch_message_get_header (message, "subject");
> +if (subject) {
> + if (strncasecmp (subject, "Re:", 3))
> + subject = talloc_asprintf (ctx, "Re: %s", subject);
> + g_mime_message_set_subject (reply, subject);
> +}
> +
> +from_addr = add_recipients_from_message (reply, config,
> + message, reply_all);
> +
> +if (from_addr == NULL)
> + from_addr = guess_from_received_header (config, message);
> +
> +if (from_addr == NULL)
> + from_addr = notmuch_config_get_user_primary_email (config);
> +
> +from_addr = talloc_asprintf (ctx, "%s <%s>",
> + notmuch_config_get_user_name (config),
> + from_addr);
> +g_mime_object_set_header (GMIME_OBJECT (reply),
> + "From", from_addr);
> +
> +in_reply_to = talloc_asprintf (ctx, "<%s>",
> +notmuch_message_get_message_id (message));
> +
> +g_mime_object_set_header (GMIME_OBJECT (reply),
> + "In-Reply-To", in_reply_to);
> +
> +orig_references = notmuch_message_get_header (message, "references");
> +references = talloc_asprintf (ctx, "%s%s%s",
> + orig_references ? orig_references : "",
> + orig_references ? " " : "",
> + in_reply_to);
> +g_mime_object_set_header (GMIME_OBJECT (reply),
> + "References", references);
> +
> +return reply;
> +}
> +
> static int
> notmuch_reply_format_default(void *ctx,
>notmuch_config_t *config,
> @@ -515,8 +570,6 @@ notmuch_reply_format_default(void *ctx,
> GMimeMessage *reply;
> notmuch_messages_t *messages;
> notmuch_message_t *message;
> -const char *subject, *from_addr = NULL;
> -const char *in_reply_to, *orig_references, *references;
> const notmuch_show_format_t *format = &format_reply;
>
> for (messages = notmuch_query_search_messages (query);
> @@ -525,48 +578,10 @@ notmuch_reply_format_default(void *ctx,
> {
> message = notmuch_messages_get (messages);
>
> - /* The 1 means we want headers in a "pretty" order. */
> - reply = g_mime_message_new (1);
> - if (reply == NULL) {
> - fprintf (stderr, "Out of memory\n");
> - return 1;
> - }
> -
> - subject = notmuch_message_get_header (message, "subject");
> - if (subject) {
> - if (strncasecmp (subject, "Re:", 3))
> - subject = talloc_asprintf (ctx, "Re: %s", subject);
> - g_mime_message_set_subject (reply, subject);
> - }
> -
> - from_addr = add_recipients_from_message (reply, config, message,
> - reply_all);
> + reply = create_reply_message (ctx, config, message, reply_all);
>
> - if (from_addr == NULL)
> - from_addr = guess_from_received_header (config, message);
> -
> - if (from_addr == NULL)
> - from_addr = notmuch_config_get_user_primary_email (config);
> -
> - from_addr = talloc_asprintf (ctx, "%s <%s>",
> - notmuch_config_get_user_name (config),
> - from_addr);
> - g_mime_object_set_header (GMIME_OBJECT (reply),
> - "From", from_addr);
> -
> - in_reply_to = talloc_asprintf (ctx, "<%s>",
> - notmuch_message_get_message_id (message));
> -
> - g_mime_object_set_header (GMIME_OBJECT (reply),
> - "In-Reply-To", in_reply_to);
> -
> - orig_references = notmuch_message_get_header (message, "references");
> - references = talloc_asprintf (ctx, "%s%s%s",
> - orig_r
Re: Parsing regression with gmime-2.6?
On Thu, 08 Mar 2012 16:40:00 -0500, Daniel Kahn Gillmor wrote: > i think the point here is that gmime used to support parsing them and > now it doesn't. > > So if you've got a pre-existing notmuch index (built from when gmime > *did* parse them) and you're now running notmuch linked against a gmime > that *doesn't* parse them, then notmuch search will produce references > to messages that notmuch show can't produce output for. This is unfortunate for those that suffer from this but again I don't think the solution is a partial parsing of mbox files. If this really is the situation, it seems like the *previous* behavior was actually the problematic one, which has now been fixed, even though it appears to be a regression. I think I would argue that users who do fall in to this situation should just simply convert all of their mbox files into proper messages. That's the position that we've actually held all along: notmuch does not support mbox and those with mbox files should just convert them to individual messages with something like mbox2maildir or the like. jamie. pgpct7ZjpjI9I.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Parsing regression with gmime-2.6?
On Thu, 08 Mar 2012 16:40:00 -0500, Daniel Kahn Gillmor wrote: > i think the point here is that gmime used to support parsing them and > now it doesn't. > > So if you've got a pre-existing notmuch index (built from when gmime > *did* parse them) and you're now running notmuch linked against a gmime > that *doesn't* parse them, then notmuch search will produce references > to messages that notmuch show can't produce output for. This is unfortunate for those that suffer from this but again I don't think the solution is a partial parsing of mbox files. If this really is the situation, it seems like the *previous* behavior was actually the problematic one, which has now been fixed, even though it appears to be a regression. I think I would argue that users who do fall in to this situation should just simply convert all of their mbox files into proper messages. That's the position that we've actually held all along: notmuch does not support mbox and those with mbox files should just convert them to individual messages with something like mbox2maildir or the like. jamie. -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 835 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120308/784e388f/attachment.pgp>
[PATCH] mime_node_open: check if the file is in mbox format, and inform gmime.
From: David Bremner
It seems that it has always been an error to try to parse an mbox
format file with gmime without calling g_mime_parser_set_scan_from.
This change reads the first 5 bytes of the file, and if they are "From ",
declares the file to be an mbox.
---
This patch seems to fix the problem for me. I don't think the
performance impact should be too bad, but I didn't really test it.
mime-node.c | 21 +
1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index a95bdab..8939147 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -72,6 +72,8 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
mime_node_context_t *mctx;
mime_node_t *root;
notmuch_status_t status;
+char from_buf[6]; /* From space NULL */
+int is_mbox = 0;
root = talloc_zero (ctx, mime_node_t);
if (root == NULL) {
@@ -96,6 +98,23 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
goto DONE;
}
+if (fread (from_buf, 1, 5, mctx->file) != 5) {
+ fprintf (stderr, "Failed to read 5 bytes from %s: %s\n",
+filename, strerror (errno));
+ status = NOTMUCH_STATUS_FILE_ERROR;
+ goto DONE;
+}
+from_buf[5] = '\0';
+
+if (fseek (mctx->file, 0L, SEEK_SET)) {
+ fprintf (stderr, "Failed to rewind %s: %s\n",
+filename, strerror (errno));
+ status = NOTMUCH_STATUS_FILE_ERROR;
+ goto DONE;
+}
+
+is_mbox = (strcmp (from_buf, "From ") == 0);
+
mctx->stream = g_mime_stream_file_new (mctx->file);
if (!mctx->stream) {
fprintf (stderr, "Out of memory.\n");
@@ -111,7 +130,9 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
goto DONE;
}
+g_mime_parser_set_scan_from (mctx->parser, is_mbox);
mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
if (!mctx->mime_message) {
fprintf (stderr, "Failed to parse %s\n", filename);
status = NOTMUCH_STATUS_FILE_ERROR;
--
1.7.9.1
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: Parsing regression with gmime-2.6?
On 03/08/2012 04:32 PM, Jameson Graef Rollins wrote: I actually agree with this position. mbox files are not proper email messages, so if gmime does not explicitly support parsing them then we really can't expect it to parse them. i think the point here is that gmime used to support parsing them and now it doesn't. So if you've got a pre-existing notmuch index (built from when gmime *did* parse them) and you're now running notmuch linked against a gmime that *doesn't* parse them, then notmuch search will produce references to messages that notmuch show can't produce output for. While i appreciate wanting to be sure that we're working with well-formed files, I don't have a good answer for what the right thing to do is, if gmime upstream agrees with your assessment of the situation. yuck, --dkg ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: Parsing regression with gmime-2.6?
On Thu, 08 Mar 2012 15:30:21 -0500, Daniel Kahn Gillmor wrote: > This does seem to be a regression in gmime 2.6. I've reported the bug > upstream, along with a simplified (non-notmuch) demonstration: > > https://bugzilla.gnome.org/show_bug.cgi?id=671680 > > We'll see what gmime's upstream has to say about it. Thanks so much for following up on this, Daniel. > As a devil's advocate, i could argue that a message in a maildir that > starts with a "From " line isn't a proper e-mail message in the first > place, and therefore gmime 2.6 is being more rigorously correct about > what it accepts. In particular, if a user were to place a multi-message > mbox file in their notmuch message store, i think that notmuch linked > against 2.4 would happily index only the first message of it, and the > rest of the message would be "hidden", whereas gmime 2.6 allows us to > detect these failures and avoid indexing them directly. I actually agree with this position. mbox files are not proper email messages, so if gmime does not explicitly support parsing them then we really can't expect it to parse them. We *can* expect gmime to return some sort of proper error message/return code/etc and not fail in a bad way, but beyond that I think the burden is on us. > That said, i understand that this is probably not an entirely rare > situation, and i lean toward the idea that gmime 2.4's behavior was > actually the Right Thing. Also, I haven't been able to find any > explicit documentation to indicate that the behavior change was > a deliberate one. And I think I don't agree with this one: I not think that handling mbox files in the way you've outlined above is the Right Thing. Only partially parsing an mbox file, by indexing only the first message for instance, seems like a bad solution to me, and one that's likely to lead to a lot of confusion (e.g. "where are the rest of the messages that were in my mbox?"). If notmuch encounters an mbox file in the store it should just skip the message and continue, while indicating that it is skipping the message because it's not a proper email message, as it does with other non-email files. jamie. pgpic1UoIFGFT.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Parsing regression with gmime-2.6?
On Thu, 08 Mar 2012 15:30:21 -0500, Daniel Kahn Gillmor wrote: > This does seem to be a regression in gmime 2.6. I've reported the bug > upstream, along with a simplified (non-notmuch) demonstration: > > https://bugzilla.gnome.org/show_bug.cgi?id=671680 > > We'll see what gmime's upstream has to say about it. Thanks so much for following up on this, Daniel. > As a devil's advocate, i could argue that a message in a maildir that > starts with a "From " line isn't a proper e-mail message in the first > place, and therefore gmime 2.6 is being more rigorously correct about > what it accepts. In particular, if a user were to place a multi-message > mbox file in their notmuch message store, i think that notmuch linked > against 2.4 would happily index only the first message of it, and the > rest of the message would be "hidden", whereas gmime 2.6 allows us to > detect these failures and avoid indexing them directly. I actually agree with this position. mbox files are not proper email messages, so if gmime does not explicitly support parsing them then we really can't expect it to parse them. We *can* expect gmime to return some sort of proper error message/return code/etc and not fail in a bad way, but beyond that I think the burden is on us. > That said, i understand that this is probably not an entirely rare > situation, and i lean toward the idea that gmime 2.4's behavior was > actually the Right Thing. Also, I haven't been able to find any > explicit documentation to indicate that the behavior change was > a deliberate one. And I think I don't agree with this one: I not think that handling mbox files in the way you've outlined above is the Right Thing. Only partially parsing an mbox file, by indexing only the first message for instance, seems like a bad solution to me, and one that's likely to lead to a lot of confusion (e.g. "where are the rest of the messages that were in my mbox?"). If notmuch encounters an mbox file in the store it should just skip the message and continue, while indicating that it is skipping the message because it's not a proper email message, as it does with other non-email files. jamie. -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 835 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120308/787f737a/attachment.pgp>
Obsolete Patches
Hi There was a comment on irc about the needs review queue on nmbug status being large. I think there may be several patches on there that have been superseded or otherwise made obsolete. This makes choosing what to review harder. My guess is that it is very much easier for the author to know this than for David (B). Hence I would like to suggest that authors say when their patches should be removed from the queue. I have several patches in this category (in each case I just list the first patch of the series) id:"1328375350-10352-2-git-send-email-markwalters1...@gmail.com" (superseded) id:"1328388317-20161-2-git-send-email-markwalters1...@gmail.com" (Dmitry suggested a better solution) id:"1329296619-7463-2-git-send-email-markwalters1...@gmail.com" (a later version has been pushed) id:"1329900529-16295-1-git-send-email-markwalters1...@gmail.com" (various problems so should be removed; perhaps marked wip) id:"1330539189-16593-2-git-send-email-markwalters1...@gmail.com" (as much as is relevant has been pushed: it's the revert exclude for 0.12 series) Best wishes Mark ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: Parsing regression with gmime-2.6?
On Tue, 06 Mar 2012 18:04:50 -0400, David Bremner wrote: > There seems to be something weird going on with gmime-2.6; maybe we > didn't catch some api change? This does seem to be a regression in gmime 2.6. I've reported the bug upstream, along with a simplified (non-notmuch) demonstration: https://bugzilla.gnome.org/show_bug.cgi?id=671680 We'll see what gmime's upstream has to say about it. As a devil's advocate, i could argue that a message in a maildir that starts with a "From " line isn't a proper e-mail message in the first place, and therefore gmime 2.6 is being more rigorously correct about what it accepts. In particular, if a user were to place a multi-message mbox file in their notmuch message store, i think that notmuch linked against 2.4 would happily index only the first message of it, and the rest of the message would be "hidden", whereas gmime 2.6 allows us to detect these failures and avoid indexing them directly. That said, i understand that this is probably not an entirely rare situation, and i lean toward the idea that gmime 2.4's behavior was actually the Right Thing. Also, I haven't been able to find any explicit documentation to indicate that the behavior change was a deliberate one. I'm happy to relay any helpful or constructive suggestions to the gmime upstream ticket, if folks don't want to deal with bugzilla directly. --dkg pgpW8csGXxLxZ.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
a DoS vulnerability associated with conflated Message-IDs?
On 03/08/2012 12:04 PM, James Vasile wrote: > On Thu, 08 Mar 2012 11:37:09 -0500, Daniel Kahn Gillmor fifthhorseman.net> wrote: >> Any ideas on how to approach this? > > Treat messages with the same ID but different hashes as different? Given that a message hash would include all headers, including Received: and other MTA-added stuff, i think that would remove all relevance of the Message-ID field. in particular, it seems like we would just be identifying messages by their digest. If you're willing to ignore the headers and just look at a digest of the body, that still doesn't provide any help for the common (legitimate) case of a message jointly-delivered to a mailing list and to a specific (already-subscribed) user. That user will get two copies of the message, and since most mailing lists modify the body of the message (usually by adding a footer section with mailing list info) their bodies will also have different digests. So i don't see how to make this suggestion work without giving up on Message-IDs as the identifier entirely (and therefore accepting many more spurious duplicates than users currently need to tolerate). Any other suggestions or ideas? --dkg
a DoS vulnerability associated with conflated Message-IDs?
On Thu, 08 Mar 2012 11:37:09 -0500, Daniel Kahn Gillmor wrote: > Any ideas on how to approach this? Treat messages with the same ID but different hashes as different? -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 489 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120308/1c970ebc/attachment.pgp>
a DoS vulnerability associated with conflated Message-IDs?
notmuch currently treats all messages with the same Message-ID as the same message. I think this could be a vulnerability :( If two messages have the same Message-ID, is there a guarantee of which of these messages will be produced during a notmuch show? Either way, it seems to create a potential DoS attack on notmuch users. --- The attack: Let's say there is a public mailing list that Mallory knows bob at example.org is subscribed to. alice at example.net sends a message to the public mailing list detailing some problem that Bob probably needs to deal with. Mallory can just craft a content-free e-mail (or a dozen?) with the same Message-ID as Alice's message, and send it to bob at example.org. If Bob uses notmuch, he is much more likely to read one of Mallory's bogus e-mails than to read Alice's original message. Mallory's e-mail could also be crafted to look like spam, in the hopes that Bob's spamfiltering scripts would mark the original message's Message-ID as spam. I don't know how to fix this, and i'd be happy to hear if someone thinks my analysis above is flawed and this isn't really a problem. Any ideas on how to approach this? --dkg -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 965 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120308/b91e806d/attachment.pgp>
[WIP PATCH] debugging gmime-2.6 fail.
From: David Bremner
Try parsing again as mbox if the first one failed.
---
I ran out of time for the moment, but the following patch gets me down from
4196 failures on the notmuch mailing list to 3422.
I'm leaning to reverting back to gmime-2.4 for the Debian 0.12 package
if I can't sort this out.
mime-node.c |9 +
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index a95bdab..1d3a184 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -111,8 +111,17 @@ mime_node_open (const void *ctx, notmuch_message_t
*message,
goto DONE;
}
+
mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
if (!mctx->mime_message) {
+ /*
+* Parsing failed, let's try again as an mbox
+*/
+ mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+ mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+}
+
+if (!mctx->mime_message) {
fprintf (stderr, "Failed to parse %s\n", filename);
status = NOTMUCH_STATUS_FILE_ERROR;
goto DONE;
--
1.7.9.1
a DoS vulnerability associated with conflated Message-IDs?
On Thu, Mar 8, 2012 at 10:16, Daniel Kahn Gillmor wrote: > Any other suggestions or ideas? What about representing the contents from both message in one apparent message? - Aggregate the headers together, perhaps? - Where headers disagree, display both - If the bodies disagree, display both.
Re: Parsing regression with gmime-2.6?
On Tue, 06 Mar 2012 18:04:50 -0400, David Bremner wrote: > There seems to be something weird going on with gmime-2.6; maybe we > didn't catch some api change? > > I noticed a surprising number of messages failing to parse, so I wrote a > script to take a random sample of 1000 messages and run notmuch show on > them. A shocking 650 to 700 of them fail to parse with gmime-2.6. No > failures are reported with gmime-2.4. This *is* shockingly high, and very confusing. Somehow all of the tests pass? That's very strange, particularly since there are enough real messages used in the tests that we should experience at least one of these failures, right? Is there really nothing special about the messages that are failing to parse? Your later patches seem to indicate that this has something to do with mbox, although you don't mention that hear. I thought I had been using gmime-2.6 for the last month, so I was very skeptical of this issue. Unfortunately I just found a bug in the configure script [0] that meant I had actually been using gmime-2.4. However, since fixing things and actually using gmime-2.6 now, I still don't see any problems. If the problem is specific to mbox, which we explicitly don't support, I don't think this is tragic enough to warrant removing gmime-2.6 as a dependency satisfier for 0.12 in Debian. In an event, a more explicit description of the problem you're seeing would be helpful. jamie. [0] id:"1331225101-24385-1-git-send-email-jroll...@finestructure.net" pgpsfRBhlNG3K.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Parsing regression with gmime-2.6?
On Tue, 06 Mar 2012 18:04:50 -0400, David Bremner wrote: > There seems to be something weird going on with gmime-2.6; maybe we > didn't catch some api change? > > I noticed a surprising number of messages failing to parse, so I wrote a > script to take a random sample of 1000 messages and run notmuch show on > them. A shocking 650 to 700 of them fail to parse with gmime-2.6. No > failures are reported with gmime-2.4. This *is* shockingly high, and very confusing. Somehow all of the tests pass? That's very strange, particularly since there are enough real messages used in the tests that we should experience at least one of these failures, right? Is there really nothing special about the messages that are failing to parse? Your later patches seem to indicate that this has something to do with mbox, although you don't mention that hear. I thought I had been using gmime-2.6 for the last month, so I was very skeptical of this issue. Unfortunately I just found a bug in the configure script [0] that meant I had actually been using gmime-2.4. However, since fixing things and actually using gmime-2.6 now, I still don't see any problems. If the problem is specific to mbox, which we explicitly don't support, I don't think this is tragic enough to warrant removing gmime-2.6 as a dependency satisfier for 0.12 in Debian. In an event, a more explicit description of the problem you're seeing would be helpful. jamie. [0] id:"1331225101-24385-1-git-send-email-jrollins at finestructure.net" -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 835 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120308/c92375b3/attachment.pgp>
Re: [WIP PATCH] debugging gmime-2.6 fail.
On Thu, 8 Mar 2012 11:35:35 -0400, David Bremner wrote:
> I ran out of time for the moment, but the following patch gets me down from
> 4196 failures on the notmuch mailing list to 3422.
That patch is of course complete nonsense. Attached is another silly
patch, which at least demonstrates the problem. With the attached patch,
notmuch (+ gmime-2.6) only fails for non-mbox messages. So I guess we
need a way to detect if a file is mbox before parsing? or a way to get
gmime to be less strict here?
>From 7fb942049ae68e09ebb9fbca40048f95543ab4b8 Mon Sep 17 00:00:00 2001
From: David Bremner
Date: Thu, 8 Mar 2012 11:11:21 -0400
Subject: [PATCH] WIP debugging gmime-2.6 problems
Unconditionally tell gmime to look for an mbox. This of course makes
it fail for non-mboxes.
---
mime-node.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index a95bdab..3e07fbf 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -111,7 +111,9 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
goto DONE;
}
+g_mime_parser_set_scan_from(mctx->parser, TRUE);
mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
if (!mctx->mime_message) {
fprintf (stderr, "Failed to parse %s\n", filename);
status = NOTMUCH_STATUS_FILE_ERROR;
--
1.7.9.1
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: a DoS vulnerability associated with conflated Message-IDs?
On Thu, Mar 8, 2012 at 10:16, Daniel Kahn Gillmor wrote: > Any other suggestions or ideas? What about representing the contents from both message in one apparent message? - Aggregate the headers together, perhaps? - Where headers disagree, display both - If the bodies disagree, display both. ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: a DoS vulnerability associated with conflated Message-IDs?
On 03/08/2012 12:04 PM, James Vasile wrote: On Thu, 08 Mar 2012 11:37:09 -0500, Daniel Kahn Gillmor wrote: Any ideas on how to approach this? Treat messages with the same ID but different hashes as different? Given that a message hash would include all headers, including Received: and other MTA-added stuff, i think that would remove all relevance of the Message-ID field. in particular, it seems like we would just be identifying messages by their digest. If you're willing to ignore the headers and just look at a digest of the body, that still doesn't provide any help for the common (legitimate) case of a message jointly-delivered to a mailing list and to a specific (already-subscribed) user. That user will get two copies of the message, and since most mailing lists modify the body of the message (usually by adding a footer section with mailing list info) their bodies will also have different digests. So i don't see how to make this suggestion work without giving up on Message-IDs as the identifier entirely (and therefore accepting many more spurious duplicates than users currently need to tolerate). Any other suggestions or ideas? --dkg ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: a DoS vulnerability associated with conflated Message-IDs?
On Thu, 08 Mar 2012 11:37:09 -0500, Daniel Kahn Gillmor wrote: > Any ideas on how to approach this? Treat messages with the same ID but different hashes as different? pgpjtq6bzoxfs.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] Fix configure script to properly detect gmime-2.6 if available.
Previously, the configure script would appear to detect gmime-2.6 if present. However, the binaries would end up being compiled against gmime-2.4. The addition of a break fixes things so that now gmime-2.6 will be used if available, falling back to gmime-2.4. --- I was wondering why I wasn't seeing any of the problems bremner is reporting, even though I thought I had been using gmime-2.6 for the last month. Apparently I had actually been using gmime-2.4 because of this bug in the configure script. Linking will now be correct after reconfigure. configure |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/configure b/configure index 8b85b9d..dedb7d8 100755 --- a/configure +++ b/configure @@ -281,6 +281,7 @@ for gmimepc in gmime-2.6 gmime-2.4; do have_gmime=1 gmime_cflags=$(pkg-config --cflags $gmimepc) gmime_ldflags=$(pkg-config --libs $gmimepc) + break fi done if [ "$have_gmime" = "0" ]; then -- 1.7.9.1 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] Fix configure script to properly detect gmime-2.6 if available.
Previously, the configure script would appear to detect gmime-2.6 if present. However, the binaries would end up being compiled against gmime-2.4. The addition of a break fixes things so that now gmime-2.6 will be used if available, falling back to gmime-2.4. --- I was wondering why I wasn't seeing any of the problems bremner is reporting, even though I thought I had been using gmime-2.6 for the last month. Apparently I had actually been using gmime-2.4 because of this bug in the configure script. Linking will now be correct after reconfigure. configure |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/configure b/configure index 8b85b9d..dedb7d8 100755 --- a/configure +++ b/configure @@ -281,6 +281,7 @@ for gmimepc in gmime-2.6 gmime-2.4; do have_gmime=1 gmime_cflags=$(pkg-config --cflags $gmimepc) gmime_ldflags=$(pkg-config --libs $gmimepc) + break fi done if [ "$have_gmime" = "0" ]; then -- 1.7.9.1
a DoS vulnerability associated with conflated Message-IDs?
notmuch currently treats all messages with the same Message-ID as the same message. I think this could be a vulnerability :( If two messages have the same Message-ID, is there a guarantee of which of these messages will be produced during a notmuch show? Either way, it seems to create a potential DoS attack on notmuch users. --- The attack: Let's say there is a public mailing list that Mallory knows b...@example.org is subscribed to. al...@example.net sends a message to the public mailing list detailing some problem that Bob probably needs to deal with. Mallory can just craft a content-free e-mail (or a dozen?) with the same Message-ID as Alice's message, and send it to b...@example.org. If Bob uses notmuch, he is much more likely to read one of Mallory's bogus e-mails than to read Alice's original message. Mallory's e-mail could also be crafted to look like spam, in the hopes that Bob's spamfiltering scripts would mark the original message's Message-ID as spam. I don't know how to fix this, and i'd be happy to hear if someone thinks my analysis above is flawed and this isn't really a problem. Any ideas on how to approach this? --dkg pgp6mSrl7Bu7a.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[WIP PATCH] debugging gmime-2.6 fail.
From: David Bremner
Try parsing again as mbox if the first one failed.
---
I ran out of time for the moment, but the following patch gets me down from
4196 failures on the notmuch mailing list to 3422.
I'm leaning to reverting back to gmime-2.4 for the Debian 0.12 package
if I can't sort this out.
mime-node.c |9 +
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index a95bdab..1d3a184 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -111,8 +111,17 @@ mime_node_open (const void *ctx, notmuch_message_t
*message,
goto DONE;
}
+
mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
if (!mctx->mime_message) {
+ /*
+* Parsing failed, let's try again as an mbox
+*/
+ mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+ mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+}
+
+if (!mctx->mime_message) {
fprintf (stderr, "Failed to parse %s\n", filename);
status = NOTMUCH_STATUS_FILE_ERROR;
goto DONE;
--
1.7.9.1
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
