On 2023-01-12 00:41, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Jan 11 2023, Matthew John Cheetham via GitGitGadget wrote:
>
>> From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
>> [...]
>> + } else if (buf.len) {
>> + const char *prev = values->v[values->nr - 1];
>> + struct strbuf append = STRBUF_INIT;
>> + strbuf_addstr(&append, prev);
>> +
>> + /* Join two non-empty values with a single space. */
>> + if (append.len)
>> + strbuf_addch(&append, ' ');
>> +
>> + strbuf_addbuf(&append, &buf);
>> +
>> + strvec_pop(values);
>> + strvec_push_nodup(values, strbuf_detach(&append, NULL));
>> + }
>> +
>
> I've written something like the strvec_push_nodup() patch that preceded
> this myself for similar reasons, and as recently noted in [1] I think
> such a thing (although I implemented a different interface) might be
> useful in general.
A fair point, and reading [1] I see there's some concerns about making the
strvec interface more complicated w.r.t. ownership vs saving a `xstrdup`.
In light of this, I'll drop the commit to add `strvec_push_nodup`.
> But this really doesn't seem like a good justification for adding this
> new API. Let's instead do:
>
> } else if (buf.len) {
> const char *prev = values->v[values->nr - 1];
> /* Join two non-empty values with a single space. */
> const char *const sp = *prev ? " " : ""
>
> strvec_pop(values);
> strvec_pushf(values, "%s%s%s", prev, sp, buf.buf);
> }
>
> There may be cases where a public strvec_push_nodup() simplifies things,
> but this doesn't seem like such a case, just use strvec_pushf() directly
> instead, and skip the strbuf & strbuf_detach().
>
> I haven't compiled/tested the above, so there may e.g. be a typo in
> there. But I think the general concept should work in this case.
>
> 1. https://lore.kernel.org/git/RFC-cover-0.5-00000000000-20221215T090226Z-avarab@xxxxxxxxx/
There's a bug in your suggestion. We're `strvec_pop`-ing from the array
which also frees the previous value that we want to use to append to in
the next call to `strvec_pushf`. We need to keep a copy of the previous
header value around.
This should work instead (adding an `xstrdup` and `free`):
} else if (buf.len) {
char *prev = xstrdup(values->v[values->nr - 1]);
/* Join two non-empty values with a single space. */
const char *const sp = *prev ? " " : "";
strvec_pop(values);
strvec_pushf(values, "%s%s%s", prev, sp, buf.buf);
free(prev);
}
Thanks,
Matthew
