[v2,0/3] Clean up leaks in commit-graph.c

Message

Linus Arver via GitGitGadget Oct. 3, 2018, 5:12 p.m. UTC

While looking at the commit-graph code, I noticed some memory leaks. These
can be found by running

valgrind --leak-check=full ./git commit-graph write --reachable

The impact of these leaks are small, as we never call write_commit_graph
_reachable in a loop, but it is best to be diligent here.

While looking at memory consumption within write_commit_graph(), I noticed
that we initialize our oid list with "object count / 4", which seems to be a
huge over-count. I reduce this by a factor of eight.

I built off of ab/commit-graph-progress, because my patch involves lines
close to those changes.

V2 includes feedback from V1 along with Martin's additional patches.

Thanks, -Stolee

Derrick Stolee (2):
  commit-graph: clean up leaked memory during write
  commit-graph: reduce initial oid allocation

Martin Ågren (1):
  builtin/commit-graph.c: UNLEAK variables

 builtin/commit-graph.c | 11 ++++++-----
 commit-graph.c         | 16 ++++++++++------
 2 files changed, 16 insertions(+), 11 deletions(-)


base-commit: 6b89a34c89fc763292f06012318b852b74825619
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-42%2Fderrickstolee%2Fcommit-graph-leak-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-42/derrickstolee/commit-graph-leak-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/42

Range-diff vs v1:

 1:  6906c25415 ! 1:  ba65680b3d commit-graph: clean up leaked memory during write
     @@ -7,17 +7,29 @@
          leaked in write_commit_graph_reachable(). Clean these up so our
          memory checking is cleaner.
      
     -    Running 'valgrind --leak-check=full git commit-graph write
     -    --reachable' demonstrates these leaks and how they are fixed after
     -    this change.
     +    Further, if we use a list of pack-files to find the commits, we
     +    can leak the packed_git structs after scanning them for commits.
      
     +    Running the following commands demonstrates the leak before and
     +    the fix after:
     +
     +    * valgrind --leak-check=full ./git commit-graph write --reachable
     +    * valgrind --leak-check=full ./git commit-graph write --stdin-packs
     +
     +    Signed-off-by: Martin Ågren <[e-mail]>
          Signed-off-by: Derrick Stolee <[e-mail]>
      
      diff --git a/commit-graph.c b/commit-graph.c
      --- a/commit-graph.c
      +++ b/commit-graph.c
      @@
     - 	string_list_init(&list, 1);
     + void write_commit_graph_reachable(const char *obj_dir, int append,
     + 				  int report_progress)
     + {
     +-	struct string_list list;
     ++	struct string_list list = STRING_LIST_INIT_DUP;
     + 
     +-	string_list_init(&list, 1);
       	for_each_ref(add_ref_to_list, &list);
       	write_commit_graph(obj_dir, NULL, &list, append, report_progress);
      +
     @@ -25,6 +37,14 @@
       }
       
       void write_commit_graph(const char *obj_dir,
     +@@
     + 				die(_("error opening index for %s"), packname.buf);
     + 			for_each_object_in_pack(p, add_packed_commits, &oids, 0);
     + 			close_pack(p);
     ++			free(p);
     + 		}
     + 		stop_progress(&oids.progress);
     + 		strbuf_release(&packname);
      @@
       	compute_generation_numbers(&commits, report_progress);
       
     @@ -45,5 +65,8 @@
      +	free(graph_name);
      +	free(commits.list);
       	free(oids.list);
     - 	oids.alloc = 0;
     - 	oids.nr = 0;
     +-	oids.alloc = 0;
     +-	oids.nr = 0;
     + }
     + 
     + #define VERIFY_COMMIT_GRAPH_ERROR_HASH 2
 -:  ---------- > 2:  13032d8475 builtin/commit-graph.c: UNLEAK variables
 2:  e29a0eaf03 = 3:  1002fd34fc commit-graph: reduce initial oid allocation