Re: [PATCH] strbuf_readlink semantics update.

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.

Look:

   6 files changed, 15 insertions(+), 5 deletions(-)

you added ten unnecessary lines, and you made the interface harder to use. 
What was the gain here?

Fix read_old_data possible leaks in case of errors, since even when no
data has been read, the strbufs may have grown to prepare the reads.
strbuf_release must be called on them.

That's a separate error, and quite frankly, the best approach to that is 
likely to instead of breaking strbuf_readlink(), just make the S_IFREG() 
case release it.

I'd suggest that strbuf_read_file() should probably also do a 
strbuf_release() if it returns a negative error value, but that's a 
separate issue (and still leaves "read_old_data()" having to release 
things, since read_old_data() wants to see exactly st_size bytes. Although 
I suspect we might want to change that, and just make it test for 
negative too).

		Linus