Date: Fri, 23 Dec 2011 22:45:00 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
Subject: Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME
	traversal.
Message-ID: <20111224034500.GA1927@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>
	<87k46572f7.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87k46572f7.fsf@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Thanks for the thorough review!

Quoth Dmitry Kurochkin on Dec 10 at  3:25 am:
> 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.

Good point.  Renamed to nchildren, which is shorter but still conveys
that it's a count.

> +    notmuch_bool_t decrypt_success;
> 
> Perhaps s/decrypt_success/is_decrypted/ for consistency with
> is_encrypted?

I actually got rid of is_encrypted/is_signed in the new version (see
below).

> +mime_node_child (const mime_node_t *parent, int child);
> +
>  #include "command-line-arguments.h"
>  #endif
> 
> #include should go above declarations.

That one's bremner's fault.  I'll follow up with a patch to move this.

> +    mime_node_t *out = talloc_zero (parent, mime_node_t);
> 
> Perhaps s/out/node/?

Done.

> +           /* 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.

Done.

> +           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.

Good point.  I replaced these fields with new fields,
decrypt_attempted and sig_attempted, which are used the way the old
fields were, but actually reflect the desired semantics and the
information that the callers need.  I first tried keeping the is_*
fields and making them reflect the purely structural properties of the
message, but then I realized that that was both redundant with the
type of the MIME part and wasn't what callers actually needed to know.
As an added benefit, sig_attempted sidesteps the question of whether a
multipart/{signed,encrypted} without any signatures is "signed" and
makes it possible for callers to distinguish between unsigned parts,
signature verification failures, and encrypted parts with no signers.

> +           /* 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 :)

It looks like they're both non-const in 2.6 (which makes more sense to
me).  I updated the comment to mention this.

> 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?

Oops, it wasn't supposed to be asymmetric.  Decryption now requires
cryptoctx to be set (it probably would have crashed before without
it).

> 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).

The above two were superseded by the next change.

> +           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);

Good idea.  This made things compact enough to simplify the rest of
this function.

> Regards,
>   Dmitry
> 

-- 
Austin Clements                                      MIT/'06/PhD/CSAIL
amdragon@mit.edu                           http://web.mit.edu/amdragon
       Somewhere in the dream we call reality you will find me,
              searching for the reality we call dreams.