[PATCH v4 1/5] cli: slightly refactor "notmuch reply" address scanning functions
On Thu, 12 Jan 2012 16:59:05 -0500, Austin Clements wrote: > LGTM. One thing you could fix below (and a few comments), but not > enough alone to warrant a new version. > > Quoth Jani Nikula on Jan 12 at 11:40 pm: > > Slightly refactor "notmuch reply" recipient and user from address scanning > > functions in preparation for reply-to-sender feature. > > > > Add support for not adding messages at all (just scan for user from > > address), and returning the number of messages added. > > > > No externally visible functional changes. > > > > Signed-off-by: Jani Nikula > > --- > > notmuch-reply.c | 74 > > -- > > 1 files changed, 38 insertions(+), 36 deletions(-) > > > > diff --git a/notmuch-reply.c b/notmuch-reply.c > > index 000f6da..4fae66f 100644 > > --- a/notmuch-reply.c > > +++ b/notmuch-reply.c > > @@ -168,22 +168,28 @@ address_is_users (const char *address, > > notmuch_config_t *config) > > return 0; > > } > > > > -/* For each address in 'list' that is not configured as one of the > > - * user's addresses in 'config', add that address to 'message' as an > > - * address of 'type'. > > +/* Scan addresses in 'list'. > > * > > - * The first address encountered that *is* the user's address will be > > - * returned, (otherwise NULL is returned). > > + * If 'message' is non-NULL, then for each address in 'list' that is not > > + * configured as one of the user's addresses in 'config', add that address > > to > > + * 'message' as an address of 'type'. > > + * > > + * If 'user_from' is non-NULL and *user_from is NULL, the first address > > + * encountered in 'list' that *is* the user's address will be set to > > *user_from. > > + * > > + * Return the number of addresses added to 'message'. (If 'message' is > > NULL, the > > + * function returns 0 by definition.) > > Ah, I like the return value. Better than adding an umpteenth argument > like I was suggesting. > > > */ > > -static const char * > > -add_recipients_for_address_list (GMimeMessage *message, > > -notmuch_config_t *config, > > -GMimeRecipientType type, > > -InternetAddressList *list) > > +static unsigned int > > +scan_address_list (InternetAddressList *list, > > + notmuch_config_t *config, > > + GMimeMessage *message, > > + GMimeRecipientType type, > > + const char **user_from) > > { > > InternetAddress *address; > > int i; > > -const char *ret = NULL; > > +unsigned int n = 0; > > > > for (i = 0; i < internet_address_list_length (list); i++) { > > address = internet_address_list_get_address (list, i); > > @@ -196,8 +202,7 @@ add_recipients_for_address_list (GMimeMessage *message, > > if (group_list == NULL) > > continue; > > > > - add_recipients_for_address_list (message, config, > > -type, group_list); > > + n += scan_address_list (group_list, config, message, type, NULL); > > Should the NULL above be user_from? You're being compatible with the > original code, which is the right thing to do in this patch, but the > new-found explicitness made me wonder if this is actually a bug. I didn't even know what a group list was, and if [1] is accurate, I don't think I've ever seen one either. This does smell like a bug, but I'm doubtful if anyone would ever notice... Anyway, as you say, it should be another patch, and independent of this series. BR, Jani. [1] http://www.cs.tut.fi/~jkorpela/rfc/822addr.html > > > } else { > > InternetAddressMailbox *mailbox; > > const char *name; > > @@ -209,40 +214,40 @@ add_recipients_for_address_list (GMimeMessage > > *message, > > addr = internet_address_mailbox_get_addr (mailbox); > > > > if (address_is_users (addr, config)) { > > - if (ret == NULL) > > - ret = addr; > > - } else { > > + if (user_from && *user_from == NULL) > > + *user_from = addr; > > + } else if (message) { > > g_mime_message_add_recipient (message, type, name, addr); > > + n++; > > } > > } > > } > > > > -return ret; > > +return n; > > } > > > > -/* For each address in 'recipients' that is not configured as one of > > - * the user's addresses in 'config', add that address to 'message' as > > - * an address of 'type'. > > +/* Scan addresses in 'recipients'. > > * > > - * The first address encountered that *is* the user's address will be > > - * returned, (otherwise NULL is returned). > > + * See the documentation of scan_address_list() above. This function does > > + * exactly the same, but converts 'recipients' to an InternetAddressList > > first. > > Just bikeshedding, but comments in the notmuch codebase almost > universally wrap at 72 columns, not 80 (based on > egrep '[
[PATCH v4 1/5] cli: slightly refactor "notmuch reply" address scanning functions
Slightly refactor "notmuch reply" recipient and user from address scanning functions in preparation for reply-to-sender feature. Add support for not adding messages at all (just scan for user from address), and returning the number of messages added. No externally visible functional changes. Signed-off-by: Jani Nikula --- notmuch-reply.c | 74 -- 1 files changed, 38 insertions(+), 36 deletions(-) diff --git a/notmuch-reply.c b/notmuch-reply.c index 000f6da..4fae66f 100644 --- a/notmuch-reply.c +++ b/notmuch-reply.c @@ -168,22 +168,28 @@ address_is_users (const char *address, notmuch_config_t *config) return 0; } -/* For each address in 'list' that is not configured as one of the - * user's addresses in 'config', add that address to 'message' as an - * address of 'type'. +/* Scan addresses in 'list'. * - * The first address encountered that *is* the user's address will be - * returned, (otherwise NULL is returned). + * If 'message' is non-NULL, then for each address in 'list' that is not + * configured as one of the user's addresses in 'config', add that address to + * 'message' as an address of 'type'. + * + * If 'user_from' is non-NULL and *user_from is NULL, the first address + * encountered in 'list' that *is* the user's address will be set to *user_from. + * + * Return the number of addresses added to 'message'. (If 'message' is NULL, the + * function returns 0 by definition.) */ -static const char * -add_recipients_for_address_list (GMimeMessage *message, -notmuch_config_t *config, -GMimeRecipientType type, -InternetAddressList *list) +static unsigned int +scan_address_list (InternetAddressList *list, + notmuch_config_t *config, + GMimeMessage *message, + GMimeRecipientType type, + const char **user_from) { InternetAddress *address; int i; -const char *ret = NULL; +unsigned int n = 0; for (i = 0; i < internet_address_list_length (list); i++) { address = internet_address_list_get_address (list, i); @@ -196,8 +202,7 @@ add_recipients_for_address_list (GMimeMessage *message, if (group_list == NULL) continue; - add_recipients_for_address_list (message, config, -type, group_list); + n += scan_address_list (group_list, config, message, type, NULL); } else { InternetAddressMailbox *mailbox; const char *name; @@ -209,40 +214,40 @@ add_recipients_for_address_list (GMimeMessage *message, addr = internet_address_mailbox_get_addr (mailbox); if (address_is_users (addr, config)) { - if (ret == NULL) - ret = addr; - } else { + if (user_from && *user_from == NULL) + *user_from = addr; + } else if (message) { g_mime_message_add_recipient (message, type, name, addr); + n++; } } } -return ret; +return n; } -/* For each address in 'recipients' that is not configured as one of - * the user's addresses in 'config', add that address to 'message' as - * an address of 'type'. +/* Scan addresses in 'recipients'. * - * The first address encountered that *is* the user's address will be - * returned, (otherwise NULL is returned). + * See the documentation of scan_address_list() above. This function does + * exactly the same, but converts 'recipients' to an InternetAddressList first. */ -static const char * -add_recipients_for_string (GMimeMessage *message, - notmuch_config_t *config, - GMimeRecipientType type, - const char *recipients) +static unsigned int +scan_address_string (const char *recipients, +notmuch_config_t *config, +GMimeMessage *message, +GMimeRecipientType type, +const char **user_from) { InternetAddressList *list; if (recipients == NULL) - return NULL; + return 0; list = internet_address_list_parse_string (recipients); if (list == NULL) - return NULL; + return 0; -return add_recipients_for_address_list (message, config, type, list); +return scan_address_list (list, config, message, type, user_from); } /* Does the address in the Reply-To header of 'message' already appear @@ -324,7 +329,7 @@ add_recipients_from_message (GMimeMessage *reply, } for (i = 0; i < ARRAY_SIZE (reply_to_map); i++) { - const char *addr, *recipients; + const char *recipients; recipients = notmuch_message_get_header (message, reply_to_map[i].header); @@ -332,11 +337,8 @@
[PATCH v4 1/5] cli: slightly refactor "notmuch reply" address scanning functions
This is nice. Two very minor nits: In your commit message you say > Add support for not adding messages at all (just scan for user from > address), and returning the number of messages added. and I think both "messages" should "recipients". > + * If 'user_from' is non-NULL and *user_from is NULL, the first address > + * encountered in 'list' that *is* the user's address will be set to > *user_from. This reads oddly: you are setting *user_from to the first address... (rather than the other way round) Best wishes Mark
[PATCH v4 1/5] cli: slightly refactor "notmuch reply" address scanning functions
LGTM. One thing you could fix below (and a few comments), but not enough alone to warrant a new version. Quoth Jani Nikula on Jan 12 at 11:40 pm: > Slightly refactor "notmuch reply" recipient and user from address scanning > functions in preparation for reply-to-sender feature. > > Add support for not adding messages at all (just scan for user from > address), and returning the number of messages added. > > No externally visible functional changes. > > Signed-off-by: Jani Nikula > --- > notmuch-reply.c | 74 -- > 1 files changed, 38 insertions(+), 36 deletions(-) > > diff --git a/notmuch-reply.c b/notmuch-reply.c > index 000f6da..4fae66f 100644 > --- a/notmuch-reply.c > +++ b/notmuch-reply.c > @@ -168,22 +168,28 @@ address_is_users (const char *address, notmuch_config_t > *config) > return 0; > } > > -/* For each address in 'list' that is not configured as one of the > - * user's addresses in 'config', add that address to 'message' as an > - * address of 'type'. > +/* Scan addresses in 'list'. > * > - * The first address encountered that *is* the user's address will be > - * returned, (otherwise NULL is returned). > + * If 'message' is non-NULL, then for each address in 'list' that is not > + * configured as one of the user's addresses in 'config', add that address to > + * 'message' as an address of 'type'. > + * > + * If 'user_from' is non-NULL and *user_from is NULL, the first address > + * encountered in 'list' that *is* the user's address will be set to > *user_from. > + * > + * Return the number of addresses added to 'message'. (If 'message' is NULL, > the > + * function returns 0 by definition.) Ah, I like the return value. Better than adding an umpteenth argument like I was suggesting. > */ > -static const char * > -add_recipients_for_address_list (GMimeMessage *message, > - notmuch_config_t *config, > - GMimeRecipientType type, > - InternetAddressList *list) > +static unsigned int > +scan_address_list (InternetAddressList *list, > +notmuch_config_t *config, > +GMimeMessage *message, > +GMimeRecipientType type, > +const char **user_from) > { > InternetAddress *address; > int i; > -const char *ret = NULL; > +unsigned int n = 0; > > for (i = 0; i < internet_address_list_length (list); i++) { > address = internet_address_list_get_address (list, i); > @@ -196,8 +202,7 @@ add_recipients_for_address_list (GMimeMessage *message, > if (group_list == NULL) > continue; > > - add_recipients_for_address_list (message, config, > - type, group_list); > + n += scan_address_list (group_list, config, message, type, NULL); Should the NULL above be user_from? You're being compatible with the original code, which is the right thing to do in this patch, but the new-found explicitness made me wonder if this is actually a bug. > } else { > InternetAddressMailbox *mailbox; > const char *name; > @@ -209,40 +214,40 @@ add_recipients_for_address_list (GMimeMessage *message, > addr = internet_address_mailbox_get_addr (mailbox); > > if (address_is_users (addr, config)) { > - if (ret == NULL) > - ret = addr; > - } else { > + if (user_from && *user_from == NULL) > + *user_from = addr; > + } else if (message) { > g_mime_message_add_recipient (message, type, name, addr); > + n++; > } > } > } > > -return ret; > +return n; > } > > -/* For each address in 'recipients' that is not configured as one of > - * the user's addresses in 'config', add that address to 'message' as > - * an address of 'type'. > +/* Scan addresses in 'recipients'. > * > - * The first address encountered that *is* the user's address will be > - * returned, (otherwise NULL is returned). > + * See the documentation of scan_address_list() above. This function does > + * exactly the same, but converts 'recipients' to an InternetAddressList > first. Just bikeshedding, but comments in the notmuch codebase almost universally wrap at 72 columns, not 80 (based on egrep '[ /]\* .{70}' *.[ch]*) > */ > -static const char * > -add_recipients_for_string (GMimeMessage *message, > -notmuch_config_t *config, > -GMimeRecipientType type, > -const char *recipients) > +static unsigned int > +scan_address_string (const char *recipients, > + notmuch_config_t *config, > + GMimeMessage *message, > + GMimeRecipientType type, > + const char **user_from) > { > InternetAddressList *list; > > if (recipients ==
[PATCH v4 1/5] cli: slightly refactor notmuch reply address scanning functions
Slightly refactor notmuch reply recipient and user from address scanning functions in preparation for reply-to-sender feature. Add support for not adding messages at all (just scan for user from address), and returning the number of messages added. No externally visible functional changes. Signed-off-by: Jani Nikula j...@nikula.org --- notmuch-reply.c | 74 -- 1 files changed, 38 insertions(+), 36 deletions(-) diff --git a/notmuch-reply.c b/notmuch-reply.c index 000f6da..4fae66f 100644 --- a/notmuch-reply.c +++ b/notmuch-reply.c @@ -168,22 +168,28 @@ address_is_users (const char *address, notmuch_config_t *config) return 0; } -/* For each address in 'list' that is not configured as one of the - * user's addresses in 'config', add that address to 'message' as an - * address of 'type'. +/* Scan addresses in 'list'. * - * The first address encountered that *is* the user's address will be - * returned, (otherwise NULL is returned). + * If 'message' is non-NULL, then for each address in 'list' that is not + * configured as one of the user's addresses in 'config', add that address to + * 'message' as an address of 'type'. + * + * If 'user_from' is non-NULL and *user_from is NULL, the first address + * encountered in 'list' that *is* the user's address will be set to *user_from. + * + * Return the number of addresses added to 'message'. (If 'message' is NULL, the + * function returns 0 by definition.) */ -static const char * -add_recipients_for_address_list (GMimeMessage *message, -notmuch_config_t *config, -GMimeRecipientType type, -InternetAddressList *list) +static unsigned int +scan_address_list (InternetAddressList *list, + notmuch_config_t *config, + GMimeMessage *message, + GMimeRecipientType type, + const char **user_from) { InternetAddress *address; int i; -const char *ret = NULL; +unsigned int n = 0; for (i = 0; i internet_address_list_length (list); i++) { address = internet_address_list_get_address (list, i); @@ -196,8 +202,7 @@ add_recipients_for_address_list (GMimeMessage *message, if (group_list == NULL) continue; - add_recipients_for_address_list (message, config, -type, group_list); + n += scan_address_list (group_list, config, message, type, NULL); } else { InternetAddressMailbox *mailbox; const char *name; @@ -209,40 +214,40 @@ add_recipients_for_address_list (GMimeMessage *message, addr = internet_address_mailbox_get_addr (mailbox); if (address_is_users (addr, config)) { - if (ret == NULL) - ret = addr; - } else { + if (user_from *user_from == NULL) + *user_from = addr; + } else if (message) { g_mime_message_add_recipient (message, type, name, addr); + n++; } } } -return ret; +return n; } -/* For each address in 'recipients' that is not configured as one of - * the user's addresses in 'config', add that address to 'message' as - * an address of 'type'. +/* Scan addresses in 'recipients'. * - * The first address encountered that *is* the user's address will be - * returned, (otherwise NULL is returned). + * See the documentation of scan_address_list() above. This function does + * exactly the same, but converts 'recipients' to an InternetAddressList first. */ -static const char * -add_recipients_for_string (GMimeMessage *message, - notmuch_config_t *config, - GMimeRecipientType type, - const char *recipients) +static unsigned int +scan_address_string (const char *recipients, +notmuch_config_t *config, +GMimeMessage *message, +GMimeRecipientType type, +const char **user_from) { InternetAddressList *list; if (recipients == NULL) - return NULL; + return 0; list = internet_address_list_parse_string (recipients); if (list == NULL) - return NULL; + return 0; -return add_recipients_for_address_list (message, config, type, list); +return scan_address_list (list, config, message, type, user_from); } /* Does the address in the Reply-To header of 'message' already appear @@ -324,7 +329,7 @@ add_recipients_from_message (GMimeMessage *reply, } for (i = 0; i ARRAY_SIZE (reply_to_map); i++) { - const char *addr, *recipients; + const char *recipients; recipients = notmuch_message_get_header (message, reply_to_map[i].header); @@ -332,11 +337,8
Re: [PATCH v4 1/5] cli: slightly refactor notmuch reply address scanning functions
This is nice. Two very minor nits: In your commit message you say Add support for not adding messages at all (just scan for user from address), and returning the number of messages added. and I think both messages should recipients. + * If 'user_from' is non-NULL and *user_from is NULL, the first address + * encountered in 'list' that *is* the user's address will be set to *user_from. This reads oddly: you are setting *user_from to the first address... (rather than the other way round) Best wishes Mark ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch