From: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>
To: david@tethera.net, notmuch@notmuchmail.org
Subject: Re: Add new dump/restore format and batch tagging.
In-Reply-To: <1353265498-3839-1-git-send-email-david@tethera.net>
References: <1353265498-3839-1-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.14+45~g6ea9330 (http://notmuchmail.org) Emacs/24.1.1
	(x86_64-pc-linux-gnu)
Date: Sun, 18 Nov 2012 16:55:43 -0500
Message-ID: <87obiu1sps.fsf@betacantrips.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

david@tethera.net writes:

> which was revied by Tomi and Ethan. I think I implemented their
> suggestions.

Actually, I don't think you implemented all of mine.

- Patch 4 still has a subject line that ends in a period. I don't think
  this is mandatory for everyone but some people consider it best
  practice. You still have the spelling "seperate" (also, patch 7 has
  "seperated").

- In patch 4, I still think this would look better if you switched the
  branches.

+    if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
+       _notmuch_message_add_term (message, "type", "mail");
+    } else {
+       return NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
+    }

- In patch 5, I still think this looks funny:

+       int num_tags = random () % (max_tags + 1);
+       int this_mid_len = random () % message_id_len + 1;

Additionally, I would like a check that message_id_len is reasonable
(more than 1, say).

- Patch 8:

+int
+parse_tag_stream (void *ctx,
+                 notmuch_database_t *notmuch,
+                 FILE *input,
+                 tag_callback_t callback,
+                 tag_op_flag_t flags,
+                 volatile sig_atomic_t *interrupted);

Am I going crazy, or does this function not get implemented?

- Patch 11: DUMP_FORMAT can change from DUMP_FORMAT_BATCH_TAG to
  DUMP_FORMAT_SUP if a paren is anywhere on the first line. I'd prefer
  this only happens if we have DUMP_FORMAT_AUTO.

You probably want to move the comment "Dump output is..." closer to the
regex.

I don't see why it's necessary to have this:

+               query_string = query_string + 3;

- Patch 13:

+    cp /dev/null EXPECTED.$test_count
+    notmuch dump --format=batch-tag -- from:cworth |\
+        awk "{ print \"+$enc1 +$enc2 +$enc3 -- \" \$5 }" > EXPECTED.$test_count

What's the purpose of the CP here? It just creates an empty file. You
could do it with touch. Why even bother since you're going to fill it
with stuff in a second? Actually, care to explain the dump and sed call?
It looks like you're using this dump to encode the message IDs. If
format=batch-tag skips a message for some reason or terminates early,
the test won't fail.

- Patch 16:

+message-ids may contained arbitrary non-null characters. Note also

I think this should be "may contain", or something else entirely if
you're trying to describe past behavior of sup?

Ethan