From: "René Scharfe" <l.s.r@web.de>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>, Git List <git@vger.kernel.org>
Subject: Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()
Date: Tue, 19 Dec 2017 19:26:06 +0100 [thread overview]
Message-ID: <f79ae9a3-de47-c41e-fea6-6a0fedf4ef63@web.de> (raw)
In-Reply-To: <20171219113855.GA24558@sigill.intra.peff.net>
Am 19.12.2017 um 12:38 schrieb Jeff King:
> On Mon, Dec 18, 2017 at 08:18:17PM +0100, René Scharfe wrote:
>
>>> I'd actually argue the other way: the simplest interface is one where
>>> the string list owns all of its pointers. That keeps the
>>> ownership/lifetime issues clear, and it's one less step for the caller
>>> to have to remember to do at the end (they do have to clear() the list,
>>> but they must do that anyway to free the array of items).
>>>
>>> It does mean that some callers may have to remember to free a temporary
>>> buffer right after adding its contents to the list. But that's a lesser
>>> evil, I think, since the memory ownership issues are all clearly
>>> resolved at the time of add.
>>>
>>> The big cost is just extra copies/allocations.
>>
>> An interface requiring callers to allocate can be used to implement a
>> wrapper that does all allocations for them -- the other way around is
>> harder. It can be used to avoid object duplication, but duplicates
>> functions. No idea if that's worth it.
>
> Sure, but would anybody actually want to _use_ the non-wrapped version?
Not sure, but cases that currently use STRING_LIST_INIT_NODUP probably
apply. Apropos: apply.c::write_out_results() looks like it might, too.
Another question is how much it would cost to let them duplicate strings
as well in order to simplify the code.
> That's the same duality we have now with string_list.
Hmm, I thought we *were* discussing string_list?
>>> Having a "format into a string" wrapper doesn't cover _every_ string you
>>> might want to add to a list, but my experience with argv_array_pushf
>>> leads me to believe that it covers quite a lot of cases.
>>
>> It would fit in with the rest of the API -- we have string_list_append()
>> as a wrapper for string_list_append_nodup()+xstrdup() already. We also
>> have similar functions for strbuf and argv_array. I find it a bit sad
>> to reimplement xstrfmt() yet again instead of using it directly, though.
>
> I dunno, I think could provide some safety and some clarity. IOW, why
> _don't_ we like:
>
> string_list_append_nodup(list, xstrfmt(fmt, ...));
>
> ? I think because:
>
> 1. It's a bit long and ugly.
>
> 2. It requires a magic "nodup", because we're violating the memory
> ownership boundary.
>
> 3. It doesn't provide any safety for the case where strdup_strings is
> not set, making it easy to leak accidentally.
Right, and at least 2 and 3 would be solved by having distinct types for
the plain and the duplicating variants. The plain one would always
"nodup" and would have no flags that need to be checked.
> Doing:
>
> string_list_appendf(list, fmt, ...);
>
> pushes the memory ownership semantics "under the hood" of the
> string_list API. And as opposed to being a simple wrapper, it could
> assert that strdup_strings is set (we already do some similar assertions
> in the split functions).
Yes, that check would guard against leaks.
Having few functions that can be combined looks like a cleaner interface
to me than having additional shortcuts for specific combinations -- less
duplication, smaller surface.
That said I'm not against adding string_list_appendf(); we already have
similar functions for other types.
René
next prev parent reply other threads:[~2017-12-19 18:26 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-07 20:22 [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog() René Scharfe
2017-12-07 21:27 ` Jeff King
2017-12-08 17:29 ` René Scharfe
2017-12-08 18:44 ` Junio C Hamano
2017-12-08 20:10 ` René Scharfe
2017-12-08 21:11 ` Jeff King
2017-12-07 21:47 ` Junio C Hamano
2017-12-08 10:14 ` Jeff King
2017-12-08 17:29 ` René Scharfe
2017-12-08 18:37 ` Junio C Hamano
2017-12-08 21:28 ` Jeff King
2017-12-18 19:18 ` René Scharfe
2017-12-19 11:38 ` Jeff King
2017-12-19 18:26 ` René Scharfe [this message]
2017-12-20 13:05 ` Jeff King
2017-12-08 21:17 ` Jeff King
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f79ae9a3-de47-c41e-fea6-6a0fedf4ef63@web.de \
--to=l.s.r@web.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).