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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 3, 2014, 21:37 UTC
Message-ID: <xmqqppc0wh33.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPig+cTsULQPxoaSQ-ZvjWJ9Rgpdf3zG7ObPg4TnxFbXT9TwnA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 10 quoted lines

>> @@ -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.

Show 12 quoted lines

>> +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.

Show 6 quoted lines

>> +                       /* 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?