[PATCH] lib: resurrect support for single-message mbox files

[PATCH] lib: resurrect support for single-message mbox files

[Jani Nikula]

This is effectively a revert of

commit 6812136bf576d894591606d9e10096719054d1f9
Author: Jani Nikula <jani@nikula.org>
Date:   Mon Mar 31 00:21:48 2014 +0300

    lib: drop support for single-message mbox files

The intention was to drop support for indexing new single-message mbox
files (and whether that was a good idea in the first place is
arguable). However this inadvertently broke support for reading
headers from previously indexed single-message mbox files, which is
far worse.

Distinguishing between the two cases would require more code than
simply bringing back support for single-message mbox files.
---
 lib/message-file.c |   30 +++++++++++++++++++++++++-----
 test/T050-new.sh   |   26 ++++++++++++++++----------
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/lib/message-file.c b/lib/message-file.c
index 6782882..483ba1e 100644
--- a/lib/message-file.c
+++ b/lib/message-file.c
@@ -117,7 +117,7 @@ notmuch_message_file_close (notmuch_message_file_t *message)
 }
 
 static notmuch_bool_t
-is_mbox (FILE *file)
+_is_mbox (FILE *file)
 {
     char from_buf[5];
     notmuch_bool_t ret = FALSE;
@@ -139,13 +139,12 @@ _notmuch_message_file_parse (notmuch_message_file_t *message)
     GMimeParser *parser;
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
     static int initialized = 0;
+    notmuch_bool_t is_mbox;
 
     if (message->message)
 	return NOTMUCH_STATUS_SUCCESS;
 
-    /* We no longer support mboxes at all. */
-    if (is_mbox (message->file))
-	return NOTMUCH_STATUS_FILE_NOT_EMAIL;
+    is_mbox = _is_mbox (message->file);
 
     if (! initialized) {
 	g_mime_init (GMIME_ENABLE_RFC2047_WORKAROUNDS);
@@ -163,7 +162,7 @@ _notmuch_message_file_parse (notmuch_message_file_t *message)
     g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);
 
     parser = g_mime_parser_new_with_stream (stream);
-    g_mime_parser_set_scan_from (parser, FALSE);
+    g_mime_parser_set_scan_from (parser, is_mbox);
 
     message->message = g_mime_parser_construct_message (parser);
     if (! message->message) {
@@ -171,6 +170,27 @@ _notmuch_message_file_parse (notmuch_message_file_t *message)
 	goto DONE;
     }
 
+    if (is_mbox) {
+	if (! g_mime_parser_eos (parser)) {
+	    /* This is a multi-message mbox. */
+	    status = NOTMUCH_STATUS_FILE_NOT_EMAIL;
+	    goto DONE;
+	}
+	/*
+	 * For historical reasons, we support single-message mboxes,
+	 * but this behavior is likely to change in the future, so
+	 * warn.
+	 */
+	static notmuch_bool_t mbox_warning = FALSE;
+	if (! mbox_warning) {
+	    mbox_warning = TRUE;
+	    fprintf (stderr, "\
+Warning: %s is an mbox containing a single message,\n\
+likely caused by misconfigured mail delivery.  Support for single-message\n\
+mboxes is deprecated and may be removed in the future.\n", message->filename);
+	}
+    }
+
   DONE:
     g_object_unref (stream);
     g_object_unref (parser);
diff --git a/test/T050-new.sh b/test/T050-new.sh
index 3c31954..ad46ee6 100755
--- a/test/T050-new.sh
+++ b/test/T050-new.sh
@@ -163,6 +163,22 @@ rm -rf "${MAIL_DIR}"/two
 output=$(NOTMUCH_NEW)
 test_expect_equal "$output" "No new mail. Removed 3 messages."
 
+test_begin_subtest "Support single-message mbox (deprecated)"
+cat > "${MAIL_DIR}"/mbox_file1 <<EOF
+From test_suite@notmuchmail.org Fri Jan  5 15:43:57 2001
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: Notmuch Test Suite <test_suite@notmuchmail.org>
+Subject: Test mbox message 1
+
+Body.
+EOF
+output=$(NOTMUCH_NEW 2>&1)
+test_expect_equal "$output" \
+"Warning: ${MAIL_DIR}/mbox_file1 is an mbox containing a single message,
+likely caused by misconfigured mail delivery.  Support for single-message
+mboxes is deprecated and may be removed in the future.
+Added 1 new message to the database."
+
 # This test requires that notmuch new has been run at least once.
 test_begin_subtest "Skip and report non-mail files"
 generate_message
@@ -184,24 +200,14 @@ Subject: Test mbox message 2
 
 Body 2.
 EOF
-cat > "${MAIL_DIR}"/mbox_file1 <<EOF
-From test_suite@notmuchmail.org Fri Jan  5 15:43:57 2001
-From: Notmuch Test Suite <test_suite@notmuchmail.org>
-To: Notmuch Test Suite <test_suite@notmuchmail.org>
-Subject: Test mbox message 1
-
-Body.
-EOF
 output=$(NOTMUCH_NEW 2>&1)
 test_expect_equal "$output" \
 "Note: Ignoring non-mail file: ${MAIL_DIR}/.git/config
 Note: Ignoring non-mail file: ${MAIL_DIR}/.ignored_hidden_file
 Note: Ignoring non-mail file: ${MAIL_DIR}/ignored_file
 Note: Ignoring non-mail file: ${MAIL_DIR}/mbox_file
-Note: Ignoring non-mail file: ${MAIL_DIR}/mbox_file1
 Added 1 new message to the database."
 rm "${MAIL_DIR}"/mbox_file
-rm "${MAIL_DIR}"/mbox_file1
 
 test_begin_subtest "Ignore files and directories specified in new.ignore"
 generate_message
-- 
1.7.2.5

Re: [PATCH] lib: resurrect support for single-message mbox files

[Mark Walters]

On Thu, 05 Jun 2014, Jani Nikula <jani@nikula.org> wrote:
> This is effectively a revert of
>
> commit 6812136bf576d894591606d9e10096719054d1f9
> Author: Jani Nikula <jani@nikula.org>
> Date:   Mon Mar 31 00:21:48 2014 +0300
>
>     lib: drop support for single-message mbox files
>
> The intention was to drop support for indexing new single-message mbox
> files (and whether that was a good idea in the first place is
> arguable). However this inadvertently broke support for reading
> headers from previously indexed single-message mbox files, which is
> far worse.
>
> Distinguishing between the two cases would require more code than
> simply bringing back support for single-message mbox files.

This LGTM +1.

Best wishes

Mark

> ---
>  lib/message-file.c |   30 +++++++++++++++++++++++++-----
>  test/T050-new.sh   |   26 ++++++++++++++++----------
>  2 files changed, 41 insertions(+), 15 deletions(-)
>
> diff --git a/lib/message-file.c b/lib/message-file.c
> index 6782882..483ba1e 100644
> --- a/lib/message-file.c
> +++ b/lib/message-file.c
> @@ -117,7 +117,7 @@ notmuch_message_file_close (notmuch_message_file_t *message)
>  }
>  
>  static notmuch_bool_t
> -is_mbox (FILE *file)
> +_is_mbox (FILE *file)
>  {
>      char from_buf[5];
>      notmuch_bool_t ret = FALSE;
> @@ -139,13 +139,12 @@ _notmuch_message_file_parse (notmuch_message_file_t *message)
>      GMimeParser *parser;
>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>      static int initialized = 0;
> +    notmuch_bool_t is_mbox;
>  
>      if (message->message)
>  	return NOTMUCH_STATUS_SUCCESS;
>  
> -    /* We no longer support mboxes at all. */
> -    if (is_mbox (message->file))
> -	return NOTMUCH_STATUS_FILE_NOT_EMAIL;
> +    is_mbox = _is_mbox (message->file);
>  
>      if (! initialized) {
>  	g_mime_init (GMIME_ENABLE_RFC2047_WORKAROUNDS);
> @@ -163,7 +162,7 @@ _notmuch_message_file_parse (notmuch_message_file_t *message)
>      g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);
>  
>      parser = g_mime_parser_new_with_stream (stream);
> -    g_mime_parser_set_scan_from (parser, FALSE);
> +    g_mime_parser_set_scan_from (parser, is_mbox);
>  
>      message->message = g_mime_parser_construct_message (parser);
>      if (! message->message) {
> @@ -171,6 +170,27 @@ _notmuch_message_file_parse (notmuch_message_file_t *message)
>  	goto DONE;
>      }
>  
> +    if (is_mbox) {
> +	if (! g_mime_parser_eos (parser)) {
> +	    /* This is a multi-message mbox. */
> +	    status = NOTMUCH_STATUS_FILE_NOT_EMAIL;
> +	    goto DONE;
> +	}
> +	/*
> +	 * For historical reasons, we support single-message mboxes,
> +	 * but this behavior is likely to change in the future, so
> +	 * warn.
> +	 */
> +	static notmuch_bool_t mbox_warning = FALSE;
> +	if (! mbox_warning) {
> +	    mbox_warning = TRUE;
> +	    fprintf (stderr, "\
> +Warning: %s is an mbox containing a single message,\n\
> +likely caused by misconfigured mail delivery.  Support for single-message\n\
> +mboxes is deprecated and may be removed in the future.\n", message->filename);
> +	}
> +    }
> +
>    DONE:
>      g_object_unref (stream);
>      g_object_unref (parser);
> diff --git a/test/T050-new.sh b/test/T050-new.sh
> index 3c31954..ad46ee6 100755
> --- a/test/T050-new.sh
> +++ b/test/T050-new.sh
> @@ -163,6 +163,22 @@ rm -rf "${MAIL_DIR}"/two
>  output=$(NOTMUCH_NEW)
>  test_expect_equal "$output" "No new mail. Removed 3 messages."
>  
> +test_begin_subtest "Support single-message mbox (deprecated)"
> +cat > "${MAIL_DIR}"/mbox_file1 <<EOF
> +From test_suite@notmuchmail.org Fri Jan  5 15:43:57 2001
> +From: Notmuch Test Suite <test_suite@notmuchmail.org>
> +To: Notmuch Test Suite <test_suite@notmuchmail.org>
> +Subject: Test mbox message 1
> +
> +Body.
> +EOF
> +output=$(NOTMUCH_NEW 2>&1)
> +test_expect_equal "$output" \
> +"Warning: ${MAIL_DIR}/mbox_file1 is an mbox containing a single message,
> +likely caused by misconfigured mail delivery.  Support for single-message
> +mboxes is deprecated and may be removed in the future.
> +Added 1 new message to the database."
> +
>  # This test requires that notmuch new has been run at least once.
>  test_begin_subtest "Skip and report non-mail files"
>  generate_message
> @@ -184,24 +200,14 @@ Subject: Test mbox message 2
>  
>  Body 2.
>  EOF
> -cat > "${MAIL_DIR}"/mbox_file1 <<EOF
> -From test_suite@notmuchmail.org Fri Jan  5 15:43:57 2001
> -From: Notmuch Test Suite <test_suite@notmuchmail.org>
> -To: Notmuch Test Suite <test_suite@notmuchmail.org>
> -Subject: Test mbox message 1
> -
> -Body.
> -EOF
>  output=$(NOTMUCH_NEW 2>&1)
>  test_expect_equal "$output" \
>  "Note: Ignoring non-mail file: ${MAIL_DIR}/.git/config
>  Note: Ignoring non-mail file: ${MAIL_DIR}/.ignored_hidden_file
>  Note: Ignoring non-mail file: ${MAIL_DIR}/ignored_file
>  Note: Ignoring non-mail file: ${MAIL_DIR}/mbox_file
> -Note: Ignoring non-mail file: ${MAIL_DIR}/mbox_file1
>  Added 1 new message to the database."
>  rm "${MAIL_DIR}"/mbox_file
> -rm "${MAIL_DIR}"/mbox_file1
>  
>  test_begin_subtest "Ignore files and directories specified in new.ignore"
>  generate_message
> -- 
> 1.7.2.5
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] lib: resurrect support for single-message mbox files

[Tomi Ollila]

On Thu, Jun 05 2014, Jani Nikula <jani@nikula.org> wrote:

> This is effectively a revert of

Looks good, tests pass. I'll mark this 0.18.1 material...

Tomi


>
> commit 6812136bf576d894591606d9e10096719054d1f9
> Author: Jani Nikula <jani@nikula.org>
> Date:   Mon Mar 31 00:21:48 2014 +0300
>
>     lib: drop support for single-message mbox files
>
> The intention was to drop support for indexing new single-message mbox
> files (and whether that was a good idea in the first place is
> arguable). However this inadvertently broke support for reading
> headers from previously indexed single-message mbox files, which is
> far worse.
>
> Distinguishing between the two cases would require more code than
> simply bringing back support for single-message mbox files.
> ---
>  lib/message-file.c |   30 +++++++++++++++++++++++++-----
>  test/T050-new.sh   |   26 ++++++++++++++++----------
>  2 files changed, 41 insertions(+), 15 deletions(-)
>
> diff --git a/lib/message-file.c b/lib/message-file.c
> index 6782882..483ba1e 100644
> --- a/lib/message-file.c
> +++ b/lib/message-file.c
> @@ -117,7 +117,7 @@ notmuch_message_file_close (notmuch_message_file_t *message)
>  }
>  
>  static notmuch_bool_t
> -is_mbox (FILE *file)
> +_is_mbox (FILE *file)
>  {
>      char from_buf[5];
>      notmuch_bool_t ret = FALSE;
> @@ -139,13 +139,12 @@ _notmuch_message_file_parse (notmuch_message_file_t *message)
>      GMimeParser *parser;
>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>      static int initialized = 0;
> +    notmuch_bool_t is_mbox;
>  
>      if (message->message)
>  	return NOTMUCH_STATUS_SUCCESS;
>  
> -    /* We no longer support mboxes at all. */
> -    if (is_mbox (message->file))
> -	return NOTMUCH_STATUS_FILE_NOT_EMAIL;
> +    is_mbox = _is_mbox (message->file);
>  
>      if (! initialized) {
>  	g_mime_init (GMIME_ENABLE_RFC2047_WORKAROUNDS);
> @@ -163,7 +162,7 @@ _notmuch_message_file_parse (notmuch_message_file_t *message)
>      g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);
>  
>      parser = g_mime_parser_new_with_stream (stream);
> -    g_mime_parser_set_scan_from (parser, FALSE);
> +    g_mime_parser_set_scan_from (parser, is_mbox);
>  
>      message->message = g_mime_parser_construct_message (parser);
>      if (! message->message) {
> @@ -171,6 +170,27 @@ _notmuch_message_file_parse (notmuch_message_file_t *message)
>  	goto DONE;
>      }
>  
> +    if (is_mbox) {
> +	if (! g_mime_parser_eos (parser)) {
> +	    /* This is a multi-message mbox. */
> +	    status = NOTMUCH_STATUS_FILE_NOT_EMAIL;
> +	    goto DONE;
> +	}
> +	/*
> +	 * For historical reasons, we support single-message mboxes,
> +	 * but this behavior is likely to change in the future, so
> +	 * warn.
> +	 */
> +	static notmuch_bool_t mbox_warning = FALSE;
> +	if (! mbox_warning) {
> +	    mbox_warning = TRUE;
> +	    fprintf (stderr, "\
> +Warning: %s is an mbox containing a single message,\n\
> +likely caused by misconfigured mail delivery.  Support for single-message\n\
> +mboxes is deprecated and may be removed in the future.\n", message->filename);
> +	}
> +    }
> +
>    DONE:
>      g_object_unref (stream);
>      g_object_unref (parser);
> diff --git a/test/T050-new.sh b/test/T050-new.sh
> index 3c31954..ad46ee6 100755
> --- a/test/T050-new.sh
> +++ b/test/T050-new.sh
> @@ -163,6 +163,22 @@ rm -rf "${MAIL_DIR}"/two
>  output=$(NOTMUCH_NEW)
>  test_expect_equal "$output" "No new mail. Removed 3 messages."
>  
> +test_begin_subtest "Support single-message mbox (deprecated)"
> +cat > "${MAIL_DIR}"/mbox_file1 <<EOF
> +From test_suite@notmuchmail.org Fri Jan  5 15:43:57 2001
> +From: Notmuch Test Suite <test_suite@notmuchmail.org>
> +To: Notmuch Test Suite <test_suite@notmuchmail.org>
> +Subject: Test mbox message 1
> +
> +Body.
> +EOF
> +output=$(NOTMUCH_NEW 2>&1)
> +test_expect_equal "$output" \
> +"Warning: ${MAIL_DIR}/mbox_file1 is an mbox containing a single message,
> +likely caused by misconfigured mail delivery.  Support for single-message
> +mboxes is deprecated and may be removed in the future.
> +Added 1 new message to the database."
> +
>  # This test requires that notmuch new has been run at least once.
>  test_begin_subtest "Skip and report non-mail files"
>  generate_message
> @@ -184,24 +200,14 @@ Subject: Test mbox message 2
>  
>  Body 2.
>  EOF
> -cat > "${MAIL_DIR}"/mbox_file1 <<EOF
> -From test_suite@notmuchmail.org Fri Jan  5 15:43:57 2001
> -From: Notmuch Test Suite <test_suite@notmuchmail.org>
> -To: Notmuch Test Suite <test_suite@notmuchmail.org>
> -Subject: Test mbox message 1
> -
> -Body.
> -EOF
>  output=$(NOTMUCH_NEW 2>&1)
>  test_expect_equal "$output" \
>  "Note: Ignoring non-mail file: ${MAIL_DIR}/.git/config
>  Note: Ignoring non-mail file: ${MAIL_DIR}/.ignored_hidden_file
>  Note: Ignoring non-mail file: ${MAIL_DIR}/ignored_file
>  Note: Ignoring non-mail file: ${MAIL_DIR}/mbox_file
> -Note: Ignoring non-mail file: ${MAIL_DIR}/mbox_file1
>  Added 1 new message to the database."
>  rm "${MAIL_DIR}"/mbox_file
> -rm "${MAIL_DIR}"/mbox_file1
>  
>  test_begin_subtest "Ignore files and directories specified in new.ignore"
>  generate_message
> -- 
> 1.7.2.5
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] lib: resurrect support for single-message mbox files

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> This is effectively a revert of
>
> commit 6812136bf576d894591606d9e10096719054d1f9
> Author: Jani Nikula <jani@nikula.org>
> Date:   Mon Mar 31 00:21:48 2014 +0300
>
>     lib: drop support for single-message mbox files

pushed. This needs a NEWS item for 0.18.1

d