From: Shawn O. Pearce <spearce@spearce.org>
Date: Thu, 13 Aug 2009 14:40:07 GMT
Subject: Re: [PATCH 1/4] fast-import: put option parsing code in seperate functions
Message-ID: <20090813144007.GJ1033@spearce.org>
In-Reply-To: <alpine.DEB.1.00.0908131208400.7429@intel-tinevez-2-302>

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Wed, 12 Aug 2009, Sverre Rabbelier wrote:
> 
> > +static void option_export_marks(const char *marks)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	strbuf_addstr(&buf, marks);
> > +	mark_file = strbuf_detach(&buf, NULL);
> > +}
> 
> Heh, this is a pretty convoluted way to write
> 
> 	mark_file = xstrdup(marks);
> 
> ;-)

Agreed.

> > +static void option_force()
> > +{
> > +	force_update = 1;
> > +}
> 
> I'm not sure that I would put these simple assignments in separate 
> functions, but that's certainly up to you!

Oh, good point.  Yes, please don't do that, please just write these
directly into the option parser.

Aside from these remarks, this patch looks good to me.

-- 
Shawn.