Am 06.09.2017 um 21:51 schrieb Junio C Hamano:
> Rene Scharfe <l.s.r@xxxxxx> writes:
>
>> Signed-off-by: Rene Scharfe <l.s.r@xxxxxx>
>> ---
>> 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é
