git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Stefan Beller <sbeller@google.com>
Cc: Jonathan Nieder <jrnieder@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf
Date: Thu, 4 Dec 2014 04:04:06 -0500	[thread overview]
Message-ID: <20141204090406.GA27455@peff.net> (raw)
In-Reply-To: <CAGZ79kby-RdwzgXWvVgD42uiEis7SCnuAdSjO2EL03uN4s=LgA@mail.gmail.com>

On Thu, Dec 04, 2014 at 12:36:46AM -0800, Stefan Beller wrote:

> > Your solution adds a strbuf. That helps with context and stomping, but
> > loses readability and adds allocation.
> 
> > If we changed the strbuf to a fixed-size buffer, that would help the
> > allocation issue. Some messages might be truncated, but it seems
> > unlikely in practice. It still loses readability, though.
> 
> > What about a struct that has an errno-like value _and_ a fixed-size
> > buffer? I'm thinking something like:
> 
> What do you mean by the allocation being an issue?

I mean that the caller has to take care of releasing the memory. This
adds boilerplate to the caller, and is a potential source of leaks.

> We're only populating the error buffer in the error case, so you're
> not talking about performance/speed I'd assume?

No, I don't care about performance here. Only code maintainability.

> As error handling breaks in the least expected ways, I'd rather go
> with well tested string buffer codes there?

We'd still be building on snprintf. And with a known-size buffer, we can
wrap the formatting so that the callers don't even have to care (see the
mkerror example I posted).

-Peff

  reply	other threads:[~2014-12-04  9:04 UTC|newest]

Thread overview: 83+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-17 22:14 [PATCH] copy.c: make copy_fd preserve meaningful errno Stefan Beller
2014-11-17 23:08 ` Junio C Hamano
2014-11-17 23:13   ` Stefan Beller
2014-11-17 23:35 ` Jonathan Nieder
2014-11-18  0:18   ` Stefan Beller
2014-11-18  0:48     ` Jonathan Nieder
2014-11-18  1:01       ` Stefan Beller
2014-12-03  5:02         ` [PATCH 0/14] " Jonathan Nieder
2014-12-03  5:10           ` [PATCH 01/14] strbuf: introduce strbuf_prefixf() Jonathan Nieder
2014-12-03 20:10             ` Eric Sunshine
2014-12-03 20:14               ` Jonathan Nieder
2014-12-03 21:45             ` Junio C Hamano
2014-12-03 21:59               ` Jonathan Nieder
2014-12-03 22:09                 ` Jonathan Nieder
2014-12-03 22:40                 ` Junio C Hamano
2014-12-03  5:12           ` [PATCH 02/14] add_to_alternates_file: respect GIT_OBJECT_DIRECTORY Jonathan Nieder
2014-12-03 18:53             ` Junio C Hamano
2014-12-03  5:13           ` [PATCH 03/14] copy_fd: pass error message back through a strbuf Jonathan Nieder
2014-12-03 19:01             ` Junio C Hamano
2014-12-03 20:28               ` Jonathan Nieder
2014-12-03 21:00               ` Jeff King
2014-12-03 21:38                 ` Jonathan Nieder
2014-12-04  7:59                   ` Jeff King
2014-12-04  8:36                     ` Stefan Beller
2014-12-04  9:04                       ` Jeff King [this message]
2014-12-10 17:02                     ` Michael Haggerty
2014-12-10 19:00                       ` Junio C Hamano
2014-12-10 19:14                         ` Jeff King
2014-12-04  3:01               ` [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf) Jonathan Nieder
2014-12-04 23:27                 ` Junio C Hamano
2014-12-04 23:37                   ` Jonathan Nieder
2014-12-04 23:41                   ` Jonathan Nieder
2014-12-04 23:44                     ` Jeff King
2014-12-04 23:52                       ` Junio C Hamano
2014-12-05  0:01                         ` Jeff King
2014-12-05 18:00                           ` Junio C Hamano
2014-12-07 10:07                             ` Jeff King
2014-12-09 18:43                               ` Junio C Hamano
2014-12-09 18:49                                 ` Jeff King
2015-02-12 23:08                                 ` Junio C Hamano
2015-02-17 15:50                                   ` Michael Haggerty
2015-02-17 16:03                                     ` Junio C Hamano
2015-02-17 16:05                                       ` Jeff King
2015-02-17 22:46                                         ` Junio C Hamano
2014-12-03 20:02             ` [PATCH 03/14] copy_fd: pass error message back through a strbuf Junio C Hamano
2014-12-03 20:18               ` Jonathan Nieder
2014-12-03 21:43                 ` Junio C Hamano
2014-12-03 21:51                   ` Jonathan Nieder
2014-12-03 20:20               ` Stefan Beller
2014-12-03  5:14           ` [PATCH 04/14] hold_lock_file_for_append: " Jonathan Nieder
2014-12-03  6:09             ` Torsten Bögershausen
2014-12-03  7:04               ` Jonathan Nieder
2014-12-03  5:16           ` [PATCH 05/14] lock_packed_refs: " Jonathan Nieder
2014-12-03  5:19           ` [PATCH 06/14] lockfile: introduce flag for locks outside .git Jonathan Nieder
2014-12-03 23:13             ` Junio C Hamano
2014-12-03 23:24               ` Jonathan Nieder
2014-12-03 23:25               ` Junio C Hamano
2014-12-03 23:29                 ` Jonathan Nieder
2014-12-03 23:38                   ` Junio C Hamano
2014-12-03 23:41                     ` Jonathan Nieder
2014-12-03 23:43                     ` Junio C Hamano
2014-12-03 23:57                   ` Jeff King
2014-12-04  5:51                   ` Junio C Hamano
2014-12-04 17:56                     ` Jonathan Nieder
2014-12-03  5:19           ` [PATCH 07/14] fast-import: use message from lockfile API when writing marks fails Jonathan Nieder
2014-12-03  5:20           ` [PATCH 08/14] credentials: use message from lockfile API when locking ~/.git-credentials fails Jonathan Nieder
2014-12-03  5:21           ` [PATCH 09/14] config: use message from lockfile API when locking config file fails Jonathan Nieder
2014-12-03 19:59             ` Junio C Hamano
2014-12-03 20:16               ` Jonathan Nieder
2014-12-03  5:22           ` [PATCH 10/14] rerere: error out on autoupdate failure Jonathan Nieder
2014-12-03  5:25           ` [PATCH 11/14] hold_locked_index: pass error message back through a strbuf Jonathan Nieder
2014-12-03  5:26           ` [PATCH 12/14] hold_lock_file_for_update: " Jonathan Nieder
2014-12-03 18:53             ` Jonathan Nieder
2014-12-03  5:26           ` [PATCH 13/14] lockfile: remove unused function 'unable_to_lock_die' Jonathan Nieder
2014-12-03  5:27           ` [PATCH 14/14] lockfile: make 'unable_to_lock_message' private Jonathan Nieder
2014-12-03 20:42             ` Stefan Beller
2014-11-18 16:32   ` [PATCH] copy.c: make copy_fd preserve meaningful errno Junio C Hamano
2014-11-18 17:08     ` Junio C Hamano
2014-11-21  9:14 ` Michael Haggerty
2014-11-21  9:17   ` Michael Haggerty
2014-11-21 17:48     ` Junio C Hamano
2014-11-21 17:54       ` Jeff King
2014-11-21 18:31         ` 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=20141204090406.GA27455@peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=mhagger@alum.mit.edu \
    --cc=sbeller@google.com \
    /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).