[PATCH v2 0/9] midx: clean up t5319 under 'SANITIZE=leak'

[Taylor Blau <me@ttaylorr.com>]

Here is a small-ish update to my series which makes t5319 leak-free (i.e., it
passes even when Git is compiled with SANITIZE=leak). It is based on
ab/only-single-progress-at-once (so I dropped the cherry-picked patch towards
the end from Ævar).

Most of the fixes are straightforward from review, but the biggest change is in
7/10, which converts get_midx_filename() to write its result to a strbuf per
Junio's helpful suggestion.

Thinking ahead, there are a few things I noted while reading through replies for
future improvements to the MIDX and pack bitmap code. From my ~/notes, they are:

  - load_multi_pack_index() should not die when missing the packfile names chunk,
    instead should pretend that no MIDX exists (maybe warn?)
  - double-close when reaching the cleanup_fail codepath after xmmap returns
  - static `verify_midx_error` is ugly
  - load_bitmap() does not clean up after itself very well (e.g.,
    bitmap_git->trees and others)

I'm pretty happy with the state of this topic, and I'll plan on taking on the
above in separate series in the future.

Taylor Blau (9):
  midx.c: clean up chunkfile after reading the MIDX
  midx.c: don't leak MIDX from verify_midx_file
  t/helper/test-read-midx.c: free MIDX within read_midx_file()
  builtin/pack-objects.c: don't leak memory via arguments
  builtin/repack.c: avoid leaking child arguments
  builtin/multi-pack-index.c: don't leak concatenated options
  midx.c: write MIDX filenames to strbuf
  pack-bitmap.c: don't leak type-level bitmaps
  pack-bitmap.c: more aggressively free in free_bitmap_index()

 builtin/multi-pack-index.c |  4 +++
 builtin/pack-objects.c     | 11 ++++---
 builtin/repack.c           | 23 +++++++++----
 midx.c                     | 66 ++++++++++++++++++++++----------------
 midx.h                     |  4 +--
 pack-bitmap.c              | 29 ++++++++++++++---
 pack-revindex.c            |  8 ++---
 t/helper/test-read-midx.c  |  3 +-
 8 files changed, 97 insertions(+), 51 deletions(-)

Range-diff against v1:
 1:  30f6f23daf =  1:  dcc5998072 midx.c: clean up chunkfile after reading the MIDX
 2:  b0c79904ab !  2:  258a9e2e57 midx.c: don't leak MIDX from verify_midx_file
    @@ Metadata
      ## Commit message ##
         midx.c: don't leak MIDX from verify_midx_file
     
    -    The function midx.c:verify_midx_file() allocate a MIDX struct by calling
    -    load_multi_pack_index(). But when cleaning up, it calls free() without
    -    freeing any resources associated with the MIDX.
    +    The function midx.c:verify_midx_file() allocates a MIDX struct by
    +    calling load_multi_pack_index(). But when cleaning up, it calls free()
    +    without freeing any resources associated with the MIDX.
     
         Call the more appropriate close_midx() which does free those resources,
         which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.
 3:  5157edb41e =  3:  84859d5b53 t/helper/test-read-midx.c: free MIDX within read_midx_file()
 4:  dd3b9a949e =  4:  aedb1713b4 builtin/pack-objects.c: don't leak memory via arguments
 5:  a68c77c006 !  5:  bcd12ecab8 builtin/repack.c: avoid leaking child arguments
    @@ Commit message
         In none of these cases do we bother to call child_process_clear(), which
         frees the memory associated with each child's arguments and environment.
     
    -    In order to do so, tweak each function that spawns a child process to
    -    have a `cleanup` label that we always visit before returning from each
    -    function. Then, make sure that we call child_process_clear() as a part
    -    of that label.
    +    Make sure that we call child_process_clear() in any functions which
    +    initialize a struct child_process before returning along a path which
    +    did not call finish_command().
    +
    +    In cmd_repack(), take a slightly different approach to use a cleanup
    +    label to clear the child_process, unless finish_command() was called.
    +    This allows us to free other memory allocated during the lifetime of
    +    that function. But it avoids calling child_process_clear() twice (the
    +    other call coming from inside of finish_command()) to avoid assuming the
    +    function's implementation is idempotent.
     
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## builtin/repack.c ##
     @@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_args *args,
    - 	struct child_process cmd = CHILD_PROCESS_INIT;
    - 	FILE *out;
    - 	struct strbuf line = STRBUF_INIT;
    -+	int ret = 0;
    + 	for_each_packed_object(write_oid, &cmd,
    + 			       FOR_EACH_OBJECT_PROMISOR_ONLY);
      
    - 	prepare_pack_objects(&cmd, args);
    - 	cmd.in = -1;
    -@@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_args *args,
    - 
    - 	if (cmd.in == -1)
    +-	if (cmd.in == -1)
    ++	if (cmd.in == -1) {
      		/* No packed objects; cmd was never started */
    --		return;
    -+		goto cleanup;
    ++		child_process_clear(&cmd);
    + 		return;
    ++	}
      
      	close(cmd.in);
      
    -@@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_args *args,
    - 		free(promisor_name);
    - 	}
    - 	fclose(out);
    --	if (finish_command(&cmd))
    -+	ret = finish_command(&cmd);
    -+
    -+cleanup:
    -+	child_process_clear(&cmd);
    -+
    -+	if (ret)
    - 		die(_("could not finish pack-objects to repack promisor objects"));
    - }
    - 
    -@@ builtin/repack.c: static int write_midx_included_packs(struct string_list *include,
    - 	struct string_list_item *item;
    - 	struct packed_git *largest = get_largest_active_pack(geometry);
    - 	FILE *in;
    --	int ret;
    -+	int ret = 0;
    - 
    - 	if (!include->nr)
    --		return 0;
    -+		goto cleanup;
    - 
    - 	cmd.in = -1;
    - 	cmd.git_cmd = 1;
     @@ builtin/repack.c: static int write_midx_included_packs(struct string_list *include,
    + 		strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
      
      	ret = start_command(&cmd);
    - 	if (ret)
    --		return ret;
    -+		goto cleanup;
    +-	if (ret)
    ++	if (ret) {
    ++		child_process_clear(&cmd);
    + 		return ret;
    ++	}
      
      	in = xfdopen(cmd.in, "w");
      	for_each_string_list_item(item, include)
    - 		fprintf(in, "%s\n", item->string);
    - 	fclose(in);
    - 
    --	return finish_command(&cmd);
    -+	ret = finish_command(&cmd);
    -+
    -+cleanup:
    -+	child_process_clear(&cmd);
    -+
    -+	return ret;
    - }
    - 
    - int cmd_repack(int argc, const char **argv, const char *prefix)
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
      	struct pack_geometry *geometry = NULL;
      	struct strbuf line = STRBUF_INIT;
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
     +	int i, ext, ret = 0;
      	FILE *out;
      	int show_progress = isatty(2);
    ++	int cmd_cleared = 0;
      
    + 	/* variables to be filled by option parsing */
    + 	int pack_everything = 0;
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
      
      	ret = start_command(&cmd);
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
      	if (geometry) {
      		FILE *in = xfdopen(cmd.in, "w");
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
    + 	}
      	fclose(out);
      	ret = finish_command(&cmd);
    ++	cmd_cleared = 1;
      	if (ret)
     -		return ret;
     +		goto cleanup;
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
      	string_list_clear(&existing_kept_packs, 0);
      	clear_pack_geometry(geometry);
      	strbuf_release(&line);
    -+	child_process_clear(&cmd);
    ++	if (!cmd_cleared)
    ++		child_process_clear(&cmd);
      
     -	return 0;
     +	return ret;
 6:  ffded80c7d =  6:  0d252cd323 builtin/multi-pack-index.c: don't leak concatenated options
 7:  f3897c3afc <  -:  ---------- pack-bitmap.c: avoid leaking via midx_bitmap_filename()
 -:  ---------- >  7:  0f293ab638 midx.c: write MIDX filenames to strbuf
 8:  29920e7735 =  8:  77a4454632 pack-bitmap.c: don't leak type-level bitmaps
 9:  e65ac7deb5 !  9:  c1e7e6cc92 pack-bitmap.c: more aggressively free in free_bitmap_index()
    @@ Commit message
         The function free_bitmap_index() is somewhat lax in what it frees. There
         are two notable examples:
     
    -      - While it does call kh_destroy_oid_map on the "bitmaps" map (which
    -        maps) commit OIDs to their corresponding bitmaps, the bitmaps
    +      - While it does call kh_destroy_oid_map on the "bitmaps" map, which
    +        maps commit OIDs to their corresponding bitmaps, the bitmaps
             themselves are not freed. Note here that we recycle already-freed
             ewah_bitmaps into a pool, but these are handled correctly by
             ewah_pool_free().
10:  cb30aa67c0 <  -:  ---------- pack-bitmap-write.c: don't return without stop_progress()
11:  f1bb8b73ff <  -:  ---------- t5319: UNLEAK() the remaining leaks
-- 
2.33.0.96.g73915697e6