Subject: Re: [PATCH v3 14/25] setup.c: convert is_git_directory() to use strbuf

From: Junio C Hamano <[email-masked]>
Date: Thu, 20 Feb 2014 11:36:57 -0800

Junio C Hamano <[email-masked]> writes:

> Duy Nguyen <[email-masked]> writes:
>
>> On Thu, Feb 20, 2014 at 3:17 AM, Junio C Hamano <[email-masked]> wrote:
>>>> +     strbuf_setlen(sb, len);
>>>> +     strbuf_add(sb, s, strlen(s));
>>>
>>> I am not sure addstr_at() gives us a good abstraction, or at least
>>> the name conveys what it does well not to confuse readers.
>>>
>>> At first after only seeing its name, I would have expected that it
>>> would splice the given string into an existing strbuf at the
>>> location, not chopping the existing strbuf at the location and
>>> appending.
>>
>> I think I invented a few new strbuf_* in this series and this is one
>> of them. We have about ~14 other places in current code that do
>> similar pattern: set length back, then add something on top.
>
> Yes, and you can count getline/getwholeline as a special case to
> chomp to empty at the beginning.  I am not opposed to a helper to
> give us an easy access to this common pattern.
>
> It was just the name "addstr-at" did not sound, at least to me, what
> it does, i.e. "replace with s from the pos to the end", which I
> think is the same thing as a single-liner:
>
>     strbuf_splice(sb, pos, sb->len - pos, s, strlen(s))

Oh, and as to other strbuf_* helpers, I am finding myself getting
very fond of strbuf_git_path() as I read the series along; it gives
us the same convenience as git_path() [*1*] while giving us tighter
control on the lifetime rules of the path buffer.

[Footnote]

*1* And the new git_path() updated in this series has to be a lot
 more than catenate($GIT_DIR, "/", $path) but needs the smart of
 adjust_git_path(), the convenience matters.