Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()

From: Jeff King <peff@peff.net>
Date: Dec 19, 2017, 11:38 UTC
Message-ID: <20171219113855.GA24558@sigill.intra.peff.net>
In-Reply-To: <f1584860-d0d6-db82-0a49-021924c3e2b7@web.de>

On Mon, Dec 18, 2017 at 08:18:17PM +0100, René Scharfe wrote:

Show 17 quoted lines

> > 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? That's the same duality we have now with string_list.

Show 8 quoted lines

> > 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.

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).

-Peff