From: Johan Herland <johan@herland.net>
Date: Tue, 16 Feb 2010 01:47:49 GMT
Subject: Re: [PATCHv13 28/30] builtin-notes: Add -c/-C options for reusing notes
Message-ID: <201002160247.49719.johan@herland.net>
In-Reply-To: <4B792347.8070208@gmail.com>

On Monday 15 February 2010, Stephen Boyd wrote:
> On 02/13/2010 01:28 PM, Johan Herland wrote:
> > @@ -199,6 +203,40 @@ static int parse_file_arg(const struct option
> > *opt, const char *arg, int unset)
> > 
> >  	return 0;
> >  
> >  }
> > 
> > +static int parse_reuse_arg(const struct option *opt, const char *arg,
> > int unset) +{
> > +	struct msg_arg *msg = opt->value;
> > +	char *buf;
> > +	unsigned char object[20];
> > +	enum object_type type;
> > +	unsigned long len;
> > +
> > +	if (!arg)
> > +		return -1;
> 
> This is impossible unless you're using the PARSE_OPT_OPTARG flag or
> allowing negation (i.e. --no-reuse-mesage). You should probably make the
> two callback options PARSE_OPT_NONEG and then drop this if statement.
> Same applies to some of the other callbacks not introduced in this patch.

Thanks. Fixed locally. Will be part of the next iteration.

> > +
> > +	if (msg->buf.len)
> > +		strbuf_addstr(&(msg->buf), "\n");
> > +
> 
> Use strbuf_addch()? I saw this in a couple other patches too.

Of course. Thanks for noticing. Fixed locally. Will be part of the next 
iteration.


Thanks for the review! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net