Web lists-archives.com

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




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.

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

So, I dunno.