Re: [PATCH v5 09/10] http: read HTTP WWW-Authenticate response headers
- From
Matthew John Cheetham <mjcheetham@outlook.com>
- Date
- Jan 17, 2023, 21:51 UTC
- Message-ID
- <AS2PR03MB9815C5C045F3138287070F1AC0C69@AS2PR03MB9815.eurprd03.prod.outlook.com>
- In-Reply-To
- <230112.86wn5s2l5r.gmgdl@evledraar.gmail.com>
On 2023-01-12 00:41, Ævar Arnfjörð Bjarmason wrote:
Show 25 quoted lines
>
> On Wed, Jan 11 2023, Matthew John Cheetham via GitGitGadget wrote:
>
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>> [...]
>> + } 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`.
Show 20 quoted lines
> 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@gmail.com/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