Re: [PATCH 06/34] clone: release strbuf after use in remove_junk()

["René Scharfe" <l.s.r@web.de>]

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