Re: [PATCH v5 1/1] http: Add Accept-Language header if possible

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> @@ -515,6 +517,9 @@ void http_cleanup(void)
>>                 cert_auth.password = NULL;
>>         }
>>         ssl_cert_password_required = 0;
>> +
>> +       if (cached_accept_language)
>> +               strbuf_release(cached_accept_language);
>
> Junio already mentioned that this is leaking the memory of the strbuf
> struct itself which was xmalloc()'d by get_accept_language().

I actually didn't ;-)  A singleton cached_accept_language strbuf
itself being kept around, with its reuse by get_accept_language(),
is fine and is not a leak.  But clearing the strbuf alone will
introduce correctness problem---the second HTTP connection will see
an empty strbuf, get_accept_language() will say "we've already
computed and the header we must issue is an empty string", which is
not correct.

In the fix-up "SQUASH???" commit I queued on top of this patch on
'pu', I had to run "sort -u" on the output to the standard error
stream, as there seemed to be two HTTP connections and the actual
output had two headers, even though the test expected only one in
the output.  I suspect that it is a fallout from this bug that the
original code passed that test that expects only one.

>> +static struct strbuf *get_accept_language(void)
>
> I find this API a bit strange. Use of strbuf to construct the returned
> string is an implementation detail of this function. From the caller's
> point of view, it should just be receiving a constant string: one
> which it needs neither to modify nor free. Also, if the caller were to
> modify the returned strbuf for some reason, then that modification
> would impact all future calls to get_accept_language() since the
> strbuf is 'static' and not recomputed. Instead, I would expect the
> declaration to be:
>
>     static const char *get_accept_language(void)

Makes sense to me.

>> +                       /* Put a q-factor only if it is less than 1.0. */
>> +                       if (q < max_q)
>> +                               strbuf_addf(cached_accept_language, q_format, q);
>> +
>> +                       if (q > 1)
>> +                               q--;

I didn't mention this but if q ever goes below 1, wouldn't it mean
that there is no point continuing this loop?