From: Tomi Ollila <tomi.ollila@iki.fi>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 2/3] cli: refactor "notmuch tag" query tagging into a
	separate function
In-Reply-To: <87limo2xpk.fsf@nikula.org>
References: <cover.1332702915.git.jani@nikula.org>
	<584067da00dd9f04a7f3982b76fd4b4918e4fe61.1332702915.git.jani@nikula.org>
	<m262dse7f0.fsf@guru.guru-group.fi>
	<87limo2xpk.fsf@nikula.org>User-Agent: Notmuch/0.12+71~gdacf76b
	(http://notmuchmail.org) Emacs/23.3.1 (x86_64-unknown-linux-gnu)
Date: Mon, 26 Mar 2012 00:26:50 +0300
Message-ID: <m2398we5id.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Jani Nikula <jani@nikula.org> writes:

> On Sun, 25 Mar 2012 23:45:39 +0300, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>> Jani Nikula <jani@nikula.org> writes:
>> 
>> > Refactor to make tagging code easier to reuse in the future. No
>> > functional changes.
>> >
>> > Signed-off-by: Jani Nikula <jani@nikula.org>
>> > ---
>> 
>> tag_query() is pretty generic name for a function which (also) does
>> tagging operations (adds and removes tags based on tag_ops).
>
> Mmmh, if you think of "tag" as a verb, it fairly accurately describes
> what the function does: tag (messages matching this) query (according
> given arguments). I don't mind changing, but can't come up with anything
> better right now either. notmuch_{search,query}_change_tags()? Meh?

Hmmm, yes... -- tag...query... ok, I can live with that if no-one
else get same impression I got first...

>
>> If, however, tag_opts != NULL (as is needs to be) but tag_opts[0] == NULL
>> (now tag_query() would not do any tagging operations, but
>> optimize_tag_query would mangle query string ( '*' to '()' and 
>> 'any_other' to '( any_other ) and ()' )
>
> I'm not sure I follow you. AFAICT the behaviour does not change from
> what it was before, apart from the tag addition and removal being mixed
> together.

The thing is that notmuch_tag_command () check that there is at least
one tagging operation to be done, but tag_query () doesn't do such
checking...

>> I.e. IMO the function name should be more spesific & the fact that this
>> function needs tag changing data in tag_ops array should be documented.
>
> Documentation good. :)

... therefore the requirement that there needs to tagging information
in tag_ops is in place.

>
>> Minor thing in patch #1:
>> 
>>  +	    tag_ops[tag_ops_count].remove = argv[i][0] == '-';
>> 
>> would be more clearer as:
>> 
>>  +	    tag_ops[tag_ops_count].remove = (argv[i][0] == '-');
>
> I usually don't add braces where they aren't needed, but can do here.

Assigment is much lower in precedence than equality comparison -- but
as this is so seldom-used construct, humans read (with understanding)
the code faster with this added clarity.

>> Everything else LGTM.
>
> Thanks for the review,
> Jani.
>
>> 
>> Tomi

Tomi

>> 
>> 
>> >  notmuch-tag.c |  101 ++++++++++++++++++++++++++++++++-------------------------
>> >  1 files changed, 57 insertions(+), 44 deletions(-)
>> >
>> > diff --git a/notmuch-tag.c b/notmuch-tag.c
>> > index c39b235..edbfdec 100644
>> > --- a/notmuch-tag.c
>> > +++ b/notmuch-tag.c
>> > @@ -106,6 +106,60 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
>> >      return query_string;
>> >  }
>> >  
>> > +static int
>> > +tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
>> > +	   tag_operation_t *tag_ops, notmuch_bool_t synchronize_flags)
>> > +{
>> > +    notmuch_query_t *query;
>> > +    notmuch_messages_t *messages;
>> > +    notmuch_message_t *message;
>> > +    int i;
>> > +
>> > +    /* Optimize the query so it excludes messages that already have
>> > +     * the specified set of tags. */
>> > +    query_string = _optimize_tag_query (ctx, query_string, tag_ops);
>> > +    if (query_string == NULL) {
>> > +	fprintf (stderr, "Out of memory.\n");
>> > +	return 1;
>> > +    }
>> > +
>> > +    query = notmuch_query_create (notmuch, query_string);
>> > +    if (query == NULL) {
>> > +	fprintf (stderr, "Out of memory.\n");
>> > +	return 1;
>> > +    }
>> > +
>> > +    /* tagging is not interested in any special sort order */
>> > +    notmuch_query_set_sort (query, NOTMUCH_SORT_UNSORTED);
>> > +
>> > +    for (messages = notmuch_query_search_messages (query);
>> > +	 notmuch_messages_valid (messages) && !interrupted;
>> > +	 notmuch_messages_move_to_next (messages))
>> > +    {
>> > +	message = notmuch_messages_get (messages);
>> > +
>> > +	notmuch_message_freeze (message);
>> > +
>> > +	for (i = 0; tag_ops[i].tag; i++) {
>> > +	    if (tag_ops[i].remove)
>> > +		notmuch_message_remove_tag (message, tag_ops[i].tag);
>> > +	    else
>> > +		notmuch_message_add_tag (message, tag_ops[i].tag);
>> > +	}
>> > +
>> > +	notmuch_message_thaw (message);
>> > +
>> > +	if (synchronize_flags)
>> > +	    notmuch_message_tags_to_maildir_flags (message);
>> > +
>> > +	notmuch_message_destroy (message);
>> > +    }
>> > +
>> > +    notmuch_query_destroy (query);
>> > +
>> > +    return interrupted;
>> > +}
>> > +
>> >  int
>> >  notmuch_tag_command (void *ctx, int argc, char *argv[])
>> >  {
>> > @@ -114,12 +168,10 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>> >      char *query_string;
>> >      notmuch_config_t *config;
>> >      notmuch_database_t *notmuch;
>> > -    notmuch_query_t *query;
>> > -    notmuch_messages_t *messages;
>> > -    notmuch_message_t *message;
>> >      struct sigaction action;
>> >      notmuch_bool_t synchronize_flags;
>> >      int i;
>> > +    int ret;
>> >  
>> >      /* Setup our handler for SIGINT */
>> >      memset (&action, 0, sizeof (struct sigaction));
>> > @@ -166,14 +218,6 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>> >  	return 1;
>> >      }
>> >  
>> > -    /* Optimize the query so it excludes messages that already have
>> > -     * the specified set of tags. */
>> > -    query_string = _optimize_tag_query (ctx, query_string, tag_ops);
>> > -    if (query_string == NULL) {
>> > -	fprintf (stderr, "Out of memory.\n");
>> > -	return 1;
>> > -    }
>> > -
>> >      config = notmuch_config_open (ctx, NULL, NULL);
>> >      if (config == NULL)
>> >  	return 1;
>> > @@ -185,40 +229,9 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>> >  
>> >      synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
>> >  
>> > -    query = notmuch_query_create (notmuch, query_string);
>> > -    if (query == NULL) {
>> > -	fprintf (stderr, "Out of memory.\n");
>> > -	return 1;
>> > -    }
>> > -
>> > -    /* tagging is not interested in any special sort order */
>> > -    notmuch_query_set_sort (query, NOTMUCH_SORT_UNSORTED);
>> > +    ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);
>> >  
>> > -    for (messages = notmuch_query_search_messages (query);
>> > -	 notmuch_messages_valid (messages) && !interrupted;
>> > -	 notmuch_messages_move_to_next (messages))
>> > -    {
>> > -	message = notmuch_messages_get (messages);
>> > -
>> > -	notmuch_message_freeze (message);
>> > -
>> > -	for (i = 0; tag_ops[i].tag; i++) {
>> > -	    if (tag_ops[i].remove)
>> > -		notmuch_message_remove_tag (message, tag_ops[i].tag);
>> > -	    else
>> > -		notmuch_message_add_tag (message, tag_ops[i].tag);
>> > -	}
>> > -
>> > -	notmuch_message_thaw (message);
>> > -
>> > -	if (synchronize_flags)
>> > -	    notmuch_message_tags_to_maildir_flags (message);
>> > -
>> > -	notmuch_message_destroy (message);
>> > -    }
>> > -
>> > -    notmuch_query_destroy (query);
>> >      notmuch_database_close (notmuch);
>> >  
>> > -    return interrupted;
>> > +    return ret;
>> >  }
>> > -- 
>> > 1.7.5.4
>> >
>> > _______________________________________________
>> > notmuch mailing list
>> > notmuch@notmuchmail.org
>> > http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch