From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME
	traversal.
In-Reply-To: <1323460468-4030-3-git-send-email-amdragon@mit.edu>
References: <1323027100-10307-1-git-send-email-amdragon@mit.edu>
	<1323460468-4030-1-git-send-email-amdragon@mit.edu>
	<1323460468-4030-3-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.10.2+82~g96a629c (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Sat, 10 Dec 2011 03:25:48 +0400
Message-ID: <87k46572f7.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Hi Austin.

+    /* The number of children of this part. */
+    int children;

Consider renaming to children_count or similar to make it clear that it
is a counter and not the actual children.

+    notmuch_bool_t decrypt_success;

Perhaps s/decrypt_success/is_decrypted/ for consistency with
is_encrypted?

+mime_node_child (const mime_node_t *parent, int child);
+
 #include "command-line-arguments.h"
 #endif

#include should go above declarations.

+    mime_node_t *out = talloc_zero (parent, mime_node_t);

Perhaps s/out/node/?

+           /* this violates RFC 3156 section 4, so we won't bother with it. */
+           fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
+                    "message (should be exactly 2)\n",
+                    out->children);

Perhaps s/should be exactly/must be exactly/?  That is what the
RFC says.  Same for signature.

+           out->is_encrypted = TRUE;
+           out->is_signed = TRUE;

These are set only if we do decryption/verification.  But their
names and comments imply that they should reflect properties of
the part and be set independently from whether we are actually
doing decryption or verification.

+           /* For some reason the GMimeSignatureValidity returned
+            * here is not a const (inconsistent with that
+            * returned by
+            * g_mime_multipart_encrypted_get_signature_validity,
+            * and therefore needs to be properly disposed of.
+            * Hopefully the API will become more consistent. */

Ouch.  In gmime 2.6 this API has changed, but looks like the
issue is still there.  Is there a bug for it?  If yes, we should
add a reference to the comment.  Otherwise, we should file the
bug and then add a reference to the comment :)

Both decryption and verification use cryptoctx.  But decryption
is done iff decrypt is true (without checking if cryptoctx is
set) and verification is done iff cryptoctx is set (without any
special flag).  Why is it asymmetric?  Do we need to check if
cryptoctx is set for decryption?

In mime_node_child():

+       GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);
+       if (child == 1 && parent->decrypted_child)
+           return _mime_node_create (parent, parent->decrypted_child);

Multipart is not needed here, please move it's declaration below
the condition.

+       GMimeObject *child = g_mime_message_get_mime_part (message);

The child variable eclipses the (int child) parameter.  Consider
using a different name for the variable (or parameter).

+           return _mime_node_create (parent, parent->decrypted_child);
+       return _mime_node_create (parent, g_mime_multipart_get_part (multipart, child));
...
+       return _mime_node_create (parent, child);

All returns are similar.  Consider declaring a local variable for
storing the part and using a single return, i.e.:

  GMimeObject *part;
  if (...)
    part = ...;
  else ...
    part = ...;

  return _mime_node_create (parent, part);

Regards,
  Dmitry