From: Junio C Hamano <gitster@pobox.com>
To: "René Scharfe" <rene.scharfe@lsrfire.ath.cx>
Cc: Pierre Habouzit <madcoder@debian.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
git@vger.kernel.org
Subject: Re: [PATCH] strbuf_readlink semantics update.
Date: Wed, 24 Dec 2008 23:23:58 -0800 [thread overview]
Message-ID: <7viqp8afap.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <4952532F.5050704@lsrfire.ath.cx> (René Scharfe's message of "Wed, 24 Dec 2008 16:20:15 +0100")
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. 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.
next prev parent reply other threads:[~2008-12-25 7:25 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-12-17 18:42 [PATCH 0/5] Be careful about lstat()-vs-readlink() Linus Torvalds
2008-12-17 18:42 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Linus Torvalds
2008-12-17 18:43 ` [PATCH 2/5] Make 'ce_compare_link()' use the new 'strbuf_readlink()' Linus Torvalds
2008-12-17 18:43 ` [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' Linus Torvalds
2008-12-17 18:44 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Linus Torvalds
2008-12-17 18:45 ` [PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks Linus Torvalds
2008-12-17 20:37 ` [PATCH 6/5] make_absolute_path(): check bounds when seeing an overlong symlink Junio C Hamano
2008-12-17 20:37 ` [PATCH 7/5] builtin-blame.c: use strbuf_readlink() Junio C Hamano
2008-12-17 20:37 ` [PATCH 8/5] combine-diff.c: " Junio C Hamano
2008-12-17 21:02 ` Linus Torvalds
2008-12-17 21:34 ` Junio C Hamano
2008-12-17 20:37 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Junio C Hamano
2008-12-18 12:11 ` Mark Burton
2008-12-18 16:55 ` Linus Torvalds
2008-12-18 17:41 ` René Scharfe
2008-12-18 17:49 ` Linus Torvalds
2008-12-18 17:56 ` Olivier Galibert
2008-12-18 16:56 ` René Scharfe
2008-12-18 17:28 ` René Scharfe
2008-12-19 22:10 ` [PATCH] diff.c: fix pointer type warning René Scharfe
2008-12-19 23:09 ` Junio C Hamano
2008-12-17 20:37 ` [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' Junio C Hamano
2008-12-17 21:26 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Jay Soffian
2008-12-17 21:44 ` Linus Torvalds
2008-12-23 10:05 ` [PATCH] strbuf_readlink semantics update Pierre Habouzit
2008-12-23 10:21 ` Pierre Habouzit
2008-12-23 18:16 ` Linus Torvalds
2008-12-24 10:11 ` Pierre Habouzit
2008-12-24 15:20 ` René Scharfe
2008-12-25 7:23 ` Junio C Hamano [this message]
2009-01-04 12:21 ` Pierre Habouzit
2009-01-06 20:41 ` [PATCH] strbuf: instate cleanup rule in case of non-memory errors René Scharfe
2009-01-07 21:19 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7viqp8afap.fsf@gitster.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=madcoder@debian.org \
--cc=rene.scharfe@lsrfire.ath.cx \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).