Date: Wed, 25 Nov 2009 22:50:08 +0100
From: Jan Janak <jan@ryngle.com>
To: Bart Trojanowski <bart@jukie.net>
Message-ID: <20091125215008.GG25119@ryngle.com>
References: <1259100630-13673-1-git-send-email-jan@ryngle.com>
	<1259100630-13673-2-git-send-email-jan@ryngle.com>
	<1259100630-13673-3-git-send-email-jan@ryngle.com>
	<20091125205639.GJ29312@jukie.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20091125205639.GJ29312@jukie.net>
User-Agent: Mutt/1.5.18 (2008-05-17)
Cc: notmuch@notmuchmail.org
Subject: Re: [notmuch] [PATCH 2/4] notmuch: Config option to specify tags	to
	be applied by 'notmuch new'.
Precedence: list

Hi Bart,

On 25-11 15:56, Bart Trojanowski wrote:
> Jan,
> 
> I really want this feature to get in, so I am going to do my best to
> review your code :)
> 
> Here are some more sticking points...
> 
> > +char **
> > +notmuch_config_get_new_tags (notmuch_config_t *config, size_t *length);
> 
> If you are not giving over control of the pointer to the caller please
> return const char * const *.

I followed Carl's style there, in particular the following function:
notmuch_config_get_user_other_email

I can, of course, change that. But maybe we should wait for Carl to see
which way he prefers.

> Similarly...
> 
> > +    char **new_tags;
> 
> ... this should probably be const char **.

That's the same story. I followed user_other_email there.

> Next...
> 
> > +char **
> > +notmuch_config_get_new_tags (notmuch_config_t *config, size_t *length)
> 
> ... but ...
> 
> > +    unsigned int count, i;
> > +
> > +    if ((tags = notmuch_config_get_new_tags (config, &count)) == NULL)
> > +	return;
> 
> size_t != unsigned int on all platforms.  Please stick with one or the
> other.  Note there are a few calls to fix here.

That's a good catch. I will fix that one. Thanks a lot for the review, I
really appreciate that!

  -- Jan