Am 06.09.2017 um 21:51 schrieb Junio C Hamano:
> Rene Scharfe <l....@web.de> writes:
> 
>> Signed-off-by: Rene Scharfe <l....@web.de>
>> ---
>>   builtin/clone.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 8d11b570a1..dbddd98f80 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -487,28 +487,28 @@ N_("Clone succeeded, but checkout failed.\n"
>> ...
>>      if (junk_git_dir) {
>>              strbuf_addstr(&sb, junk_git_dir);
>>              remove_dir_recursively(&sb, 0);
>>              strbuf_reset(&sb);
>>      }
>>      if (junk_work_tree) {
>>              strbuf_addstr(&sb, junk_work_tree);
>>              remove_dir_recursively(&sb, 0);
>> -            strbuf_reset(&sb);
>>      }
>> +    strbuf_release(&sb);
>>   }
> 
> The code definitely needs a _release() at the end, but I feel
> lukewarm about the "if we are about to _release(), do not bother to
> _reset()" micro-optimization.  Keeping the existing two users that
> use sb as a (shared and reused) temporary similar would help those
> who add the third one or reuse the pattern in their code elsewhere.

That's not intended as an optimization, but as a promotion -- the reset
is moved to the outer block and upgraded to a release.  The result is
consistent with builtin/worktree.c::remove_junk().

> We could flip the "be nice to the next user by clearing after use"
> pattern to "clear any potential cruft before you use", i.e.
> 
>       if (...) {
>               strbuf_reset(&sb);
>               strbuf_addstr(&sb, ...);
>       }
>       if (...) {
>               strbuf_reset(&sb);
>               strbuf_addstr(&sb, ...);
>       }
>       strbuf_release(&sb);
> 
> but that still tempts us for the same micro-optimization at the
> beginning where sb hasn't been used since STRBUF_INIT, so it is not
> a real "solution".

If code reuse is the goal then a different micro-optimization is more
of a hindrance IMHO: Reusing the same strbuf for both deletions.  A
deletion function that takes a plain string and handles strbuf
formalities for the caller would have avoided the leak, be easier to
use for deleting a third file and probably not have a measurable
performance impact due to the low number of calls.

René

Reply via email to