Re: Re*: [PATCH v4 3/4] string-list: provide `string_list_appendf()`

On Mon, May 21, 2018 at 09:01:05AM +0900, Junio C Hamano wrote:

Jacob Keller [off-list ref] writes:

quoted

On Sun, May 20, 2018 at 3:17 AM, Martin Ågren [off-list ref] wrote:

quoted

+/**
+ * Add formatted string to the end of `list`. This function ignores
+ * the value of `list->strdup_strings` and always appends a freshly
+ * allocated string, so you will probably not want to use it with
+ * `strdup_strings = 0`.
+ */
+struct string_list_item *string_list_appendf(struct string_list *list,
+                                            const char *fmt, ...);
+

Would it make sense to verify that strdup_strings == 0? I guess we'd
have to use die or BUG(), but that would mean that the program could
crash..

It probably is clear to readers that any reasonable implementation
of *_appendf() will create a new and unique string, as the point of
*f() is to give a customized instantiation of fmt string for given
parameters.  So it would be natural to expect that the storage that
holds the generated string will belong to the list.  We _could_ make
it honor strdup_strings and make one extra copy when strdup_strings
is set to true, but the only effect such a stupid implementation has
is to unnecessarily leak ;-)

I think it is probably OK to check and BUG() when strdup_strings==0,
but such a check means that we now declare that a string list must
either borrow all of its strings from elsewhere or own all of its
strings itself, and mixture is not allowed.

The (overly) flexible string_list API could be used to mix both
borrowed and owned strings (an obvious strategy to do this without
leaking and crashing is to use the .util field to mark which ones
are owned and which ones are borrowed), so there might already be
current users of the API that violates that rule.

IMHO such a mixed use is mildly crazy. At any rate, we would know that
anybody using appendf() would not have this problem, since we are just
introducing it now.

I have a feeling that argv_array might be a better fit for the
purpose of keeping track of to_free[] strings in the context of this
series.  Moving away from string_list would allow us to sidestep the
storage ownership issues the API has, and we do not need the .util
thing string_list gives us (which is one distinct advantage string_list
has over argv_array, if the application needs that feature).

I do agree that argv_array is generally a better fit for most cases.
Didn't we want to rename it to strarray or something? That's probably
too much yak-shaving for this series, though. :)

We would need to make _pushf() and friends return "const char *" if
we go that route to make the resulting API more useful, though.

This is the first time I think that's been suggested, but I agree it's
the only sensible thing for the functions to return.

-- >8 --
Subject: argv-array: return the pushed string from argv_push*()

Such an API change allows us to use an argv_array this way:

	struct argv_array to_free = ARGV_ARRAY_INIT;
        const char *msg;

        if (some condition) {
		msg = "constant string message";
		... other logic ...
	} else {
		msg = argv_pushf(&to_free, "format %s", var);
	}
	... use "msg" ...
	... do other things ...
	argv_clear(&to_free);

Note that argv_array_pushl() and argv_array_pushv() are used to push
one or more strings with a single call, so we do not return any one
of these strings from these two functions in order to reduce the
chance to misuse the API.

Signed-off-by: Junio C Hamano <redacted>
---

Yup, this looks good to me.

-Peff