Re: [PATCH] strbuf_readlink semantics update.

From: Pierre Habouzit <madcoder@debian.org>
Date: Jan 4, 2009, 12:21 UTC
Message-ID: <20090104122108.GC29325@artemis.corp>
In-Reply-To: <7viqp8afap.fsf@gitster.siamese.dyndns.org>

On Thu, Dec 25, 2008 at 07:23:58AM +0000, Junio C Hamano wrote:

Show 20 quoted lines

> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
> > Pierre Habouzit schrieb:
> >> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:
> >>>
> >>> On Tue, 23 Dec 2008, Pierre Habouzit wrote:
> >>>> when readlink fails, the strbuf shall not be destroyed. It's not how
> >>>> read_file_or_gitlink works for example.
> >>> I disagree.
> >>>
> >>> This patch just makes things worse. Just leave the "strbuf_release()" in 
> >>> _one_ place.
> > ...
> > The "append or do nothing" rule is broken by strbuf_getline(), but I agree
> > to your reasoning.  How about refining this rule a bit to "do your thing
> > and roll back changes if an error occurs"?  I think it's not worth to undo
> > allocation extensions, but making reverting first time allocations seems
> > like a good idea.  Something like this?
> 
> I think this is much better than Pierre's.

I agree it's a fine semantics.

Show 5 quoted lines

> Pierre's "if it is called strbuf_*, it should always append" is a good
> uniformity to have in an API, but making the caller suffer for
> clean-up is going backwards.  The reason we use strbuf when we can is
> so that the callers do not have to worry about memory allocation
> issues too much.

Ack.

Sorry for the delay I was on vacation.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org