All of lore.kernel.org
 help / color / mirror / Atom feed
From: Pierre Habouzit <madcoder@debian.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: "René Scharfe" <rene.scharfe@lsrfire.ath.cx>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	git@vger.kernel.org
Subject: Re: [PATCH] strbuf_readlink semantics update.
Date: Sun, 04 Jan 2009 13:21:08 +0100	[thread overview]
Message-ID: <20090104122108.GC29325@artemis.corp> (raw)
In-Reply-To: <7viqp8afap.fsf@gitster.siamese.dyndns.org>

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

On Thu, Dec 25, 2008 at 07:23:58AM +0000, Junio C Hamano wrote:
> 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.

> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

  reply	other threads:[~2009-01-04 12:22 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
2009-01-04 12:21               ` Pierre Habouzit [this message]
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=20090104122108.GC29325@artemis.corp \
    --to=madcoder@debian.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.