Re: [PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename()

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> But that intermediate allocation feels wasteful. Since this function is
> simple enough, we can instead "inline" its implementation into the
> xstrfmt call itself. That is a little gross, but arguably better than
> allocating memory unnecessarily.

The function being "simple enough" does not help maintenance burden
this change brings in at all, does it?  Unless it is accompanied by
a comment "the template string used here must be kept in sync with
what is used in the get_midx_filename() defined elsewhere" near the
lines this patch touches, perhaps in big red letters, that is.

Without this caller, get_midx_filename() could be file-scope static
in midx.c and can be eliminated from midx.h, it seems.  If there are
many other filenames like midx_bitmap that are derrived from
midx_filename, it might be worth extending get_midx_filename to take
an extra format parameter (i.e. "I want midx-related filename for
this object directory, but I am getting this .bitmap kind").  Another
remedy may be to move this function to midx.c and make it sit next
to what this function must be always in sync with.

Or use a C preprocessor macro for "%s/pack/multi-pack-index" in
midx.h and use it here and there to make sure they will stay in
sync.

But after reading all the users of get_midx_filename(), especially
given that it is mostly internal implementation detail inside
midx.c, I think rewriting

	f() {
	char *midx = get_midx_filename(...);

	... use midx ...

	cleanup:
	free(midx);
	}

into

	f() {
	struct strbuf midx;

	get_midx_filename(&midx, ...);

	... use midx.buf ...

	cleanup:
	strbuf_release(&midx);
	}

is not too bad a fix for this.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  pack-bitmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index f47a0a7db4..d292e81af1 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -292,8 +292,8 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
>  
>  char *midx_bitmap_filename(struct multi_pack_index *midx)
>  {
> -	return xstrfmt("%s-%s.bitmap",
> -		       get_midx_filename(midx->object_dir),
> +	return xstrfmt("%s/pack/multi-pack-index-%s.bitmap",
> +		       midx->object_dir,
>  		       hash_to_hex(get_midx_checksum(midx)));
>  }