On Mon, May 21, 2018 at 09:01:05AM +0900, Junio C Hamano wrote:
> Jacob Keller <jacob.keller@xxxxxxxxx> writes:
>
> > On Sun, May 20, 2018 at 3:17 AM, Martin Ågren <martin.agren@xxxxxxxxx> wrote:
> >> +/**
> >> + * 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 <gitster@xxxxxxxxx>
> ---
Yup, this looks good to me.
-Peff
