Re: [PATCH 1/3] tree.c: update read_tree_recursive callback to pass strbuf as base

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 3, 2014, 16:13 UTC
Message-ID: <xmqq1toglnjf.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPig+cS1OK6pv5A0vuJf=j6eFrNv70=AYgVz3zQny-md15_xKg@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 19 quoted lines

> On Tue, Dec 2, 2014 at 7:11 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Tue, Dec 2, 2014 at 2:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>>
>>>> This allows the callback to use 'base' as a temporary buffer to
>>>> quickly assemble full path "without" extra allocation. The callback
>>>> has to restore it afterwards of course.
>>>
>>> Hmph, what's the quote around 'without' doing there?
>>
>> because it's only true if you haven't used up all preallocated space
>> in strbuf. If someone passes an empty strbuf, then underneath strbuf
>> may do a few realloc until the buffer is large enough.
>
> Would it be easier to understand if written like this?
>
>     This allows the callback to use 'base' as a temporary buffer to
>     quickly assemble full path, thus avoiding allocation/deallocation
>     for each iteration step.

Thanks.

I am not Duy so I do not know if that matches what he wanted to say, or if it matches what the change gives us. Your phrasing wouldn't have made me say "Hmph...?" and it is an improvement that goes in the right direction, I think.

A question during the review, especially on proposed log messages and documentation changes, is rarely just a request to explain it to the questioner in the discussion. It is an indication that what is being commented on needs tweaking to be understood.