Re: [PATCH v6 6/7] git-reflog: add create and exists functions

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +       for (i = start; i < argc; i++) {
>>> +               if (safe_create_reflog(argv[i], &err, 1)) {
>>> +                       error("could not create reflog %s: %s", argv[i],
>>> +                             err.buf);
>>> +                       status = 1;
>>> +                       strbuf_release(&err);
>>
>> This feels a bit dirty.
>
> Hmm, I do not share that feeling.  I wouldn't be surprised if you
> found a lot of existing codepaths that run _init() a strbuf once,
> work on it and then _release() once a section of code is done with
> it, reuse it for further (unrelated) processing, knowing that
> _release() implicitly did _init() and the strbuf is ready to use
> after that.  I thought that was a very well established pattern.

>> While it's true that the current
>> implementation of strbuf_release() re-initializes the strbuf (so
>> you're not technically wrong by re-using it), that's an implementation
>> detail; and indeed, the strbuf_release() documentation says:
>>
>>     Release a string buffer and the memory it used. You should not
>>     use the string buffer after using this function, unless you
>>     initialize it again.
>
> Hmph. Perhaps the doc is wrong? ;-)

>> Alternatives would be strbuf_reset() or declaring and releasing the
>> strbuf within the for-loop scope.
>
> Because _reset() just rewinds the .len pointer without deallocating,
> you would need an extra _release() before it goes out of scope. If
> it is expected that the strbuf will be reused for a number of times,
> the length of the string each iteration uses is similar, and you
> will iterate the loop many times, "_reset() each time and _release()
> to clean-up" pattern would save many calls to realloc/free.