Re: [PATCH] strbuf_readlink semantics update.

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

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 "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 "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?

René


PS: only nine lines! ;-)


 strbuf.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index bdf4954..6ed0684 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -256,18 +256,21 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
 	size_t res;
+	size_t oldalloc = sb->alloc;
 
 	strbuf_grow(sb, size);
 	res = fread(sb->buf + sb->len, 1, size, f);
-	if (res > 0) {
+	if (res > 0)
 		strbuf_setlen(sb, sb->len + res);
-	}
+	else if (res < 0 && oldalloc == 0)
+		strbuf_release(sb);
 	return res;
 }
 
 ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 {
 	size_t oldlen = sb->len;
+	size_t oldalloc = sb->alloc;
 
 	strbuf_grow(sb, hint ? hint : 8192);
 	for (;;) {
@@ -275,7 +278,10 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 		cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
 		if (cnt < 0) {
-			strbuf_setlen(sb, oldlen);
+			if (oldalloc == 0)
+				strbuf_release(sb);
+			else
+				strbuf_setlen(sb, oldlen);
 			return -1;
 		}
 		if (!cnt)
@@ -292,6 +298,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 {
+	size_t oldalloc = sb->alloc;
+
 	if (hint < 32)
 		hint = 32;
 
@@ -311,7 +319,8 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 		/* .. the buffer was too small - try again */
 		hint *= 2;
 	}
-	strbuf_release(sb);
+	if (oldalloc == 0)
+		strbuf_release(sb);
 	return -1;
 }