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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2015, 16:07 UTC
Message-ID: <xmqqbnfx89iz.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPig+cROJJNTcZnZfMP0meA8ZWGcSHcQCMTCkuC+kn_+OQZ-zA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 8 quoted lines

>> +       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.

Show 8 quoted lines

> 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.