git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain()

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Oct 4, 2021, 09:38 UTC
Message-ID
<ebc72146-4431-c29c-ebab-37cf886cf28f@gmail.com>
In-Reply-To
<patch-07.10-646e610d8dd-20211004T002226Z-avarab@gmail.com>
Hi Ævar
On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
Show 23 quoted lines
> Since a preceding commit we've been using
> clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer
> a function that corresponds to setup_unpack_trees_porcelain(), as it
> was when it was added in 1c41d2805e4 (unpack_trees_options: free
> messages when done, 2018-05-21).
> 
> Instead it's similar to strbuf_release() and other similar generic
> "free" functions. Let's rename it to avoid any future confusion on the
> topic.
> 
> Let's also update the API documentation for it to note this, and to
> cover e.g. the code added around update_sparsity() in
> 4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain
> messages, 2020-03-27).
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/checkout.c        | 2 +-
>   builtin/sparse-checkout.c | 2 +-
>   merge-ort.c               | 2 +-
>   merge-recursive.c         | 2 +-
>   merge.c                   | 4 ++--
>   reset.c                   | 2 +-

I was expecting to see sequencer.c here, but it appears it does not call clear_unpack_trees_porcelain() to free the memory allocated with setup_unpack_trees_porcelain(). If you're interested in fixing memory leaks from unpack trees it might be worth checking that all the calls to setup_unpack_trees_porcelain() have a matching call to clear_unpack_trees_porcelain().

Best Wishes
Phillip
Show 120 quoted lines
>   unpack-trees.c            | 2 +-
>   unpack-trees.h            | 8 +++++---
>   8 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index d4b88affba7..482d17676a0 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -757,7 +757,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>   		init_tree_desc(&trees[1], tree->buffer, tree->size);
>   
>   		ret = unpack_trees(2, trees, &topts);
> -		clear_unpack_trees_porcelain(&topts);
> +		unpack_trees_options_release(&topts);
>   		if (ret == -1) {
>   			/*
>   			 * Unpack couldn't do a trivial merge; either
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 4c3c29fb580..b1221fd01d3 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -219,7 +219,7 @@ static int update_working_directory(struct pattern_list *pl)
>   
>   	setup_unpack_trees_porcelain(&o, "sparse-checkout");
>   	result = update_sparsity(&o);
> -	clear_unpack_trees_porcelain(&o);
> +	unpack_trees_options_release(&o);
>   
>   	if (result == UPDATE_SPARSITY_WARNINGS)
>   		/*
> diff --git a/merge-ort.c b/merge-ort.c
> index e526b78b88d..0a5937364c9 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4054,7 +4054,7 @@ static int checkout(struct merge_options *opt,
>   	init_tree_desc(&trees[1], next->buffer, next->size);
>   
>   	ret = unpack_trees(2, trees, &unpack_opts);
> -	clear_unpack_trees_porcelain(&unpack_opts);
> +	unpack_trees_options_release(&unpack_opts);
>   	return ret;
>   }
>   
> diff --git a/merge-recursive.c b/merge-recursive.c
> index a77f66b006c..316cb2ca907 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -441,7 +441,7 @@ static int unpack_trees_start(struct merge_options *opt,
>   static void unpack_trees_finish(struct merge_options *opt)
>   {
>   	discard_index(&opt->priv->orig_index);
> -	clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
> +	unpack_trees_options_release(&opt->priv->unpack_opts);
>   	unpack_trees_options_init(&opt->priv->unpack_opts);
>   }
>   
> diff --git a/merge.c b/merge.c
> index 9cb32990dd9..2f618425aff 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,
>   
>   	if (unpack_trees(nr_trees, t, &opts)) {
>   		rollback_lock_file(&lock_file);
> -		clear_unpack_trees_porcelain(&opts);
> +		unpack_trees_options_release(&opts);
>   		return -1;
>   	}
> -	clear_unpack_trees_porcelain(&opts);
> +	unpack_trees_options_release(&opts);
>   
>   	if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
>   		return error(_("unable to write new index file"));
> diff --git a/reset.c b/reset.c
> index d13984ab781..f4bf3fbfac0 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -133,7 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
>   leave_reset_head:
>   	strbuf_release(&msg);
>   	rollback_lock_file(&lock);
> -	clear_unpack_trees_porcelain(&unpack_tree_opts);
> +	unpack_trees_options_release(&unpack_tree_opts);
>   	while (nr)
>   		free((void *)desc[--nr].buffer);
>   	return ret;
> diff --git a/unpack-trees.c b/unpack-trees.c
> index e7365322e82..bea598c9ece 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -193,7 +193,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)
>   	memcpy(o, &blank, sizeof(*o));
>   }
>   
> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
> +void unpack_trees_options_release(struct unpack_trees_options *opts)
>   {
>   	strvec_clear(&opts->msgs_to_free);
>   	dir_clear(&opts->dir);
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 40c4841748d..a8d1f083b33 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -41,10 +41,12 @@ enum unpack_trees_error_types {
>   void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>   				  const char *cmd);
>   
> -/*
> - * Frees resources allocated by setup_unpack_trees_porcelain().
> +/**
> + * Frees resources allocated by function that take the "struct
> + * unpack_trees_options". Always call this after using unpack_trees(),
> + * update_sparsity() etc.
>    */
> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
> +void unpack_trees_options_release(struct unpack_trees_options *opts);
>   
>   struct unpack_trees_options {
>   	unsigned int reset,
> 
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 23 of 31 in “unpack-trees & dir APIs: fix memory leaks”
  1. 00/10 unpack-trees & dir APIs: fix memory leaksÆvar Arnfjörð Bjarmason, Oct 4, 2021
  2. 02/10 merge-recursive.c: call a new unpack_trees_options_init() functionÆvar Arnfjörð Bjarmason, Oct 4, 2021
  3. Elijah NewrenOct 4, 2021
  4. Ævar Arnfjörð BjarmasonOct 4, 2021
  5. Elijah NewrenOct 4, 2021
  6. 04/10 unpack-trees API: don't have clear_unpack_trees_porcelain() resetÆvar Arnfjörð Bjarmason, Oct 4, 2021
  7. Phillip WoodOct 4, 2021
  8. Ævar Arnfjörð BjarmasonOct 4, 2021
  9. Elijah NewrenOct 4, 2021
  10. Ævar Arnfjörð BjarmasonOct 4, 2021
  11. Elijah NewrenOct 4, 2021
  12. 09/10 merge.c: avoid duplicate unpack_trees_options_release() codeÆvar Arnfjörð Bjarmason, Oct 4, 2021
  13. Elijah NewrenOct 4, 2021
  14. Ævar Arnfjörð BjarmasonOct 4, 2021
  15. 03/10 unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options"Ævar Arnfjörð Bjarmason, Oct 4, 2021
  16. Elijah NewrenOct 4, 2021
  17. 01/10 unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INITÆvar Arnfjörð Bjarmason, Oct 4, 2021
  18. 05/10 dir.[ch]: make DIR_INIT mandatoryÆvar Arnfjörð Bjarmason, Oct 4, 2021
  19. Elijah NewrenOct 4, 2021
  20. 06/10 dir.c: get rid of lazy initializationÆvar Arnfjörð Bjarmason, Oct 4, 2021
  21. Elijah NewrenOct 4, 2021
  22. 07/10 unpack-trees API: rename clear_unpack_trees_porcelain()Ævar Arnfjörð Bjarmason, Oct 4, 2021
  23. Phillip WoodOct 4, 2021
  24. Ævar Arnfjörð BjarmasonOct 4, 2021
  25. Elijah NewrenOct 4, 2021
  26. 08/10 unpack-trees: don't leak memory in verify_clean_subdirectory()Ævar Arnfjörð Bjarmason, Oct 4, 2021
  27. Elijah NewrenOct 4, 2021
  28. 10/10 built-ins: plug memory leaks with unpack_trees_options_release()Ævar Arnfjörð Bjarmason, Oct 4, 2021
  29. Elijah NewrenOct 4, 2021
  30. Ævar Arnfjörð BjarmasonOct 4, 2021
  31. Elijah NewrenOct 4, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.