Re: [PATCH v9 2/6] submodule: rename strbuf variable

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

Calvin Wan <calvinwan@google.com> writes:

> On Thu, Mar 2, 2023 at 4:25 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Calvin Wan <calvinwan@google.com> writes:
>>
>> > A prepatory change for a future patch that moves the status parsing
>> > logic to a separate function.
>> >
>> > Signed-off-by: Calvin Wan <calvinwan@google.com>
>> > ---
>> >  submodule.c | 23 +++++++++++++----------
>> >  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> > Subject: Re: [PATCH v9 2/6] submodule: rename strbuf variable
>>
>> What strbuf variable renamed to what?
>>
>> I have a feeling that squashing this and 3/6 into a single patch,
>> and pass buf.buf and buf.len to the new helper function without
>> introducing an intermediate variables in the caller, would make the
>> resulting code easier to follow.
>>
>> In any case, nice factoring out of a useful helper function.
>>
>
> A much earlier version squashed those changes together, but it was
> recommended to split those changes up; I think I am indifferent either way
> since the refactoring is clear to me whether it is split up or not.
> https://lore.kernel.org/git/221012.868rllo545.gmgdl@evledraar.gmail.com/

I am indifferent, either, but with or without them squashed into a
single patch, "rename strbuf" would not be how you would describe
the value of this refactoring, which is to make the interface not
depend on strbuf.  Some callers may have separate <ptr,len> pair
that is not in strbuf, and with the current interface they are
forced to wrap the pair in a throw-away strbuf which is not nice.

And squashing them together into a single patch, it becomes a lot
clear what the point of these two steps combined is.