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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2014, 19:06 UTC
Message-ID: <xmqqwqgp61tl.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACsJy8Ab0OVQw4ATgv6E7rv-B3OwbhyrJBbXiXjYiUvtMknMrg@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 15 quoted lines

> On Thu, Feb 20, 2014 at 3:17 AM, Junio C Hamano <gitster@pobox.com> 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))