david@tethera.net writes: > This obsoletes the series at: > > id:"1344888831-4301-1-git-send-email-bremner@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