Re: [PATCH] strbuf_readlink semantics update.

From: Pierre Habouzit <madcoder@debian.org>
Date: Dec 24, 2008, 10:11 UTC
Message-ID: <20081224101146.GA10008@artemis.corp>
In-Reply-To: <alpine.LFD.2.00.0812231009220.3535@localhost.localdomain>

On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:

Show 11 quoted lines

> 
> 
> 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 "problem" is that the strbuf API usually works that way: functions append things to a buffer, or do nothing, but always keep the buffer in a state where you can append more stuff to it.

If read_file_or_gitlink or strbuf_readlink destroy the buffer, then you break the second expectation people (should) have about the strbuf API.

The reason is that if you built things in the buffer, you really don't want it to be undone just because the last bit you add went wrong for some reason. Or if you have a buffer that is reused in a loop, you don't want the buffer you allocated to be dropped just because one error occurred.

Alternatively, we could pass a flag to tell function performing reads (fread, read, readlink, whatever) that those should destroy the buffer on error or just report it. I don't really know. It sounds like over-engineering though.

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